[openstreetmap/openstreetmap-website] Communities - communities (PR #3717)

[Andy Allan]

@gravitystorm requested changes on this pull request.

This is only a partial review, since it's a big PR and I've run out of time today.

A few other thoughts here as a reminder for myself for the next review:
* Resolve communities vs communities name clash
* Consider if communities is already overloaded in the wider osm community e.g. community.osm.org etc
* list of commits still has fixup commits
* javascript etc unreviewed
* can this be slimmed down further e.g. move clever sorting to followup PR etc

> @@ -147,7 +147,7 @@
   get "/help" => "site#help"
   get "/about/:about_locale" => "site#about"
   get "/about" => "site#about"
-  get "/communities" => "site#communities"
+  get "/community_index" => "site#communities"

We need to think carefully about doing this, since it would break all external links to the existing communities page.

> @@ -321,6 +321,17 @@
   # redactions
   resources :redactions
 
+  # user nested resources
+  resources :users, :path => "user", :param => :display_name do
+    resources :communities, :only => [:index]
+  end

This could be combined with the existing `resources :users` statement, so that `resources :users` isn't defined twice

> +  def test_edit_get_no_session
+    # arrange
+    c = create(:community)
+    # act
+    get edit_community_path(c)
+    # assert
+    assert_response :redirect
+    assert_redirected_to login_path(:referer => edit_community_path(c))
+  end

I don't think the hundreds of repeated `# arrange`, `# act`, `# assert` comments are helpful - they are just more text to read and maintain (e.g. when refactoring tests).

I suggest removing the `# arrange` comments entirely, and changing the other two to blank lines. For example:

```ruby
  def test_edit_get_no_session
    c = create(:community)

    get edit_community_path(c)

    assert_response :redirect
    assert_redirected_to login_path(:referer => edit_community_path(c))
  end
```
This keeps the readability of having the three sections being distinct from one another, but with less clutter.

> +#
+#  id           :bigint(8)        not null, primary key
+#  community_id :integer          not null
+#  site         :string           not null
+#  url          :string           not null
+#  created_at   :datetime         not null
+#  updated_at   :datetime         not null
+#
+# Indexes
+#
+#  index_community_links_on_community_id  (community_id)
+#
+
+class CommunityLink < ApplicationRecord
+  belongs_to :community
+  validates :site, :presence => true, :length => 1..255, :characters => true

>From previous review, "site" here is not clear. Perhaps one of: name, label, text. 

(You previously suggested "link" but I think that's even more confusing with "link" vs "url"!)

> @@ -8,7 +8,21 @@ en:
   helpers:
     file:
       prompt: Choose file
+    label:
+      community:
+        description: "Description"
+        lat: "Latitude"

I thought the model had attributes of "latitude", not "lat"? Same for lon.

>      submit:
+      community:
+        create: "Create Community"

Isn't "Create %{model_name}" the default here?

> @@ -527,4 +527,9 @@ def self.http_client
   def self.maxmind_database
     @maxmind_database ||= MaxMindDB.new(Settings.maxmind_database) if Settings.key?(:maxmind_database)
   end
+
+  # Convert any longitude input to the range -180..180.
+  def self.normalize_longitude(longitude_in)

just `longitude` since there's no other longitude in this function

> @@ -59,6 +71,15 @@ class UserAbilityTest < AbilityTest
       assert ability.cannot?(action, Issue), "should not be able to #{action} Issues"
     end
   end
+
+  # This does not take into account object level access control.

note to self: should it?

> +    lat1 = Random.rand(-90.0..90.0)
+    lat2 = Random.rand(-90.0..90.0)
+    lat1, lat2 = lat2, lat1 if lat2 < lat1

A more compact version, using sort:

`lat1, lat2 = [Random.rand(-90.0..90.0), Random.rand(-90.0..90.0)].sort`

To avoid duplicating the random number bit, you can use an Array construction with a block, but it's perhaps less readable? Probably a personal preference between these two.

`lat1, lat2 = Array.new(2) { Random.rand(-90.0..90.0) }.sort`


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

Message ID: <openstreetmap/openstreetmap-website/pull/3717/review/1467949237 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230607/b5c1e38a/attachment.htm>