[openstreetmap/openstreetmap-website] [WIP] Simplify GPS trace visibility levels (PR #7146)

[Pablo Brasero]

@pablobm commented on this pull request.

Is this PR ready or not? It's marked as WIP but not as draft, and a test is failing.

> @@ -103,6 +103,9 @@ def edit
   def create
     @title = t ".upload_trace"
 
+    # New traces can only be trackable or identifiable.
+    return head :bad_request unless %w[trackable identifiable].include?(params[:trace][:visibility])

How about moving this question to the model?

```suggestion
    return head :bad_request unless Trace.valid_visibility?(params[:trace][:visibility])
```

Same in other instances.

>  
-    if visibility
-      visibility.v
-    elsif current_user.preferences.find_by(:k => "gps.trace.public", :v => "default").nil?
-      "private"
-    else
-      "public"
-    end
+    "trackable"

Similarly, this perhaps should be `Trace.default_visibility`.

And since I'm at it: the whole concept of default visibility for a specific user could be moved to something like `User#default_trace_visibility`, similar to how there's `User#preferred_color_scheme` or `User#default_diary_language`.

> +               [[t("traces.visibility.trackable"), "trackable"],
                 [t("traces.visibility.identifiable"), "identifiable"]],

I think this list belongs in an appropriate model too 🙂 Or at least a helper. Then it can be used in both edit and new.

> @@ -9,9 +9,7 @@
   <%= f.text_field :description, :maxlength => 255 %>
   <%= f.text_field :tagstring %>
   <%= f.select :visibility,

This is related to the failing test, so I'm guessing you are thinking about this, but just in case I'll phrase it here.

You say "User can still change their own existing traces to trackable or identifiable whenever they want", but this form risks doing it without the user noticing:

1. User has a pre-existing "private" trace.
2. User visits to edit the trace.
3. User changes the description, updates.
4. The trace is now "trackable" and the user may not have noticed.

Options that spring to mind:
1. This is fine.
2. If the trace has a deprecated visibility, add it to the options, then:
    a. the user can update the trace and keep the unsupported visibility unchanged.
    b. on submit we show an error. The user must change the visibility to a supported one if they want to edit the trace at all.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7146#pullrequestreview-4477050943
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/7146/review/4477050943 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260611/0a9a88c5/attachment.htm>