@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/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
