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

[Andy Allan]

@gravitystorm requested changes on this pull request.

Initial thoughts inline.

On a general overview, I'm happy with the scope of this initial PR, and then following up with a separate PR to sort out the "detection api" topic, then actually creating the zones via a UI. 

> @@ -89,6 +89,8 @@ def create
       lat = OSM.parse_float(params[:lat], OSM::APIBadUserInput, "lat was not a number")
       description = params[:text]
 
+      raise OSM::APIAccessDenied if current_user.nil? && GeoblockZone.falls_within_any?(:lon => lon, :lat => lat)

I suspect we'll need a separate error. Otherwise it's hard for data consumers to know what sort of error message to present.

I note here that in future we'll probably want to narrow the exemption list to moderators and admins rather than all signed in users.

> @@ -6,7 +6,8 @@ source "https://rubygems.org"
 gem "rails", "~> 8.1.0"
 gem "turbo-rails"
 
-# Use postgres as the database
+# Use postgres+postgis as the database
+gem "activerecord-postgis-adapter"

You asked about which gem to use, but I don't have a strong opinion on this, other people might have more experience with the alternative gem. It would be good to check if they are easily interchangeable - i.e. is the reason for their difference more to do with monkeypatching, or do they also have e.g. differences in how to write the queries?

> +# Table name: geoblock_zones
+#
+#  id         :bigint           not null, primary key
+#  name       :string
+#  zone       :geography        geometry, 4326
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#
+class GeoblockZone < ApplicationRecord
+  def self.falls_within_any?(opts = {})
+    lon = opts.fetch(:lon)
+    lat = opts.fetch(:lat)
+
+    GeoblockZone.exists?(
+      [
+        # FIXME: do proper interpolation of lon/lat

I think the fixme would be to write the queries using arel. If the st_covers isn't part of either gem, then now would be a good time to report that upstream and see if that can be fixed - maybe it will be available before we go further.

> +#  id         :bigint           not null, primary key
+#  name       :string
+#  zone       :geography        geometry, 4326
+#  created_at :datetime         not null
+#  updated_at :datetime         not null
+#
+class GeoblockZone < ApplicationRecord
+  def self.falls_within_any?(opts = {})
+    lon = opts.fetch(:lon)
+    lat = opts.fetch(:lat)
+
+    GeoblockZone.exists?(
+      [
+        # FIXME: do proper interpolation of lon/lat
+        <<~SQL.squish
+          ST_Covers(

I don't have strong opinions on st_covers vs st_contains in our context - the difference between the two is not relevant here. The bigger difference is geometry vs geography support, see notes elsewhere.

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

This will need the relevant "postgis" packages installed, both in CI but also in Docker, devcontainers and the Install notes. I don't think we need a full-blown extra github action for it though, especially because the one you mention does things to the db (e.g. enabling the extension) that we don't want to be done externally.

> @@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class CreateGeoblockZones < ActiveRecord::Migration[8.1]
+  def change
+    create_table :geoblock_zones do |t|
+      t.string :name
+      t.geometry :zone, :geographic => true

We want to get geography vs geometry sorted out here, and this is something I'd like feedback from other people on.

I lean slightly towards geometry, for both theoretical and practical reasons. For example, let's say we draw a rectangular geoblock_zone, with four corners and a top corners at 70°N. For a geometry, the top edge is along the 70°N parallel. But for a geography, the edges of shapes are defined by great-circle arcs between the corners, and this might be unexpected behaviour. Halfway between the top corners, the arc could include 72°N, for example, or go over the pole if the corners are 180° longitude apart. Is this way moderators and mappers would expect?

We (generally?) treat OSM coordinates as a rectangular grid, and ways are (typically?) drawn as straight lines on a plane, rather than great-circle arcs.

So geography is a valid option, but I'm not sure if the extra complexity is worth it. One example of complexity is that many fewer PostGIS functions are implemented for geography compared to geometry.

> @@ -0,0 +1,12 @@
+# frozen_string_literal: true
+
+class CreateGeoblockZones < ActiveRecord::Migration[8.1]
+  def change
+    create_table :geoblock_zones do |t|
+      t.string :name
+      t.geometry :zone, :geographic => true
+

I would consider mirroring the `user_blocks` table here, and add creator_id, and revoker_id here. We will want to have them in at the start, since we will want to "not null" those columns at some point.

We have "reason" in user_blocks, should we use name, reason, or both for geoblocks? We also have ends_at with user_blocks, which makes sure that all blocks automatically end, without moderators having to revisit them to end them manually.

> @@ -202,6 +202,14 @@ def test_create_anonymous_fail
         end
       end
       assert_response :bad_request
+
+      create_geoblock_zone

Should be a different test, rather than adding to a megatest.

> @@ -1254,5 +1276,28 @@ def assert_notes_in_order(notes)
         assert_select "osm>note:nth-child(#{index + 1})>id", :text => note.id.to_s, :count => 1
       end
     end
+
+    def create_geoblock_zone

Should be part of the factory, perhaps as a named trait

> @@ -0,0 +1,33 @@
+# frozen_string_literal: true
+
+require "test_helper"
+
+class GeoblockZoneTest < ActiveSupport::TestCase
+  def test_falls_within_any
+    GeoblockZone.create(

needs a factory

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

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