From e238fe98420f5bdcd5f31e404518b3229a7de114 Mon Sep 17 00:00:00 2001 From: Duncan Stuart Date: Fri, 22 Nov 2024 00:12:50 +0100 Subject: [PATCH 1/2] Increase size of screenshots These have always been too tiny to show the relevant detail - particularly with validation errors, where the screen goes back to the top. I don't think there's much cost to making the page big enough to fit the largest form. --- spec/support/system/drivers.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/support/system/drivers.rb b/spec/support/system/drivers.rb index d7a338c4..232d0f07 100644 --- a/spec/support/system/drivers.rb +++ b/spec/support/system/drivers.rb @@ -25,6 +25,7 @@ module Drivers config.before(:each, :js, type: :system) do driven_by :selenium_chrome_headless + current_window.resize_to(750, 1900) # 750x1900 is enough to fit the whole event form end end end From 364b305114975e5813e8344619933b068ce9558c Mon Sep 17 00:00:00 2001 From: Duncan Stuart Date: Fri, 22 Nov 2024 00:14:03 +0100 Subject: [PATCH 2/2] Validations: handle one-offs One of the editors has been doing a cludge to add one-off workshops by creating a weekly class, but then setting the first and last dates to the same date. This is not OK. There's also an odd edge case I hadn't considered: listing a social as weekly but making it a one-off by adding first and last dates the same: this should have been an occasional event. This isn't foolproof: if they just remove the start date then the event will be valid. Maybe weekly events with an end date in the next week should be invalid, but that feels like it's going down a path of madness. --- app/validators/valid_social_or_class.rb | 22 ++++++ spec/factories.rb | 15 ++++ .../events/form/validates_class_and_social.rb | 37 ++++++++++ .../events/validates_class_and_social.rb | 69 +++++++++++++++++++ .../validates_no_one_off_workshops.rb | 26 +++++++ 5 files changed, 169 insertions(+) create mode 100644 spec/support/shared_examples/validates_no_one_off_workshops.rb diff --git a/app/validators/valid_social_or_class.rb b/app/validators/valid_social_or_class.rb index 71525118..7c9d0605 100644 --- a/app/validators/valid_social_or_class.rb +++ b/app/validators/valid_social_or_class.rb @@ -4,6 +4,8 @@ class ValidSocialOrClass < ActiveModel::Validator def validate(event) socials_must_have_titles(event) classes_must_have_organisers(event) + one_off_socials_occasional(event) + no_one_off_workshops(event) end private @@ -20,4 +22,24 @@ def classes_must_have_organisers(event) event.errors.add(:class_organiser_id, "must be present for classes") end + + def one_off_socials_occasional(event) + return unless event.has_social? && event.weekly? && one_off(event) + + event.errors.add(:frequency, 'must be "Monthly or occasionally" if a social is only happening once') + end + + def no_one_off_workshops(event) + return unless event.has_class? && one_off(event) && !event.has_social? + + message = <<~MESSAGE.chomp + It looks like you're trying to list a one-off workshop. + Please don't do this: we only list weekly classes on SOLDN. + MESSAGE + event.errors.add(:base, message) + end + + def one_off(event) + event.first_date.present? && event.first_date == event.last_date + end end diff --git a/spec/factories.rb b/spec/factories.rb index 2c2a08fd..95a7f8bc 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -88,6 +88,21 @@ venue_id { rand(999) } end + trait(:social_dance) do + # default + end + + trait(:with_class) do + social_has_class { true } + class_organiser_id { rand(999) } + end + + trait(:weekly_class) do + title { "" } + event_type { "weekly_class" } + class_organiser_id { rand(999) } + end + factory :create_event_form do event_form end diff --git a/spec/support/shared_examples/events/form/validates_class_and_social.rb b/spec/support/shared_examples/events/form/validates_class_and_social.rb index 8e907bd2..dc2b59ec 100644 --- a/spec/support/shared_examples/events/form/validates_class_and_social.rb +++ b/spec/support/shared_examples/events/form/validates_class_and_social.rb @@ -1,6 +1,10 @@ # frozen_string_literal: true RSpec.shared_examples "validates class and social (form)" do |model_name| + include ActiveSupport::Testing::TimeHelpers + + before { travel_to Date.parse("2010-01-01") } + it "is valid if it's a class without a title" do expect(build(model_name, event_type: "weekly_class", title: nil, class_organiser_id: 7)).to be_valid end @@ -22,4 +26,37 @@ model.valid? expect(model.errors.messages).to eq(class_organiser_id: ["must be present for classes"]) end + + it "is invalid for socials with classes which are weekly but only happening on just one date" do + model = build(model_name, :social_dance, :with_class, :weekly, first_date: "2011-11-01", last_date: "2011-11-01") + + model.valid? + expect(model.errors.messages) + .to eq(frequency: ['must be "Monthly or occasionally" if a social is only happening once']) + end + + it "is invalid for socials with no classes which are weekly but only happening on just one date" do + model = build(model_name, :social_dance, :with_class, :weekly, first_date: "2011-11-01", last_date: "2011-11-01") + + model.valid? + expect(model.errors.messages) + .to eq(frequency: ['must be "Monthly or occasionally" if a social is only happening once']) + end + + it "is invalid for classes happening on just one date" do + model = build(model_name, :weekly_class, first_date: "2011-11-01", last_date: "2011-11-01") + + model.valid? + message = <<~MESSAGE.chomp + It looks like you're trying to list a one-off workshop. + Please don't do this: we only list weekly classes on SOLDN. + MESSAGE + expect(model.errors.messages).to eq(base: [message]) + end + + it "is valid for occasional socials with classes happening on just one date" do + model = build(model_name, :social_dance, :with_class, :occasional, first_date: "2011-11-01", last_date: "2011-11-01") + + expect(model).to be_valid + end end diff --git a/spec/support/shared_examples/events/validates_class_and_social.rb b/spec/support/shared_examples/events/validates_class_and_social.rb index c6278cdd..0bc44362 100644 --- a/spec/support/shared_examples/events/validates_class_and_social.rb +++ b/spec/support/shared_examples/events/validates_class_and_social.rb @@ -47,4 +47,73 @@ model.valid? expect(model.errors.messages).to eq(frequency: ["can't be blank"]) end + + it "is invalid for socials with classes which are weekly but only happening on just one date" do + model = build( + model_name, + :weekly, + has_taster: false, + has_social: true, + has_class: true, + class_organiser_id: 7, + first_date: "2011-11-01", + last_date: "2011-11-01" + ) + + model.valid? + expect(model.errors.messages) + .to eq(frequency: ['must be "Monthly or occasionally" if a social is only happening once']) + end + + it "is invalid for socials with no classes which are weekly but only happening on just one date" do + model = build( + model_name, + :weekly, + has_taster: false, + has_social: true, + has_class: false, + class_organiser_id: 7, + first_date: "2011-11-01", + last_date: "2011-11-01" + ) + + model.valid? + expect(model.errors.messages) + .to eq(frequency: ['must be "Monthly or occasionally" if a social is only happening once']) + end + + it "is invalid for classes happening on just one date" do + model = build( + model_name, + :weekly, + has_taster: false, + has_social: false, + has_class: true, + class_organiser_id: 7, + first_date: "2011-11-01", + last_date: "2011-11-01" + ) + + model.valid? + message = <<~MESSAGE.chomp + It looks like you're trying to list a one-off workshop. + Please don't do this: we only list weekly classes on SOLDN. + MESSAGE + expect(model.errors.messages).to eq(base: [message]) + end + + it "is valid for occasional socials with classes happening on just one date" do + model = build( + model_name, + :occasional, + has_taster: false, + has_social: true, + has_class: true, + class_organiser_id: 7, + first_date: "2011-11-01", + last_date: "2011-11-01" + ) + + expect(model).to be_valid + end end diff --git a/spec/support/shared_examples/validates_no_one_off_workshops.rb b/spec/support/shared_examples/validates_no_one_off_workshops.rb new file mode 100644 index 00000000..166fc3a9 --- /dev/null +++ b/spec/support/shared_examples/validates_no_one_off_workshops.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +RSpec.shared_examples "validates no one off workshops" do |model_name| + include ActiveSupport::Testing::TimeHelpers + + before { travel_to Date.parse("2010-01-01") } + + it "is invalid for classes happening on just one date" do + model = build(model_name, event_type: "weekly_class", first_date: "2011-11-01", last_date: "2011-11-01") + + model.valid? + expect(model.errors.messages).to eq(base: ["It looks like you're trying to list a one-off workshop"]) + end + + it "is valid with a valid http url" do + model = build(model_name, url: "http://foo.com") + + expect(model).to be_valid + end + + it "is invalid with url missing a scheme" do + model = build(model_name, url: "www.foo.com") + model.valid? + expect(model.errors.messages).to eq(url: ["is not a valid URI"]) + end +end