<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<p dir="auto">I've had a first pass at reviewing this PR, but even at 52 files it's still a lot of stuff in one go.</p>
<p dir="auto">There's a few things that stand out as also needing more thought on my side, beyond what I've put into the comments:</p>
<ul dir="auto">
<li>Is there any way to simplify the PR further. For example, perhaps we can cut out the issue reporting for now, and add it as a follow up PR</li>
<li>Given that we might want to decouple reviewing PRs incrementally from launching the whole feature, do we need to put this behind a feature flag?</li>
<li>I haven't reviewed the friendly_id stuff, it seems to add a lot of complexity and doesn't follow the "id-friendly-text" format that I would expect (e.g. <code class="notranslate">/foo/123-name-here</code>) for stable links.</li>
<li>We need to think through the "one organizer for now" bit of this, and make sure we're not storing up trouble for later <g-emoji class="g-emoji" alias="smile" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f604.png">😄</g-emoji> Either in terms of migrations or model relationships etc.</li>
</ul>
<p dir="auto">Thanks for all your work so far on this.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970640347">app/assets/stylesheets/microcosms.scss</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,22 @@
+// Place all the styles related to the Microcosms controller here.
+// They will automatically be included in application.css.
+// You can use Sass (SCSS) here: https://sass-lang.com/
+
+.mic_details {
+ h1 {
+ margin-top: 0;
</pre>
<p dir="auto">Does <code class="notranslate">h1</code> even have a top margin? Bootstrap by default doesn't since it avoids margin-top in the reboot.</p>
<p dir="auto"><a href="https://getbootstrap.com/docs/5.1/content/reboot/" rel="nofollow">https://getbootstrap.com/docs/5.1/content/reboot/</a></p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970640796">app/assets/stylesheets/microcosms.scss</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,22 @@
+// Place all the styles related to the Microcosms controller here.
+// They will automatically be included in application.css.
+// You can use Sass (SCSS) here: https://sass-lang.com/
+
+.mic_details {
+ h1 {
+ margin-top: 0;
+ }
+ label {
+ font-weight: bold;
</pre>
<p dir="auto">form labels should receive the default styling to match the rest of the site</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970643825">app/assets/stylesheets/microcosms.scss</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,22 @@
+// Place all the styles related to the Microcosms controller here.
+// They will automatically be included in application.css.
+// You can use Sass (SCSS) here: https://sass-lang.com/
+
+.mic_details {
+ h1 {
+ margin-top: 0;
+ }
+ label {
+ font-weight: bold;
+ }
+ ul {
</pre>
<p dir="auto">consider using bootstrap utility classes for lists rather than custom css.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970643876">app/assets/stylesheets/microcosms.scss</a>:</p>
<pre style='color:#555'>> +.mic_details {
+ h1 {
+ margin-top: 0;
+ }
+ label {
+ font-weight: bold;
+ }
+ ul {
+ display: inline-block;
+ }
+ ul > li {
+ display: inline-block;
+ }
+}
+
+#microcosm_map, #microcosm_map_form {
</pre>
<p dir="auto">consider using the existing <code class="notranslate">.content_map</code> class, which has responsive breakpoints. Maps in forms should have the same styling as existing maps in forms (e.g. on the user profile page) for consistency</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970647053">app/controllers/microcosms_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,94 @@
+class MicrocosmsController < ApplicationController
+ layout "site"
+ before_action :authorize_web
+
+ before_action :set_microcosm, :only => [:edit, :show, :update]
+
+ helper_method :recent_changesets
+
+ load_and_authorize_resource :except => [:create, :new]
+ authorize_resource
+
+ def index
+ # TODO: OMG is the math right here?
</pre>
<p dir="auto">I've no idea if this maths is correct, but it's certainly impenetrable! Needs to be moved to a method (with an self-explanatory name), both to make it easier to test and also easier to understand what you are trying to do.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970648274">app/controllers/microcosms_controller.rb</a>:</p>
<pre style='color:#555'>> + before_action :set_microcosm, :only => [:edit, :show, :update]
+
+ helper_method :recent_changesets
+
+ load_and_authorize_resource :except => [:create, :new]
+ authorize_resource
+
+ def index
+ # TODO: OMG is the math right here?
+ minute_of_day = "(60 * EXTRACT(HOUR FROM current_timestamp) + EXTRACT(MINUTE FROM current_timestamp))"
+ morning = "(60 * 6)" # 6 AM
+ long_facing_sun = "(#{minute_of_day} + #{morning}) / 4"
+ # Using Arel.sql here because we're using known-safe values.
+ @microcosms = Microcosm.order(Arel.sql("longitude + 180 + #{long_facing_sun} DESC"))
+
+ @microcosms_i_organize = current_user ? current_user.microcosms_i_organize : []
</pre>
<p dir="auto">Don't use "I" to refer to the current user. "I" am a developer who is reading the code <g-emoji class="g-emoji" alias="smile" fallback-src="https://github.githubassets.com/images/icons/emoji/unicode/1f604.png">😄</g-emoji></p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970649500">app/controllers/microcosms_controller.rb</a>:</p>
<pre style='color:#555'>> +
+ load_and_authorize_resource :except => [:create, :new]
+ authorize_resource
+
+ def index
+ # TODO: OMG is the math right here?
+ minute_of_day = "(60 * EXTRACT(HOUR FROM current_timestamp) + EXTRACT(MINUTE FROM current_timestamp))"
+ morning = "(60 * 6)" # 6 AM
+ long_facing_sun = "(#{minute_of_day} + #{morning}) / 4"
+ # Using Arel.sql here because we're using known-safe values.
+ @microcosms = Microcosm.order(Arel.sql("longitude + 180 + #{long_facing_sun} DESC"))
+
+ @microcosms_i_organize = current_user ? current_user.microcosms_i_organize : []
+ end
+
+ def of_user
</pre>
<p dir="auto">If this can't be covered by the standard index/edit/update then it that indicates that the method should be in a different controller. I'll discuss more in a comment on the routes.rb file</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970652021">app/controllers/microcosms_controller.rb</a>:</p>
<pre style='color:#555'>> + def new
+ @title = t "microcosms.new.title"
+ @microcosm = Microcosm.new
+ end
+
+ def create
+ @microcosm = Microcosm.new(microcosm_params)
+ @microcosm.organizer = current_user
+ if @microcosm.save
+ redirect_to @microcosm, :notice => t(".success")
+ else
+ render "new"
+ end
+ end
+
+ def recent_changesets
</pre>
<p dir="auto">I don't think this should be a public method, those normally only correspond to routes.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970653499">app/controllers/microcosms_controller.rb</a>:</p>
<pre style='color:#555'>> + private
+
+ def set_microcosm
+ @microcosm = Microcosm.friendly.find(params[:id])
+ end
+
+ def microcosm_params
+ normalize_longitude(params[:microcosm])
+ params.require(:microcosm).permit(
+ :name, :location, :latitude, :longitude,
+ :min_lat, :max_lat, :min_lon, :max_lon,
+ :description
+ )
+ end
+
+ def normalize_longitude(microcosm_params)
</pre>
<p dir="auto">Could this normalisation be done inside the microcosm model?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970659624">app/models/microcosm_link.rb</a>:</p>
<pre style='color:#555'>> + validates :site, :presence => true, :length => 1..255, :characters => true
+ validates :url, :presence => true, :length => 1..255, :url => { :schemes => ["https"] }
</pre>
<p dir="auto">What's a <code class="notranslate">site</code> in this context? Is it the "name" of the Link?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970661047">app/views/microcosm_links/_form.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,9 @@
+<p>
+ All fields are required.
</pre>
<ol dir="auto">
<li>internationalisation 2) so long as the model has marked them as required, there's generally no need for additional text. I personally don't think we're clear enough on indicating required vs optional form fields right now, but that should be dealt with outside this PR across the whole site.</li>
</ol>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970661579">app/views/microcosms/_empty_list.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1 @@
+There are no items in this list.
</pre>
<p dir="auto">Internationalization</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970661736">app/views/microcosms/_form.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,33 @@
+<%= stylesheet_link_tag "microcosms" %>
+<%= javascript_include_tag "microcosms" %>
+
+<p>
+ All fields are required.
</pre>
<p dir="auto">See above</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970663496">app/views/microcosms/_form.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,33 @@
+<%= stylesheet_link_tag "microcosms" %>
+<%= javascript_include_tag "microcosms" %>
+
+<p>
+ All fields are required.
+</p>
+<%= bootstrap_form_for @microcosm do |form| %>
+ <%= form.text_field :name, :id => :microcosm_name %>
+ <%= form.text_field :location, :help => "for example city name and country", :id => :microcosm_location %>
</pre>
<p dir="auto">Please use the built-in form help system. This approach breaks i18n. For an example see the <code class="notranslate">en.activerecord.help</code> translation strings, these show on forms automatically.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970666430">db/migrate/20220820220849_add_slug_to_microcosms.rb</a>:</p>
<pre style='color:#555'>> + # This migration will be run at the same time as the migration to create
+ # microcosms, so there will be no records yet.
+ safety_assured do
</pre>
<p dir="auto">This should be combined into the initial migration. It's hard to review a PR that creates a database table and also contains lots of changes to that same table.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970667167">db/migrate/20220820221326_remove_key_from_microcosms.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,5 @@
+class RemoveKeyFromMicrocosms < ActiveRecord::Migration[7.0]
+ def change
+ safety_assured { remove_column :microcosms, :key, :string }
</pre>
<p dir="auto">As per previous comment - this should all be in the initial migration</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970670692">test/models/microcosm_test.rb</a>:</p>
<pre style='color:#555'>> + site_name = "site_name"
+ site_url = "https://example.com"
+ m = create(:microcosm)
+ # act
+ m.set_link(site_name, site_url)
+ # assert
+ ml = MicrocosmLink.find_by(:microcosm_id => m.id, :site => site_name)
+ assert_equal ml.url, site_url
+ end
+
+ def test_set_link_that_already_exists
+ # arrange
+ m = create(:microcosm)
+ site_name = "site_name"
+ site_url_old = "https://example1.com"
+ MicrocosmLink.new(:microcosm => m, :site => site_name, :url => site_url_old)
</pre>
<p dir="auto">Aha! That answers my question about the "site" attribute. So I suspect it does need renaming.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970674531">test/models/microcosm_test.rb</a>:</p>
<pre style='color:#555'>> + # arrange
+ site_name = "site_name"
+ site_url = "https://example.com"
+ m = create(:microcosm)
+ # act
+ m.set_link(site_name, site_url)
+ # assert
+ ml = MicrocosmLink.find_by(:microcosm_id => m.id, :site => site_name)
+ assert_equal ml.url, site_url
+ end
+
+ def test_set_link_that_already_exists
+ # arrange
+ m = create(:microcosm)
+ site_name = "site_name"
+ site_url_old = "https://example1.com"
</pre>
<p dir="auto">This is an utterly trivial nitpick, but we should use RFC 6761 reserved domain names ("example.com", "example.net" etc) in our tests. To make the test clear, you can use descriptive subdomains like "old.example.com", "new.example.com" or similar.</p>
<p dir="auto">"example1.com" is a legitimate domain, with content - it's not a reserved domain name.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970677490">test/test_helper.rb</a>:</p>
<pre style='color:#555'>> @@ -332,6 +332,19 @@ def xml_node_for_relation(relation)
el
end
+ def check_page_basics
+ assert_response :success
+ assert_no_missing_translations
</pre>
<p dir="auto">This is normally controlled by <code class="notranslate">config/environments/test.rb</code> which contains <code class="notranslate">config.i18n.raise_on_missing_translations = true</code> - is the <code class="notranslate">assert_no_missing_translations</code> doing something different here?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970684525">config/routes.rb</a>:</p>
<pre style='color:#555'>> @@ -323,6 +323,11 @@
# redactions
resources :redactions
+ # microcosms
+ get "/microcosms/of_user/:display_name" => "microcosms#of_user", :as => :microcosms_of_user
+ resources :microcosms
+ resources :microcosm_links, :except => [:show]
</pre>
<p dir="auto">Does it makes sense to nest these? e.g. have</p>
<pre class="notranslate"><code class="notranslate">resources :microcosms do
resources :microcosm_links, :except => [:show]
end
</code></pre>
<p dir="auto">I was wondering that when I saw the stuff around making sure that the microcosm_link form has a hidden value, and then you need to provide that value in the <code class="notranslate">:new</code> method in the links controller, and guard against it missing etc etc. Since you aren't choosing the microcosm in the microcosm_link form, it feels like this might be a sub-resource for microcosms. That way whenever you are creating a link, the microcosm id is in the url already.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970690896">config/routes.rb</a>:</p>
<pre style='color:#555'>> @@ -323,6 +323,11 @@
# redactions
resources :redactions
+ # microcosms
+ get "/microcosms/of_user/:display_name" => "microcosms#of_user", :as => :microcosms_of_user
</pre>
<p dir="auto">This should be rethought to be a resourceful route, which will probably help figure out what the action name should be too (see comment in the controller).</p>
<p dir="auto">Given that this is a list of micrososm for a particular user, I think it would make sense to have it as a nested resource of the user resource, e.g.</p>
<pre class="notranslate"><code class="notranslate">resources :users, ..., ..., do
resources :microcosms, :only => :index
end
</code></pre>
<p dir="auto">That way we end up with urls like <code class="notranslate">/user/Foo/microcosms</code>.</p>
<p dir="auto">This "of_user" page is a list of microcosms, if I understand correctly? If so, then <code class="notranslate">#index</code> is usually the right action name for a list of things.</p>
<p dir="auto">I also recognise that this could be just the first stage for microcosms, in that you might want to have users joining microcosms in future, and reserve the url <code class="notranslate">/user/Foo/microcosms</code> for a list of microcosms that that user is a member of. If so tweaking the url is fine e.g. <code class="notranslate">/user/Foo/organized_microcosms</code> or whatever you think best.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970694340">app/models/microcosm.rb</a>:</p>
<pre style='color:#555'>> +# latitude :float not null
+# longitude :float not null
+# min_lat :float not null
+# max_lat :float not null
+# min_lon :float not null
+# max_lon :float not null
+#
+# At this time a microcosm has one organizer. The first organizer is
+# the user that created the microcosm.
+
+class Microcosm < ApplicationRecord
+ extend FriendlyId
+ friendly_id :name, :use => :slugged
+ self.ignored_columns = ["key"]
+
+ belongs_to :organizer, :class_name => "User"
</pre>
<p dir="auto">We should avoid having to have class name overrides etc whenever possible. We do it elsewhere (e.g. author_id for changeset_comments) and it's totally not worth the additional complexity. I think the only place it makes sense is when there would otherwise be two user_ids (e.g. in Messages).</p>
<p dir="auto">So I would recommend sticking with <code class="notranslate">user_id</code> here.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3683#discussion_r970711751">app/models/user.rb</a>:</p>
<pre style='color:#555'>> @@ -83,6 +83,8 @@ class User < ApplicationRecord
has_many :reports
+ has_many :microcosms_i_organize, :class_name => "Microcosm", :foreign_key => :organizer_id, :inverse_of => :organizer
</pre>
<p dir="auto">Avoid the use of "I" here, as elsewhere. A user has many microcosms that <em>they</em> organise. I think something like <code class="notranslate">has_many :microcosms, ...</code> (or has_many :organized_microcosms, ...` could be better) but I'm open to suggestions.</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/3683#pullrequestreview-1107235207">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLONFIGFIRYD7DOCZC3V6G5S5ANCNFSM6AAAAAAQJPEGAY">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLNPQZUEWTNWAV2EKRLV6G5S5A5CNFSM6AAAAAAQJPEGA2WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTSB74IYO.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/3683/review/1107235207</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/3683#pullrequestreview-1107235207",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/3683#pullrequestreview-1107235207",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>