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

[Pablo Brasero]

@pablobm commented on this pull request.



> @@ -294,6 +294,15 @@ def default_diary_language=(language)
     preference.update!(:v => language)
   end
 
+  def default_trace_visibility
+    visibility = preferences.find_by(:k => "gps.trace.visibility")&.v
+    if Trace.valid_visibility?(visibility)
+      visibility
+    else
+      "trackable"

I'd put this in `Trace.default_visibility` or similar.

> -      auth_header = bearer_authorization_header second_user
-      post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 0 }, :headers => auth_header
-      assert_response :success
-      trace = Trace.find(response.body.to_i)
-      assert_equal "a.gpx", trace.name
-      assert_equal "New Trace", trace.description
-      assert_equal %w[new trace], trace.tags.order(:tag).collect(&:tag)
-      assert_equal "private", trace.visibility
-      assert_not trace.inserted
-      assert_equal File.new(fixture).read, trace.file.blob.download
-      trace.destroy
-      assert_equal "private", second_user.preferences.find_by(:k => "gps.trace.visibility").v
+      assert_no_difference "Trace.count" do
+        post api_traces_path, :params => { :file => file, :description => "New Trace", :tags => "new,trace", :public => 0 }, :headers => auth_header
+      end
+      assert_response :bad_request

Since we are changing this, how about moving it to a separate test? `test_create_legacy_public_flag` or something like that. I find these long tests difficult to follow (although it already helped that you shortened it significantly).

Also how about a test for a legacy visibility? Just one is enough, no need for both. Also in a separate test if possible: `test_create_legacy_visibility`.

> @@ -427,6 +430,15 @@ def test_create_post
     assert_equal File.new(fixture).read, trace.file.blob.download
     trace.destroy
     assert_equal "trackable", user.preferences.find_by(:k => "gps.trace.visibility").v
+
+    # Rewind the file
+    file.rewind
+
+    # The legacy public and private values are no longer accepted on upload
+    assert_no_difference "Trace.count" do
+      post traces_path, :params => { :trace => { :gpx_file => file, :description => "New Trace", :tagstring => "new,trace", :visibility => "public" } }
+    end
+    assert_response :bad_request

Again, a good opportunity to put this in a separate test.

> @@ -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)

Actually, this is probably best moved to an `:on => :create` validation. Also avoids repeating it on the non-API controller.

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

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