[openstreetmap/openstreetmap-website] Add microcosms to the website (#2390)

[Andy Allan]

gravitystorm requested changes on this pull request.

Some initial thoughts from me on various details - not comprehensive

> @@ -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

`safety_assured` 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.

On a production database, this could be a multi-step process of creating shadow columns, backfilling data and so on. See https://github.com/ankane/strong_migrations for more details. However, since this is going to be an empty table, that would be overkill. 

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 #1576, where even though there were substantial changes to the database during the development of the PR, there was in the end only one migration.

> @@ -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."

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.

> @@ -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!

model.save! (with an exclamation mark) raises an error if the save is unsuccessful. So here you can use `attendance.save` (without the exclamation mark) which returns a boolean. 

> @@ -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

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?

> @@ -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

We normally use either integers (e.g. for nodes) or floats (e.g. for user home locations).

> +#  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"]

This is, I think, a side effect of having multiple migrations, so can be removed after the migrations are combined.

> @@ -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

Could be an enum

> +      <%= @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" %>

We'll need to change this so that what the user sees is via i18n, while the value submitted doesn't change

> @@ -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>

Placeholder text needs updating

> @@ -0,0 +1,9 @@
+FactoryBot.define do
+  factory :event do
+    title { "MyString" }

Why is everything hardcoded in the factories?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/2390#pullrequestreview-301699031
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20191015/53480f10/attachment.html>