[openstreetmap/openstreetmap-website] Allow users to change trace visibility in bulk (PR #7324)

Mon Aug 24 11:43:11 UTC 2026

@pablobm commented on this pull request.

> +  <p><%= t ".empty" %></p>
+<% else %>
+  <%= form_tag({ :action => :update_visibility }, :method => :patch) do %>
+    <%= hidden_field_tag :visibility, params[:visibility] %>
+    <%= hidden_field_tag :from, params[:from] %>
+    <%= hidden_field_tag :to, params[:to] %>
+
+    <div class="row g-3 align-items-end mb-3">
+      <div class="col-sm-4">
+        <%= label_tag :target, t(".apply.target"), :class => "form-label" %>
+        <%= select_tag :target,
+                       options_for_select(Trace::VISIBILITIES.map { |v| [t("traces.visibility.#{v}"), v] }, params[:target]),
+                       :class => "form-select" %>
+      </div>
+      <div class="col-sm-4">
+        <button type="button" class="btn btn-primary" data-bs-toggle="modal" data-bs-target="#confirm_visibility_change">

I think that this button should be marked `.btn-danger`. Same with the one in the modal.

> @@ -0,0 +1,70 @@
+<% content_for :heading do %>
+  <h1><%= @title %></h1>
+  <p class="text-body-secondary"><%= t ".description" %></p>
+<% end %>
+
+<%= form_tag({ :action => :change_visibility }, :method => :get, :class => "row g-3 align-items-end mb-3") do %>
+  <div class="col-sm-4">
+    <%= label_tag :visibility, t(".filter.visibility"), :class => "form-label" %>
+    <%= select_tag :visibility,
+                   options_for_select([[t(".filter.any"), ""]] + (Trace::VISIBILITIES + Trace::LEGACY_VISIBILITIES).map { |v| [t("traces.visibility.#{v}"), v] }, params[:visibility]),

I think we should only allow legacy visibilities to be selected here. If this is intended as a temporary facility, with the very specific aim to migrate away from legacy values, we should avoid giving any more functionality than that.

Unless we accept this is not temporary and we decide we actually want to provide this facility. This is a question for @Rub21, @1ec5, and the maintainers.

> @@ -11,6 +11,11 @@
           <%= link_to t(".remove_tag_filter", :tag => params[:tag]), { :controller => "traces", :action => "index", :tag => nil } %>
         </li>
       <% end %>
+      <% if current_user && current_user == @target_user %>
+        <li>
+          <%= link_to t(".change_visibility"), change_visibility_traces_path %>
+        </li>
+      <% end %>

I feel that this button is a bit hidden:

Also it's a non-standard location, next to the descriptor of the current tab. At the very least, I think it should be a new tab.

Additionally I wonder what the strategy is in terms of publicising this feature to users. Email, forum, modal, other? Does this link need to be more prominent? Perhaps nag users who to have remaining legacy traces? I don't mean to say it should be one way or the other, but to openly raise whether this is the way to go.

Having said that: this can be refined later.

> +      count = 0
+      Trace.transaction do
+        filtered_traces.find_each do |trace|
+          trace.update!(:visibility => target_visibility)
+          count += 1
+        end
+      end

To discuss: any reason not to do a plain update? This action is going to get very slow in the cases with hundreds/thousands of traces.

```suggestion
      count = filtered_traces.update_all(:visibility => target_visibility)
```

> +
+  def test_update_visibility_requires_login
+    patch update_visibility_traces_path, :params => { :target => "identifiable" }
+    assert_response :forbidden
+  end
+
+  def test_update_visibility
+    user = create(:user)
+    old_public = create(:trace, :without_validations, :visibility => "public", :user => user, :timestamp => Date.new(2015, 6, 1))
+    old_private = create(:trace, :without_validations, :visibility => "private", :user => user, :timestamp => Date.new(2019, 6, 1))
+    recent_public = create(:trace, :without_validations, :visibility => "public", :user => user, :timestamp => Date.new(2022, 6, 1))
+    other_users_trace = create(:trace, :without_validations, :visibility => "public")
+
+    session_for(user)
+    patch update_visibility_traces_path, :params => { :visibility => "public", :to => "2020-01-01", :target => "identifiable" }
+    assert_redirected_to change_visibility_traces_path(:visibility => "public", :from => nil, :to => "2020-01-01")

We should also test that the flash message shows the correct count.

>    private

+  # Traces belonging to the current user, narrowed down by the optional
+  # visibility/from/to filter params shared by #change_visibility and #update_visibility.
+  def filtered_traces
+    traces = current_user.traces
+    traces = traces.where(:visibility => params[:visibility]) if params[:visibility].present?
+    traces = traces.where(:timestamp => filter_from..) if filter_from
+    traces = traces.where(:timestamp => ...(filter_to + 1)) if filter_to

Would this work?

```suggestion
    traces = traces.where(:timestamp => ..filter_to) if filter_to
```

Not 100% sure. I might be missing something.

> @@ -170,8 +170,63 @@ def mine
     redirect_to :action => :index, :display_name => current_user.display_name
   end

+  # Lets a user preview which of their own traces match a visibility/date filter,
+  # before changing all of them to a new visibility in bulk via #update_visibility.
+  def change_visibility

This reads like an action to actually perform a change, when instead it's intended to list the traces. Moving this functionality to a separate controller/resource (as already mentioned in other comments) should fix that.

> @@ -0,0 +1,70 @@
+<% content_for :heading do %>
+  <h1><%= @title %></h1>
+  <p class="text-body-secondary"><%= t ".description" %></p>
+<% end %>
+
+<%= form_tag({ :action => :change_visibility }, :method => :get, :class => "row g-3 align-items-end mb-3") do %>
+  <div class="col-sm-4">
+    <%= label_tag :visibility, t(".filter.visibility"), :class => "form-label" %>
+    <%= select_tag :visibility,
+                   options_for_select([[t(".filter.any"), ""]] + (Trace::VISIBILITIES + Trace::LEGACY_VISIBILITIES).map { |v| [t("traces.visibility.#{v}"), v] }, params[:visibility]),

Also the first argument to `options_for_select` is more complex than I would like on a template. I would create a helper `trace_visibility_options_for_filter` or something like that. Same with the next one, perhaps `trace_visibility_options_for_update`.

> @@ -11,6 +11,11 @@
           <%= link_to t(".remove_tag_filter", :tag => params[:tag]), { :controller => "traces", :action => "index", :tag => nil } %>
         </li>
       <% end %>
+      <% if current_user && current_user == @target_user %>
+        <li>
+          <%= link_to t(".change_visibility"), change_visibility_traces_path %>
+        </li>
+      <% end %>

Additionally, "Change Visibility" may lead to users thinking that this is a general feature, rather than a "temporary" one with a very specific aim. Should we instead say something like "Migrate legacy traces", "Change visibility of legacy traces", or something like that?

Not only in this link though: in general the copy should highlight the intent of this feature wherever possible, with the consequences (no undo) beyond just the confirmation modal.

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

Message ID: <openstreetmap/openstreetmap-website/pull/7324/review/5007226373 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260824/a4deb33b/attachment.htm>