From 26d2c0a57e6e22e6488f66c110f710f7d1624c19 Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Tue, 7 Nov 2023 14:11:27 +0000 Subject: [PATCH 1/4] Add is_oda attribute to reports We have decided to use the same attribute name and the same conventions for reports that we do for activities, in order to add the least amount of confusion and friction to development. The conventions are: - For "pure ODA" funds such as GCRF, Newton, and Other ODA, the value of `is_oda` is `nil`; this is due to the fact that these funds predate the existence of hybrid (ODA and non-ODA) funds such as ISPF; these funds and all their activities are *implicitly* ODA. At the time of adding ISPF to RODA, we had decided against backfilling the value of `is_oda` for existing activities. This allows us to enforce the presence of `is_oda` for ISPF activities. We acknowledge the tension that comes from having a boolean attribute whose `nil` value does not translate to a "falsey" meaning in the business domain. However, many RODA workflows now depend on this meaning for decision paths. - For hybrid funds such as ISPF, `is_oda` must be intentionally set at programme level (level B), and is required; projects and third-party projects (levels C and D) inherit the `is_oda` value of their parent activities. Only `is_oda: true` means that the activity is ODA, and only `is_oda: false` means that the activity is non-ODA. We are populating the `is_oda` value for all existing ISPF reports to `false`. We have confirmed with DSIT that all existing ISPF reports are for non-ODA activities. --- CHANGELOG.md | 1 + db/migrate/20231107133740_add_is_oda_to_reports.rb | 12 ++++++++++++ db/schema.rb | 3 ++- 3 files changed, 15 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20231107133740_add_is_oda_to_reports.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 73b86c83d..bb0d1b274 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ [Full changelog][unreleased] - Remove ispf_fund_in_stealth_mode that hides ISPF funds from users +- Add `is_oda` attribute to reports and populate it to `false` for all existing ISPF reports ## Release 138 - 2023-10-27 diff --git a/db/migrate/20231107133740_add_is_oda_to_reports.rb b/db/migrate/20231107133740_add_is_oda_to_reports.rb new file mode 100644 index 000000000..622c15532 --- /dev/null +++ b/db/migrate/20231107133740_add_is_oda_to_reports.rb @@ -0,0 +1,12 @@ +class AddIsOdaToReports < ActiveRecord::Migration[6.1] + def up + add_column :reports, :is_oda, :boolean + + ispf = Activity.by_roda_identifier("ISPF") + Report.where(fund_id: ispf.id).update_all(is_oda: false) if ispf + end + + def down + remove_column :reports, :is_oda + end +end diff --git a/db/schema.rb b/db/schema.rb index b19c720e1..b70542fb6 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.define(version: 2023_03_07_102933) do +ActiveRecord::Schema.define(version: 2023_11_07_133740) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -310,6 +310,7 @@ t.integer "financial_year" t.string "export_filename" t.datetime "approved_at" + t.boolean "is_oda" t.index ["fund_id", "organisation_id"], name: "enforce_one_editable_report_per_series", unique: true, where: "((state)::text <> 'approved'::text)" t.index ["fund_id", "organisation_id"], name: "enforce_one_historic_report_per_series", unique: true, where: "((financial_quarter IS NULL) OR (financial_year IS NULL))" t.index ["fund_id"], name: "index_reports_on_fund_id" From 1e363d3c44cb3443043c03bab75b61a3880af1c9 Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Tue, 7 Nov 2023 14:26:18 +0000 Subject: [PATCH 2/4] Amend report validation to include is_oda Amend the report validation (enforced as Rails model validation and as database unique index) to take into account the ODA type of the report. This allows non-ISPF reports (whose `is_oda` value is `nil`) to function as usual, and it allows ISPF to have one ODA and one non-ODA report editable at the same time for each partner organisation. --- CHANGELOG.md | 1 + app/models/report.rb | 3 +- ..._editable_report_per_series_by_oda_type.rb | 13 ++++++++ db/schema.rb | 4 +-- spec/models/report_spec.rb | 33 ++++++++++++++----- 5 files changed, 43 insertions(+), 11 deletions(-) create mode 100644 db/migrate/20231107141900_enforce_one_editable_report_per_series_by_oda_type.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index bb0d1b274..4096d81bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ - Remove ispf_fund_in_stealth_mode that hides ISPF funds from users - Add `is_oda` attribute to reports and populate it to `false` for all existing ISPF reports +- Amend report validation to permit two editable reports per fund and organisation, as long as they have different ODA types ## Release 138 - 2023-10-27 diff --git a/app/models/report.rb b/app/models/report.rb index dbb8ff631..87437b55e 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -91,7 +91,8 @@ def no_unapproved_reports_per_series unless Report.where( fund: fund, - organisation: organisation + organisation: organisation, + is_oda: is_oda ).all?(&:approved?) errors.add(:base, I18n.t("activerecord.errors.models.report.unapproved_reports_html")) end diff --git a/db/migrate/20231107141900_enforce_one_editable_report_per_series_by_oda_type.rb b/db/migrate/20231107141900_enforce_one_editable_report_per_series_by_oda_type.rb new file mode 100644 index 000000000..a405ad469 --- /dev/null +++ b/db/migrate/20231107141900_enforce_one_editable_report_per_series_by_oda_type.rb @@ -0,0 +1,13 @@ +class EnforceOneEditableReportPerSeriesByOdaType < ActiveRecord::Migration[6.1] + def change + remove_index :reports, [:fund_id, :organisation_id], + where: "state <> 'approved'", + unique: true, + name: "enforce_one_editable_report_per_series" + + add_index :reports, [:fund_id, :organisation_id, :is_oda], + where: "state <> 'approved'", + unique: true, + name: "enforce_one_editable_report_per_series" + end +end diff --git a/db/schema.rb b/db/schema.rb index b70542fb6..db3eaf81c 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.define(version: 2023_11_07_133740) do +ActiveRecord::Schema.define(version: 2023_11_07_141900) do # These are extensions that must be enabled in order to support this database enable_extension "pgcrypto" @@ -311,7 +311,7 @@ t.string "export_filename" t.datetime "approved_at" t.boolean "is_oda" - t.index ["fund_id", "organisation_id"], name: "enforce_one_editable_report_per_series", unique: true, where: "((state)::text <> 'approved'::text)" + t.index ["fund_id", "organisation_id", "is_oda"], name: "enforce_one_editable_report_per_series", unique: true, where: "((state)::text <> 'approved'::text)" t.index ["fund_id", "organisation_id"], name: "enforce_one_historic_report_per_series", unique: true, where: "((financial_quarter IS NULL) OR (financial_year IS NULL))" t.index ["fund_id"], name: "index_reports_on_fund_id" t.index ["organisation_id"], name: "index_reports_on_organisation_id" diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb index 2c6035c02..add3b2e3f 100644 --- a/spec/models/report_spec.rb +++ b/spec/models/report_spec.rb @@ -12,16 +12,33 @@ end context "in the :new validation context" do - it "validates there are no unapproved reports for the organisation and fund" do - organisation = create(:partner_organisation) - existing_approved_report = create(:report, :approved, organisation: organisation) - existing_unapproved_report = create(:report, state: "awaiting_changes", organisation: organisation) + context "for an ODA-only fund" do + it "validates there are no unapproved reports for the organisation and fund" do + organisation = create(:partner_organisation) + existing_approved_report = create(:report, :approved, organisation: organisation) + existing_unapproved_report = create(:report, state: "awaiting_changes", organisation: organisation) - new_valid_report = build(:report, fund: existing_approved_report.fund, organisation: organisation) - new_invalid_report = build(:report, fund: existing_unapproved_report.fund, organisation: organisation) + new_valid_report = build(:report, fund: existing_approved_report.fund, organisation: organisation) + new_invalid_report = build(:report, fund: existing_unapproved_report.fund, organisation: organisation) - expect(new_invalid_report.valid?(:new)).to be(false) - expect(new_valid_report.valid?).to be(true) + expect(new_invalid_report.valid?(:new)).to be(false) + expect(new_valid_report.valid?(:new)).to be(true) + end + end + + context "for a hybrid ODA and non-ODA fund such as ISPF" do + it "validates there are no unapproved reports for the organisation, fund, and ODA type" do + organisation = create(:partner_organisation) + _existing_approved_oda_report = create(:report, :for_ispf, :approved, is_oda: true, organisation: organisation) + _existing_approved_non_oda_report = create(:report, :for_ispf, :approved, is_oda: false, organisation: organisation) + _existing_unapproved_oda_report = create(:report, :for_ispf, is_oda: true, state: "awaiting_changes", organisation: organisation) + + new_valid_report = build(:report, :for_ispf, is_oda: false, organisation: organisation) + new_invalid_report = build(:report, :for_ispf, is_oda: true, organisation: organisation) + + expect(new_invalid_report.valid?(:new)).to be(false) + expect(new_valid_report.valid?(:new)).to be(true) + end end it "validates the presence of financial_quarter and financial_year" do From 41a32706ebd9a0dbcd55a834d2c89c8259f65c78 Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Tue, 7 Nov 2023 15:25:56 +0000 Subject: [PATCH 3/4] Pre-existing later report validation considers ODA --- app/models/report.rb | 2 +- spec/models/report_spec.rb | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/report.rb b/app/models/report.rb index 87437b55e..150a6711e 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -137,7 +137,7 @@ def financial_quarter_is_not_in_the_future? def no_prexisting_later_report? return false unless financial_quarter && financial_year - latest_report = organisation.reports.in_historical_order.where(fund_id: fund_id).first + latest_report = organisation.reports.in_historical_order.where(fund_id: fund_id, is_oda: is_oda).first if latest_report && latest_report.financial_period.last > financial_period.last errors.add(:financial_period, "A report already exists for a later period.") end diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb index add3b2e3f..aa92823f3 100644 --- a/spec/models/report_spec.rb +++ b/spec/models/report_spec.rb @@ -78,6 +78,7 @@ describe "Ensuring that a new report does not attempt to rewrite history" do let(:organisation) { create(:partner_organisation) } let(:fund) { create(:fund_activity) } + context "where a report already exists for a period later than that of the new report" do it "is not valid" do create(:report, :approved, organisation: organisation, fund: fund, financial_quarter: 4, financial_year: 2018) @@ -86,7 +87,18 @@ expect(new_report.valid?(:new)).to be(false) end end + + context "and the existing report has a different ODA type than the new report" do + it "is valid" do + create(:report, :for_ispf, :approved, is_oda: false, organisation: organisation, financial_quarter: 4, financial_year: 2018) + travel_to(Date.parse("01-04-2020")) do + new_report = build(:report, :for_ispf, is_oda: true, organisation: organisation, financial_quarter: 3, financial_year: 2018) + expect(new_report.valid?(:new)).to be(true) + end + end + end end + context "where a report does not exist for a period later than that of the new report" do it "is valid" do create(:report, :approved, organisation: organisation, fund: fund, financial_quarter: 4, financial_year: 2018) From 6d8adfe89aff131f18c47e4b2fdf482e5e5b8435 Mon Sep 17 00:00:00 2001 From: Cristina <579522+CristinaRO@users.noreply.github.com> Date: Tue, 7 Nov 2023 15:26:10 +0000 Subject: [PATCH 4/4] Fix typos --- spec/models/report_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/report_spec.rb b/spec/models/report_spec.rb index aa92823f3..e990dcc34 100644 --- a/spec/models/report_spec.rb +++ b/spec/models/report_spec.rb @@ -49,13 +49,13 @@ end context "validates that the financial quarter is previous to the current quarter" do - it "when creating a report for the next finanical quarter in the same financial year" do + it "when creating a report for the next financial quarter in the same financial year" do travel_to(Date.parse("01-04-2020")) do new_report = build(:report, financial_quarter: 2, financial_year: 2020) expect(new_report.valid?(:new)).to be(false) end end - it "when creating a report for the next finanical quarter in the next financial year" do + it "when creating a report for the next financial quarter in the next financial year" do travel_to(Date.parse("01-02-2020")) do new_report = build(:report, financial_quarter: 1, financial_year: 2020) expect(new_report.valid?(:new)).to be(false)