From f8f67905766b2e675dbfa930de33e2ff4ceca340 Mon Sep 17 00:00:00 2001 From: Ed Davey Date: Wed, 18 Dec 2024 10:07:46 +0000 Subject: [PATCH] Add new foreign key for project creator The `Project#regional_delivery_officer` association is misleading. It dates from an earlier time when only RGOs could create projects. However, today Regional Casework Services (RCSs) can also create projects and so can have their `user.ids` set in `projects.regional_delivery_officer_id`. That's a recipe for confusion, which has been documented in [The project's "Added by" user is wrong][]. A more accurate name for this association is `Project#creator`. However, renaming a database column (in this case also a foreign key) is best done in steps to avoid errors in code or in referential integrity checks e.g. this scenario at a point during the deployment process: - our migration has run and renamed `projects.regional_delivery_officer_id` to `projects.creator_id` - before our new code which references `projects.creator_id` rather than `projects.regional_delivery_officer_id` has deployed, a user attempts to create a transfer/conversion. BOOM! To avoid these errors and to make the change without downtime we'll stage the refactoring over 4 steps, to be performed **in 4 separate deployments**: 1. (**THIS PR**) introduce new foreign key column --- - create migration to: - add `projects.creator_id` column, allow to be null - add new index on `projects.creator_id` - add new foreign key constraint on `projects.creator_id` - start setting new `projects.creator_id` in addition to existing `projects.regional_delivery_officer_id` (e.g. when a project is created, when the 'added_by' is updated) 2. back-fill new foreign key column --- - run a data migration to populate the new `projects.creator_id` with existing references from `projects.regional_delivery_officer_id` 3. discontinue use of old foreign key column --- - run migration to make `projects.creator_id` non-nullable - Change code to use new `projects.creator_id` in all places. 4. remove old foreign key column --- `remove_foreign_key :projects, regional_delivery_officer_id ` [The project's "Added by" user is wrong]: https://dfe-gov-uk.visualstudio.com/Academies-and-Free-Schools-SIP/_wiki/wikis/Academies-and-Free-Schools-SIP.wiki/8813/Troubleshooting-common-issues-for-support-users-and-support-developers?anchor=the-project%27s-%22added-by%22-user-is-wrong --- app/forms/conversion/create_project_form.rb | 1 + app/forms/internal_contacts/edit_added_by_user_form.rb | 2 +- app/forms/transfer/create_project_form.rb | 1 + app/models/project.rb | 8 +++++++- app/services/api/conversions/create_project_service.rb | 1 + app/services/api/transfers/create_project_service.rb | 1 + db/migrate/20241217170822_add_project_creator_id.rb | 7 +++++++ db/schema.rb | 5 ++++- 8 files changed, 23 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20241217170822_add_project_creator_id.rb diff --git a/app/forms/conversion/create_project_form.rb b/app/forms/conversion/create_project_form.rb index 2af224f58..66c6d196a 100644 --- a/app/forms/conversion/create_project_form.rb +++ b/app/forms/conversion/create_project_form.rb @@ -51,6 +51,7 @@ def save(context = nil) conversion_date: provisional_conversion_date, advisory_board_date: advisory_board_date, regional_delivery_officer_id: user.id, + creator_id: user.id, team: team, assigned_to: assigned_to, assigned_at: assigned_at, diff --git a/app/forms/internal_contacts/edit_added_by_user_form.rb b/app/forms/internal_contacts/edit_added_by_user_form.rb index 0ff42fc5f..1d9e97279 100644 --- a/app/forms/internal_contacts/edit_added_by_user_form.rb +++ b/app/forms/internal_contacts/edit_added_by_user_form.rb @@ -24,7 +24,7 @@ def initialize(attrs = {}) def update if valid? - project.update(regional_delivery_officer: user) + project.update(regional_delivery_officer: user, creator: user) else false end diff --git a/app/forms/transfer/create_project_form.rb b/app/forms/transfer/create_project_form.rb index 1138309fb..c692e1fe2 100644 --- a/app/forms/transfer/create_project_form.rb +++ b/app/forms/transfer/create_project_form.rb @@ -50,6 +50,7 @@ def save(context = nil) transfer_date: provisional_transfer_date, two_requires_improvement: two_requires_improvement, regional_delivery_officer_id: user.id, + creator_id: user.id, team: user.team, assigned_to: user, assigned_at: DateTime.now, diff --git a/app/models/project.rb b/app/models/project.rb index 50d148954..5f398bbf3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -39,9 +39,15 @@ class Project < ApplicationRecord validate :establishment_exists, if: -> { urn.present? } belongs_to :caseworker, class_name: "User", optional: true - belongs_to :regional_delivery_officer, class_name: "User", optional: true belongs_to :assigned_to, class_name: "User", optional: true + # Project#regional_delivery_officer is deprecated, in favour of Project#creator + # as RCSs can also create transfers and conversions. + # We'll leave the projects.regional_delivery_officer_id FK initially + # and remove it in an additional PR / Deployment + belongs_to :creator, class_name: "User", foreign_key: :regional_delivery_officer_id, optional: true + belongs_to :regional_delivery_officer, class_name: "User", optional: true + scope :conversions, -> { where(type: "Conversion::Project") } scope :transfers, -> { where(type: "Transfer::Project") } diff --git a/app/services/api/conversions/create_project_service.rb b/app/services/api/conversions/create_project_service.rb index c2e984de3..dbcad9dcc 100644 --- a/app/services/api/conversions/create_project_service.rb +++ b/app/services/api/conversions/create_project_service.rb @@ -20,6 +20,7 @@ def call advisory_board_conditions: advisory_board_conditions, directive_academy_order: directive_academy_order, regional_delivery_officer_id: user.id, + creator_id: user.id, tasks_data: tasks_data, region: establishment.region_code, prepare_id: prepare_id, diff --git a/app/services/api/transfers/create_project_service.rb b/app/services/api/transfers/create_project_service.rb index b3960af85..5c4bf3f74 100644 --- a/app/services/api/transfers/create_project_service.rb +++ b/app/services/api/transfers/create_project_service.rb @@ -35,6 +35,7 @@ def call transfer_date: provisional_transfer_date, two_requires_improvement: two_requires_improvement, regional_delivery_officer_id: user.id, + creator_id: user.id, tasks_data: tasks_data, region: establishment.region_code, new_trust_reference_number: new_trust_reference_number, diff --git a/db/migrate/20241217170822_add_project_creator_id.rb b/db/migrate/20241217170822_add_project_creator_id.rb new file mode 100644 index 000000000..46b9fdd32 --- /dev/null +++ b/db/migrate/20241217170822_add_project_creator_id.rb @@ -0,0 +1,7 @@ +class AddProjectCreatorId < ActiveRecord::Migration[7.1] + def change + add_column :projects, :creator_id, :uuid, null: true + add_index :projects, :creator_id + add_foreign_key :projects, :users, column: :creator_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 641ee5756..a7d8ed3d8 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_11_21_125140) do +ActiveRecord::Schema[7.1].define(version: 2024_12_17_170822) do create_table "api_keys", id: :uuid, default: -> { "newid()" }, force: :cascade do |t| t.datetime "created_at", null: false t.datetime "updated_at", null: false @@ -331,8 +331,10 @@ t.integer "prepare_id" t.uuid "local_authority_main_contact_id" t.uuid "group_id" + t.uuid "creator_id" t.index ["assigned_to_id"], name: "index_projects_on_assigned_to_id" t.index ["caseworker_id"], name: "index_projects_on_caseworker_id" + t.index ["creator_id"], name: "index_projects_on_creator_id" t.index ["incoming_trust_ukprn"], name: "index_projects_on_incoming_trust_ukprn" t.index ["outgoing_trust_ukprn"], name: "index_projects_on_outgoing_trust_ukprn" t.index ["regional_delivery_officer_id"], name: "index_projects_on_regional_delivery_officer_id" @@ -498,5 +500,6 @@ add_foreign_key "notes", "users" add_foreign_key "projects", "users", column: "assigned_to_id" add_foreign_key "projects", "users", column: "caseworker_id" + add_foreign_key "projects", "users", column: "creator_id" add_foreign_key "projects", "users", column: "regional_delivery_officer_id" end