@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/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
