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

[Tom Hughes]

@tomhughes commented on this pull request.

I suspect the second commit, to change the default visibility, would be better done first before you start trying to deprecate the public and private visibilities.

> @@ -14,7 +14,7 @@
 #  timestamp   :datetime         not null
 #  description :string           default(""), not null
 #  inserted    :boolean          not null
-#  visibility  :enum             default("public"), not null
+#  visibility  :enum             default("trackable"), not null

There's something mixed up in your commit history - this comment is changing in the first commit before the database migration that would trigger it has been added.

> +  def trace_visibility_options(trace)
+    visibilities = Trace::VISIBILITIES.dup
+    visibilities.unshift(trace.visibility) if Trace.legacy_visibility?(trace.visibility)
+    visibilities.map do |visibility|
+      [t("traces.visibility.#{visibility}"), visibility]
+    end
+  end

Might this be better as a method on the Trace model? Given it is mostly accessing attributes of the model and a list of visibilities from the model and it's value is deliberately varying with the details of the model record?

Also this function has some comments added in the first commit that you then remove in the third commit which seems pointless.

> +    # A legacy visibility (public or private) is no longer accepted on upload
+    def test_create_legacy_visibility
+      fixture = Rails.root.join("test/gpx/fixtures/a.gpx")
+      file = Rack::Test::UploadedFile.new(fixture, "application/gpx+xml")
+      user = create(:user)
+      auth_header = bearer_authorization_header user
+
+      assert_no_difference "Trace.count" do
+        post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :visibility => "public" }, :headers => auth_header
+      end
+      assert_response :bad_request
     end

Shouldn't this test both legacy visibilities not just one of them? Note that this varies over time through the commits but in the first commit it is initially added as "public and private no longer supported" but only tests public.

>    validates :user, :associated => true
   validates :name, :presence => true, :length => 1..255, :characters => true
   validates :description, :presence => { :on => :create }, :length => 1..255, :characters => true
   validates :timestamp, :presence => true
-  validates :visibility, :inclusion => %w[private public trackable identifiable]
+  validates :visibility, :inclusion => VISIBILITIES + LEGACY_VISIBILITIES
+  validates :visibility, :inclusion => { :in => VISIBILITIES }, :on => :create

This is added in the second commit but wouldn't it be better placed in the first?

You also then wouldn't need to add a check in the controller in the first commit only to remove it again in the second.

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

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