From 89545db3c0f78ec6d90228d86b49c16c0cdd438b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 20 May 2021 11:26:41 +0200 Subject: [PATCH 1/5] Fix typos in processes dates validation specs --- spec/models/legislation/process_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/legislation/process_spec.rb b/spec/models/legislation/process_spec.rb index 5302497a0..f40b7dadc 100644 --- a/spec/models/legislation/process_spec.rb +++ b/spec/models/legislation/process_spec.rb @@ -30,14 +30,14 @@ describe Legislation::Process do expect(process.errors.messages[:debate_start_date]).to include("can't be blank") end - it "is invalid if allegations_start_date is present but debate_end_date is not" do + it "is invalid if allegations_start_date is present but allegations_end_date is not" do process = build(:legislation_process, allegations_start_date: Date.current, allegations_end_date: "") expect(process).to be_invalid expect(process.errors.messages[:allegations_end_date]).to include("can't be blank") end - it "is invalid if debate_end_date is present but allegations_start_date is not" do + it "is invalid if allegations_end_date is present but allegations_start_date is not" do process = build(:legislation_process, allegations_start_date: nil, allegations_end_date: Date.current) expect(process).to be_invalid From 53c53e26a86f7f9ad2a37107220272e247784fd7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 20 May 2021 11:28:30 +0200 Subject: [PATCH 2/5] Add tests for draft and proposal phases dates We had tests for other phases, but not for these ones. --- spec/models/legislation/process_spec.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/spec/models/legislation/process_spec.rb b/spec/models/legislation/process_spec.rb index f40b7dadc..b6cebbf6c 100644 --- a/spec/models/legislation/process_spec.rb +++ b/spec/models/legislation/process_spec.rb @@ -18,6 +18,21 @@ describe Legislation::Process do end describe "dates validations" do + + it "is invalid if draft_start_date is present but draft_end_date is not" do + process = build(:legislation_process, draft_start_date: Date.current, draft_end_date: "") + + expect(process).to be_invalid + expect(process.errors.messages[:draft_end_date]).to include("can't be blank") + end + + it "is invalid if draft_end_date is present but draft_start_date is not" do + process = build(:legislation_process, draft_start_date: nil, draft_end_date: Date.current) + + expect(process).to be_invalid + expect(process.errors.messages[:draft_start_date]).to include("can't be blank") + end + it "is invalid if debate_start_date is present but debate_end_date is not" do process = build(:legislation_process, debate_start_date: Date.current, debate_end_date: "") expect(process).to be_invalid @@ -30,6 +45,14 @@ describe Legislation::Process do expect(process.errors.messages[:debate_start_date]).to include("can't be blank") end + it "is invalid if proposals_phase_start_date is present but proposals_phase_end_date is not" do + process = build(:legislation_process, proposals_phase_start_date: Date.current, + proposals_phase_end_date: "") + + expect(process).to be_invalid + expect(process.errors.messages[:proposals_phase_end_date]).to include("can't be blank") + end + it "is invalid if allegations_start_date is present but allegations_end_date is not" do process = build(:legislation_process, allegations_start_date: Date.current, allegations_end_date: "") From 5bcc70eda818091a3697b38a58e507eb33fbf23a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 20 May 2021 11:33:54 +0200 Subject: [PATCH 3/5] Fix proposals phase end date validation So now it uses the same rules as the other phases. --- app/models/legislation/process.rb | 1 + spec/models/legislation/process_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+) diff --git a/app/models/legislation/process.rb b/app/models/legislation/process.rb index abf8875a7..85bb5fde5 100644 --- a/app/models/legislation/process.rb +++ b/app/models/legislation/process.rb @@ -52,6 +52,7 @@ class Legislation::Process < ApplicationRecord validates :draft_end_date, presence: true, if: :draft_start_date? validates :allegations_start_date, presence: true, if: :allegations_end_date? validates :allegations_end_date, presence: true, if: :allegations_start_date? + validates :proposals_phase_start_date, presence: true, if: :proposals_phase_end_date? validates :proposals_phase_end_date, presence: true, if: :proposals_phase_start_date? validate :valid_date_ranges validates :background_color, format: { allow_blank: true, with: CSS_HEX_COLOR } diff --git a/spec/models/legislation/process_spec.rb b/spec/models/legislation/process_spec.rb index b6cebbf6c..59962e0fa 100644 --- a/spec/models/legislation/process_spec.rb +++ b/spec/models/legislation/process_spec.rb @@ -53,6 +53,14 @@ describe Legislation::Process do expect(process.errors.messages[:proposals_phase_end_date]).to include("can't be blank") end + it "is invalid if proposals_phase_end_date is present but proposals_phase_start_date is not" do + process = build(:legislation_process, proposals_phase_start_date: nil, + proposals_phase_end_date: Date.current) + + expect(process).to be_invalid + expect(process.errors.messages[:proposals_phase_start_date]).to include("can't be blank") + end + it "is invalid if allegations_start_date is present but allegations_end_date is not" do process = build(:legislation_process, allegations_start_date: Date.current, allegations_end_date: "") From d5b3b25e1e9553cdd7943ea1057032085f6b5fa9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 20 May 2021 11:34:51 +0200 Subject: [PATCH 4/5] Refactor legislation phases date validation rules So now they're easier to change and we make sure the same rules apply to all phases. --- app/models/legislation/process.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/app/models/legislation/process.rb b/app/models/legislation/process.rb index 85bb5fde5..d90b69544 100644 --- a/app/models/legislation/process.rb +++ b/app/models/legislation/process.rb @@ -46,14 +46,15 @@ class Legislation::Process < ApplicationRecord validates_translation :title, presence: true validates :start_date, presence: true validates :end_date, presence: true - validates :debate_start_date, presence: true, if: :debate_end_date? - validates :debate_end_date, presence: true, if: :debate_start_date? - validates :draft_start_date, presence: true, if: :draft_end_date? - validates :draft_end_date, presence: true, if: :draft_start_date? - validates :allegations_start_date, presence: true, if: :allegations_end_date? - validates :allegations_end_date, presence: true, if: :allegations_start_date? - validates :proposals_phase_start_date, presence: true, if: :proposals_phase_end_date? - validates :proposals_phase_end_date, presence: true, if: :proposals_phase_start_date? + + %i[draft debate proposals_phase allegations].each do |phase_name| + start_attribute = :"#{phase_name}_start_date" + end_attribute = :"#{phase_name}_end_date" + + validates start_attribute, presence: true, if: :"#{end_attribute}?" + validates end_attribute, presence: true, if: :"#{start_attribute}?" + end + validate :valid_date_ranges validates :background_color, format: { allow_blank: true, with: CSS_HEX_COLOR } validates :font_color, format: { allow_blank: true, with: CSS_HEX_COLOR } From f55d2ab891803de90ed46524062383eed79549a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Thu, 20 May 2021 11:43:39 +0200 Subject: [PATCH 5/5] Validate process dates depending on enabled phases When configuring phases in a process, we were validating the start date or the end date is present, the other date is present too. However, in other parts of the application we were checking whether a phase is enabled and then assumed its dates were present if the phase was enabled. However, we weren't validating this behavior, so it was possible to enable a phase and leaving its dates blank, causing the application to crash. So, as suggested by Alberto, we're changing the validation rule so phase dates are mandatory when a phase is enabled. With this rule, the old validation rules are not necessary. I've considered leaving them in order to avoid database inconsistencies. However, I realized records having a disabled phase with its start and end dates have always been valid. This means applications checking for the presence of these dates instead of checking whether the phase is enabled have never worked properly. We don't have to change the logic anywhere else because as mentioned we were already checking phases are enabled before using their dates. --- app/models/legislation/process.rb | 7 +- spec/models/legislation/process_spec.rb | 133 ++++++++++++++---- .../admin/legislation/processes_spec.rb | 18 +++ 3 files changed, 124 insertions(+), 34 deletions(-) diff --git a/app/models/legislation/process.rb b/app/models/legislation/process.rb index d90b69544..12b195cce 100644 --- a/app/models/legislation/process.rb +++ b/app/models/legislation/process.rb @@ -48,11 +48,10 @@ class Legislation::Process < ApplicationRecord validates :end_date, presence: true %i[draft debate proposals_phase allegations].each do |phase_name| - start_attribute = :"#{phase_name}_start_date" - end_attribute = :"#{phase_name}_end_date" + enabled_attribute = :"#{phase_name.to_s.gsub("_phase", "")}_phase_enabled?" - validates start_attribute, presence: true, if: :"#{end_attribute}?" - validates end_attribute, presence: true, if: :"#{start_attribute}?" + validates :"#{phase_name}_start_date", presence: true, if: enabled_attribute + validates :"#{phase_name}_end_date", presence: true, if: enabled_attribute end validate :valid_date_ranges diff --git a/spec/models/legislation/process_spec.rb b/spec/models/legislation/process_spec.rb index 59962e0fa..0d0519407 100644 --- a/spec/models/legislation/process_spec.rb +++ b/spec/models/legislation/process_spec.rb @@ -18,61 +18,134 @@ describe Legislation::Process do end describe "dates validations" do - - it "is invalid if draft_start_date is present but draft_end_date is not" do - process = build(:legislation_process, draft_start_date: Date.current, draft_end_date: "") - - expect(process).to be_invalid - expect(process.errors.messages[:draft_end_date]).to include("can't be blank") - end - - it "is invalid if draft_end_date is present but draft_start_date is not" do - process = build(:legislation_process, draft_start_date: nil, draft_end_date: Date.current) + it "is invalid if draft is enabled but draft_start_date is not present" do + process = build(:legislation_process, draft_phase_enabled: true, draft_start_date: nil) expect(process).to be_invalid expect(process.errors.messages[:draft_start_date]).to include("can't be blank") end - it "is invalid if debate_start_date is present but debate_end_date is not" do - process = build(:legislation_process, debate_start_date: Date.current, debate_end_date: "") + it "is invalid if draft is enabled but draft_end_date is not present" do + process = build(:legislation_process, draft_phase_enabled: true, draft_end_date: "") + expect(process).to be_invalid - expect(process.errors.messages[:debate_end_date]).to include("can't be blank") + expect(process.errors.messages[:draft_end_date]).to include("can't be blank") end - it "is invalid if debate_end_date is present but debate_start_date is not" do - process = build(:legislation_process, debate_start_date: nil, debate_end_date: Date.current) + it "is invalid if debate phase is enabled but debate_start_date is not present" do + process = build(:legislation_process, debate_phase_enabled: true, debate_start_date: nil) + expect(process).to be_invalid expect(process.errors.messages[:debate_start_date]).to include("can't be blank") end - it "is invalid if proposals_phase_start_date is present but proposals_phase_end_date is not" do - process = build(:legislation_process, proposals_phase_start_date: Date.current, - proposals_phase_end_date: "") + it "is invalid if debate phase is enabled but debate_end_date is not present" do + process = build(:legislation_process, debate_phase_enabled: true, debate_end_date: "") expect(process).to be_invalid - expect(process.errors.messages[:proposals_phase_end_date]).to include("can't be blank") + expect(process.errors.messages[:debate_end_date]).to include("can't be blank") end - it "is invalid if proposals_phase_end_date is present but proposals_phase_start_date is not" do - process = build(:legislation_process, proposals_phase_start_date: nil, - proposals_phase_end_date: Date.current) + it "is invalid if proposals phase is enabled but proposals_phase_start_date is not present" do + process = build(:legislation_process, proposals_phase_enabled: true, proposals_phase_start_date: nil) expect(process).to be_invalid expect(process.errors.messages[:proposals_phase_start_date]).to include("can't be blank") end - it "is invalid if allegations_start_date is present but allegations_end_date is not" do - process = build(:legislation_process, allegations_start_date: Date.current, - allegations_end_date: "") + it "is invalid if proposals phase is enabled but proposals_phase_end_date is not present" do + process = build(:legislation_process, proposals_phase_enabled: true, proposals_phase_end_date: "") + + expect(process).to be_invalid + expect(process.errors.messages[:proposals_phase_end_date]).to include("can't be blank") + end + + it "is invalid if allegations phase is enabled but allegations_start_date is not present" do + process = build(:legislation_process, allegations_phase_enabled: true, + allegations_start_date: nil,) + + expect(process).to be_invalid + expect(process.errors.messages[:allegations_start_date]).to include("can't be blank") + end + + it "is invalid if allegations phase is enabled but allegations_end_date is not present" do + process = build(:legislation_process, allegations_phase_enabled: true, allegations_end_date: "") + expect(process).to be_invalid expect(process.errors.messages[:allegations_end_date]).to include("can't be blank") end - it "is invalid if allegations_end_date is present but allegations_start_date is not" do - process = build(:legislation_process, allegations_start_date: nil, - allegations_end_date: Date.current) - expect(process).to be_invalid - expect(process.errors.messages[:allegations_start_date]).to include("can't be blank") + it "is valid if start dates are missing and the phase is disabled" do + draft_disabled = build(:legislation_process, + draft_phase_enabled: false, + draft_start_date: nil) + + debate_disabled = build(:legislation_process, + debate_phase_enabled: false, + debate_start_date: nil) + + proposals_disabled = build(:legislation_process, + proposals_phase_enabled: false, + proposals_phase_start_date: nil) + + allegations_disabled = build(:legislation_process, + allegations_phase_enabled: false, + allegations_start_date: nil) + + expect(draft_disabled).to be_valid + expect(debate_disabled).to be_valid + expect(proposals_disabled).to be_valid + expect(allegations_disabled).to be_valid + end + + it "is valid if end dates are missing and the phase is disabled" do + draft_disabled = build(:legislation_process, + draft_phase_enabled: false, + draft_end_date: nil) + + debate_disabled = build(:legislation_process, + debate_phase_enabled: false, + debate_end_date: nil) + + proposals_disabled = build(:legislation_process, + proposals_phase_enabled: false, + proposals_phase_end_date: nil) + + allegations_disabled = build(:legislation_process, + allegations_phase_enabled: false, + allegations_end_date: nil) + + expect(draft_disabled).to be_valid + expect(debate_disabled).to be_valid + expect(proposals_disabled).to be_valid + expect(allegations_disabled).to be_valid + end + + it "is valid if start and end dates are missing and the phase is disabled" do + draft_disabled = build(:legislation_process, + draft_phase_enabled: false, + draft_start_date: nil, + draft_end_date: nil) + + debate_disabled = build(:legislation_process, + debate_phase_enabled: false, + debate_start_date: nil, + debate_end_date: nil) + + proposals_disabled = build(:legislation_process, + proposals_phase_enabled: false, + proposals_phase_start_date: nil, + proposals_phase_end_date: nil) + + allegations_disabled = build(:legislation_process, + allegations_phase_enabled: false, + allegations_start_date: nil, + allegations_end_date: nil) + + expect(draft_disabled).to be_valid + expect(debate_disabled).to be_valid + expect(proposals_disabled).to be_valid + expect(allegations_disabled).to be_valid end end diff --git a/spec/system/admin/legislation/processes_spec.rb b/spec/system/admin/legislation/processes_spec.rb index 1ce14063c..432a3baad 100644 --- a/spec/system/admin/legislation/processes_spec.rb +++ b/spec/system/admin/legislation/processes_spec.rb @@ -289,6 +289,24 @@ describe "Admin collaborative legislation", :admin do expect(page).to have_field "draft_publication_date", disabled: true end + scenario "Enabling comments phase with blank dates" do + visit edit_admin_legislation_process_path(process) + + within_fieldset "Comments phase" do + check "Enabled" + fill_in "Start", with: "" + fill_in "End", with: "" + end + + click_button "Save changes" + + expect(page).to have_content "errors prevented this process from being saved" + + within_fieldset "Comments phase" do + expect(page).to have_content "can't be blank" + end + end + scenario "Change proposal categories" do visit edit_admin_legislation_process_path(process) within(".admin-content") { click_link "Proposals" }