<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<p dir="auto">This is only a partial review, since it's a big PR and I've run out of time today.</p>
<p dir="auto">A few other thoughts here as a reminder for myself for the next review:</p>
<ul dir="auto">
<li>Resolve communities vs communities name clash</li>
<li>Consider if communities is already overloaded in the wider osm community e.g. community.osm.org etc</li>
<li>list of commits still has fixup commits</li>
<li>javascript etc unreviewed</li>
<li>can this be slimmed down further e.g. move clever sorting to followup PR etc</li>
</ul><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221769433">config/routes.rb</a>:</p>
<pre style='color:#555'>> @@ -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"
</pre>
<p dir="auto">We need to think carefully about doing this, since it would break all external links to the existing communities page.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221776171">config/routes.rb</a>:</p>
<pre style='color:#555'>> @@ -321,6 +321,17 @@
# redactions
resources :redactions
+ # user nested resources
+ resources :users, :path => "user", :param => :display_name do
+ resources :communities, :only => [:index]
+ end
</pre>
<p dir="auto">This could be combined with the existing <code class="notranslate">resources :users</code> statement, so that <code class="notranslate">resources :users</code> isn't defined twice</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221800538">test/controllers/communities_controller_test.rb</a>:</p>
<pre style='color:#555'>> + 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
</pre>
<p dir="auto">I don't think the hundreds of repeated <code class="notranslate"># arrange</code>, <code class="notranslate"># act</code>, <code class="notranslate"># assert</code> comments are helpful - they are just more text to read and maintain (e.g. when refactoring tests).</p>
<p dir="auto">I suggest removing the <code class="notranslate"># arrange</code> comments entirely, and changing the other two to blank lines. For example:</p>
<div class="highlight highlight-source-ruby" dir="auto"><pre class="notranslate"> <span class="pl-k">def</span> <span class="pl-en">test_edit_get_no_session</span>
<span class="pl-s1">c</span> <span class="pl-c1">=</span> <span class="pl-en">create</span><span class="pl-kos">(</span><span class="pl-pds">:community</span><span class="pl-kos">)</span>
<span class="pl-en">get</span> <span class="pl-en">edit_community_path</span><span class="pl-kos">(</span><span class="pl-s1">c</span><span class="pl-kos">)</span>
<span class="pl-en">assert_response</span> <span class="pl-pds">:redirect</span>
<span class="pl-en">assert_redirected_to</span> <span class="pl-en">login_path</span><span class="pl-kos">(</span><span class="pl-pds">:referer</span> <span class="pl-c1">=></span> <span class="pl-en">edit_community_path</span><span class="pl-kos">(</span><span class="pl-s1">c</span><span class="pl-kos">)</span><span class="pl-kos">)</span>
<span class="pl-k">end</span></pre></div>
<p dir="auto">This keeps the readability of having the three sections being distinct from one another, but with less clutter.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221819988">app/models/community_link.rb</a>:</p>
<pre style='color:#555'>> +#
+# 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
</pre>
<p dir="auto">From previous review, "site" here is not clear. Perhaps one of: name, label, text.</p>
<p dir="auto">(You previously suggested "link" but I think that's even more confusing with "link" vs "url"!)</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221822949">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> @@ -8,7 +8,21 @@ en:
helpers:
file:
prompt: Choose file
+ label:
+ community:
+ description: "Description"
+ lat: "Latitude"
</pre>
<p dir="auto">I thought the model had attributes of "latitude", not "lat"? Same for lon.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221826147">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> submit:
+ community:
+ create: "Create Community"
</pre>
<p dir="auto">Isn't "Create %{model_name}" the default here?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221830005">lib/osm.rb</a>:</p>
<pre style='color:#555'>> @@ -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)
</pre>
<p dir="auto">just <code class="notranslate">longitude</code> since there's no other longitude in this function</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221830752">test/abilities/abilities_test.rb</a>:</p>
<pre style='color:#555'>> @@ -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.
</pre>
<p dir="auto">note to self: should it?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#discussion_r1221848986">test/factories/communities.rb</a>:</p>
<pre style='color:#555'>> + lat1 = Random.rand(-90.0..90.0)
+ lat2 = Random.rand(-90.0..90.0)
+ lat1, lat2 = lat2, lat1 if lat2 < lat1
</pre>
<p dir="auto">A more compact version, using sort:</p>
<p dir="auto"><code class="notranslate">lat1, lat2 = [Random.rand(-90.0..90.0), Random.rand(-90.0..90.0)].sort</code></p>
<p dir="auto">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.</p>
<p dir="auto"><code class="notranslate">lat1, lat2 = Array.new(2) { Random.rand(-90.0..90.0) }.sort</code></p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3717#pullrequestreview-1467949237">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLMHYFKKWFIY3DKRTZLXKCVIRANCNFSM6AAAAAAQUYXU54">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLJWY24QQLMDZND4HPDXKCVIRA5CNFSM6AAAAAAQUYXU56WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTSXP4QLK.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/3717/review/1467949237</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/3717#pullrequestreview-1467949237",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/3717#pullrequestreview-1467949237",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>