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

[Tom Hughes]

@tomhughes commented on this pull request.

I'm a bit confused about the intended scope of "moderation zones" to be honest... I hadn't looked at this in much detail before but my understanding was that this was mostly creating a framework to define zones where various activities could be restricted, a bit like the existing ACL model but for geographic areas.

It appears that despite the generic name these zones are in fact tied explicitly to notes, so should they have a more explicit name? or alternatively something like a type field that indicates what is being moderated and then the note moderation is only applied to zones of the right type, or maybe even a separate one-many table that defines what restrictions are applied to each zone?

Other than that my main comments are that the commit structure doesn't look very good - quite a lot of the last three commits probably needs to be merged into the earlier commit where postgis is added to the database I think so that we don't have commits where, for example, lint can't possibly work.

I'd also question the split with most of the model being introduced in one commit and then the `belongs_to` rules being added in the next one and would move those into the first commit.

Finally "proof of concept" is a horribly generic commit message that doesn't tell us what it's doing at all.

> @@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class EnablePostgis < ActiveRecord::Migration[8.1]
+  def change
+    enable_extension "postgis"

A bigger problem is that enabling postgis requires a postgres superuser which means that, for example, this would fail in our production environment. Indeed it did just fail to deploy this branch to the dev server and I had to manually hack it.

> @@ -12,7 +12,7 @@ require:
   - ./.rubocop/specific_action_names.rb
 
 AllCops:
-  TargetRubyVersion: 3.2
+  TargetRubyVersion: 3.3

Yes but 3.3 is our baseline minimum supported ruby so that is what we test with.

> @@ -157,6 +161,10 @@ jobs:
         cache: yarn
     - name: Install node modules
       run: bundle exec bin/yarn install
+    - name: Install packages
+      run: |
+        sudo apt-get -yqq update
+        sudo apt-get -yqq install postgis

Because that linter is the database consistency check, so it needs to run the migrations to build the database, which means it needs postgis.

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

Message ID: <openstreetmap/openstreetmap-website/pull/6713/review/3739604147 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260429/2340f700/attachment.htm>