[openstreetmap/openstreetmap-website] Simplify GPS trace visibility levels (PR #7146)
Pablo Brasero
notifications at github.com
Thu Jul 2 13:50:47 UTC 2026
@pablobm 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)
My concern is that then we leave the tests littered with references to legacy values, giving the impression that they are the normal case.
I think the tests need to be converted to use the new values, while still acknowledging that legacy values will remain in DB and need to be handled appropriately.
These are the steps to get there:
- Generally, convert instances of public/private to identifiable/trackable...
- ..except where it makes sense to test legacy values. This might be existing tests (eg: adding records to `test_index`) or new ones that exist specifically to test these edge cases.
- When the tests need legacy values, they can be created with this:
```
# Source - https://stackoverflow.com/a/25391890
# Posted by Jason Denney, modified by community. See post 'Timeline' for change history
# Retrieved 2026-07-02, License - CC BY-SA 4.0
trait :without_validations do
to_create { |instance| instance.save(validate: false) }
end
```
If you put these changes in the commit titled "Validate the create visibility in the model", I want to think it'll make the PR easier to review. Hopefully it will tuck all those changes away in a commit with a clear intent.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/7146#discussion_r3513559371
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/7146/review/4618094977 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260702/6b04d37d/attachment.htm>
More information about the rails-dev
mailing list