Event create and edit use form objects

I'd like to decouple the form more from the model in order to provide a
better UX with form fields which don't exist on the model.

A first step is getting the existing functionality working with form
objects.

Note that on Edit we print the dates rather than returning the date
array from the model.

get rid of usage of date array - this hasn't actually been what we use
for making the listings for a while now.

Building the URL separately based on @event is a tradeoff: without this
we need to somehow get the event ID into the form so that `form_with`
can do its thing and infer which URL to use, but I couldn't find an
approach for that which works well:

  - adding an extra initialisation parameter breaks the validation specs
    which are expecting to be able to `build` an activemodel instance
    with FactoryBot
  - we could add ID as an attribute and set it in the validation error
    case on update, but that feels awkward.

Note that we need to monkeypatch Shoulda Matchers to run it without
ActiveRecord. This is NOT a "responsible" way of monkeypatching, but I
couldn't get it to work and all the examples I can find online are about
monkeypatching instance methods not class methods. Raised an issue on
the Shoulda Matchers repo here:

  thoughtbot/shoulda-matchers#1553

Gemfile.lock

@@ -345,7 +345,7 @@ GEM
       rexml (~> 3.2, >= 3.2.5)
       rubyzip (>= 1.2.2)
     sexp_processor (4.16.1)
-    shoulda-matchers (5.1.0)
+    shoulda-matchers (5.3.0)
       activesupport (>= 5.2.0)
     simplecov (0.21.2)
       docile (~> 1.1)

app/controllers/events_controller.rb

@@ -13,20 +13,21 @@ def show
   end
 
   def new
-    venue = Venue.find_by(id: params[:venue_id])
-    @event = Event.new(venue:)
+    @form = CreateEventForm.new(venue_id: params[:venue_id])
   end
 
   def edit
     @event = Event.find(params[:id])
+    @form = EditEventForm.from_event(@event)
   end
 
   def create
-    @event = Event.new(event_params)
+    @form = CreateEventForm.new(event_params)
 
-    if @event.save
+    if @form.valid?
+      event = Event.create!(@form.to_h)
       flash[:notice] = t("flash.success", model: "Event", action: "created")
-      redirect_to(@event)
+      redirect_to(event)
     else
       render action: "new"
     end
@@ -35,10 +36,11 @@ def create
   def update
     @event = Event.find(params[:id])
 
-    audit_comment = EventParamsCommenter.new.comment(@event, event_params)
-    update_params = event_params.merge!(audit_comment)
-
-    if @event.update(update_params)
+    @form = EditEventForm.new(event_params)
+    if @form.valid?
+      audit_comment = EventParamsCommenter.new.comment(@event, event_params)
+      update_params = @form.to_h.merge!(audit_comment)
+      @event.update!(update_params)
       flash[:notice] = t("flash.success", model: "Event", action: "updated")
       redirect_to(@event)
     else
@@ -78,8 +80,8 @@ def event_params
         course_length
         day
         frequency
-        date_array
-        cancellation_array
+        dates
+        cancellations
         first_date
         last_date
         url

app/forms/create_event_form.rb

@@ -0,0 +1,69 @@
+# frozen_string_literal: true
+
+class CreateEventForm
+  include ActiveModel::Model
+  include ActiveModel::Attributes
+
+  attribute :title, :string
+  attribute :url, :string
+  attribute :venue_id, :integer
+  attribute :event_type, :string
+
+  attribute :has_social, :boolean
+  attribute :social_organiser_id, :integer
+
+  attribute :has_class, :boolean
+  attribute :has_taster, :boolean
+  attribute :class_style, :string
+  attribute :course_length, :string
+  attribute :class_organiser_id, :integer
+
+  attribute :frequency, :integer
+  attribute :day, :string
+  attribute :dates, :string
+  attribute :cancellations, :string
+  attribute :first_date, :string
+  attribute :last_date, :string
+
+  def self.model_name
+    Event.model_name
+  end
+
+  validates :url, presence: true, uri: true
+  validates :venue_id, presence: true
+  validates :event_type, presence: true
+  validates :frequency, presence: true
+  validates :course_length, numericality: { only_integer: true, greater_than: 0, allow_blank: true }
+
+  validates_with ValidSocialOrClass
+  validates_with ValidWeeklyEvent
+
+  def action
+    "Create"
+  end
+
+  def weekly?
+    frequency == 1
+  end
+
+  def infrequent?
+    return false if frequency.nil?
+
+    frequency.zero? || frequency >= 4
+  end
+
+  def has_social? # rubocop:disable Naming/PredicateName
+    !!has_social
+  end
+
+  def has_class? # rubocop:disable Naming/PredicateName
+    !!has_class
+  end
+
+  def to_h
+    attributes.merge(
+      "dates" => DatesStringParser.new.parse(dates),
+      "cancellations" => DatesStringParser.new.parse(cancellations)
+    )
+  end
+end

app/forms/edit_event_form.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+
+class EditEventForm < CreateEventForm
+  include ActiveModel::Model
+  include ActiveModel::Attributes
+
+  class << self
+    def from_event(event)
+      new(
+        {
+          title: event.title,
+          url: event.url,
+          venue_id: event.venue_id,
+          event_type: event.event_type,
+
+          has_social: event.has_social,
+          social_organiser_id: event.social_organiser_id,
+
+          has_class: event.has_class,
+          has_taster: event.has_taster,
+          class_style: event.class_style,
+          course_length: event.course_length,
+          class_organiser_id: event.class_organiser_id,
+
+          frequency: event.frequency,
+          day: event.day,
+          dates: event.print_dates,
+          cancellations: event.print_cancellations,
+          first_date: event.first_date,
+          last_date: event.last_date
+        }
+      )
+    end
+  end
+
+  def action
+    "Update"
+  end
+
+  def persisted?
+    true
+  end
+end

app/helpers/listings_helper.rb

@@ -122,7 +122,7 @@ def postcode_link(postcode_string, map_url = nil)
 
   # Return a span containing a message about cancelled dates:
   def swingclass_cancelledmsg(swingclass)
-    return "" if swingclass.cancellation_array(future: true).empty?
+    return "" if swingclass.future_cancellations.empty?
 
     date_printer = DatePrinter.new(separator: ", ", format: :short_date)
     cancellations = date_printer.print(swingclass.future_cancellations)

app/models/event.rb

@@ -144,10 +144,6 @@ def dates=(array_of_new_dates)
     array_of_new_dates.each { |nd| add_date(nd) }
   end
 
-  def date_array=(date_string)
-    self.dates = DatesStringParser.new.parse(date_string)
-  end
-
   def cancellations
     swing_cancellations.collect(&:date)
   end
@@ -161,24 +157,10 @@ def add_cancellation(new_cancellation)
     swing_cancellations << SwingDate.find_or_initialize_by(date: new_cancellation)
   end
 
-  def cancellation_array=(date_string)
-    self.cancellations = DatesStringParser.new.parse(date_string)
-  end
-
   def future_cancellations
     filter_future(cancellations)
   end
 
-  # READ METHODS #
-
-  def date_array(future: false)
-    return_array_of_dates(dates, future:)
-  end
-
-  def cancellation_array(future: false)
-    return_array_of_dates(cancellations, future:)
-  end
-
   private
 
   # Given an array of dates, return an appropriately filtered array

app/services/event_params_commenter.rb

@@ -17,19 +17,23 @@ def comment(event, update_params)
   private
 
   def updated_dates_comment(event, update_params)
-    comment_text("Updated dates", event.print_dates, update_params[:date_array])
+    comment_text("Updated dates", event.print_dates, update_params[:dates])
   end
 
   def updated_cancellations_comment(event, update_params)
-    comment_text("Updated cancellations", event.print_cancellations, update_params[:cancellation_array])
+    comment_text("Updated cancellations", event.print_cancellations, update_params[:cancellations])
   end
 
   def changed_dates(event, update_params)
-    (event.print_dates != update_params[:date_array])
+    return false if update_params[:dates].nil?
+
+    (event.print_dates != update_params[:dates])
   end
 
   def changed_cancellations(event, update_params)
-    (event.print_cancellations != update_params[:cancellation_array])
+    return false if update_params[:cancellations].nil?
+
+    (event.print_cancellations != update_params[:cancellations])
   end
 
   def comment_text(message, old, new)

app/validators/valid_weekly_event.rb

@@ -15,8 +15,8 @@ def weekly_events_must_have_day(event)
   end
 
   def cannot_be_weekly_and_have_dates(event)
-    return unless event.weekly? && !event.dates.empty?
+    return unless event.weekly? && event.dates.present?
 
-    event.errors.add(:date_array, "must be empty for weekly events")
+    event.errors.add(:dates, "must be empty for weekly events")
   end
 end

app/views/events/_event_listing_table_row.html.erb

@@ -30,7 +30,7 @@
     <td class="actions last">
       <%= link_to 'Show', event %>
       <%= link_to 'Delete', event, data: { confirm: 'Are you sure you want to delete this event?' }, method: :delete %>
-      <%= link_to 'Archive', archive_event_path(event), :method => :put %>
+      <%= link_to 'Archive', archive_event_path(event), data: { "turbo-method" => :put } %>
     </td>
   </tr>
 <% end %>

app/views/events/_form.html.erb

@@ -1,5 +1,5 @@
-<%= form_with(model: @event, class: "event-form") do |f| %>
-  <%= render "shared/error_messages", target: @event %>
+<%= form_with(model: @form, url: submit_path, class: "event-form") do |f| %>
+  <%= render "shared/error_messages", target: @form %>
 
   <section>
     <div class='form-group'>
@@ -24,7 +24,7 @@
     </div>
   </section>
 
-  <% if @event.persisted? %>
+  <% if @form.persisted? %>
     <%= render "events/organiser_link", event: @event %>
   <% end %>
 
@@ -66,14 +66,14 @@
     <div class='form-group' id="class-style-selection">
       <h4 class='form-group-title'>Class style</h3>
       <div class='radio-group'>
-        <%= radio_button_tag 'class_style_option', 'lindy', @event.class_style.blank? %>
+        <%= radio_button_tag 'class_style_option', 'lindy', @form.class_style.blank? %>
         <%= label_tag :class_style_option_lindy, t('forms.events.default_class_style') %>
       </div>
       <div class='radio-group'>
-        <%= radio_button_tag 'class_style_option', 'other', !@event.class_style.blank?, data: { target: 'class-style-group' }%>
+        <%= radio_button_tag 'class_style_option', 'other', !@form.class_style.blank?, data: { target: 'class-style-group' }%>
         <%= label_tag 'class_style_option_other', 'Other (balboa, shag etc)' %>
       </div>
-      <div class='form-group form-group-nested <%= 'hidden' if @event.class_style.blank? %>' id="class-style-group">
+      <div class='form-group form-group-nested <%= 'hidden' if @form.class_style.blank? %>' id="class-style-group">
         <%= f.label :class_style, "Dance style:" %>
         <%= f.text_field :class_style %>
       </div>
@@ -96,23 +96,23 @@
         <%= f.radio_button(:frequency, 0, data: { action: "event-frequency#setInfrequently" }) %>
         Monthly or occasionally
       </label>
-      <% if @event.persisted? && [0, 1].exclude?(@event.frequency) %>
-        <p class="help">Legacy frequency: <%= @event.frequency %></p>
+      <% if @form.persisted? && [0, 1].exclude?(@form.frequency) %>
+        <p class="help">Legacy frequency: <%= @form.frequency %></p>
       <% end %>
     </div>
-    <div class='form-group <%= "hidden" unless @event.weekly? %>' data-event-frequency-target="daySelect">
+    <div class='form-group <%= "hidden" unless @form.weekly? %>' data-event-frequency-target="daySelect">
       <%= f.label :day %>
       <%= f.select :day, DAYNAMES, include_blank: true %>
     </div>
-    <div class='form-group <%= "hidden" unless @event.infrequent? %>' data-event-frequency-target="datesInput">
-      <%= f.label :date_array, 'Upcoming dates' %>
+    <div class='form-group <%= "hidden" unless @form.infrequent? %>' data-event-frequency-target="datesInput">
+      <%= f.label :dates, 'Upcoming dates' %>
       <p class='help'><%= t('forms.help.dates') %></p>
-      <%= f.text_field :date_array, value: @event.print_dates %>
+      <%= f.text_field :dates, value: @form.dates %>
     </div>
     <div class='form-group'>
-      <%= f.label :cancellation_array, 'Cancelled dates' %>
+      <%= f.label :cancellations, 'Cancelled dates' %>
       <p class='help'><%= t('forms.help.dates') %></p>
-      <%= f.text_field :cancellation_array, value: @event.print_cancellations %>
+      <%= f.text_field :cancellations, value: @form.cancellations %>
     </div>
     <div class='form-group'>
       <%= f.label :first_date %>
@@ -121,17 +121,17 @@
         A <span class="new-label">NEW!</span> label will be shown on the listing
         for <%= distance_of_time_in_words(Event::CONSIDERED_NEW_FOR) %> after this date.
       </p>
-      <%= f.text_field :first_date, value: @event.first_date.to_s, size: 10, class: "shortfield" %>
+      <%= f.text_field :first_date, value: @form.first_date.to_s, size: 10, class: "shortfield" %>
     </div>
     <div class='form-group'>
       <%= f.label :last_date %>
       <p class='help'>The event will stop being included in the listings after this date</p>
-      <%= f.text_field :last_date, value: @event.last_date.to_s, size: 10, class: "shortfield" %>
+      <%= f.text_field :last_date, value: @form.last_date.to_s, size: 10, class: "shortfield" %>
     </div>
   </section>
 
   <div class='form-group actions'>
-    <%= f.submit action, class: 'button' %>
+    <%= f.submit @form.action, class: 'button' %>
     <%= link_to 'Cancel', cancel_path, class: "button button-secondary" %>
   </div>
 <% end %>

app/views/events/edit.html.erb

@@ -1,3 +1,3 @@
 <h1>Editing event</h1>
 
-<%=render partial: "form", locals: { action: "Update", cancel_path: event_path(@event) } %>
+<%= render "form", submit_path: event_path(@event), cancel_path: event_path(@event) %>

app/views/events/new.html.erb

@@ -1,3 +1,3 @@
 <h1>New event</h1>
 
-<%=render partial: "form", locals: { action: "Create", cancel_path: events_path } %>
+<%= render "form", submit_path: events_path, cancel_path: events_path %>

spec/controllers/events_controller_spec.rb

@@ -32,26 +32,4 @@
       expect(assigns[:warning]).to be_nil
     end
   end
-
-  describe "GET new" do
-    context "when a venue id is provided which matches a venue" do
-      it "creates an event at that venue" do
-        venue = create(:venue, id: 23)
-        get :new, params: { venue_id: 23 }
-        expect(assigns(:event).venue).to eq venue
-      end
-    end
-
-    context "when a venue id is provided which doesn't match a venue" do
-      [
-        23,
-        nil
-      ].each do |vid|
-        it "creates an event with a null venue" do
-          get :new, params: { venue_id: vid }
-          expect(assigns(:event).venue).to be_nil
-        end
-      end
-    end
-  end
 end