[openstreetmap/openstreetmap-website] Microcosms microcosms (PR #3683)
Andy Allan
notifications at github.com
Wed Sep 14 12:06:06 UTC 2022
@gravitystorm requested changes on this pull request.
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.
There's a few things that stand out as also needing more thought on my side, beyond what I've put into the comments:
* 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
* 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?
* 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. `/foo/123-name-here`) for stable links.
* We need to think through the "one organizer for now" bit of this, and make sure we're not storing up trouble for later :smile: Either in terms of migrations or model relationships etc.
Thanks for all your work so far on this.
> @@ -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;
Does `h1` even have a top margin? Bootstrap by default doesn't since it avoids margin-top in the reboot.
https://getbootstrap.com/docs/5.1/content/reboot/
> @@ -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;
form labels should receive the default styling to match the rest of the site
> @@ -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 {
consider using bootstrap utility classes for lists rather than custom css.
> +.mic_details {
+ h1 {
+ margin-top: 0;
+ }
+ label {
+ font-weight: bold;
+ }
+ ul {
+ display: inline-block;
+ }
+ ul > li {
+ display: inline-block;
+ }
+}
+
+#microcosm_map, #microcosm_map_form {
consider using the existing `.content_map` 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
> @@ -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?
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.
> + 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 : []
Don't use "I" to refer to the current user. "I" am a developer who is reading the code :smile:
> +
+ 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
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
> + 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
I don't think this should be a public method, those normally only correspond to routes.
> + 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)
Could this normalisation be done inside the microcosm model?
> + validates :site, :presence => true, :length => 1..255, :characters => true
+ validates :url, :presence => true, :length => 1..255, :url => { :schemes => ["https"] }
What's a `site` in this context? Is it the "name" of the Link?
> @@ -0,0 +1,9 @@
+<p>
+ All fields are required.
1) 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.
> @@ -0,0 +1 @@
+There are no items in this list.
Internationalization
> @@ -0,0 +1,33 @@
+<%= stylesheet_link_tag "microcosms" %>
+<%= javascript_include_tag "microcosms" %>
+
+<p>
+ All fields are required.
See above
> @@ -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 %>
Please use the built-in form help system. This approach breaks i18n. For an example see the `en.activerecord.help` translation strings, these show on forms automatically.
> + # 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
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.
> @@ -0,0 +1,5 @@
+class RemoveKeyFromMicrocosms < ActiveRecord::Migration[7.0]
+ def change
+ safety_assured { remove_column :microcosms, :key, :string }
As per previous comment - this should all be in the initial migration
> + 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)
Aha! That answers my question about the "site" attribute. So I suspect it does need renaming.
> + # 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"
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.
"example1.com" is a legitimate domain, with content - it's not a reserved domain name.
> @@ -332,6 +332,19 @@ def xml_node_for_relation(relation)
el
end
+ def check_page_basics
+ assert_response :success
+ assert_no_missing_translations
This is normally controlled by `config/environments/test.rb` which contains `config.i18n.raise_on_missing_translations = true` - is the `assert_no_missing_translations` doing something different here?
> @@ -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]
Does it makes sense to nest these? e.g. have
```
resources :microcosms do
resources :microcosm_links, :except => [:show]
end
```
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 `:new` 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.
> @@ -323,6 +323,11 @@
# redactions
resources :redactions
+ # microcosms
+ get "/microcosms/of_user/:display_name" => "microcosms#of_user", :as => :microcosms_of_user
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).
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.
```
resources :users, ..., ..., do
resources :microcosms, :only => :index
end
```
That way we end up with urls like `/user/Foo/microcosms`.
This "of_user" page is a list of microcosms, if I understand correctly? If so, then `#index` is usually the right action name for a list of things.
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 `/user/Foo/microcosms` for a list of microcosms that that user is a member of. If so tweaking the url is fine e.g. `/user/Foo/organized_microcosms` or whatever you think best.
> +# 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"
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).
So I would recommend sticking with `user_id` here.
> @@ -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
Avoid the use of "I" here, as elsewhere. A user has many microcosms that *they* organise. I think something like `has_many :microcosms, ...` (or has_many :organized_microcosms, ...` could be better) but I'm open to suggestions.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3683#pullrequestreview-1107235207
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/pull/3683/review/1107235207 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20220914/23e18e0c/attachment-0001.htm>
More information about the rails-dev
mailing list