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

[Ruben L. Mendoza]

@Rub21 commented on this pull request.



> @@ -28,13 +28,8 @@ def create
       description = params[:description] || ""
       visibility = params[:visibility]
 
-      if visibility.nil?
-        visibility = if params.fetch(:public, "0").to_i.nonzero?
-                       "public"
-                     else
-                       "private"
-                     end
-      end
+      # New traces can only be trackable or identifiable.
+      return head :bad_request unless Trace.valid_visibility?(visibility)

Quick update. The controller check was the original version here, and all tests passed with it.

```
      # New traces can only be trackable or identifiable.
      return head :bad_request unless Trace.valid_visibility?(visibility)
```

I tried moving it to a model validation like you suggested, but it breaks the legacy tests. The factory creates public/private traces to set up existing-data cases, and the validation blocks that on create. Rewriting those tests would not be a good option. Some of them test legacy behavior on purpose, and public and private are still valid values in the database for existing traces.


So I would keep the check in the controllers, calling Trace.valid_visibility?. The rule is really about the upload, and a trace can still hold a legacy value that exists in the database. What do you think?



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

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