[openstreetmap/openstreetmap-website] Add moderation zones where anynomous notes are not allowed (PR #6713)

[Andy Allan]

@gravitystorm requested changes on this pull request.

This looks good to me, just a few minor comments inline.

The big topic here for me now is the ruby 3.3+ requirement. This means that developers can't install the codebase on the latest Ubuntu LTS, which we've traditionally supported for DX reasons. While experienced devs might be fine with ruby version managers, for many people it's a confusing nightmare of unclear choices, particularly if they aren't familiar with ruby. (I even had to change my local ruby version to test this PR!)

I can think of a few options:
* review the upstream gem to see if ruby 3.2 support is trivial? Long shot, but might be an easy option. I'd even consider using a fork for a few months if it's a minor delta.
* Move the ruby 3.3+ stuff into a separate PR, so that we can discuss/merge that bit first
* Wait for a few months until the next LTS is out and some of this stuff resolves itself!

Aside from that, I'm keen to hear if anyone else has review comments.

> @@ -19,8 +19,18 @@ def status
 
   # Raised when access is denied.
   class APIAccessDenied < APIError
-    def initialize
-      super("Access denied")
+    def initialize(message = "Access denied")
+      super
+    end
+
+    def status
+      :forbidden
+    end
+  end
+
+  class APIModerationZoneError < APIAccessDenied
+    def initialize(message = "Attempted to edit a Moderation Zone without enough trust")

I think the message needs rephrasing:

* The user did not "attempt to edit a moderation zone", it will be moderators who edit the zones
* I know what you mean by "without enough trust" but perhaps it's a little cryptic?

> @@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :coordinates, :class => Struct.new(:lat, :lon) do
+    trait :inside_seville_cathedral do

It's a little unusual to have a factory that doesn't represent a model. Would it be more clear if it was just a helper method within the relevant test file?

> @@ -0,0 +1,26 @@
+# frozen_string_literal: true
+
+FactoryBot.define do
+  factory :moderation_zone do
+    sequence(:name) { |n| "Zone block #{n}" }
+    sequence(:reason) { |n| "Reason for block #{n}" }

"reason for moderation zone"?

> @@ -0,0 +1,40 @@
+# frozen_string_literal: true
+
+# == Schema Information
+#
+# Table name: moderation_zones
+#
+#  id            :bigint           not null, primary key
+#  name          :string           not null
+#  reason        :string           not null
+#  reason_format :enum             default("markdown")
+#  zone          :st_geometry      not null, geometry, 0

The 0 at the end makes me think about projections. I'm not sure here whether an srid of 0 or 4326 would be better here.

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

Message ID: <openstreetmap/openstreetmap-website/pull/6713/review/3751759307 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260204/1060d85c/attachment-0001.htm>