From c7fee933993bcef9c86da46dbe12a8b4deb48972 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Mon, 22 Jul 2024 08:34:11 -0600 Subject: [PATCH 01/10] Replaces reminder_day with reminder_schedule Via db migrations, replaces the simple integer `reminder_day` with the more complex `reminder_schedule` which is an ical string that can be parsed to a repeating Schedule class. Adjusts the logic in the `fetch_partners_to_reminder_now_service` to use the repeating schedule. Updates related tests. --- Gemfile | 2 + Gemfile.lock | 3 +- .../admin/organizations_controller.rb | 2 +- app/controllers/organizations_controller.rb | 2 +- app/controllers/partner_groups_controller.rb | 2 +- app/models/concerns/deadlinable.rb | 37 +++++++++++++-- app/models/organization.rb | 2 +- app/models/partner_group.rb | 18 +++---- .../fetch_partners_to_remind_now_service.rb | 19 ++++++-- ..._add_reminder_schedule_to_organizations.rb | 6 +++ ...40715162837_seed_reminder_schedule_data.rb | 17 +++++++ ..._remove_reminder_day_from_organizations.rb | 6 +++ db/schema.rb | 7 ++- spec/factories/organizations.rb | 9 ++-- spec/factories/partner_groups.rb | 24 ++++++---- spec/mailers/reminder_deadline_mailer_spec.rb | 3 +- spec/models/concerns/deadlinable_spec.rb | 26 ++++++---- spec/models/organization_spec.rb | 21 +++++---- spec/models/partner_group_spec.rb | 26 +++++----- ...tch_partners_to_remind_now_service_spec.rb | 47 +++++++++++++++---- 20 files changed, 198 insertions(+), 81 deletions(-) create mode 100644 db/migrate/20240715155823_add_reminder_schedule_to_organizations.rb create mode 100644 db/migrate/20240715162837_seed_reminder_schedule_data.rb create mode 100644 db/migrate/20240715163348_remove_reminder_day_from_organizations.rb diff --git a/Gemfile b/Gemfile index 1305f90f1c..f7c949e50c 100644 --- a/Gemfile +++ b/Gemfile @@ -92,6 +92,8 @@ gem "geocoder" gem 'httparty' # Generate .ics calendars for use with Google Calendar gem 'icalendar', require: false +# Offers functionality for date reocccurances +gem "ice_cube" # JSON Web Token encoding / decoding (e.g. for links in e-mails) gem "jwt" # Use Newrelic for logs and APM diff --git a/Gemfile.lock b/Gemfile.lock index facf9953d0..e685bd0739 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -731,6 +731,7 @@ DEPENDENCIES guard-rspec httparty icalendar + ice_cube image_processing importmap-rails (~> 2.0) jbuilder @@ -781,4 +782,4 @@ DEPENDENCIES webmock (~> 3.23) BUNDLED WITH - 2.5.14 + 2.5.15 diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb index 11313338e7..5eaf22993f 100644 --- a/app/controllers/admin/organizations_controller.rb +++ b/app/controllers/admin/organizations_controller.rb @@ -82,7 +82,7 @@ def destroy def organization_params params.require(:organization) - .permit(:name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, :reminder_day, :deadline_day, + .permit(:name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, :reminder_schedule, :deadline_day, users_attributes: %i(name email organization_admin), account_request_attributes: %i(ndbn_member_id id)) end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index 2a0fbf99d3..1675d0958b 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -92,7 +92,7 @@ def organization_params :name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_storage_location, :default_email_text, - :invitation_text, :reminder_day, :deadline_day, + :invitation_text, :reminder_schedule, :deadline_day, :repackage_essentials, :distribute_monthly, :ndbn_member_id, :enable_child_based_requests, :enable_individual_requests, :enable_quantity_based_requests, diff --git a/app/controllers/partner_groups_controller.rb b/app/controllers/partner_groups_controller.rb index f54339d95c..bc1734e83f 100644 --- a/app/controllers/partner_groups_controller.rb +++ b/app/controllers/partner_groups_controller.rb @@ -33,6 +33,6 @@ def update private def partner_group_params - params.require(:partner_group).permit(:name, :send_reminders, :deadline_day, :reminder_day, item_category_ids: []) + params.require(:partner_group).permit(:name, :send_reminders, :deadline_day, :reminder_schedule, item_category_ids: []) end end diff --git a/app/models/concerns/deadlinable.rb b/app/models/concerns/deadlinable.rb index dfb35c6d64..608b0d36d9 100644 --- a/app/models/concerns/deadlinable.rb +++ b/app/models/concerns/deadlinable.rb @@ -1,15 +1,44 @@ module Deadlinable extend ActiveSupport::Concern - MIN_DAY_OF_MONTH = 1 MAX_DAY_OF_MONTH = 28 included do validates :deadline_day, numericality: {only_integer: true, less_than_or_equal_to: MAX_DAY_OF_MONTH, greater_than_or_equal_to: MIN_DAY_OF_MONTH, allow_nil: true} - validates :reminder_day, numericality: {only_integer: true, less_than_or_equal_to: MAX_DAY_OF_MONTH, - greater_than_or_equal_to: MIN_DAY_OF_MONTH, allow_nil: true} + validate :reminder_on_deadline_day? + validate :reminder_schedule_is_within_range? + end + + def convert_to_reminder_schedule(day) + schedule = IceCube::Schedule.new + schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(day) + schedule.to_ical + end + + private + + def reminder_on_deadline_day? + if reminder_schedule.nil? + return + end + + schedule = IceCube::Schedule.from_ical reminder_schedule + if schedule.first.day == deadline_day + errors.add(:reminder_schedule, "Reminder must not be the same as deadline date") + end + end - validates :reminder_day, numericality: {other_than: :deadline_day}, if: :deadline_day? + def reminder_schedule_is_within_range? + if reminder_schedule.nil? + return + end + schedule = IceCube::Schedule.from_ical reminder_schedule + reminder_day = schedule.first.day + # IceCube converts negative or zero days to valid days (e.g. -1 becomes the last day of the month, 0 becomes 1) + # The minimum check should no longer be necessary, but keeping it in case IceCube changes + if reminder_day < 0 || reminder_day > MAX_DAY_OF_MONTH + errors.add(:reminder_schedule, "Reminder day must be between #{MIN_DAY_OF_MONTH} and #{MAX_DAY_OF_MONTH}") + end end end diff --git a/app/models/organization.rb b/app/models/organization.rb index cd03a56f2a..c092ca65fc 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -20,7 +20,7 @@ # name :string # one_step_partner_invite :boolean default(FALSE), not null # partner_form_fields :text default([]), is an Array -# reminder_day :integer +# reminder_schedule :string saved in iCal format, eg "RRULE:FREQ=MONTHLY;BYMONTHDAY=14" # repackage_essentials :boolean default(FALSE), not null # short_name :string # signature_for_distribution_pdf :boolean default(FALSE) diff --git a/app/models/partner_group.rb b/app/models/partner_group.rb index e7ac55c8fd..fa8efa225c 100644 --- a/app/models/partner_group.rb +++ b/app/models/partner_group.rb @@ -2,14 +2,14 @@ # # Table name: partner_groups # -# id :bigint not null, primary key -# deadline_day :integer -# name :string -# reminder_day :integer -# send_reminders :boolean default(FALSE), not null -# created_at :datetime not null -# updated_at :datetime not null -# organization_id :bigint +# id :bigint not null, primary key +# deadline_day :integer +# name :string +# reminder_schedule :string saved in iCal format, eg "RRULE:FREQ=MONTHLY;BYMONTHDAY=14" +# send_reminders :boolean default(FALSE), not null +# created_at :datetime not null +# updated_at :datetime not null +# organization_id :bigint # class PartnerGroup < ApplicationRecord has_paper_trail @@ -21,5 +21,5 @@ class PartnerGroup < ApplicationRecord validates :organization, presence: true validates :name, presence: true, uniqueness: { scope: :organization } - validates :deadline_day, :reminder_day, presence: true, if: :send_reminders? + validates :deadline_day, :reminder_schedule, presence: true, if: :send_reminders? end diff --git a/app/services/partners/fetch_partners_to_remind_now_service.rb b/app/services/partners/fetch_partners_to_remind_now_service.rb index 0f112b76d5..f61187f140 100644 --- a/app/services/partners/fetch_partners_to_remind_now_service.rb +++ b/app/services/partners/fetch_partners_to_remind_now_service.rb @@ -1,22 +1,31 @@ module Partners class FetchPartnersToRemindNowService def fetch - current_day = Time.current.day + current_day = Time.current deactivated_status = ::Partner.statuses[:deactivated] partners_with_group_reminders = ::Partner.left_joins(:partner_group) - .where(partner_groups: {reminder_day: current_day}) + .where.not(partner_groups: {reminder_schedule: nil}) .where.not(partner_groups: {deadline_day: nil}) .where.not(status: deactivated_status) + # where partner groups have reminder schedule match + filtered_partner_groups = partners_with_group_reminders.select do |partner| + sched = IceCube::Schedule.from_ical partner.partner_group.reminder_schedule + sched.occurs_on?(current_day) + end + partners_with_only_organization_reminders = ::Partner.left_joins(:partner_group, :organization) - .where(partner_groups: {reminder_day: nil}) + .where(partner_groups: {reminder_schedule: nil}) .where(send_reminders: true) - .where(organizations: {reminder_day: current_day}) .where.not(organizations: {deadline_day: nil}) .where.not(status: deactivated_status) - (partners_with_group_reminders + partners_with_only_organization_reminders).flatten.uniq + filtered_organizations = partners_with_only_organization_reminders.select do |partner| + sched = IceCube::Schedule.from_ical partner.organization.reminder_schedule + sched.occurs_on?(current_day) + end + (filtered_partner_groups + filtered_organizations).flatten.uniq end end end diff --git a/db/migrate/20240715155823_add_reminder_schedule_to_organizations.rb b/db/migrate/20240715155823_add_reminder_schedule_to_organizations.rb new file mode 100644 index 0000000000..4da938f248 --- /dev/null +++ b/db/migrate/20240715155823_add_reminder_schedule_to_organizations.rb @@ -0,0 +1,6 @@ +class AddReminderScheduleToOrganizations < ActiveRecord::Migration[7.1] + def change + add_column :organizations, :reminder_schedule, :string + add_column :partner_groups, :reminder_schedule, :string + end +end diff --git a/db/migrate/20240715162837_seed_reminder_schedule_data.rb b/db/migrate/20240715162837_seed_reminder_schedule_data.rb new file mode 100644 index 0000000000..5fdc96bbe7 --- /dev/null +++ b/db/migrate/20240715162837_seed_reminder_schedule_data.rb @@ -0,0 +1,17 @@ +class SeedReminderScheduleData < ActiveRecord::Migration[7.1] + def change + for o in Organization.all + if o.reminder_day.present? + reminder_schedule = o.convert_to_reminder_schedule(o.reminder_day) + o.update(reminder_schedule: reminder_schedule) + end + end + + for pg in PartnerGroup.all + if pg.reminder_day.present? + reminder_schedule = pg.convert_to_reminder_schedule(pg.reminder_day) + pg.update(reminder_schedule: reminder_schedule) + end + end + end +end diff --git a/db/migrate/20240715163348_remove_reminder_day_from_organizations.rb b/db/migrate/20240715163348_remove_reminder_day_from_organizations.rb new file mode 100644 index 0000000000..db0d3e0dfc --- /dev/null +++ b/db/migrate/20240715163348_remove_reminder_day_from_organizations.rb @@ -0,0 +1,6 @@ +class RemoveReminderDayFromOrganizations < ActiveRecord::Migration[7.1] + def change + safety_assured { remove_column :organizations, :reminder_day } + safety_assured { remove_column :partner_groups, :reminder_day } + end +end diff --git a/db/schema.rb b/db/schema.rb index bcbb2930f3..5f64a60a13 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.1].define(version: 2024_06_24_185108) do +ActiveRecord::Schema[7.1].define(version: 2024_07_15_163348) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -468,7 +468,6 @@ t.string "zipcode" t.float "latitude" t.float "longitude" - t.integer "reminder_day" t.integer "deadline_day" t.text "invitation_text" t.integer "default_storage_location" @@ -485,6 +484,7 @@ t.boolean "hide_value_columns_on_receipt", default: false t.boolean "hide_package_column_on_receipt", default: false t.boolean "signature_for_distribution_pdf", default: false + t.string "reminder_schedule" t.index ["latitude", "longitude"], name: "index_organizations_on_latitude_and_longitude" t.index ["short_name"], name: "index_organizations_on_short_name" end @@ -502,12 +502,11 @@ t.datetime "created_at", null: false t.datetime "updated_at", null: false t.boolean "send_reminders", default: false, null: false - t.integer "reminder_day" t.integer "deadline_day" + t.string "reminder_schedule" t.index ["name", "organization_id"], name: "index_partner_groups_on_name_and_organization_id", unique: true t.index ["organization_id"], name: "index_partner_groups_on_organization_id" t.check_constraint "deadline_day <= 28", name: "deadline_day_of_month_check" - t.check_constraint "reminder_day <= 28", name: "reminder_day_of_month_check" end create_table "partner_profiles", force: :cascade do |t| diff --git a/spec/factories/organizations.rb b/spec/factories/organizations.rb index 30ef97daa7..447117ef5f 100644 --- a/spec/factories/organizations.rb +++ b/spec/factories/organizations.rb @@ -20,7 +20,7 @@ # name :string # one_step_partner_invite :boolean default(FALSE), not null # partner_form_fields :text default([]), is an Array -# reminder_day :integer +# reminder_schedule :string # repackage_essentials :boolean default(FALSE), not null # short_name :string # signature_for_distribution_pdf :boolean default(FALSE) @@ -41,6 +41,9 @@ skip_items { false } end + recurrence_schedule = IceCube::Schedule.new + recurrence_schedule.add_recurrence_rule IceCube::Rule.monthly(1).day_of_month(10) + recurrence_schedule_ical = recurrence_schedule.to_ical sequence(:name) { |n| "Essentials Bank #{n}" } # 037000863427 sequence(:short_name) { |n| "db_#{n}" } # 037000863427 sequence(:email) { |n| "email#{n}@example.com" } # 037000863427 @@ -49,13 +52,13 @@ city { 'Front Royal' } state { 'VA' } zipcode { '22630' } - reminder_day { 10 } + reminder_schedule { recurrence_schedule_ical } deadline_day { 20 } logo { Rack::Test::UploadedFile.new(Rails.root.join("spec/fixtures/files/logo.jpg"), "image/jpeg") } trait :without_deadlines do - reminder_day { nil } + reminder_schedule { nil } deadline_day { nil } end diff --git a/spec/factories/partner_groups.rb b/spec/factories/partner_groups.rb index b3723feda1..99a74042fa 100644 --- a/spec/factories/partner_groups.rb +++ b/spec/factories/partner_groups.rb @@ -2,25 +2,29 @@ # # Table name: partner_groups # -# id :bigint not null, primary key -# deadline_day :integer -# name :string -# reminder_day :integer -# send_reminders :boolean default(FALSE), not null -# created_at :datetime not null -# updated_at :datetime not null -# organization_id :bigint +# id :bigint not null, primary key +# deadline_day :integer +# name :string +# reminder_schedule :string +# send_reminders :boolean default(FALSE), not null +# created_at :datetime not null +# updated_at :datetime not null +# organization_id :bigint # FactoryBot.define do + recurrence_schedule = IceCube::Schedule.new + recurrence_schedule.add_recurrence_rule IceCube::Rule.monthly(1).day_of_month(10) + recurrence_schedule_ical = recurrence_schedule.to_ical + factory :partner_group do sequence(:name) { |n| "Group #{n}" } organization { Organization.try(:first) || create(:organization) } - reminder_day { 14 } + reminder_schedule { recurrence_schedule_ical } deadline_day { 28 } trait :without_deadlines do - reminder_day { nil } + reminder_schedule { nil } deadline_day { nil } end end diff --git a/spec/mailers/reminder_deadline_mailer_spec.rb b/spec/mailers/reminder_deadline_mailer_spec.rb index fde33fa564..a18cb9717a 100644 --- a/spec/mailers/reminder_deadline_mailer_spec.rb +++ b/spec/mailers/reminder_deadline_mailer_spec.rb @@ -4,9 +4,8 @@ describe 'notify deadline' do let(:today) { Date.new(2022, 1, 10) } let(:partner) { create(:partner, organization: organization) } - before(:each) do - organization.update!(reminder_day: today.day, deadline_day: 1) + organization.update!(deadline_day: 1) end subject { described_class.notify_deadline(partner) } diff --git a/spec/models/concerns/deadlinable_spec.rb b/spec/models/concerns/deadlinable_spec.rb index 9f8b235c22..3ca33d6dc0 100644 --- a/spec/models/concerns/deadlinable_spec.rb +++ b/spec/models/concerns/deadlinable_spec.rb @@ -10,7 +10,7 @@ def self.name include ActiveModel::Model include Deadlinable - attr_accessor :deadline_day, :reminder_day + attr_accessor :deadline_day, :reminder_schedule def deadline_day? !!deadline_day @@ -19,6 +19,12 @@ def deadline_day? end subject(:dummy) { dummy_class.new } + let(:current_day) { Time.current } + let(:schedule) { IceCube::Schedule.new(current_day) } + + before do + dummy.deadline_day = 7 + end describe "validations" do it do @@ -29,20 +35,20 @@ def deadline_day? .allow_nil end - it do - is_expected.to validate_numericality_of(:reminder_day) - .only_integer - .is_greater_than_or_equal_to(1) - .is_less_than_or_equal_to(28) - .allow_nil + it "validates that the reminder schedule's date fall within the range" do + schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(31) + dummy.reminder_schedule = schedule.to_ical + + expect(dummy).not_to be_valid + expect(dummy.errors.added?(:reminder_schedule, "Reminder day must be between 1 and 28")).to be_truthy end it "validates that reminder day is not the same as deadline day" do - dummy.deadline_day = 7 - dummy.reminder_day = 7 + schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(7) + dummy.reminder_schedule = schedule.to_ical expect(dummy).not_to be_valid - expect(dummy.errors.added?(:reminder_day, "must be other than 7")).to be_truthy + expect(dummy.errors.added?(:reminder_schedule, "Reminder must not be the same as deadline date")).to be_truthy end end end diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 89438098d9..eca78f7b48 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -20,7 +20,7 @@ # name :string # one_step_partner_invite :boolean default(FALSE), not null # partner_form_fields :text default([]), is an Array -# reminder_day :integer +# reminder_schedule :string # repackage_essentials :boolean default(FALSE), not null # short_name :string # signature_for_distribution_pdf :boolean default(FALSE) @@ -447,13 +447,18 @@ end end - describe 'reminder_day' do - it "can only contain numbers 1-28" do - expect(build(:organization, reminder_day: 28)).to be_valid - expect(build(:organization, reminder_day: 1)).to be_valid - expect(build(:organization, reminder_day: 0)).to_not be_valid - expect(build(:organization, reminder_day: -5)).to_not be_valid - expect(build(:organization, reminder_day: 29)).to_not be_valid + describe 'reminder_schedule' do + it "cannot exceed 28" do + schedule = IceCube::Schedule.new(Date.new(2022, 1, 1)) + valid_days = [1, 28] + valid_days.each do |day| + schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(day) + expect(build(:organization, reminder_schedule: schedule.to_ical)).to be_valid + schedule.remove_recurrence_rule(schedule.recurrence_rules.first) + end + schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(29) + expect(build(:organization, reminder_schedule: schedule.to_ical)).to_not be_valid + schedule.remove_recurrence_rule(schedule.recurrence_rules.first) end end describe 'deadline_day' do diff --git a/spec/models/partner_group_spec.rb b/spec/models/partner_group_spec.rb index 10030476ce..14cdb2900b 100644 --- a/spec/models/partner_group_spec.rb +++ b/spec/models/partner_group_spec.rb @@ -2,14 +2,14 @@ # # Table name: partner_groups # -# id :bigint not null, primary key -# deadline_day :integer -# name :string -# reminder_day :integer -# send_reminders :boolean default(FALSE), not null -# created_at :datetime not null -# updated_at :datetime not null -# organization_id :bigint +# id :bigint not null, primary key +# deadline_day :integer +# name :string +# reminder_schedule :string +# send_reminders :boolean default(FALSE), not null +# created_at :datetime not null +# updated_at :datetime not null +# organization_id :bigint # RSpec.describe PartnerGroup, type: :model do describe 'associations' do @@ -34,9 +34,11 @@ end end - describe 'reminder_day <= 28' do + describe 'reminder_schedule day <= 28' do it 'raises error if unmet' do - expect { partner_group.update_column(:reminder_day, 29) }.to raise_error(ActiveRecord::StatementInvalid) + schedule = IceCube::Schedule.new(Time.current) + schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(30) + expect(build(:partner_group, reminder_schedule: schedule.to_ical)).to_not be_valid end end end @@ -54,8 +56,8 @@ expect(build(:partner, name: "Foo", organization: build(:organization))).to be_valid end - describe "deadline_day && reminder_day must be defined if send_reminders=true" do - let(:partner_group) { build(:partner_group, send_reminders: true, deadline_day: nil, reminder_day: nil) } + describe "deadline_day && reminder_schedule must be defined if send_reminders=true" do + let(:partner_group) { build(:partner_group, send_reminders: true, deadline_day: nil, reminder_schedule: nil) } it "should not be valid" do expect(partner_group).not_to be_valid diff --git a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb index 9c2a44d3ab..142dc5c6a2 100644 --- a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb +++ b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb @@ -4,16 +4,19 @@ describe ".fetch" do subject { described_class.new.fetch } let(:current_day) { 14 } + let(:schedule_1) { IceCube::Schedule.new(Date.new(2022, 6, 1)) } + let(:schedule_2) { IceCube::Schedule.new(Date.new(2022, 6, 1)) } + let(:schedule_3) { IceCube::Schedule.new(Date.new(2022, 5, 1)) } before { travel_to(Time.zone.local(2022, 6, current_day, 1, 1, 1)) } after { travel_back } context "when there is a partner" do let!(:partner) { create(:partner) } - context "that has an organization with a global reminder & deadline" do context "that is for today" do before do - partner.organization.update(reminder_day: current_day) + schedule_1.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day) + partner.organization.update(reminder_schedule: schedule_1.to_ical) partner.organization.update(deadline_day: current_day + 2) end @@ -21,6 +24,28 @@ expect(subject).to include(partner) end + context "as matched by day of the week" do + before do + schedule_2.add_recurrence_rule IceCube::Rule.monthly.day_of_week(tuesday: [2]) + partner.organization.update(reminder_schedule: schedule_2.to_ical) + end + it "should include that partner" do + expect Time.current.day == 2 + expect(subject).to include(partner) + end + end + + context "but the reoccurrence rule is not for the current month" do + before do + schedule_3.add_recurrence_rule IceCube::Rule.monthly(2).day_of_month(current_day) + partner.organization.update(reminder_schedule: schedule_3.to_ical) + end + + it "should NOT include that partner" do + expect(subject).not_to include(partner) + end + end + context "but the partner is deactivated" do before do partner.deactivated! @@ -44,7 +69,8 @@ context "that is not for today" do before do - partner.organization.update(reminder_day: current_day - 1) + new_schedule = IceCube::Schedule.new(Date.new(2022, 6, current_day - 1)).to_ical + partner.organization.update(reminder_schedule: new_schedule) partner.organization.update(deadline_day: current_day + 2) end @@ -55,11 +81,12 @@ context "AND a partner group that does have them defined" do before do - partner_group = create(:partner_group, reminder_day: current_day, deadline_day: current_day + 2) + schedule_1.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day) + schedule_2.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day - 1) + partner_group = create(:partner_group, reminder_schedule: schedule_1.to_ical, deadline_day: current_day + 2) partner_group.partners << partner - partner.organization.update(reminder_day: current_day - 1) - partner.organization.update(deadline_day: current_day + 2) + partner.organization.update(reminder_schedule: schedule_2.to_ical) end it "should remind based on the partner group instead of the organization level reminder" do @@ -80,13 +107,14 @@ context "that does NOT have a organization with a global reminder & deadline" do before do - partner.organization.update(reminder_day: nil, deadline_day: nil) + partner.organization.update(reminder_schedule: nil, deadline_day: nil) end context "and is a part of a partner group that does have them defined" do context "that is for today" do before do - partner_group = create(:partner_group, reminder_day: current_day, deadline_day: current_day + 2) + schedule_1.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day) + partner_group = create(:partner_group, reminder_schedule: schedule_1.to_ical, deadline_day: current_day + 2) partner_group.partners << partner end @@ -107,7 +135,8 @@ context "that is not for today" do before do - partner_group = create(:partner_group, reminder_day: current_day - 1, deadline_day: current_day + 2) + new_schedule = IceCube::Schedule.new(Date.new(2022, 6, current_day - 1)) + partner_group = create(:partner_group, reminder_schedule: new_schedule.to_ical, deadline_day: current_day + 2) partner_group.partners << partner end From 8363b883dcbf9b8512fb0bc2f281dcf8f3f4e548 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Tue, 20 Aug 2024 09:09:59 -0600 Subject: [PATCH 02/10] Adds input for the reminder schedule in the Edit Partner Group page Builds an ActiveModel, ReminderSchedule, which takes the necessary information and turns it into an IceCubeSchedule, which is what ultimate get saved in the db. Builds the form for this reminder schedule. Still needs to conditionally show or hide sections depending on if the user wants date or day of the week. --- Gemfile.lock | 20 ++++++- app/controllers/partner_groups_controller.rb | 10 +++- app/models/reminder_schedule.rb | 54 +++++++++++++++++++ .../shared/_deadline_day_fields.html.erb | 51 +++++++++++++++--- 4 files changed, 124 insertions(+), 11 deletions(-) create mode 100644 app/models/reminder_schedule.rb diff --git a/Gemfile.lock b/Gemfile.lock index e685bd0739..1abd82aadd 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -139,6 +139,13 @@ GEM activesupport tzinfo coderay (1.1.3) + coffee-rails (5.0.0) + coffee-script (>= 2.2.0) + railties (>= 5.2.0) + coffee-script (2.4.1) + coffee-script-source + execjs + coffee-script-source (1.12.2) concurrent-ruby (1.3.3) connection_pool (2.4.1) coverband (6.1.2) @@ -313,6 +320,10 @@ GEM jbuilder (2.12.0) actionview (>= 5.0.0) activesupport (>= 5.0.0) + jquery-rails (4.6.0) + rails-dom-testing (>= 1, < 3) + railties (>= 4.2.0) + thor (>= 0.14, < 2.0) json (2.7.2) jwt (2.8.2) base64 @@ -525,6 +536,12 @@ GEM rdoc (6.7.0) psych (>= 4.0.0) recaptcha (5.17.0) + recurring_select (3.0.1) + coffee-rails (>= 3.1) + ice_cube (>= 0.11) + jquery-rails (>= 3.0) + rails (>= 5.2) + sass-rails (>= 4.0) redis (5.2.0) redis-client (>= 0.22.0) redis-client (0.22.1) @@ -762,6 +779,7 @@ DEPENDENCIES rails-controller-testing rails-erd recaptcha + recurring_select redis (~> 5.2) rolify (~> 6.0) rspec-rails (~> 6.1.3) @@ -782,4 +800,4 @@ DEPENDENCIES webmock (~> 3.23) BUNDLED WITH - 2.5.15 + 2.5.16 diff --git a/app/controllers/partner_groups_controller.rb b/app/controllers/partner_groups_controller.rb index bc1734e83f..e9a1b9e02f 100644 --- a/app/controllers/partner_groups_controller.rb +++ b/app/controllers/partner_groups_controller.rb @@ -17,12 +17,14 @@ def create def edit @partner_group = current_organization.partner_groups.find(params[:id]) + @reminder_schedule = ReminderSchedule.from_ical(@partner_group.reminder_schedule) @item_categories = current_organization.item_categories end def update @partner_group = current_organization.partner_groups.find(params[:id]) - if @partner_group.update(partner_group_params) + reminder_schedule = ReminderSchedule.new(reminder_schedule_params).create_schedule + if @partner_group.update(partner_group_params.merge!(reminder_schedule:)) redirect_to partners_path + "#nav-partner-groups", notice: "Partner group edited!" else flash[:error] = "Something didn't work quite right -- try again?" @@ -33,6 +35,10 @@ def update private def partner_group_params - params.require(:partner_group).permit(:name, :send_reminders, :deadline_day, :reminder_schedule, item_category_ids: []) + params.require(:partner_group).permit(:name, :send_reminders, :reminder_schedule, :deadline_day, item_category_ids: []) + end + + def reminder_schedule_params + params.require(:reminder_schedule).permit(:every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day) end end diff --git a/app/models/reminder_schedule.rb b/app/models/reminder_schedule.rb new file mode 100644 index 0000000000..303fb4d2ce --- /dev/null +++ b/app/models/reminder_schedule.rb @@ -0,0 +1,54 @@ +class ReminderSchedule + include ActiveModel::Model + + attr_accessor :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day + attr_reader :every_nth_collection, :week_day_collection, :date_or_week_day_collection + + validates :every_n_months, presence: true, inclusion: 1..12 + validates :date_or_week_day, presence: true, inclusion: { in: %w[date week_day] } + validates :date, presence: true, if: -> { date_or_week_day == 'date' } + validates :day_of_week, presence: true, if: -> { date_or_week_day == 'week_day' }, inclusion: 1..7 + validates :every_nth_day, presence: true, if: -> { date_or_week_day == 'date' }, inclusion: 1..4 + + + def initialize(attributes = {}) + super + @every_n_months = every_n_months.to_i + @date = date.to_i + @day_of_week = day_of_week.to_i + @every_nth_day = every_nth_day.to_i + @every_nth_collection = EVERY_NTH_COLLECTION + @week_day_collection = WEEK_DAY_COLLECTION + @date_or_week_day_collection = DATE_OR_WEEK_DAY_COLLECTION + end + + def create_schedule + binding.pry + schedule = IceCube::Schedule.new(Time.zone.now.to_date) + if date_or_week_day == 'date' + schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months).day_of_month(date)) + else + schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months).day_of_week(day_of_week => [every_nth_day])) + end + schedule.to_ical + end + + def self.from_ical(ical) + schedule = IceCube::Schedule.from_ical(ical) + rule = schedule.recurrence_rules.first.instance_values + date = rule["validations"][:day_of_month]&.first&.value + new( + every_n_months: rule['interval'], + date_or_week_day: date ? 'date' : 'week_day', + date: date, + day_of_week: rule["validations"][:day_of_week]&.first&.day, + every_nth_day: rule["validations"][:day_of_week]&.first&.occ + ) + end + + private + EVERY_NTH_COLLECTION = [['First', 1], ['Second', 2], ['Third', 3], ['Fourth', 4], ['Last', -1]].freeze + WEEK_DAY_COLLECTION = [['Monday', 0], ['Tuesday', 1], ['Wednesday', 2], ['Thursday', 3], ['Friday', 4], ['Saturday', 5], ['Sunday', 6]].freeze + DATE_OR_WEEK_DAY_COLLECTION = [['date', 'Date'] ,['week_day', 'Day of the Week']].freeze + +end diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index cf9387075f..b8ad40b01d 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -6,16 +6,51 @@ current_month: Date::MONTHNAMES[Date.current.month], next_month: Date::MONTHNAMES[Date.current.next_month.month] }) do %> -
- <%= f.input :reminder_day, wrapper: :input_group, wrapper_html: { class: 'mb-0' }, - label: 'Reminder day (Day of month an e-mail reminder is sent to partners to submit requests)' do %> + + <%= simple_fields_for @reminder_schedule do |t| %> + <%= t.input :every_n_months, + as: :integer, + label: 'Send reminders every X months', + class: "deadline-day-pickers__reminder-day form-control", + hint: "Enter the number of months between reminders", + :input_html => {:style=> 'width: 100px'} + %> + + <%= t.input :date_or_week_day, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, + label: 'Send reminders on a specific date (eg "the 5th") or a day of the week (eg "the first Tuesday")?' do %> + <%= t.collection_radio_buttons :date_or_week_day, t.object.date_or_week_day_collection, :first, :last do |b| %> + <%= b.label(class:"input-group-text mr-3") { b.radio_button + b.text } %> + <% end %> + <% end %> + + <%= t.input :date, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, + label: 'Reminder date' do %> - <%= f.number_field :reminder_day, - class: "deadline-day-pickers__reminder-day form-control", - placeholder: "Reminder day" %> + <%= t.number_field :date, as: :integer, + class: "deadline-day-pickers__reminder-day form-control", + placeholder: "Reminder day", + :input_html => {:style=> 'width: 100px'} + %> + <% end %> + + <%= t.input :every_nth_day, wrapper: :input_group, wrapper_html: { class: 'mb-3'}, + label: 'Reminder day of the week' do %> + <%= t.input :every_nth_day, collection: t.object.every_nth_collection, + class: "deadline-day-pickers__reminder-day form-control", + label: false, + default: 1, + :input_html => {} + %> + + <%= t.input :day_of_week, collection: t.object.week_day_collection, + class: "deadline-day-pickers__reminder-day form-control", + label: false, + default: 0, + :input_html => {:style=> 'width: 200px'} + %> + <% end %> + <% end %> - -
<%= f.input :deadline_day, wrapper: :input_group, wrapper_html: { class: 'mb-0' }, From 409209dcef25fdc5fe48f89d969228b7a9478790 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Tue, 20 Aug 2024 11:11:49 -0600 Subject: [PATCH 03/10] Adds Reminder Schedule fields to New and Edit Organization Now either while creating new Organization/Partner Groups or editing them, all the transfers from Reminder Date to Reminder Day have been made. --- .../admin/organizations_controller.rb | 15 +++++++++++---- app/controllers/organizations_controller.rb | 9 +++++++-- app/controllers/partner_groups_controller.rb | 5 ++++- app/models/reminder_schedule.rb | 18 ++++++++++++------ app/views/admin/organizations/new.html.erb | 3 +-- app/views/organizations/_details.html.erb | 4 ++-- .../partners/_partner_groups_table.html.erb | 2 +- app/views/shared/_deadline_day_fields.html.erb | 6 ++++-- 8 files changed, 42 insertions(+), 20 deletions(-) diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb index 5eaf22993f..b4851ec9ce 100644 --- a/app/controllers/admin/organizations_controller.rb +++ b/app/controllers/admin/organizations_controller.rb @@ -2,12 +2,13 @@ class Admin::OrganizationsController < AdminController def edit @organization = Organization.find(params[:id]) + @reminder_schedule = ReminderSchedule.from_ical(@organization.reminder_schedule) end def update @organization = Organization.find(params[:id]) - - if OrganizationUpdateService.update(@organization, organization_params) + reminder_schedule = ReminderSchedule.new(reminder_schedule_params).create_schedule + if OrganizationUpdateService.update(@organization, organization_params.merge!(reminder_schedule:)) redirect_to admin_organizations_path, notice: "Updated organization!" else flash[:error] = @organization.errors.full_messages.join("\n") @@ -31,6 +32,7 @@ def index def new @organization = Organization.new + @reminder_schedule = ReminderSchedule.new account_request = params[:token] && AccountRequest.get_by_identity_token(params[:token]) @user = User.new @@ -45,8 +47,9 @@ def new end def create - @organization = Organization.new(organization_params) - + @reminder_schedule = ReminderSchedule.new(reminder_schedule_params) + final_params = organization_params.merge!(reminder_schedule: @reminder_schedule.create_schedule) + @organization = Organization.new(final_params) if @organization.save Organization.seed_items(@organization) @user = UserInviteService.invite(name: user_params[:name], @@ -89,4 +92,8 @@ def organization_params def user_params params.require(:organization).require(:user).permit(:name, :email) end + + def reminder_schedule_params + params.require(:reminder_schedule).permit(:every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day) + end end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index 1675d0958b..919596ecff 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -10,12 +10,13 @@ def show def edit @organization = current_organization + @reminder_schedule = ReminderSchedule.from_ical(@organization.reminder_schedule) end def update @organization = current_organization - - if OrganizationUpdateService.update(@organization, organization_params) + @reminder_schedule = ReminderSchedule.new(reminder_schedule_params) + if OrganizationUpdateService.update(@organization, organization_params.merge!(reminder_schedule:@reminder_schedule.create_schedule)) redirect_to organization_path, notice: "Updated your organization!" else flash[:error] = @organization.errors.full_messages.join("\n") @@ -103,6 +104,10 @@ def organization_params ) end + def reminder_schedule_params + params.require(:reminder_schedule).permit(:every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day) + end + def request_type_formatter(params) if params[:organization][:enable_individual_requests] == "false" params[:organization][:enable_child_based_requests] = false diff --git a/app/controllers/partner_groups_controller.rb b/app/controllers/partner_groups_controller.rb index e9a1b9e02f..129e1bbf88 100644 --- a/app/controllers/partner_groups_controller.rb +++ b/app/controllers/partner_groups_controller.rb @@ -1,11 +1,14 @@ class PartnerGroupsController < ApplicationController def new @partner_group = current_organization.partner_groups.new + @reminder_schedule = ReminderSchedule.new @item_categories = current_organization.item_categories end def create - @partner_group = current_organization.partner_groups.new(partner_group_params) + @reminder_schedule = ReminderSchedule.new(reminder_schedule_params) + final_params = partner_group_params.merge!(reminder_schedule: @reminder_schedule.create_schedule) + @partner_group = current_organization.partner_groups.new(final_params) if @partner_group.save # Redirect to groups tab in Partner page. redirect_to partners_path + "#nav-partner-groups", notice: "Partner group added!" diff --git a/app/models/reminder_schedule.rb b/app/models/reminder_schedule.rb index 303fb4d2ce..8858812cef 100644 --- a/app/models/reminder_schedule.rb +++ b/app/models/reminder_schedule.rb @@ -13,17 +13,18 @@ class ReminderSchedule def initialize(attributes = {}) super + @every_nth_collection = EVERY_NTH_COLLECTION + @week_day_collection = WEEK_DAY_COLLECTION + @date_or_week_day_collection = DATE_OR_WEEK_DAY_COLLECTION + return if attributes.blank? + @every_n_months = every_n_months.to_i @date = date.to_i @day_of_week = day_of_week.to_i @every_nth_day = every_nth_day.to_i - @every_nth_collection = EVERY_NTH_COLLECTION - @week_day_collection = WEEK_DAY_COLLECTION - @date_or_week_day_collection = DATE_OR_WEEK_DAY_COLLECTION end def create_schedule - binding.pry schedule = IceCube::Schedule.new(Time.zone.now.to_date) if date_or_week_day == 'date' schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months).day_of_month(date)) @@ -33,6 +34,11 @@ def create_schedule schedule.to_ical end + def self.show_description(ical) + schedule = IceCube::Schedule.from_ical(ical) + schedule.recurrence_rules.first.to_s + end + def self.from_ical(ical) schedule = IceCube::Schedule.from_ical(ical) rule = schedule.recurrence_rules.first.instance_values @@ -47,8 +53,8 @@ def self.from_ical(ical) end private - EVERY_NTH_COLLECTION = [['First', 1], ['Second', 2], ['Third', 3], ['Fourth', 4], ['Last', -1]].freeze - WEEK_DAY_COLLECTION = [['Monday', 0], ['Tuesday', 1], ['Wednesday', 2], ['Thursday', 3], ['Friday', 4], ['Saturday', 5], ['Sunday', 6]].freeze + EVERY_NTH_COLLECTION = [['First', 1], ['Second', 2], ['Third', 3], ['Fourth', 4]].freeze + WEEK_DAY_COLLECTION = [['Sunday'], ['Monday', 1], ['Tuesday', 2], ['Wednesday', 3], ['Thursday', 4], ['Friday', 5], ['Saturday', 6]].freeze DATE_OR_WEEK_DAY_COLLECTION = [['date', 'Date'] ,['week_day', 'Day of the Week']].freeze end diff --git a/app/views/admin/organizations/new.html.erb b/app/views/admin/organizations/new.html.erb index 62fd4906a8..cb574e4eb2 100644 --- a/app/views/admin/organizations/new.html.erb +++ b/app/views/admin/organizations/new.html.erb @@ -48,8 +48,7 @@ <%= f.input :city %> <%= f.input :state, collection: us_states, class: "form-control", placeholder: "state" %> <%= f.input :zipcode %> - <%= f.input :reminder_day, class: "form-control", placeholder: "Reminder day" %> - <%= f.input :deadline_day, class: "form-control", placeholder: "Deadline day" %> + <%= render 'shared/deadline_day_fields', f: f %> <%= f.simple_fields_for :account_request do |account_request| %> <%= account_request.input :ndbn_member, label: 'NDBN Membership', wrapper: :input_group do %> <%= account_request.association :ndbn_member, label_method: :full_name, value_method: :id, label: false %> diff --git a/app/views/organizations/_details.html.erb b/app/views/organizations/_details.html.erb index d92788f59f..110923d3fa 100644 --- a/app/views/organizations/_details.html.erb +++ b/app/views/organizations/_details.html.erb @@ -63,10 +63,10 @@

-

Reminder day

+

Reminder Schedule

<%= fa_icon "calendar" %> - <%= @organization.reminder_day.blank? ? 'Not defined' : "The #{@organization.reminder_day.ordinalize} of each month" %> + <%= @organization.reminder_schedule.blank? ? 'Not defined' : ReminderSchedule.show_description(@organization.reminder_schedule) %>

diff --git a/app/views/partners/_partner_groups_table.html.erb b/app/views/partners/_partner_groups_table.html.erb index abc7dad2bb..669981d1aa 100644 --- a/app/views/partners/_partner_groups_table.html.erb +++ b/app/views/partners/_partner_groups_table.html.erb @@ -45,7 +45,7 @@ <% if pg.send_reminders %> - Reminder emails are sent on the <%= pg.reminder_day.ordinalize %> of every month. + Reminder emails are sent <%= ReminderSchedule.show_description(pg.reminder_schedule) %>.
Deadlines are the <%= pg.deadline_day.ordinalize %> of every month. <% else %> diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index b8ad40b01d..e226a6e09c 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -13,6 +13,7 @@ label: 'Send reminders every X months', class: "deadline-day-pickers__reminder-day form-control", hint: "Enter the number of months between reminders", + :placeholder => "1", :input_html => {:style=> 'width: 100px'} %> @@ -26,7 +27,7 @@ <%= t.input :date, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, label: 'Reminder date' do %> - <%= t.number_field :date, as: :integer, + <%= t.number_field :date, class: "deadline-day-pickers__reminder-day form-control", placeholder: "Reminder day", :input_html => {:style=> 'width: 100px'} @@ -45,7 +46,8 @@ <%= t.input :day_of_week, collection: t.object.week_day_collection, class: "deadline-day-pickers__reminder-day form-control", label: false, - default: 0, + show_blank: true, + default: 1, :input_html => {:style=> 'width: 200px'} %> <% end %> From 03cd829f12c5f6eba35f7454a4befe6bfbf811f8 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Thu, 22 Aug 2024 10:55:21 -0600 Subject: [PATCH 04/10] Makes the day of week or date hide depending on selection This uses css for ease and accessibility. --- app/assets/stylesheets/custom.scss | 12 +++++++++++ .../shared/_deadline_day_fields.html.erb | 20 +++++++++++-------- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/app/assets/stylesheets/custom.scss b/app/assets/stylesheets/custom.scss index ad5a229332..b3cb83cb61 100644 --- a/app/assets/stylesheets/custom.scss +++ b/app/assets/stylesheets/custom.scss @@ -98,3 +98,15 @@ margin-top: 40px; } } + +#week_day_fields, #date_fields { + display: none; +} + +#reminder_schedule_date_or_week_day_week_day:checked ~ #week_day_fields { + display: block; +} + +#reminder_schedule_date_or_week_day_date:checked ~ #date_fields { + display: block +} \ No newline at end of file diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index e226a6e09c..dd856ed097 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -16,14 +16,16 @@ :placeholder => "1", :input_html => {:style=> 'width: 100px'} %> - - <%= t.input :date_or_week_day, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, - label: 'Send reminders on a specific date (eg "the 5th") or a day of the week (eg "the first Tuesday")?' do %> - <%= t.collection_radio_buttons :date_or_week_day, t.object.date_or_week_day_collection, :first, :last do |b| %> - <%= b.label(class:"input-group-text mr-3") { b.radio_button + b.text } %> - <% end %> - <% end %> + <%= t.label :date_or_week_day, 'Send reminders on a specific date (eg "the 5th") or a day of the week (eg "the first Tuesday")?' %> +
+ <%= t.radio_button :date_or_week_day, 'date', label: 'Date'%> + <%= t.label :date_or_week_day, 'Date' %> +
+ <%= t.radio_button :date_or_week_day, 'week_day', label: 'Week Day' %> + <%= t.label :date_or_week_day, 'Week Day' %> + +
<%= t.input :date, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, label: 'Reminder date' do %> @@ -33,7 +35,9 @@ :input_html => {:style=> 'width: 100px'} %> <% end %> +
+
<%= t.input :every_nth_day, wrapper: :input_group, wrapper_html: { class: 'mb-3'}, label: 'Reminder day of the week' do %> <%= t.input :every_nth_day, collection: t.object.every_nth_collection, @@ -50,8 +54,8 @@ default: 1, :input_html => {:style=> 'width: 200px'} %> +
<% end %> - <% end %>
From 80f6c027f660975a5f22c29ffc85ea404bd7daad Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Wed, 28 Aug 2024 08:02:05 -0600 Subject: [PATCH 05/10] Adds tests and does refactoring Adds a number of refactors, including moving all the create_schedule logic to the deadlinable helper, and creates unit and system tests for the new functionality. --- Gemfile.lock | 20 +----- app/assets/stylesheets/custom.scss | 6 +- .../admin/organizations_controller.rb | 18 ++---- app/controllers/organizations_controller.rb | 12 ++-- app/controllers/partner_groups_controller.rb | 18 ++---- app/models/concerns/deadlinable.rb | 64 ++++++++++++++----- app/models/organization.rb | 6 +- app/models/partner_group.rb | 8 ++- app/models/reminder_schedule.rb | 60 ----------------- app/services/organization_update_service.rb | 1 - .../fetch_partners_to_remind_now_service.rb | 1 + app/views/organizations/_details.html.erb | 2 +- .../partners/_partner_groups_table.html.erb | 6 +- .../shared/_deadline_day_fields.html.erb | 50 +++++++-------- ..._remove_reminder_day_from_organizations.rb | 4 +- spec/models/concerns/deadlinable_spec.rb | 32 ++++++++-- spec/models/organization_spec.rb | 16 ++--- spec/models/partner_group_spec.rb | 9 ++- ...tch_partners_to_remind_now_service_spec.rb | 48 +++++--------- .../system/admin/organizations_system_spec.rb | 5 +- spec/system/organization_system_spec.rb | 8 ++- spec/system/partner_system_spec.rb | 26 ++++++++ 22 files changed, 197 insertions(+), 223 deletions(-) delete mode 100644 app/models/reminder_schedule.rb diff --git a/Gemfile.lock b/Gemfile.lock index 1abd82aadd..a6da26d438 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -139,13 +139,6 @@ GEM activesupport tzinfo coderay (1.1.3) - coffee-rails (5.0.0) - coffee-script (>= 2.2.0) - railties (>= 5.2.0) - coffee-script (2.4.1) - coffee-script-source - execjs - coffee-script-source (1.12.2) concurrent-ruby (1.3.3) connection_pool (2.4.1) coverband (6.1.2) @@ -320,10 +313,6 @@ GEM jbuilder (2.12.0) actionview (>= 5.0.0) activesupport (>= 5.0.0) - jquery-rails (4.6.0) - rails-dom-testing (>= 1, < 3) - railties (>= 4.2.0) - thor (>= 0.14, < 2.0) json (2.7.2) jwt (2.8.2) base64 @@ -536,12 +525,6 @@ GEM rdoc (6.7.0) psych (>= 4.0.0) recaptcha (5.17.0) - recurring_select (3.0.1) - coffee-rails (>= 3.1) - ice_cube (>= 0.11) - jquery-rails (>= 3.0) - rails (>= 5.2) - sass-rails (>= 4.0) redis (5.2.0) redis-client (>= 0.22.0) redis-client (0.22.1) @@ -779,7 +762,6 @@ DEPENDENCIES rails-controller-testing rails-erd recaptcha - recurring_select redis (~> 5.2) rolify (~> 6.0) rspec-rails (~> 6.1.3) @@ -800,4 +782,4 @@ DEPENDENCIES webmock (~> 3.23) BUNDLED WITH - 2.5.16 + 2.5.18 diff --git a/app/assets/stylesheets/custom.scss b/app/assets/stylesheets/custom.scss index b3cb83cb61..44a3f648d3 100644 --- a/app/assets/stylesheets/custom.scss +++ b/app/assets/stylesheets/custom.scss @@ -99,14 +99,14 @@ } } -#week_day_fields, #date_fields { +#week-day-fields, #date-fields { display: none; } -#reminder_schedule_date_or_week_day_week_day:checked ~ #week_day_fields { +#toggle-to-week-day:checked ~ #week-day-fields { display: block; } -#reminder_schedule_date_or_week_day_date:checked ~ #date_fields { +#toggle-to-date:checked ~ #date-fields { display: block } \ No newline at end of file diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb index b4851ec9ce..47ef000dc1 100644 --- a/app/controllers/admin/organizations_controller.rb +++ b/app/controllers/admin/organizations_controller.rb @@ -2,13 +2,12 @@ class Admin::OrganizationsController < AdminController def edit @organization = Organization.find(params[:id]) - @reminder_schedule = ReminderSchedule.from_ical(@organization.reminder_schedule) + @organization.from_ical(@organization.reminder_schedule) end def update @organization = Organization.find(params[:id]) - reminder_schedule = ReminderSchedule.new(reminder_schedule_params).create_schedule - if OrganizationUpdateService.update(@organization, organization_params.merge!(reminder_schedule:)) + if OrganizationUpdateService.update(@organization, organization_params) redirect_to admin_organizations_path, notice: "Updated organization!" else flash[:error] = @organization.errors.full_messages.join("\n") @@ -32,7 +31,7 @@ def index def new @organization = Organization.new - @reminder_schedule = ReminderSchedule.new + @organization.from_ical(@organization.reminder_schedule) account_request = params[:token] && AccountRequest.get_by_identity_token(params[:token]) @user = User.new @@ -47,9 +46,7 @@ def new end def create - @reminder_schedule = ReminderSchedule.new(reminder_schedule_params) - final_params = organization_params.merge!(reminder_schedule: @reminder_schedule.create_schedule) - @organization = Organization.new(final_params) + @organization = Organization.new(organization_params) if @organization.save Organization.seed_items(@organization) @user = UserInviteService.invite(name: user_params[:name], @@ -85,15 +82,12 @@ def destroy def organization_params params.require(:organization) - .permit(:name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, :reminder_schedule, :deadline_day, + .permit(:name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, + :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day, :deadline_day, users_attributes: %i(name email organization_admin), account_request_attributes: %i(ndbn_member_id id)) end def user_params params.require(:organization).require(:user).permit(:name, :email) end - - def reminder_schedule_params - params.require(:reminder_schedule).permit(:every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day) - end end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index 919596ecff..a2319fb3de 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -10,13 +10,12 @@ def show def edit @organization = current_organization - @reminder_schedule = ReminderSchedule.from_ical(@organization.reminder_schedule) + @organization.from_ical(@organization.reminder_schedule) end def update @organization = current_organization - @reminder_schedule = ReminderSchedule.new(reminder_schedule_params) - if OrganizationUpdateService.update(@organization, organization_params.merge!(reminder_schedule:@reminder_schedule.create_schedule)) + if OrganizationUpdateService.update(@organization, organization_params) redirect_to organization_path, notice: "Updated your organization!" else flash[:error] = @organization.errors.full_messages.join("\n") @@ -99,15 +98,12 @@ def organization_params :enable_individual_requests, :enable_quantity_based_requests, :ytd_on_distribution_printout, :one_step_partner_invite, :hide_value_columns_on_receipt, :hide_package_column_on_receipt, - :signature_for_distribution_pdf, + :signature_for_distribution_pdf, :every_n_months, + :date_or_week_day, :date, :day_of_week, :every_nth_day, partner_form_fields: [] ) end - def reminder_schedule_params - params.require(:reminder_schedule).permit(:every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day) - end - def request_type_formatter(params) if params[:organization][:enable_individual_requests] == "false" params[:organization][:enable_child_based_requests] = false diff --git a/app/controllers/partner_groups_controller.rb b/app/controllers/partner_groups_controller.rb index 129e1bbf88..a2a13f36fb 100644 --- a/app/controllers/partner_groups_controller.rb +++ b/app/controllers/partner_groups_controller.rb @@ -1,14 +1,11 @@ class PartnerGroupsController < ApplicationController def new @partner_group = current_organization.partner_groups.new - @reminder_schedule = ReminderSchedule.new @item_categories = current_organization.item_categories end def create - @reminder_schedule = ReminderSchedule.new(reminder_schedule_params) - final_params = partner_group_params.merge!(reminder_schedule: @reminder_schedule.create_schedule) - @partner_group = current_organization.partner_groups.new(final_params) + @partner_group = current_organization.partner_groups.new(partner_group_params) if @partner_group.save # Redirect to groups tab in Partner page. redirect_to partners_path + "#nav-partner-groups", notice: "Partner group added!" @@ -20,14 +17,13 @@ def create def edit @partner_group = current_organization.partner_groups.find(params[:id]) - @reminder_schedule = ReminderSchedule.from_ical(@partner_group.reminder_schedule) + @partner_group.from_ical(@partner_group.reminder_schedule) @item_categories = current_organization.item_categories end def update @partner_group = current_organization.partner_groups.find(params[:id]) - reminder_schedule = ReminderSchedule.new(reminder_schedule_params).create_schedule - if @partner_group.update(partner_group_params.merge!(reminder_schedule:)) + if @partner_group.update(partner_group_params) redirect_to partners_path + "#nav-partner-groups", notice: "Partner group edited!" else flash[:error] = "Something didn't work quite right -- try again?" @@ -38,10 +34,8 @@ def update private def partner_group_params - params.require(:partner_group).permit(:name, :send_reminders, :reminder_schedule, :deadline_day, item_category_ids: []) - end - - def reminder_schedule_params - params.require(:reminder_schedule).permit(:every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day) + params.require(:partner_group).permit(:name, :send_reminders, :reminder_schedule, + :deadline_day, :every_n_months, :date_or_week_day, + :date, :day_of_week, :every_nth_day, item_category_ids: []) end end diff --git a/app/models/concerns/deadlinable.rb b/app/models/concerns/deadlinable.rb index 608b0d36d9..4041e51a11 100644 --- a/app/models/concerns/deadlinable.rb +++ b/app/models/concerns/deadlinable.rb @@ -2,12 +2,20 @@ module Deadlinable extend ActiveSupport::Concern MIN_DAY_OF_MONTH = 1 MAX_DAY_OF_MONTH = 28 + EVERY_NTH_COLLECTION = [["First", 1], ["Second", 2], ["Third", 3], ["Fourth", 4]].freeze + WEEK_DAY_COLLECTION = [["Sunday"], ["Monday", 1], ["Tuesday", 2], ["Wednesday", 3], ["Thursday", 4], ["Friday", 5], ["Saturday", 6]].freeze included do + attr_accessor :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day + attr_reader :every_nth_collection, :week_day_collection, :date_or_week_day_collection validates :deadline_day, numericality: {only_integer: true, less_than_or_equal_to: MAX_DAY_OF_MONTH, greater_than_or_equal_to: MIN_DAY_OF_MONTH, allow_nil: true} - validate :reminder_on_deadline_day? - validate :reminder_schedule_is_within_range? + validate :reminder_on_deadline_day?, if: -> { every_n_months.present? } + validate :reminder_is_within_range?, if: -> { every_n_months.present? } + validates :date_or_week_day, inclusion: {in: %w[date week_day]}, if: -> { every_n_months.present? } + validates :date, presence: true, if: -> { date_or_week_day == "date" && every_n_months.present? } + validates :day_of_week, presence: true, if: -> { date_or_week_day == "week_day" && every_n_months.present? }, inclusion: {in: %w[1 2 3 4 5 6 7]} + validates :every_nth_day, presence: true, if: -> { date_or_week_day == "week_day" && every_n_months.present? }, inclusion: {in: %w[1 2 3 4]} end def convert_to_reminder_schedule(day) @@ -16,29 +24,51 @@ def convert_to_reminder_schedule(day) schedule.to_ical end + def show_description(ical) + schedule = IceCube::Schedule.from_ical(ical) + schedule.recurrence_rules.first.to_s + end + + def from_ical(ical) + return if ical.blank? + schedule = IceCube::Schedule.from_ical(ical) + rule = schedule.recurrence_rules.first.instance_values + date = rule["validations"][:day_of_month]&.first&.value + self.every_n_months = rule["interval"] + self.date_or_week_day = date ? "date" : "week_day" + self.date = date + self.day_of_week = rule["validations"][:day_of_week]&.first&.day, + self.every_nth_day = rule["validations"][:day_of_week]&.first&.occ + rescue + nil + end + private def reminder_on_deadline_day? - if reminder_schedule.nil? - return - end - - schedule = IceCube::Schedule.from_ical reminder_schedule - if schedule.first.day == deadline_day - errors.add(:reminder_schedule, "Reminder must not be the same as deadline date") + if date_or_week_day == "date" && date.to_i == deadline_day + errors.add(:date, "Reminder must not be the same as deadline date") end end - def reminder_schedule_is_within_range? - if reminder_schedule.nil? - return - end - schedule = IceCube::Schedule.from_ical reminder_schedule - reminder_day = schedule.first.day + def reminder_is_within_range? # IceCube converts negative or zero days to valid days (e.g. -1 becomes the last day of the month, 0 becomes 1) # The minimum check should no longer be necessary, but keeping it in case IceCube changes - if reminder_day < 0 || reminder_day > MAX_DAY_OF_MONTH - errors.add(:reminder_schedule, "Reminder day must be between #{MIN_DAY_OF_MONTH} and #{MAX_DAY_OF_MONTH}") + if date_or_week_day == "date" && date.to_i < MIN_DAY_OF_MONTH || date.to_i > MAX_DAY_OF_MONTH + errors.add(:date, "Reminder day must be between #{MIN_DAY_OF_MONTH} and #{MAX_DAY_OF_MONTH}") end end + + def create_schedule + schedule = IceCube::Schedule.new(Time.zone.now.to_date) + return nil if every_n_months.blank? || every_n_months.to_i.zero? + if date_or_week_day == "date" + schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months.to_i).day_of_month(date.to_i)) + else + schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months.to_i).day_of_week(day_of_week.to_i => [every_nth_day.to_i])) + end + schedule.to_ical + rescue + nil + end end diff --git a/app/models/organization.rb b/app/models/organization.rb index c092ca65fc..4aa5a3b210 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -20,7 +20,7 @@ # name :string # one_step_partner_invite :boolean default(FALSE), not null # partner_form_fields :text default([]), is an Array -# reminder_schedule :string saved in iCal format, eg "RRULE:FREQ=MONTHLY;BYMONTHDAY=14" +# reminder_schedule :string # repackage_essentials :boolean default(FALSE), not null # short_name :string # signature_for_distribution_pdf :boolean default(FALSE) @@ -110,6 +110,10 @@ def upcoming end end + before_save do + self.reminder_schedule = create_schedule + end + after_create do account_request&.update!(status: "admin_approved") end diff --git a/app/models/partner_group.rb b/app/models/partner_group.rb index fa8efa225c..acd8563ed6 100644 --- a/app/models/partner_group.rb +++ b/app/models/partner_group.rb @@ -5,7 +5,7 @@ # id :bigint not null, primary key # deadline_day :integer # name :string -# reminder_schedule :string saved in iCal format, eg "RRULE:FREQ=MONTHLY;BYMONTHDAY=14" +# reminder_schedule :string # send_reminders :boolean default(FALSE), not null # created_at :datetime not null # updated_at :datetime not null @@ -19,7 +19,11 @@ class PartnerGroup < ApplicationRecord has_many :partners, dependent: :nullify has_and_belongs_to_many :item_categories + before_save do + self.reminder_schedule = create_schedule + end + validates :organization, presence: true validates :name, presence: true, uniqueness: { scope: :organization } - validates :deadline_day, :reminder_schedule, presence: true, if: :send_reminders? + validates :deadline_day, presence: true, if: :send_reminders? end diff --git a/app/models/reminder_schedule.rb b/app/models/reminder_schedule.rb deleted file mode 100644 index 8858812cef..0000000000 --- a/app/models/reminder_schedule.rb +++ /dev/null @@ -1,60 +0,0 @@ -class ReminderSchedule - include ActiveModel::Model - - attr_accessor :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day - attr_reader :every_nth_collection, :week_day_collection, :date_or_week_day_collection - - validates :every_n_months, presence: true, inclusion: 1..12 - validates :date_or_week_day, presence: true, inclusion: { in: %w[date week_day] } - validates :date, presence: true, if: -> { date_or_week_day == 'date' } - validates :day_of_week, presence: true, if: -> { date_or_week_day == 'week_day' }, inclusion: 1..7 - validates :every_nth_day, presence: true, if: -> { date_or_week_day == 'date' }, inclusion: 1..4 - - - def initialize(attributes = {}) - super - @every_nth_collection = EVERY_NTH_COLLECTION - @week_day_collection = WEEK_DAY_COLLECTION - @date_or_week_day_collection = DATE_OR_WEEK_DAY_COLLECTION - return if attributes.blank? - - @every_n_months = every_n_months.to_i - @date = date.to_i - @day_of_week = day_of_week.to_i - @every_nth_day = every_nth_day.to_i - end - - def create_schedule - schedule = IceCube::Schedule.new(Time.zone.now.to_date) - if date_or_week_day == 'date' - schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months).day_of_month(date)) - else - schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months).day_of_week(day_of_week => [every_nth_day])) - end - schedule.to_ical - end - - def self.show_description(ical) - schedule = IceCube::Schedule.from_ical(ical) - schedule.recurrence_rules.first.to_s - end - - def self.from_ical(ical) - schedule = IceCube::Schedule.from_ical(ical) - rule = schedule.recurrence_rules.first.instance_values - date = rule["validations"][:day_of_month]&.first&.value - new( - every_n_months: rule['interval'], - date_or_week_day: date ? 'date' : 'week_day', - date: date, - day_of_week: rule["validations"][:day_of_week]&.first&.day, - every_nth_day: rule["validations"][:day_of_week]&.first&.occ - ) - end - - private - EVERY_NTH_COLLECTION = [['First', 1], ['Second', 2], ['Third', 3], ['Fourth', 4]].freeze - WEEK_DAY_COLLECTION = [['Sunday'], ['Monday', 1], ['Tuesday', 2], ['Wednesday', 3], ['Thursday', 4], ['Friday', 5], ['Saturday', 6]].freeze - DATE_OR_WEEK_DAY_COLLECTION = [['date', 'Date'] ,['week_day', 'Day of the Week']].freeze - -end diff --git a/app/services/organization_update_service.rb b/app/services/organization_update_service.rb index 0b5b3c72c6..4729b78318 100644 --- a/app/services/organization_update_service.rb +++ b/app/services/organization_update_service.rb @@ -11,7 +11,6 @@ class << self # @return [Boolean] def update(organization, params) return false unless valid?(organization, params) - if params.has_key?("partner_form_fields") params["partner_form_fields"].delete_if { |field| field == "" } end diff --git a/app/services/partners/fetch_partners_to_remind_now_service.rb b/app/services/partners/fetch_partners_to_remind_now_service.rb index f61187f140..c6e792fd9c 100644 --- a/app/services/partners/fetch_partners_to_remind_now_service.rb +++ b/app/services/partners/fetch_partners_to_remind_now_service.rb @@ -19,6 +19,7 @@ def fetch .where(partner_groups: {reminder_schedule: nil}) .where(send_reminders: true) .where.not(organizations: {deadline_day: nil}) + .where.not(organizations: {reminder_schedule: nil}) .where.not(status: deactivated_status) filtered_organizations = partners_with_only_organization_reminders.select do |partner| diff --git a/app/views/organizations/_details.html.erb b/app/views/organizations/_details.html.erb index 110923d3fa..edb1135bca 100644 --- a/app/views/organizations/_details.html.erb +++ b/app/views/organizations/_details.html.erb @@ -66,7 +66,7 @@

Reminder Schedule

<%= fa_icon "calendar" %> - <%= @organization.reminder_schedule.blank? ? 'Not defined' : ReminderSchedule.show_description(@organization.reminder_schedule) %> + <%= @organization.reminder_schedule.blank? ? 'Not defined' : @organization.show_description(@organization.reminder_schedule) %>

diff --git a/app/views/partners/_partner_groups_table.html.erb b/app/views/partners/_partner_groups_table.html.erb index 669981d1aa..0fa3730f48 100644 --- a/app/views/partners/_partner_groups_table.html.erb +++ b/app/views/partners/_partner_groups_table.html.erb @@ -45,8 +45,10 @@ <% if pg.send_reminders %> - Reminder emails are sent <%= ReminderSchedule.show_description(pg.reminder_schedule) %>. -
+ <% if pg.reminder_schedule.present? %> + Reminder emails are sent <%= pg.show_description(pg.reminder_schedule) %>. +
+ <% end %> Deadlines are the <%= pg.deadline_day.ordinalize %> of every month. <% else %> No diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index dd856ed097..2f79d13a0e 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -1,68 +1,62 @@ <%= tag.div(class: 'deadline-day-pickers', data: { - min: Deadlinable::MIN_DAY_OF_MONTH, - max: Deadlinable::MAX_DAY_OF_MONTH, current_day: Date.current.mday, current_month: Date::MONTHNAMES[Date.current.month], next_month: Date::MONTHNAMES[Date.current.next_month.month] }) do %> - <%= simple_fields_for @reminder_schedule do |t| %> - <%= t.input :every_n_months, + <%= f.input :every_n_months, as: :integer, label: 'Send reminders every X months', class: "deadline-day-pickers__reminder-day form-control", hint: "Enter the number of months between reminders", :placeholder => "1", - :input_html => {:style=> 'width: 100px'} - %> + :input_html => {:style=> 'width: 100px', :min => 0, :max => 12} %> - <%= t.label :date_or_week_day, 'Send reminders on a specific date (eg "the 5th") or a day of the week (eg "the first Tuesday")?' %> + <%= f.label :date_or_week_day, 'Send reminders on a specific date (eg "the 5th") or a day of the week (eg "the first Tuesday")?' %>
- <%= t.radio_button :date_or_week_day, 'date', label: 'Date'%> - <%= t.label :date_or_week_day, 'Date' %> + <%= f.radio_button :date_or_week_day, 'date', label: 'Date', id: 'toggle-to-date' %> + <%= f.label :date_or_week_day, 'Date' %>
- <%= t.radio_button :date_or_week_day, 'week_day', label: 'Week Day' %> - <%= t.label :date_or_week_day, 'Week Day' %> + <%= f.radio_button :date_or_week_day, 'week_day', label: 'Week Day', id: 'toggle-to-week-day' %> + <%= f.label :date_or_week_day, 'Week Day' %> -
- <%= t.input :date, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, +
+ <%= f.input :date, as: :integer, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, label: 'Reminder date' do %> - <%= t.number_field :date, + <%= f.number_field :date, + min: Deadlinable::MIN_DAY_OF_MONTH, + max: Deadlinable::MAX_DAY_OF_MONTH, class: "deadline-day-pickers__reminder-day form-control", - placeholder: "Reminder day", - :input_html => {:style=> 'width: 100px'} - %> + placeholder: "Reminder day" %> <% end %>
-
- <%= t.input :every_nth_day, wrapper: :input_group, wrapper_html: { class: 'mb-3'}, +
+ <%= f.input :every_nth_day, wrapper: :input_group, wrapper_html: { class: 'mb-3'}, label: 'Reminder day of the week' do %> - <%= t.input :every_nth_day, collection: t.object.every_nth_collection, + <%= f.input :every_nth_day, collection: Deadlinable::EVERY_NTH_COLLECTION, class: "deadline-day-pickers__reminder-day form-control", - label: false, default: 1, - :input_html => {} - %> + label: false %> - <%= t.input :day_of_week, collection: t.object.week_day_collection, + <%= f.input :day_of_week, collection: Deadlinable::WEEK_DAY_COLLECTION, class: "deadline-day-pickers__reminder-day form-control", label: false, show_blank: true, default: 1, - :input_html => {:style=> 'width: 200px'} - %> + :input_html => {:style=> 'width: 200px'} %>
<% end %> - <% end %>
- <%= f.input :deadline_day, wrapper: :input_group, wrapper_html: { class: 'mb-0' }, + <%= f.input :deadline_day, wrapper: :input_group, wrapper_html: { class: 'mb-0', min: 0, max: 28 }, label: 'Deadline day (Final day of month to submit requests)' do %> <%= f.number_field :deadline_day, + min: Deadlinable::MIN_DAY_OF_MONTH, + max: Deadlinable::MAX_DAY_OF_MONTH, class: "deadline-day-pickers__deadline-day form-control", placeholder: "Deadline day" %> <% end %> diff --git a/db/migrate/20240715163348_remove_reminder_day_from_organizations.rb b/db/migrate/20240715163348_remove_reminder_day_from_organizations.rb index db0d3e0dfc..df697e0c90 100644 --- a/db/migrate/20240715163348_remove_reminder_day_from_organizations.rb +++ b/db/migrate/20240715163348_remove_reminder_day_from_organizations.rb @@ -1,6 +1,6 @@ class RemoveReminderDayFromOrganizations < ActiveRecord::Migration[7.1] def change - safety_assured { remove_column :organizations, :reminder_day } - safety_assured { remove_column :partner_groups, :reminder_day } + safety_assured { remove_column :organizations, :reminder_day, :integer } + safety_assured { remove_column :partner_groups, :reminder_day, :integer } end end diff --git a/spec/models/concerns/deadlinable_spec.rb b/spec/models/concerns/deadlinable_spec.rb index 3ca33d6dc0..f2cb16ae3f 100644 --- a/spec/models/concerns/deadlinable_spec.rb +++ b/spec/models/concerns/deadlinable_spec.rb @@ -35,20 +35,40 @@ def deadline_day? .allow_nil end + it 'validates the date field presence if date_or_week_day is "date"' do + dummy.every_n_months = 1 + dummy.date_or_week_day = "date" + is_expected.to validate_presence_of(:date) + end + + it 'validate the day_of_week field presence if date_or_week_day is "week_day"' do + dummy.every_n_months = 1 + dummy.date_or_week_day = "week_day" + is_expected.to validate_presence_of(:day_of_week) + is_expected.to validate_presence_of(:every_nth_day) + end + + it "validates the date_or_week_day field inclusion" do + dummy.every_n_months = 1 + is_expected.to validate_inclusion_of(:date_or_week_day).in_array(%w[date week_day]) + end + it "validates that the reminder schedule's date fall within the range" do - schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(31) - dummy.reminder_schedule = schedule.to_ical + dummy.every_n_months = 1 + dummy.date_or_week_day = "date" + dummy.date = 29 expect(dummy).not_to be_valid - expect(dummy.errors.added?(:reminder_schedule, "Reminder day must be between 1 and 28")).to be_truthy + expect(dummy.errors.added?(:date, "Reminder day must be between 1 and 28")).to be_truthy end it "validates that reminder day is not the same as deadline day" do - schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(7) - dummy.reminder_schedule = schedule.to_ical + dummy.every_n_months = 1 + dummy.date_or_week_day = "date" + dummy.date = dummy.deadline_day expect(dummy).not_to be_valid - expect(dummy.errors.added?(:reminder_schedule, "Reminder must not be the same as deadline date")).to be_truthy + expect(dummy.errors.added?(:date, "Reminder must not be the same as deadline date")).to be_truthy end end end diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index eca78f7b48..3af5ca89b4 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -448,17 +448,11 @@ end describe 'reminder_schedule' do - it "cannot exceed 28" do - schedule = IceCube::Schedule.new(Date.new(2022, 1, 1)) - valid_days = [1, 28] - valid_days.each do |day| - schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(day) - expect(build(:organization, reminder_schedule: schedule.to_ical)).to be_valid - schedule.remove_recurrence_rule(schedule.recurrence_rules.first) - end - schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(29) - expect(build(:organization, reminder_schedule: schedule.to_ical)).to_not be_valid - schedule.remove_recurrence_rule(schedule.recurrence_rules.first) + it "cannot exceed 28 if date_or_week_day is date" do + expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: 28)).to be_valid + expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: 29)).to_not be_valid + expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: 0)).to_not be_valid + expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: -5)).to_not be_valid end end describe 'deadline_day' do diff --git a/spec/models/partner_group_spec.rb b/spec/models/partner_group_spec.rb index 14cdb2900b..76a2061eb4 100644 --- a/spec/models/partner_group_spec.rb +++ b/spec/models/partner_group_spec.rb @@ -28,17 +28,16 @@ end end - describe 'deadline_day <= 28' do + describe 'deadline_day > 28' do it 'raises error if unmet' do expect { partner_group.update_column(:deadline_day, 29) }.to raise_error(ActiveRecord::StatementInvalid) end end - describe 'reminder_schedule day <= 28' do + describe 'reminder_schedule day > 28 and <=0' do it 'raises error if unmet' do - schedule = IceCube::Schedule.new(Time.current) - schedule.add_recurrence_rule IceCube::Rule.monthly.day_of_month(30) - expect(build(:partner_group, reminder_schedule: schedule.to_ical)).to_not be_valid + expect { partner_group.update!(every_n_months: 1, date_or_week_day: 'date', date: 29) }.to raise_error(ActiveRecord::RecordInvalid) + expect { partner_group.update!(every_n_months: 1, date_or_week_day: 'date', date: -5) }.to raise_error(ActiveRecord::RecordInvalid) end end end diff --git a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb index 142dc5c6a2..8732777c81 100644 --- a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb +++ b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb @@ -4,9 +4,6 @@ describe ".fetch" do subject { described_class.new.fetch } let(:current_day) { 14 } - let(:schedule_1) { IceCube::Schedule.new(Date.new(2022, 6, 1)) } - let(:schedule_2) { IceCube::Schedule.new(Date.new(2022, 6, 1)) } - let(:schedule_3) { IceCube::Schedule.new(Date.new(2022, 5, 1)) } before { travel_to(Time.zone.local(2022, 6, current_day, 1, 1, 1)) } after { travel_back } @@ -15,37 +12,27 @@ context "that has an organization with a global reminder & deadline" do context "that is for today" do before do - schedule_1.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day) - partner.organization.update(reminder_schedule: schedule_1.to_ical) - partner.organization.update(deadline_day: current_day + 2) + partner.organization.update(every_n_months: 1, date_or_week_day: "date", date: current_day, deadline_day: current_day + 2) end it "should include that partner" do + schedule = IceCube::Schedule.from_ical partner.organization.reminder_schedule + expect(schedule.occurs_on?(Time.current)).to be_truthy expect(subject).to include(partner) end context "as matched by day of the week" do before do - schedule_2.add_recurrence_rule IceCube::Rule.monthly.day_of_week(tuesday: [2]) - partner.organization.update(reminder_schedule: schedule_2.to_ical) + partner.organization.update(every_n_months: 1, date_or_week_day: "week_day", + day_of_week: 2, every_nth_day: 2, deadline_day: current_day + 2) end it "should include that partner" do - expect Time.current.day == 2 + schedule = IceCube::Schedule.from_ical partner.organization.reminder_schedule + expect(schedule.occurs_on?(Time.current)).to be_truthy expect(subject).to include(partner) end end - context "but the reoccurrence rule is not for the current month" do - before do - schedule_3.add_recurrence_rule IceCube::Rule.monthly(2).day_of_month(current_day) - partner.organization.update(reminder_schedule: schedule_3.to_ical) - end - - it "should NOT include that partner" do - expect(subject).not_to include(partner) - end - end - context "but the partner is deactivated" do before do partner.deactivated! @@ -69,9 +56,8 @@ context "that is not for today" do before do - new_schedule = IceCube::Schedule.new(Date.new(2022, 6, current_day - 1)).to_ical - partner.organization.update(reminder_schedule: new_schedule) - partner.organization.update(deadline_day: current_day + 2) + partner.organization.update(every_n_months: 1, date_or_week_day: "date", + date: current_day - 1, deadline_day: current_day + 2) end it "should NOT include that partner" do @@ -81,12 +67,12 @@ context "AND a partner group that does have them defined" do before do - schedule_1.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day) - schedule_2.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day - 1) - partner_group = create(:partner_group, reminder_schedule: schedule_1.to_ical, deadline_day: current_day + 2) + partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", + date: current_day, deadline_day: current_day + 2) partner_group.partners << partner - partner.organization.update(reminder_schedule: schedule_2.to_ical) + partner.organization.update(every_n_months: 1, date_or_week_day: "date", + date: current_day - 1, deadline_day: current_day + 2) end it "should remind based on the partner group instead of the organization level reminder" do @@ -113,8 +99,8 @@ context "and is a part of a partner group that does have them defined" do context "that is for today" do before do - schedule_1.add_recurrence_rule IceCube::Rule.monthly.day_of_month(current_day) - partner_group = create(:partner_group, reminder_schedule: schedule_1.to_ical, deadline_day: current_day + 2) + partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", + date: current_day, deadline_day: current_day + 2) partner_group.partners << partner end @@ -135,8 +121,8 @@ context "that is not for today" do before do - new_schedule = IceCube::Schedule.new(Date.new(2022, 6, current_day - 1)) - partner_group = create(:partner_group, reminder_schedule: new_schedule.to_ical, deadline_day: current_day + 2) + partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", + date: current_day - 1, deadline_day: current_day + 2) partner_group.partners << partner end diff --git a/spec/system/admin/organizations_system_spec.rb b/spec/system/admin/organizations_system_spec.rb index aa94b8a792..b0fb5543e6 100644 --- a/spec/system/admin/organizations_system_spec.rb +++ b/spec/system/admin/organizations_system_spec.rb @@ -116,6 +116,10 @@ fill_in "organization_user_name", with: admin_user_params[:name] fill_in "organization_user_email", with: admin_user_params[:email] + fill_in "organization_every_n_months", with: 1 + choose 'toggle-to-date' + fill_in "organization_date", with: 1 + click_on "Save" end @@ -124,7 +128,6 @@ within("tr.#{org_params[:short_name]}") do first(:link, "View").click end - expect(page).to have_content(org_params[:name]) expect(page).to have_content("Remount") expect(page).to have_content("Front Royal") diff --git a/spec/system/organization_system_spec.rb b/spec/system/organization_system_spec.rb index 76b10ee645..0e3f372d43 100644 --- a/spec/system/organization_system_spec.rb +++ b/spec/system/organization_system_spec.rb @@ -68,10 +68,16 @@ end it "can set a reminder and a deadline day" do - fill_in "organization_reminder_day", with: 12 + # TODO: change here + fill_in "organization_every_n_months", with: 1 + choose 'toggle-to-week-day' + select "First", from: "organization_every_nth_day" + select "Friday", from: "organization_day_of_week" + fill_in "organization_deadline_day", with: 16 click_on "Save" expect(page.find(".alert")).to have_content "Updated" + expect(page).to have_content("Monthly on the 1st Friday") end it 'can select if the org repackages essentials' do diff --git a/spec/system/partner_system_spec.rb b/spec/system/partner_system_spec.rb index e27747f8da..ac1d84ec22 100644 --- a/spec/system/partner_system_spec.rb +++ b/spec/system/partner_system_spec.rb @@ -496,6 +496,13 @@ # Click on the second item category find("input#partner_group_item_category_ids_#{item_category_2.id}").click + # Opt in to sending deadline reminders + check 'Yes' + + fill_in "partner_group_every_n_months", with: 1, wait: page_content_wait + choose 'toggle-to-date' + fill_in "partner_group_date", with: 1 + fill_in "partner_group_deadline_day", with: 25 find_button('Add Partner Group').click assert page.has_content? 'Group Name', wait: page_content_wait @@ -531,6 +538,25 @@ refute page.has_content? item_category_1.name assert page.has_content? item_category_2.name end + + it 'should be able to edit a custom reminder schedule' do + visit partners_path + + click_on 'Groups' + assert page.has_content? existing_partner_group.name, wait: page_content_wait + + click_on 'Edit' + # Opt in to sending deadline reminders + check 'Yes' + fill_in "partner_group_every_n_months", with: 2, wait: page_content_wait + choose 'toggle-to-week-day' + select "Second", from: "partner_group_every_nth_day" + select "Thursday", from: "partner_group_day_of_week" + fill_in "partner_group_deadline_day", with: 24 + + find_button('Update Partner Group').click + assert page.has_content? 'Every 2 months on the 2nd Thursday', wait: page_content_wait + end end end end From 8b2524a5a73f7594a46a53b05048bcff87b04459 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Wed, 11 Sep 2024 14:38:14 -0600 Subject: [PATCH 06/10] 4473 - Makes same warning appear for reminder date Reintroduces the functionality of having warnings for having the reminder date the same as the deadline date. --- app/javascript/utils/deadline_day_pickers.js | 1 - app/views/shared/_deadline_day_fields.html.erb | 3 ++- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/javascript/utils/deadline_day_pickers.js b/app/javascript/utils/deadline_day_pickers.js index d538c4a76a..31a86dc459 100644 --- a/app/javascript/utils/deadline_day_pickers.js +++ b/app/javascript/utils/deadline_day_pickers.js @@ -28,7 +28,6 @@ $(document).ready(function () { if (reminder_day) { $(container).find(reminder_container_selector).find(server_validation_selector).remove(); - if (reminder_day === deadline_day) { $reminder_text.removeClass('text-muted').addClass('text-danger'); diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index 2f79d13a0e..5d036a7f59 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -22,7 +22,7 @@ <%= f.label :date_or_week_day, 'Week Day' %>
- <%= f.input :date, as: :integer, wrapper: :input_group, wrapper_html: { class: 'mb-3' }, + <%= f.input :date, as: :integer, wrapper: :input_group, label: 'Reminder date' do %> <%= f.number_field :date, @@ -32,6 +32,7 @@ placeholder: "Reminder day" %> <% end %>
+
<%= f.input :every_nth_day, wrapper: :input_group, wrapper_html: { class: 'mb-3'}, From bea7a031d7c6c1510b35ff1f42cf03142b0e9469 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Thu, 12 Sep 2024 10:38:06 -0600 Subject: [PATCH 07/10] Only create new schedule if details have changed We don't want to create a new schedule if nothing has changed, as it will reset the start date and potentially mess with those who have every 2/3/etc months reminders. --- .../admin/organizations_controller.rb | 4 +- app/controllers/organizations_controller.rb | 4 +- app/controllers/partner_groups_controller.rb | 2 +- app/models/concerns/deadlinable.rb | 40 +++++++++++++++---- app/models/organization.rb | 4 +- .../shared/_deadline_day_fields.html.erb | 1 - ...tch_partners_to_remind_now_service_spec.rb | 12 +++--- 7 files changed, 47 insertions(+), 20 deletions(-) diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb index 47ef000dc1..599e5d34ad 100644 --- a/app/controllers/admin/organizations_controller.rb +++ b/app/controllers/admin/organizations_controller.rb @@ -2,7 +2,7 @@ class Admin::OrganizationsController < AdminController def edit @organization = Organization.find(params[:id]) - @organization.from_ical(@organization.reminder_schedule) + @organization.get_values_from_reminder_schedule end def update @@ -31,7 +31,7 @@ def index def new @organization = Organization.new - @organization.from_ical(@organization.reminder_schedule) + @organization.get_values_from_reminder_schedule account_request = params[:token] && AccountRequest.get_by_identity_token(params[:token]) @user = User.new diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index c0de679c15..8f76630b2b 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -10,7 +10,7 @@ def show def edit @organization = current_organization - @organization.from_ical(@organization.reminder_schedule) + @organization.get_values_from_reminder_schedule end def update @@ -100,7 +100,7 @@ def organization_params :hide_value_columns_on_receipt, :hide_package_column_on_receipt, :signature_for_distribution_pdf, :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day, - partner_form_fields: [] + partner_form_fields: [], request_unit_names: [] ) end diff --git a/app/controllers/partner_groups_controller.rb b/app/controllers/partner_groups_controller.rb index a2a13f36fb..d86e9ab4b0 100644 --- a/app/controllers/partner_groups_controller.rb +++ b/app/controllers/partner_groups_controller.rb @@ -17,7 +17,7 @@ def create def edit @partner_group = current_organization.partner_groups.find(params[:id]) - @partner_group.from_ical(@partner_group.reminder_schedule) + @partner_group.get_values_from_reminder_schedule @item_categories = current_organization.item_categories end diff --git a/app/models/concerns/deadlinable.rb b/app/models/concerns/deadlinable.rb index 4041e51a11..3d10705410 100644 --- a/app/models/concerns/deadlinable.rb +++ b/app/models/concerns/deadlinable.rb @@ -3,7 +3,7 @@ module Deadlinable MIN_DAY_OF_MONTH = 1 MAX_DAY_OF_MONTH = 28 EVERY_NTH_COLLECTION = [["First", 1], ["Second", 2], ["Third", 3], ["Fourth", 4]].freeze - WEEK_DAY_COLLECTION = [["Sunday"], ["Monday", 1], ["Tuesday", 2], ["Wednesday", 3], ["Thursday", 4], ["Friday", 5], ["Saturday", 6]].freeze + WEEK_DAY_COLLECTION = [["Sunday", 0], ["Monday", 1], ["Tuesday", 2], ["Wednesday", 3], ["Thursday", 4], ["Friday", 5], ["Saturday", 6]].freeze included do attr_accessor :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day @@ -14,7 +14,7 @@ module Deadlinable validate :reminder_is_within_range?, if: -> { every_n_months.present? } validates :date_or_week_day, inclusion: {in: %w[date week_day]}, if: -> { every_n_months.present? } validates :date, presence: true, if: -> { date_or_week_day == "date" && every_n_months.present? } - validates :day_of_week, presence: true, if: -> { date_or_week_day == "week_day" && every_n_months.present? }, inclusion: {in: %w[1 2 3 4 5 6 7]} + validates :day_of_week, presence: true, if: -> { date_or_week_day == "week_day" && every_n_months.present? }, inclusion: {in: %w[0 1 2 3 4 5 6]} validates :every_nth_day, presence: true, if: -> { date_or_week_day == "week_day" && every_n_months.present? }, inclusion: {in: %w[1 2 3 4]} end @@ -34,15 +34,29 @@ def from_ical(ical) schedule = IceCube::Schedule.from_ical(ical) rule = schedule.recurrence_rules.first.instance_values date = rule["validations"][:day_of_month]&.first&.value - self.every_n_months = rule["interval"] - self.date_or_week_day = date ? "date" : "week_day" - self.date = date - self.day_of_week = rule["validations"][:day_of_week]&.first&.day, - self.every_nth_day = rule["validations"][:day_of_week]&.first&.occ + + results = {} + results[:every_n_months] = rule["interval"] + results[:date_or_week_day] = date ? "date" : "week_day" + results[:date] = date + results[:day_of_week] = rule["validations"][:day_of_week]&.first&.day + results[:every_nth_day] = rule["validations"][:day_of_week]&.first&.occ + results rescue nil end + def get_values_from_reminder_schedule + return if reminder_schedule.blank? + results = from_ical(reminder_schedule) + return if results.nil? + self.every_n_months = results[:every_n_months] + self.date_or_week_day = results[:date_or_week_day] + self.date = results[:date] + self.day_of_week = results[:day_of_week] + self.every_nth_day = results[:every_nth_day] + end + private def reminder_on_deadline_day? @@ -59,6 +73,18 @@ def reminder_is_within_range? end end + def should_update_reminder_schedule + if reminder_schedule.blank? + return every_n_months.present? + end + sched = from_ical(reminder_schedule) + every_n_months != sched[:every_n_months].presence.to_s || + date_or_week_day != sched[:date_or_week_day].presence.to_s || + date != sched[:date].presence.to_s || + day_of_week != sched[:day_of_week].presence.to_s || + every_nth_day != sched[:every_nth_day].presence.to_s + end + def create_schedule schedule = IceCube::Schedule.new(Time.zone.now.to_date) return nil if every_n_months.blank? || every_n_months.to_i.zero? diff --git a/app/models/organization.rb b/app/models/organization.rb index a8c9071ef8..56b4686937 100644 --- a/app/models/organization.rb +++ b/app/models/organization.rb @@ -111,7 +111,9 @@ def upcoming end before_save do - self.reminder_schedule = create_schedule + if should_update_reminder_schedule + self.reminder_schedule = create_schedule + end end after_create do diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index 5d036a7f59..2cb91f12e9 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -39,7 +39,6 @@ label: 'Reminder day of the week' do %> <%= f.input :every_nth_day, collection: Deadlinable::EVERY_NTH_COLLECTION, class: "deadline-day-pickers__reminder-day form-control", - default: 1, label: false %> <%= f.input :day_of_week, collection: Deadlinable::WEEK_DAY_COLLECTION, diff --git a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb index c753354035..f576cb97b9 100644 --- a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb +++ b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb @@ -22,7 +22,7 @@ context "as matched by day of the week" do before do partner.organization.update(every_n_months: 1, date_or_week_day: "week_day", - day_of_week: 2, every_nth_day: 2, deadline_day: current_day + 2) + day_of_week: 2, every_nth_day: 2, deadline_day: current_day + 2) end it "should include that partner" do schedule = IceCube::Schedule.from_ical partner.organization.reminder_schedule @@ -55,7 +55,7 @@ context "that is not for today" do before do partner.organization.update(every_n_months: 1, date_or_week_day: "date", - date: current_day - 1, deadline_day: current_day + 2) + date: current_day - 1, deadline_day: current_day + 2) end it "should NOT include that partner" do @@ -66,11 +66,11 @@ context "AND a partner group that does have them defined" do before do partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", - date: current_day, deadline_day: current_day + 2) + date: current_day, deadline_day: current_day + 2) partner_group.partners << partner partner.organization.update(every_n_months: 1, date_or_week_day: "date", - date: current_day - 1, deadline_day: current_day + 2) + date: current_day - 1, deadline_day: current_day + 2) end it "should remind based on the partner group instead of the organization level reminder" do @@ -98,7 +98,7 @@ context "that is for today" do before do partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", - date: current_day, deadline_day: current_day + 2) + date: current_day, deadline_day: current_day + 2) partner_group.partners << partner end @@ -120,7 +120,7 @@ context "that is not for today" do before do partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", - date: current_day - 1, deadline_day: current_day + 2) + date: current_day - 1, deadline_day: current_day + 2) partner_group.partners << partner end From a0d1a658292d9938ac153e1c539504769463308c Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Thu, 19 Sep 2024 14:40:54 -0600 Subject: [PATCH 08/10] 4473 - Removes Every N Months All reminders should be Monthly, so every N Months is removed. Also allows users to select "Last" as an option. --- .../admin/organizations_controller.rb | 2 +- app/controllers/organizations_controller.rb | 4 +-- app/controllers/partner_groups_controller.rb | 3 +- app/models/concerns/deadlinable.rb | 30 +++++++++---------- .../shared/_deadline_day_fields.html.erb | 8 ----- spec/models/concerns/deadlinable_spec.rb | 27 +++++++++-------- spec/models/organization_spec.rb | 8 ++--- spec/models/partner_group_spec.rb | 4 +-- ...tch_partners_to_remind_now_service_spec.rb | 14 ++++----- .../system/admin/organizations_system_spec.rb | 1 - spec/system/partner_system_spec.rb | 6 ++-- 11 files changed, 47 insertions(+), 60 deletions(-) diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb index 6c7b02337b..9f92aa5614 100644 --- a/app/controllers/admin/organizations_controller.rb +++ b/app/controllers/admin/organizations_controller.rb @@ -84,7 +84,7 @@ def destroy def organization_params params.require(:organization) .permit(:name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, - :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day, :deadline_day, + :date_or_week_day, :date, :day_of_week, :every_nth_day, :deadline_day, users_attributes: %i(name email organization_admin), account_request_attributes: %i(ndbn_member_id id)) end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index 8f76630b2b..bbde50403d 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -98,8 +98,8 @@ def organization_params :enable_individual_requests, :enable_quantity_based_requests, :ytd_on_distribution_printout, :one_step_partner_invite, :hide_value_columns_on_receipt, :hide_package_column_on_receipt, - :signature_for_distribution_pdf, :every_n_months, - :date_or_week_day, :date, :day_of_week, :every_nth_day, + :signature_for_distribution_pdf, :date_or_week_day, :date, :day_of_week, + :every_nth_day, partner_form_fields: [], request_unit_names: [] ) diff --git a/app/controllers/partner_groups_controller.rb b/app/controllers/partner_groups_controller.rb index d86e9ab4b0..a94d0a1306 100644 --- a/app/controllers/partner_groups_controller.rb +++ b/app/controllers/partner_groups_controller.rb @@ -35,7 +35,6 @@ def update def partner_group_params params.require(:partner_group).permit(:name, :send_reminders, :reminder_schedule, - :deadline_day, :every_n_months, :date_or_week_day, - :date, :day_of_week, :every_nth_day, item_category_ids: []) + :deadline_day, :date_or_week_day, :date, :day_of_week, :every_nth_day, item_category_ids: []) end end diff --git a/app/models/concerns/deadlinable.rb b/app/models/concerns/deadlinable.rb index 3d10705410..2a9fd6d063 100644 --- a/app/models/concerns/deadlinable.rb +++ b/app/models/concerns/deadlinable.rb @@ -2,20 +2,19 @@ module Deadlinable extend ActiveSupport::Concern MIN_DAY_OF_MONTH = 1 MAX_DAY_OF_MONTH = 28 - EVERY_NTH_COLLECTION = [["First", 1], ["Second", 2], ["Third", 3], ["Fourth", 4]].freeze + EVERY_NTH_COLLECTION = [["First", 1], ["Second", 2], ["Third", 3], ["Fourth", 4], ["Last", -1]].freeze WEEK_DAY_COLLECTION = [["Sunday", 0], ["Monday", 1], ["Tuesday", 2], ["Wednesday", 3], ["Thursday", 4], ["Friday", 5], ["Saturday", 6]].freeze included do - attr_accessor :every_n_months, :date_or_week_day, :date, :day_of_week, :every_nth_day + attr_accessor :date_or_week_day, :date, :day_of_week, :every_nth_day attr_reader :every_nth_collection, :week_day_collection, :date_or_week_day_collection validates :deadline_day, numericality: {only_integer: true, less_than_or_equal_to: MAX_DAY_OF_MONTH, greater_than_or_equal_to: MIN_DAY_OF_MONTH, allow_nil: true} - validate :reminder_on_deadline_day?, if: -> { every_n_months.present? } - validate :reminder_is_within_range?, if: -> { every_n_months.present? } - validates :date_or_week_day, inclusion: {in: %w[date week_day]}, if: -> { every_n_months.present? } - validates :date, presence: true, if: -> { date_or_week_day == "date" && every_n_months.present? } - validates :day_of_week, presence: true, if: -> { date_or_week_day == "week_day" && every_n_months.present? }, inclusion: {in: %w[0 1 2 3 4 5 6]} - validates :every_nth_day, presence: true, if: -> { date_or_week_day == "week_day" && every_n_months.present? }, inclusion: {in: %w[1 2 3 4]} + validate :reminder_on_deadline_day?, if: -> { date.present? } + validate :reminder_is_within_range?, if: -> { date.present? } + validates :date_or_week_day, inclusion: {in: %w[date week_day]}, if: -> { date_or_week_day.present? } + validates :day_of_week, if: -> { day_of_week.present? }, inclusion: {in: %w[0 1 2 3 4 5 6]} + validates :every_nth_day, if: -> { every_nth_day.present? }, inclusion: {in: %w[1 2 3 4 -1]} end def convert_to_reminder_schedule(day) @@ -36,7 +35,6 @@ def from_ical(ical) date = rule["validations"][:day_of_month]&.first&.value results = {} - results[:every_n_months] = rule["interval"] results[:date_or_week_day] = date ? "date" : "week_day" results[:date] = date results[:day_of_week] = rule["validations"][:day_of_week]&.first&.day @@ -50,7 +48,6 @@ def get_values_from_reminder_schedule return if reminder_schedule.blank? results = from_ical(reminder_schedule) return if results.nil? - self.every_n_months = results[:every_n_months] self.date_or_week_day = results[:date_or_week_day] self.date = results[:date] self.day_of_week = results[:day_of_week] @@ -75,11 +72,10 @@ def reminder_is_within_range? def should_update_reminder_schedule if reminder_schedule.blank? - return every_n_months.present? + return date_or_week_day.present? end sched = from_ical(reminder_schedule) - every_n_months != sched[:every_n_months].presence.to_s || - date_or_week_day != sched[:date_or_week_day].presence.to_s || + date_or_week_day != sched[:date_or_week_day].presence.to_s || date != sched[:date].presence.to_s || day_of_week != sched[:day_of_week].presence.to_s || every_nth_day != sched[:every_nth_day].presence.to_s @@ -87,11 +83,13 @@ def should_update_reminder_schedule def create_schedule schedule = IceCube::Schedule.new(Time.zone.now.to_date) - return nil if every_n_months.blank? || every_n_months.to_i.zero? + return nil if date_or_week_day.blank? if date_or_week_day == "date" - schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months.to_i).day_of_month(date.to_i)) + return nil if date.blank? + schedule.add_recurrence_rule(IceCube::Rule.monthly(1).day_of_month(date.to_i)) else - schedule.add_recurrence_rule(IceCube::Rule.monthly(every_n_months.to_i).day_of_week(day_of_week.to_i => [every_nth_day.to_i])) + return nil if day_of_week.blank? || every_nth_day.blank? + schedule.add_recurrence_rule(IceCube::Rule.monthly(1).day_of_week(day_of_week.to_i => [every_nth_day.to_i])) end schedule.to_ical rescue diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index 2cb91f12e9..7097e188a2 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -5,14 +5,6 @@ next_month: Date::MONTHNAMES[Date.current.next_month.month] }) do %> - <%= f.input :every_n_months, - as: :integer, - label: 'Send reminders every X months', - class: "deadline-day-pickers__reminder-day form-control", - hint: "Enter the number of months between reminders", - :placeholder => "1", - :input_html => {:style=> 'width: 100px', :min => 0, :max => 12} %> - <%= f.label :date_or_week_day, 'Send reminders on a specific date (eg "the 5th") or a day of the week (eg "the first Tuesday")?' %>
<%= f.radio_button :date_or_week_day, 'date', label: 'Date', id: 'toggle-to-date' %> diff --git a/spec/models/concerns/deadlinable_spec.rb b/spec/models/concerns/deadlinable_spec.rb index aeb37e259d..869f9ae5db 100644 --- a/spec/models/concerns/deadlinable_spec.rb +++ b/spec/models/concerns/deadlinable_spec.rb @@ -33,35 +33,36 @@ def deadline_day? .allow_nil end - it 'validates the date field presence if date_or_week_day is "date"' do - dummy.every_n_months = 1 - dummy.date_or_week_day = "date" - is_expected.to validate_presence_of(:date) + it "validates the date_or_week_day field inclusion" do + is_expected.to validate_inclusion_of(:date_or_week_day).in_array(%w[date week_day]) end - it 'validate the day_of_week field presence if date_or_week_day is "week_day"' do - dummy.every_n_months = 1 - dummy.date_or_week_day = "week_day" - is_expected.to validate_presence_of(:day_of_week) - is_expected.to validate_presence_of(:every_nth_day) + it "validates the day of week field inclusion" do + dummy.day_of_week = "0" + expect(dummy).to be_valid + dummy.day_of_week = "A" + expect(dummy).not_to be_valid end it "validates the date_or_week_day field inclusion" do - dummy.every_n_months = 1 - is_expected.to validate_inclusion_of(:date_or_week_day).in_array(%w[date week_day]) + dummy.every_nth_day = "1" + expect(dummy).to be_valid + dummy.every_nth_day = "B" + expect(dummy).not_to be_valid end it "validates that the reminder schedule's date fall within the range" do - dummy.every_n_months = 1 dummy.date_or_week_day = "date" dummy.date = 29 expect(dummy).not_to be_valid expect(dummy.errors.added?(:date, "Reminder day must be between 1 and 28")).to be_truthy + + dummy.date = -1 + expect(dummy).not_to be_valid end it "validates that reminder day is not the same as deadline day" do - dummy.every_n_months = 1 dummy.date_or_week_day = "date" dummy.date = dummy.deadline_day diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 3af5ca89b4..3de14f631c 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -449,10 +449,10 @@ describe 'reminder_schedule' do it "cannot exceed 28 if date_or_week_day is date" do - expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: 28)).to be_valid - expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: 29)).to_not be_valid - expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: 0)).to_not be_valid - expect(build(:organization, every_n_months: 1, date_or_week_day: 'date', date: -5)).to_not be_valid + expect(build(:organization, date_or_week_day: 'date', date: 28)).to be_valid + expect(build(:organization, date_or_week_day: 'date', date: 29)).to_not be_valid + expect(build(:organization, date_or_week_day: 'date', date: 0)).to_not be_valid + expect(build(:organization, date_or_week_day: 'date', date: -5)).to_not be_valid end end describe 'deadline_day' do diff --git a/spec/models/partner_group_spec.rb b/spec/models/partner_group_spec.rb index 76a2061eb4..2a62000621 100644 --- a/spec/models/partner_group_spec.rb +++ b/spec/models/partner_group_spec.rb @@ -36,8 +36,8 @@ describe 'reminder_schedule day > 28 and <=0' do it 'raises error if unmet' do - expect { partner_group.update!(every_n_months: 1, date_or_week_day: 'date', date: 29) }.to raise_error(ActiveRecord::RecordInvalid) - expect { partner_group.update!(every_n_months: 1, date_or_week_day: 'date', date: -5) }.to raise_error(ActiveRecord::RecordInvalid) + expect { partner_group.update!(date_or_week_day: 'date', date: 29) }.to raise_error(ActiveRecord::RecordInvalid) + expect { partner_group.update!(date_or_week_day: 'date', date: -5) }.to raise_error(ActiveRecord::RecordInvalid) end end end diff --git a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb index f576cb97b9..6d666fa54d 100644 --- a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb +++ b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb @@ -10,7 +10,7 @@ context "that has an organization with a global reminder & deadline" do context "that is for today" do before do - partner.organization.update(every_n_months: 1, date_or_week_day: "date", date: current_day, deadline_day: current_day + 2) + partner.organization.update(date_or_week_day: "date", date: current_day, deadline_day: current_day + 2) end it "should include that partner" do @@ -21,7 +21,7 @@ context "as matched by day of the week" do before do - partner.organization.update(every_n_months: 1, date_or_week_day: "week_day", + partner.organization.update(date_or_week_day: "week_day", day_of_week: 2, every_nth_day: 2, deadline_day: current_day + 2) end it "should include that partner" do @@ -54,7 +54,7 @@ context "that is not for today" do before do - partner.organization.update(every_n_months: 1, date_or_week_day: "date", + partner.organization.update(date_or_week_day: "date", date: current_day - 1, deadline_day: current_day + 2) end @@ -65,11 +65,11 @@ context "AND a partner group that does have them defined" do before do - partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", + partner_group = create(:partner_group, date_or_week_day: "date", date: current_day, deadline_day: current_day + 2) partner_group.partners << partner - partner.organization.update(every_n_months: 1, date_or_week_day: "date", + partner.organization.update(date_or_week_day: "date", date: current_day - 1, deadline_day: current_day + 2) end @@ -97,7 +97,7 @@ context "and is a part of a partner group that does have them defined" do context "that is for today" do before do - partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", + partner_group = create(:partner_group, date_or_week_day: "date", date: current_day, deadline_day: current_day + 2) partner_group.partners << partner end @@ -119,7 +119,7 @@ context "that is not for today" do before do - partner_group = create(:partner_group, every_n_months: 1, date_or_week_day: "date", + partner_group = create(:partner_group, date_or_week_day: "date", date: current_day - 1, deadline_day: current_day + 2) partner_group.partners << partner end diff --git a/spec/system/admin/organizations_system_spec.rb b/spec/system/admin/organizations_system_spec.rb index b0fb5543e6..05c41b5de7 100644 --- a/spec/system/admin/organizations_system_spec.rb +++ b/spec/system/admin/organizations_system_spec.rb @@ -116,7 +116,6 @@ fill_in "organization_user_name", with: admin_user_params[:name] fill_in "organization_user_email", with: admin_user_params[:email] - fill_in "organization_every_n_months", with: 1 choose 'toggle-to-date' fill_in "organization_date", with: 1 diff --git a/spec/system/partner_system_spec.rb b/spec/system/partner_system_spec.rb index 4e436bf082..4bb2b397fd 100644 --- a/spec/system/partner_system_spec.rb +++ b/spec/system/partner_system_spec.rb @@ -499,7 +499,6 @@ # Opt in to sending deadline reminders check 'Yes' - fill_in "partner_group_every_n_months", with: 1, wait: page_content_wait choose 'toggle-to-date' fill_in "partner_group_date", with: 1 fill_in "partner_group_deadline_day", with: 25 @@ -548,14 +547,13 @@ click_on 'Edit' # Opt in to sending deadline reminders check 'Yes' - fill_in "partner_group_every_n_months", with: 2, wait: page_content_wait - choose 'toggle-to-week-day' + choose 'toggle-to-week-day', wait: page_content_wait select "Second", from: "partner_group_every_nth_day" select "Thursday", from: "partner_group_day_of_week" fill_in "partner_group_deadline_day", with: 24 find_button('Update Partner Group').click - assert page.has_content? 'Every 2 months on the 2nd Thursday', wait: page_content_wait + assert page.has_content? 'Monthly on the 2nd Thursday', wait: page_content_wait end end end From a14719cfb262bfa4becb6833fb39c94073a485d0 Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Wed, 2 Oct 2024 10:04:30 -0600 Subject: [PATCH 09/10] 4473 - Updates names of Month vs Week fields Rather than "date" and "week_day", we are now using "day_of_month" and "day_of_week" for clarity. --- .../admin/organizations_controller.rb | 2 +- app/controllers/organizations_controller.rb | 2 +- app/controllers/partner_groups_controller.rb | 2 +- app/models/concerns/deadlinable.rb | 43 +++++++++---------- .../shared/_deadline_day_fields.html.erb | 16 +++---- spec/models/concerns/deadlinable_spec.rb | 20 ++++----- spec/models/organization_spec.rb | 10 ++--- spec/models/partner_group_spec.rb | 4 +- ...tch_partners_to_remind_now_service_spec.rb | 24 +++++------ .../system/admin/organizations_system_spec.rb | 2 +- spec/system/partner_system_spec.rb | 2 +- 11 files changed, 63 insertions(+), 64 deletions(-) diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb index 9f92aa5614..a3592f0c6d 100644 --- a/app/controllers/admin/organizations_controller.rb +++ b/app/controllers/admin/organizations_controller.rb @@ -84,7 +84,7 @@ def destroy def organization_params params.require(:organization) .permit(:name, :short_name, :street, :city, :state, :zipcode, :email, :url, :logo, :intake_location, :default_email_text, :account_request_id, - :date_or_week_day, :date, :day_of_week, :every_nth_day, :deadline_day, + :by_month_or_week, :day_of_month, :day_of_week, :every_nth_day, :deadline_day, users_attributes: %i(name email organization_admin), account_request_attributes: %i(ndbn_member_id id)) end diff --git a/app/controllers/organizations_controller.rb b/app/controllers/organizations_controller.rb index bbde50403d..cec0c5783d 100644 --- a/app/controllers/organizations_controller.rb +++ b/app/controllers/organizations_controller.rb @@ -98,7 +98,7 @@ def organization_params :enable_individual_requests, :enable_quantity_based_requests, :ytd_on_distribution_printout, :one_step_partner_invite, :hide_value_columns_on_receipt, :hide_package_column_on_receipt, - :signature_for_distribution_pdf, :date_or_week_day, :date, :day_of_week, + :signature_for_distribution_pdf, :by_month_or_week, :day_of_month, :day_of_week, :every_nth_day, partner_form_fields: [], request_unit_names: [] diff --git a/app/controllers/partner_groups_controller.rb b/app/controllers/partner_groups_controller.rb index a94d0a1306..eb51430059 100644 --- a/app/controllers/partner_groups_controller.rb +++ b/app/controllers/partner_groups_controller.rb @@ -35,6 +35,6 @@ def update def partner_group_params params.require(:partner_group).permit(:name, :send_reminders, :reminder_schedule, - :deadline_day, :date_or_week_day, :date, :day_of_week, :every_nth_day, item_category_ids: []) + :deadline_day, :by_month_or_week, :day_of_month, :day_of_week, :every_nth_day, item_category_ids: []) end end diff --git a/app/models/concerns/deadlinable.rb b/app/models/concerns/deadlinable.rb index 2a9fd6d063..5d1529b01a 100644 --- a/app/models/concerns/deadlinable.rb +++ b/app/models/concerns/deadlinable.rb @@ -3,16 +3,15 @@ module Deadlinable MIN_DAY_OF_MONTH = 1 MAX_DAY_OF_MONTH = 28 EVERY_NTH_COLLECTION = [["First", 1], ["Second", 2], ["Third", 3], ["Fourth", 4], ["Last", -1]].freeze - WEEK_DAY_COLLECTION = [["Sunday", 0], ["Monday", 1], ["Tuesday", 2], ["Wednesday", 3], ["Thursday", 4], ["Friday", 5], ["Saturday", 6]].freeze + DAY_OF_WEEK_COLLECTION = [["Sunday", 0], ["Monday", 1], ["Tuesday", 2], ["Wednesday", 3], ["Thursday", 4], ["Friday", 5], ["Saturday", 6]].freeze included do - attr_accessor :date_or_week_day, :date, :day_of_week, :every_nth_day - attr_reader :every_nth_collection, :week_day_collection, :date_or_week_day_collection + attr_accessor :by_month_or_week, :day_of_month, :day_of_week, :every_nth_day validates :deadline_day, numericality: {only_integer: true, less_than_or_equal_to: MAX_DAY_OF_MONTH, greater_than_or_equal_to: MIN_DAY_OF_MONTH, allow_nil: true} - validate :reminder_on_deadline_day?, if: -> { date.present? } - validate :reminder_is_within_range?, if: -> { date.present? } - validates :date_or_week_day, inclusion: {in: %w[date week_day]}, if: -> { date_or_week_day.present? } + validate :reminder_on_deadline_day?, if: -> { day_of_month.present? } + validate :reminder_is_within_range?, if: -> { day_of_month.present? } + validates :by_month_or_week, inclusion: {in: %w[day_of_month day_of_week]}, if: -> { by_month_or_week.present? } validates :day_of_week, if: -> { day_of_week.present? }, inclusion: {in: %w[0 1 2 3 4 5 6]} validates :every_nth_day, if: -> { every_nth_day.present? }, inclusion: {in: %w[1 2 3 4 -1]} end @@ -32,11 +31,11 @@ def from_ical(ical) return if ical.blank? schedule = IceCube::Schedule.from_ical(ical) rule = schedule.recurrence_rules.first.instance_values - date = rule["validations"][:day_of_month]&.first&.value + day_of_month = rule["validations"][:day_of_month]&.first&.value results = {} - results[:date_or_week_day] = date ? "date" : "week_day" - results[:date] = date + results[:by_month_or_week] = day_of_month ? "day_of_month" : "day_of_week" + results[:day_of_month] = day_of_month results[:day_of_week] = rule["validations"][:day_of_week]&.first&.day results[:every_nth_day] = rule["validations"][:day_of_week]&.first&.occ results @@ -48,8 +47,8 @@ def get_values_from_reminder_schedule return if reminder_schedule.blank? results = from_ical(reminder_schedule) return if results.nil? - self.date_or_week_day = results[:date_or_week_day] - self.date = results[:date] + self.by_month_or_week = results[:by_month_or_week] + self.day_of_month = results[:day_of_month] self.day_of_week = results[:day_of_week] self.every_nth_day = results[:every_nth_day] end @@ -57,36 +56,36 @@ def get_values_from_reminder_schedule private def reminder_on_deadline_day? - if date_or_week_day == "date" && date.to_i == deadline_day - errors.add(:date, "Reminder must not be the same as deadline date") + if by_month_or_week == "day_of_month" && day_of_month.to_i == deadline_day + errors.add(:day_of_month, "Reminder must not be the same as deadline date") end end def reminder_is_within_range? # IceCube converts negative or zero days to valid days (e.g. -1 becomes the last day of the month, 0 becomes 1) # The minimum check should no longer be necessary, but keeping it in case IceCube changes - if date_or_week_day == "date" && date.to_i < MIN_DAY_OF_MONTH || date.to_i > MAX_DAY_OF_MONTH - errors.add(:date, "Reminder day must be between #{MIN_DAY_OF_MONTH} and #{MAX_DAY_OF_MONTH}") + if by_month_or_week == "day_of_month" && day_of_month.to_i < MIN_DAY_OF_MONTH || day_of_month.to_i > MAX_DAY_OF_MONTH + errors.add(:day_of_month, "Reminder day must be between #{MIN_DAY_OF_MONTH} and #{MAX_DAY_OF_MONTH}") end end def should_update_reminder_schedule if reminder_schedule.blank? - return date_or_week_day.present? + return by_month_or_week.present? end sched = from_ical(reminder_schedule) - date_or_week_day != sched[:date_or_week_day].presence.to_s || - date != sched[:date].presence.to_s || + by_month_or_week != sched[:by_month_or_week].presence.to_s || + day_of_month != sched[:day_of_month].presence.to_s || day_of_week != sched[:day_of_week].presence.to_s || every_nth_day != sched[:every_nth_day].presence.to_s end def create_schedule schedule = IceCube::Schedule.new(Time.zone.now.to_date) - return nil if date_or_week_day.blank? - if date_or_week_day == "date" - return nil if date.blank? - schedule.add_recurrence_rule(IceCube::Rule.monthly(1).day_of_month(date.to_i)) + return nil if by_month_or_week.blank? + if by_month_or_week == "day_of_month" + return nil if day_of_month.blank? + schedule.add_recurrence_rule(IceCube::Rule.monthly(1).day_of_month(day_of_month.to_i)) else return nil if day_of_week.blank? || every_nth_day.blank? schedule.add_recurrence_rule(IceCube::Rule.monthly(1).day_of_week(day_of_week.to_i => [every_nth_day.to_i])) diff --git a/app/views/shared/_deadline_day_fields.html.erb b/app/views/shared/_deadline_day_fields.html.erb index 7097e188a2..6760f8efc5 100644 --- a/app/views/shared/_deadline_day_fields.html.erb +++ b/app/views/shared/_deadline_day_fields.html.erb @@ -5,19 +5,19 @@ next_month: Date::MONTHNAMES[Date.current.next_month.month] }) do %> - <%= f.label :date_or_week_day, 'Send reminders on a specific date (eg "the 5th") or a day of the week (eg "the first Tuesday")?' %> + <%= f.label :by_month_or_week, 'Send reminders on a specific day of the month (e.g. "the 5th") or a day of the week (eg "the first Tuesday")?' %>
- <%= f.radio_button :date_or_week_day, 'date', label: 'Date', id: 'toggle-to-date' %> - <%= f.label :date_or_week_day, 'Date' %> + <%= f.radio_button :by_month_or_week, 'day_of_month', label: 'Day of Month', id: 'toggle-to-date' %> + <%= f.label :by_month_or_week, 'Day of Month' %>
- <%= f.radio_button :date_or_week_day, 'week_day', label: 'Week Day', id: 'toggle-to-week-day' %> - <%= f.label :date_or_week_day, 'Week Day' %> + <%= f.radio_button :by_month_or_week, 'day_of_week', label: 'Day of the Week', id: 'toggle-to-week-day' %> + <%= f.label :by_month_or_week, 'Day of the Week' %>
- <%= f.input :date, as: :integer, wrapper: :input_group, + <%= f.input :day_of_month, as: :integer, wrapper: :input_group, label: 'Reminder date' do %> - <%= f.number_field :date, + <%= f.number_field :day_of_month, min: Deadlinable::MIN_DAY_OF_MONTH, max: Deadlinable::MAX_DAY_OF_MONTH, class: "deadline-day-pickers__reminder-day form-control", @@ -33,7 +33,7 @@ class: "deadline-day-pickers__reminder-day form-control", label: false %> - <%= f.input :day_of_week, collection: Deadlinable::WEEK_DAY_COLLECTION, + <%= f.input :day_of_week, collection: Deadlinable::DAY_OF_WEEK_COLLECTION, class: "deadline-day-pickers__reminder-day form-control", label: false, show_blank: true, diff --git a/spec/models/concerns/deadlinable_spec.rb b/spec/models/concerns/deadlinable_spec.rb index 869f9ae5db..4d56993c2f 100644 --- a/spec/models/concerns/deadlinable_spec.rb +++ b/spec/models/concerns/deadlinable_spec.rb @@ -33,8 +33,8 @@ def deadline_day? .allow_nil end - it "validates the date_or_week_day field inclusion" do - is_expected.to validate_inclusion_of(:date_or_week_day).in_array(%w[date week_day]) + it "validates the by_month_or_week field inclusion" do + is_expected.to validate_inclusion_of(:by_month_or_week).in_array(%w[day_of_month day_of_week]) end it "validates the day of week field inclusion" do @@ -44,7 +44,7 @@ def deadline_day? expect(dummy).not_to be_valid end - it "validates the date_or_week_day field inclusion" do + it "validates the by_month_or_week field inclusion" do dummy.every_nth_day = "1" expect(dummy).to be_valid dummy.every_nth_day = "B" @@ -52,22 +52,22 @@ def deadline_day? end it "validates that the reminder schedule's date fall within the range" do - dummy.date_or_week_day = "date" - dummy.date = 29 + dummy.by_month_or_week = "day_of_month" + dummy.day_of_month = 29 expect(dummy).not_to be_valid - expect(dummy.errors.added?(:date, "Reminder day must be between 1 and 28")).to be_truthy + expect(dummy.errors.added?(:day_of_month, "Reminder day must be between 1 and 28")).to be_truthy - dummy.date = -1 + dummy.day_of_month = -1 expect(dummy).not_to be_valid end it "validates that reminder day is not the same as deadline day" do - dummy.date_or_week_day = "date" - dummy.date = dummy.deadline_day + dummy.by_month_or_week = "day_of_month" + dummy.day_of_month = dummy.deadline_day expect(dummy).not_to be_valid - expect(dummy.errors.added?(:date, "Reminder must not be the same as deadline date")).to be_truthy + expect(dummy.errors.added?(:day_of_month, "Reminder must not be the same as deadline date")).to be_truthy end end end diff --git a/spec/models/organization_spec.rb b/spec/models/organization_spec.rb index 3de14f631c..289ab8ebc9 100644 --- a/spec/models/organization_spec.rb +++ b/spec/models/organization_spec.rb @@ -448,11 +448,11 @@ end describe 'reminder_schedule' do - it "cannot exceed 28 if date_or_week_day is date" do - expect(build(:organization, date_or_week_day: 'date', date: 28)).to be_valid - expect(build(:organization, date_or_week_day: 'date', date: 29)).to_not be_valid - expect(build(:organization, date_or_week_day: 'date', date: 0)).to_not be_valid - expect(build(:organization, date_or_week_day: 'date', date: -5)).to_not be_valid + it "cannot exceed 28 if by_month_or_week is day_of_month" do + expect(build(:organization, by_month_or_week: 'day_of_month', day_of_month: 28)).to be_valid + expect(build(:organization, by_month_or_week: 'day_of_month', day_of_month: 29)).to_not be_valid + expect(build(:organization, by_month_or_week: 'day_of_month', day_of_month: 0)).to_not be_valid + expect(build(:organization, by_month_or_week: 'day_of_month', day_of_month: -5)).to_not be_valid end end describe 'deadline_day' do diff --git a/spec/models/partner_group_spec.rb b/spec/models/partner_group_spec.rb index 2a62000621..493e3b5520 100644 --- a/spec/models/partner_group_spec.rb +++ b/spec/models/partner_group_spec.rb @@ -36,8 +36,8 @@ describe 'reminder_schedule day > 28 and <=0' do it 'raises error if unmet' do - expect { partner_group.update!(date_or_week_day: 'date', date: 29) }.to raise_error(ActiveRecord::RecordInvalid) - expect { partner_group.update!(date_or_week_day: 'date', date: -5) }.to raise_error(ActiveRecord::RecordInvalid) + expect { partner_group.update!(by_month_or_week: 'day_of_month', day_of_month: 29) }.to raise_error(ActiveRecord::RecordInvalid) + expect { partner_group.update!(by_month_or_week: 'day_of_month', day_of_month: -5) }.to raise_error(ActiveRecord::RecordInvalid) end end end diff --git a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb index 6d666fa54d..9ff880698f 100644 --- a/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb +++ b/spec/services/partners/fetch_partners_to_remind_now_service_spec.rb @@ -10,7 +10,7 @@ context "that has an organization with a global reminder & deadline" do context "that is for today" do before do - partner.organization.update(date_or_week_day: "date", date: current_day, deadline_day: current_day + 2) + partner.organization.update(by_month_or_week: "day_of_month", day_of_month: current_day, deadline_day: current_day + 2) end it "should include that partner" do @@ -21,7 +21,7 @@ context "as matched by day of the week" do before do - partner.organization.update(date_or_week_day: "week_day", + partner.organization.update(by_month_or_week: "day_of_week", day_of_week: 2, every_nth_day: 2, deadline_day: current_day + 2) end it "should include that partner" do @@ -54,8 +54,8 @@ context "that is not for today" do before do - partner.organization.update(date_or_week_day: "date", - date: current_day - 1, deadline_day: current_day + 2) + partner.organization.update(by_month_or_week: "day_of_month", + day_of_month: current_day - 1, deadline_day: current_day + 2) end it "should NOT include that partner" do @@ -65,12 +65,12 @@ context "AND a partner group that does have them defined" do before do - partner_group = create(:partner_group, date_or_week_day: "date", - date: current_day, deadline_day: current_day + 2) + partner_group = create(:partner_group, by_month_or_week: "day_of_month", + day_of_month: current_day, deadline_day: current_day + 2) partner_group.partners << partner - partner.organization.update(date_or_week_day: "date", - date: current_day - 1, deadline_day: current_day + 2) + partner.organization.update(by_month_or_week: "day_of_month", + day_of_month: current_day - 1, deadline_day: current_day + 2) end it "should remind based on the partner group instead of the organization level reminder" do @@ -97,8 +97,8 @@ context "and is a part of a partner group that does have them defined" do context "that is for today" do before do - partner_group = create(:partner_group, date_or_week_day: "date", - date: current_day, deadline_day: current_day + 2) + partner_group = create(:partner_group, by_month_or_week: "day_of_month", + day_of_month: current_day, deadline_day: current_day + 2) partner_group.partners << partner end @@ -119,8 +119,8 @@ context "that is not for today" do before do - partner_group = create(:partner_group, date_or_week_day: "date", - date: current_day - 1, deadline_day: current_day + 2) + partner_group = create(:partner_group, by_month_or_week: "day_of_month", + day_of_month: current_day - 1, deadline_day: current_day + 2) partner_group.partners << partner end diff --git a/spec/system/admin/organizations_system_spec.rb b/spec/system/admin/organizations_system_spec.rb index 05c41b5de7..0e8b8d170f 100644 --- a/spec/system/admin/organizations_system_spec.rb +++ b/spec/system/admin/organizations_system_spec.rb @@ -117,7 +117,7 @@ fill_in "organization_user_email", with: admin_user_params[:email] choose 'toggle-to-date' - fill_in "organization_date", with: 1 + fill_in "organization_day_of_month", with: 1 click_on "Save" end diff --git a/spec/system/partner_system_spec.rb b/spec/system/partner_system_spec.rb index 4bb2b397fd..2c1cfd8582 100644 --- a/spec/system/partner_system_spec.rb +++ b/spec/system/partner_system_spec.rb @@ -500,7 +500,7 @@ check 'Yes' choose 'toggle-to-date' - fill_in "partner_group_date", with: 1 + fill_in "partner_group_day_of_month", with: 1 fill_in "partner_group_deadline_day", with: 25 find_button('Add Partner Group').click From 10833e0d9220d591bbe20fab7f32ab63c3a2751c Mon Sep 17 00:00:00 2001 From: Jesse Landis-Eigsti Date: Fri, 4 Oct 2024 14:23:16 -0600 Subject: [PATCH 10/10] 4473 - fixes reminder day vs deadline display bug There was a bug in which if the reminder day of the month was the same as the deadline day, it would dislplay the error message even if the user changed to the day of the week option. Now the error message is hidden if the user is selecting the day of the week option. --- app/javascript/utils/deadline_day_pickers.js | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/app/javascript/utils/deadline_day_pickers.js b/app/javascript/utils/deadline_day_pickers.js index d538c4a76a..1ab5082ec2 100644 --- a/app/javascript/utils/deadline_day_pickers.js +++ b/app/javascript/utils/deadline_day_pickers.js @@ -6,6 +6,7 @@ $(document).ready(function () { const deadline_selector = '.deadline-day-pickers__deadline-day'; const reminder_container_selector = '.deadline-day-pickers__reminder-container'; const deadline_container_selector = '.deadline-day-pickers__deadline-container'; + const day_of_week_toggle_selector = '#toggle-to-week-day'; const reminder_text_selector = '.deadline-day-pickers__reminder-day-text'; const deadline_text_selector = '.deadline-day-pickers__deadline-day-text'; @@ -18,6 +19,7 @@ $(document).ready(function () { const $deadline = $container.find(deadline_selector); const $reminder_text = $container.find(reminder_text_selector); const $deadline_text = $container.find(deadline_text_selector); + const $day_of_week_toggle = $container.find(day_of_week_toggle_selector)[0]; const reminder_day = parseInt($reminder.val()); const deadline_day = parseInt($deadline.val()); @@ -29,17 +31,20 @@ $(document).ready(function () { if (reminder_day) { $(container).find(reminder_container_selector).find(server_validation_selector).remove(); - if (reminder_day === deadline_day) { + if (reminder_day === deadline_day && !$day_of_week_toggle.checked) { $reminder_text.removeClass('text-muted').addClass('text-danger'); $reminder_text.text('Reminder day cannot be the same as deadline day.'); - } else { + } + else if ($day_of_week_toggle.checked){ + $reminder_text.text(''); + } + else { $reminder_text.removeClass('text-danger').addClass('text-muted'); - - const next_reminder_month = (current_day >= reminder_day) ? next_month : current_month; - $reminder_text.text(`Your next reminder will be sent on ${reminder_day} ${next_reminder_month}.`); + const next_reminder_month = (current_day >= reminder_day) ? next_month : current_month; + $reminder_text.text(`Your next reminder will be sent on ${reminder_day} ${next_reminder_month}.`); + } } - } if (deadline_day) { $(container).find(deadline_container_selector).find(server_validation_selector).remove();