<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<p>Some initial thoughts from me on various details - not comprehensive</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334788646">db/migrate/20190903030453_add_location_to_microcosms.rb</a>:</p>
<pre style='color:#555'>> @@ -1,13 +1,16 @@
class AddLocationToMicrocosms < ActiveRecord::Migration[5.2]
def change
- change_table "microcosms", :bulk => true do |t|
- t.string "location", :null => false
- t.decimal "lat", :null => false
- t.decimal "lon", :null => false
- t.integer "min_lat", :null => false
- t.integer "max_lat", :null => false
- t.integer "min_lon", :null => false
- t.integer "max_lon", :null => false
+ # This group of migrations for microcosms will be run together.
+ safety_assured do
</pre>
<p><code>safety_assured</code> doesn't work like this! It's a warning that this migration could lock the table (potentially for hours, if there's lots of data) and so you need to find a different approach.</p>
<p>On a production database, this could be a multi-step process of creating shadow columns, backfilling data and so on. See <a href="https://github.com/ankane/strong_migrations">https://github.com/ankane/strong_migrations</a> for more details. However, since this is going to be an empty table, that would be overkill.</p>
<p>So the best approach for pull requests is to avoid including multiple database migrations. Sure, during development you might realise that the initial database structure was incorrect, and add a fixup migration like this one. But for the PR, please make one migration with your preferred structure. For example, see <a class="issue-link js-issue-link" data-error-text="Failed to load issue title" data-id="240686497" data-permission-text="Issue title is private" data-url="https://github.com/openstreetmap/openstreetmap-website/issues/1576" data-hovercard-type="pull_request" data-hovercard-url="/openstreetmap/openstreetmap-website/pull/1576/hovercard" href="https://github.com/openstreetmap/openstreetmap-website/pull/1576">#1576</a>, where even though there were substantial changes to the database during the development of the PR, there was in the end only one migration.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334790993">app/controllers/event_attendances_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,50 @@
+class EventAttendancesController < ApplicationController
+ layout "site"
+ before_action :authorize_web
+ before_action :set_event_attendance, :only => [:update]
+
+ authorize_resource
+
+ def create
+ attendance = EventAttendance.new(attendance_params)
+ attendance.intention = intention
+ if attendance.save!
+ redirect_to event_path(attendance.event), :notice => "Attendance was successfully saved."
</pre>
<p>All these strings need to be through the i18n system. I find it best to do so right from the start, since it's time consuming to go back through all the controllers and views later on.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334792119">app/controllers/event_attendances_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,50 @@
+class EventAttendancesController < ApplicationController
+ layout "site"
+ before_action :authorize_web
+ before_action :set_event_attendance, :only => [:update]
+
+ authorize_resource
+
+ def create
+ attendance = EventAttendance.new(attendance_params)
+ attendance.intention = intention
+ if attendance.save!
</pre>
<p>model.save! (with an exclamation mark) raises an error if the save is unsuccessful. So here you can use <code>attendance.save</code> (without the exclamation mark) which returns a boolean.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334792894">app/models/event_attendance.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,21 @@
+# == Schema Information
+#
+# Table name: event_attendances
+#
+# id :bigint(8) not null, primary key
+# user_id :integer not null
+# event_id :integer not null
+# intention :string not null
</pre>
<p>I see "Yes" and "No" as the values for the intention. Are there likely to be other values? Would an enum be more useful here?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334794673">app/models/microcosm.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,43 @@
+# == Schema Information
+#
+# Table name: microcosms
+#
+# id :bigint(8) not null, primary key
+# name :string not null
+# description :text
+# created_at :datetime not null
+# updated_at :datetime not null
+# slug :string not null
+# location :string not null
+# lat :decimal(, ) not null
+# lon :decimal(, ) not null
</pre>
<p>We normally use either integers (e.g. for nodes) or floats (e.g. for user home locations).</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334797484">app/models/microcosm.rb</a>:</p>
<pre style='color:#555'>> +# created_at :datetime not null
+# updated_at :datetime not null
+# slug :string not null
+# location :string not null
+# lat :decimal(, ) not null
+# lon :decimal(, ) not null
+# min_lat :integer not null
+# max_lat :integer not null
+# min_lon :integer not null
+# max_lon :integer not null
+#
+
+class Microcosm < ActiveRecord::Base
+ extend FriendlyId
+ friendly_id :name, :use => :slugged
+ self.ignored_columns = ["key"]
</pre>
<p>This is, I think, a side effect of having multiple migrations, so can be removed after the migrations are combined.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334797814">app/models/microcosm_member.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,30 @@
+# == Schema Information
+#
+# Table name: microcosm_members
+#
+# id :bigint(8) not null, primary key
+# microcosm_id :integer not null
+# user_id :integer not null
+# role :string(64) not null
</pre>
<p>Could be an enum</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334799154">app/views/events/show.html.erb</a>:</p>
<pre style='color:#555'>> + <%= @event.title %>
+ </h1>
+ <p>
+ Hosted by <strong><%= link_to @event.microcosm.name, microcosm_path(@event.microcosm) %></strong>
+ </p>
+ <p>
+ Organized by: <strong>Andrew Wiseman</strong>
+ </p>
+ </div>
+ <div>
+ <p><%= @event.attendees.size %> people are going.</p>
+ <p>Are you going?</p>
+ <%= form_with :model => @my_attendance do |form| %>
+ <%= form.hidden_field(:event_id, :value => @event.id) %>
+ <%= form.hidden_field(:user_id, :value => current_user&.id) %>
+ <%= form.submit :value => "Yes" %>
</pre>
<p>We'll need to change this so that what the user sees is via i18n, while the value submitted doesn't change</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334799250">app/views/events/show.html.erb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,49 @@
+<div class="flex_row">
+ <div>
+ <div class="when">
+ <%= l @event.moment, :format => :friendly %>
+ </div>
+
+ <h1 class="title">
+ <%= @event.title %>
+ </h1>
+ <p>
+ Hosted by <strong><%= link_to @event.microcosm.name, microcosm_path(@event.microcosm) %></strong>
+ </p>
+ <p>
+ Organized by: <strong>Andrew Wiseman</strong>
</pre>
<p>Placeholder text needs updating</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390#discussion_r334802092">test/factories/events.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,9 @@
+FactoryBot.define do
+ factory :event do
+ title { "MyString" }
</pre>
<p>Why is everything hardcoded in the factories?</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/2390?email_source=notifications&email_token=AAK2OLJZD2OFGFGKCRGNTSLQOVZ7LA5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCH5Y7VY#pullrequestreview-301699031">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLJ4M3U24XPOJJX4PM3QOVZ7LANCNFSM4JAHYW4Q">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AAK2OLJ2KUKRHJG3OPJ4TP3QOVZ7LA5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCH5Y7VY.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/2390?email_source=notifications\u0026email_token=AAK2OLJZD2OFGFGKCRGNTSLQOVZ7LA5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCH5Y7VY#pullrequestreview-301699031",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/2390?email_source=notifications\u0026email_token=AAK2OLJZD2OFGFGKCRGNTSLQOVZ7LA5CNFSM4JAHYW42YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCH5Y7VY#pullrequestreview-301699031",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>