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