AP-5699 Fix incorrect PDA data #7591

naseberry · 2025-01-21T14:28:18Z

What

The new PDA added Provider to incorrect firms and therefore office.
A few new firms where then created because of the above.

This is to transfer application to the correct office; providers to the correct firms; and to remove Firms where no providers have been onboarded.

Checklist

Before you ask people to review this PR:

Tests and rubocop should be passing: bundle exec rake
Github should not be reporting conflicts; you should have recently run git rebase main.
The standards in the Git Workflow document on Confluence should be followed
There should be no unnecessary whitespace changes. These make diffs harder to read and conflicts more likely.
The PR description should say what you changed and why, with a link to the JIRA story.
You should have looked at the diff against main and ensured that nothing unexpected is included in your changes.
You should have checked that the commit messages say why the change was made.

jsugarman · 2025-01-21T15:31:39Z

lib/tasks/fixes/ap_5699.rake

+    # Transfer provider LLARNI from firm POTTER DERBY LTD to WACE MORGAN SOLICITORS
+    Rake::Task["pda:provider_transfer_firm"].invoke("701c89c5-8672-4fd3-997c-a08a08a1cc7c", "2eee972d-4697-480f-be61-29e339bf9945")


Already looks like llarni is in the firm WACE MORGAN SOLICITORS on production

laa-apply-for-legal-aid(prod)> Provider.find_by(id: "701c89c5-8672-4fd3-997c-a08a08a1cc7c").firm.name => "WACE MORGAN SOLICITORS"

jsugarman · 2025-01-21T15:38:11Z

lib/tasks/fixes/ap_5699.rake

+    Rake::Task["pda:laa_transfer_office"].invoke("992c3763-6087-405d-bb0f-be46c24e0b57", "2a7babf2-d080-4181-b395-0244542ff54a")
+
+    # Transfer provider DSYKES from firm CAROLE BURGHER SOLICITORS to BUTCHER & BARLOW LLP
+    Rake::Task["pda:provider_transfer_firm"].invoke("992c3763-6087-405d-bb0f-be46c24e0b57", "2a7babf2-d080-4181-b395-0244542ff54a")


looks like this provider is already in the correct firm

Provider.find_by(id: "992c3763-6087-405d-bb0f-be46c24e0b57").firm.name => "BUTCHER & BARLOW LLP"

lib/tasks/fixes/ap_5699.rake

lib/tasks/pda/pda_migrations.rake

lib/tasks/fixes/ap_5699.rake

jsugarman

So some unnecessary albeit unharmful actions are in there, but also some ids are wrong and will fail.

It might be good to have a dry run argument that defaults to true so that it cannot be run by mistake.

lib/tasks/fixes/ap_5699.rake

jsugarman

I reckon the dry_run naming needs some clarity but, for safety, it might be better to use != "false" as you have done

dry_run = args[:dry_run].to_s.downcase.strip != "false"
# rathar than
dry_run = args[:dry_run].to_s.downcase.strip == "true"

But the actual updates need to use unless not if gaurd clauses, for example

laa.update!(office: office) unless dry_run

Might be good to test the reusable tasks as discussed in tech time

jsugarman · 2025-01-23T10:26:31Z

lib/tasks/pda/pda_migrations.rake

+    if laa
+      Rails.logger.info "LegalAidApplication (#{laa.application_ref}) current office code: #{laa.office.code}"
+
+      laa.update!(office: office) if dry_run


Naming wise I would expect do_something unless dry_run

jsugarman · 2025-01-23T10:32:17Z

lib/tasks/fixes/ap_5699.rake

+namespace :fixes do
+  desc "Fixes for AP-5699"
+  task :ap_5699, [:dry_run] => :environment do |_task, args|
+    args.with_defaults(dry_run: true)


All args are strings in rake i believe

Suggested change

args.with_defaults(dry_run: true)

args.with_defaults(dry_run: "true")

jsugarman · 2025-01-23T10:32:23Z

lib/tasks/fixes/ap_5699.rake

+  desc "Fixes for AP-5699"
+  task :ap_5699, [:dry_run] => :environment do |_task, args|
+    args.with_defaults(dry_run: true)
+    dry_run = args[:dry_run] != "false"


dry_run = args[:dry_run].to_s.downcase.strip == "true"

jsugarman · 2025-01-23T10:41:42Z

lib/tasks/fixes/ap_5699.rake

+
+    # Transfer applications from POTTER DERBY LTD office 0M434D to WACE MORGAN SOLICITORS office 0H036Y
+    potter_derby_application_ids.each do |appid|
+      Rake::Task["pda:laa_transfer_office"].execute(dry_run: dry_run.to_s, appid: appid, officeid: "c2d4f6df-0a06-4ade-b1c8-1bba1406a874")


this should not be needed once dry run default is set i think

Suggested change

Rake::Task["pda:laa_transfer_office"].execute(dry_run: dry_run.to_s, appid: appid, officeid: "c2d4f6df-0a06-4ade-b1c8-1bba1406a874")

Rake::Task["pda:laa_transfer_office"].execute(dry_run: dry_run, appid: appid, officeid: "c2d4f6df-0a06-4ade-b1c8-1bba1406a874")

jsugarman · 2025-01-23T10:42:20Z

lib/tasks/fixes/ap_5699.rake

+    Rake::Task["pda:laa_transfer_office"].execute(dry_run: dry_run.to_s, appid: "6db37c7a-7350-4384-aa2d-c6a6ccfc22e9", officeid: "fe5dfac8-baa6-4a22-b0c9-681d97068508")
+
+    # Delete the firm and offices of CAROLE BURGHER SOLICITORS
+    Rake::Task["pda:delete_firm_office"].execute(dry_run: dry_run.to_s, firmid: "29bbd5ca-5820-4502-b74a-76b11e8bff64")


to_s not be needed once dry run default is set to a string i think

jsugarman · 2025-01-23T10:42:42Z

lib/tasks/fixes/ap_5699.rake

+    Rails.logger.info "Start of HAZELBACON"
+
+    # Transfer provider HAZELBACON from firm MCARAS to CANTER LEVIN & BERG LTD, Office: 5T620Z
+    Rake::Task["pda:provider_transfer_firm"].execute(dry_run: dry_run.to_s, providerid: "03ec8aa6-56a4-494d-91e6-88d4974671db", firmid: "02e2f920-c5fd-4e6c-9d6c-af1a16dfe7fc")


to_s not be needed once dry run default is set to a string i think

jsugarman · 2025-01-23T10:43:02Z

lib/tasks/pda/pda_migrations.rake

+namespace :pda do
+  desc "Transfer a LegalAidApplication from one Office to another"
+  task :laa_transfer_office, %i[dry_run appid officeid] => :environment do |_task, args|
+    args.with_defaults(dry_run: true)


Suggested change

args.with_defaults(dry_run: true)

args.with_defaults(dry_run: "true")

jsugarman · 2025-01-23T10:43:34Z

lib/tasks/pda/pda_migrations.rake

+  desc "Transfer a LegalAidApplication from one Office to another"
+  task :laa_transfer_office, %i[dry_run appid officeid] => :environment do |_task, args|
+    args.with_defaults(dry_run: true)
+    dry_run = args[:dry_run] != "false"


Suggested change

dry_run = args[:dry_run] != "false"

dry_run = args[:dry_run].to_s.downcase.strip == "true"

jsugarman · 2025-01-23T10:49:45Z

lib/tasks/pda/pda_migrations.rake

+
+  desc "Transfer a Provider from one Firm to another"
+  task :provider_transfer_firm, %i[dry_run providerid firmid] => :environment do |_task, args|
+    args.with_defaults(dry_run: true)


Suggested change

args.with_defaults(dry_run: true)

args.with_defaults(dry_run: "true")

jsugarman · 2025-01-23T10:50:06Z

lib/tasks/pda/pda_migrations.rake

+  desc "Transfer a Provider from one Firm to another"
+  task :provider_transfer_firm, %i[dry_run providerid firmid] => :environment do |_task, args|
+    args.with_defaults(dry_run: true)
+    dry_run = args[:dry_run] != "false"


Suggested change

dry_run = args[:dry_run] != "false"

dry_run = args[:dry_run].to_s.downcase.strip == "true"

- Task to transfer the selected office of an application - Task to transfer a provider to a different firm - Task to remove unused Firm

- Cleanup incorrect PDA data

sonarqubecloud · 2025-01-27T13:19:54Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

agoldstone93

Added some nits and some other comments

agoldstone93 · 2025-01-28T15:13:28Z

lib/tasks/provider_transfer/ptr_migrations.rake

+    firm = Firm.find_by(id: args[:firmid])
+
+    if firm.nil?
+      Rails.logger.info "FIRM does not exists"


Nit: exists should say exist

agoldstone93 · 2025-01-28T15:38:59Z

spec/lib/tasks/provider_transfer/ptr_migrations_spec.rb

+      end
+    end
+
+    context "when LegalAidApplication does not exist" do


Is it worth adding an expect to verify that office doesn't change in these scenarios?

I'd say yes, it's worth it - this could be destructive if it goes wrong! 😬

agoldstone93 · 2025-01-28T15:40:30Z

spec/lib/tasks/provider_transfer/ptr_migrations_spec.rb

+      end
+    end
+
+    context "when Provider does not exist" do


Is it worth adding an expect to verify that provider doesn't change in these scenarios?

agoldstone93 · 2025-01-28T15:47:32Z

spec/lib/tasks/provider_transfer/ptr_migrations_spec.rb

+  let(:firm) { create(:firm) }
+  let(:office) { create(:office) }


Nit: As these are only used in the final spec, could they be defined within it?

agoldstone93 · 2025-01-28T15:49:36Z

lib/tasks/provider_transfer/ptr_migrations.rake

@@ -0,0 +1,86 @@
+namespace :ptr_migrations do
+  desc "Transfer a LegalAidApplication from one Office to another"
+  task :laa_transfer_office, %i[mock appid officeid] => :environment do |_task, args|


Nit: Could these be called app_id and office_id for readability?

agoldstone93 · 2025-01-28T15:54:00Z

spec/lib/tasks/provider_transfer/ptr_migrations_spec.rb

+
+      it "skips deleting the Office and logs a message" do
+        expect(Rails.logger).to receive(:info).with("Firm (#{firm.name}), Office (#{office.code}) has 1 legal aid application associated with it. Skipping!")
+        expect(Rails.logger).to receive(:info).with("Firm (#{firm.name}) has 1 office associated with it. Skipping!")


Is this supposed to work when there are multiple offices? Could there a be a test for this scenario?

colinbruce · 2025-01-29T12:49:46Z

lib/tasks/provider_transfer/ptr_migrations.rake

+
+        if applications_in_office.positive?
+          Rails.logger.info "Firm (#{firm.name}), Office (#{office.code}) has #{applications_in_office} legal aid application associated with it. Skipping!"
+          # next


commented out next

colinbruce · 2025-01-29T13:02:23Z

lib/tasks/provider_transfer/ptr_migrations.rake

+  end
+
+  desc "Transfer a Provider from one Firm to another"
+  task :provider_transfer_firm, %i[mock providerid firmid] => :environment do |_task, args|


Ditto nit: clarity of names

Suggested change

task :provider_transfer_firm, %i[mock providerid firmid] => :environment do |_task, args|

task :provider_transfer_firm, %i[mock provider_id firm_id] => :environment do |_task, args|

jsugarman reviewed Jan 21, 2025

View reviewed changes

lib/tasks/fixes/ap_5699.rake Outdated Show resolved Hide resolved

jsugarman reviewed Jan 21, 2025

View reviewed changes

lib/tasks/pda/pda_migrations.rake Outdated Show resolved Hide resolved

jsugarman reviewed Jan 21, 2025

View reviewed changes

lib/tasks/fixes/ap_5699.rake Outdated Show resolved Hide resolved

jsugarman reviewed Jan 21, 2025

View reviewed changes

naseberry commented Jan 22, 2025

View reviewed changes

lib/tasks/fixes/ap_5699.rake Outdated Show resolved Hide resolved

naseberry force-pushed the ap-5699-incorrect-pda-data branch 3 times, most recently from 101e676 to 0643d8a Compare January 23, 2025 10:15

jsugarman reviewed Jan 23, 2025

View reviewed changes

naseberry force-pushed the ap-5699-incorrect-pda-data branch from e267472 to 82613ee Compare January 24, 2025 16:26

naseberry added 3 commits January 27, 2025 09:05

AP-5699: Provider transfer rake tasks

f6b3891

- Task to transfer the selected office of an application - Task to transfer a provider to a different firm - Task to remove unused Firm

AP-5699: Rake fixes for incorrect Provider and Office

35d9906

- Cleanup incorrect PDA data

AP-5699: Add specs for Provider transfer take tasks

e6ed4f3

naseberry force-pushed the ap-5699-incorrect-pda-data branch 3 times, most recently from 236061b to 60a820a Compare January 27, 2025 12:35

naseberry marked this pull request as ready for review January 27, 2025 13:03

naseberry requested a review from a team as a code owner January 27, 2025 13:03

AP-5699: Change rake invocation in spec

7f4ab3a

naseberry force-pushed the ap-5699-incorrect-pda-data branch from 60a820a to 7f4ab3a Compare January 27, 2025 13:19

agoldstone93 reviewed Jan 28, 2025

View reviewed changes

colinbruce reviewed Jan 29, 2025

View reviewed changes

AP-5699: Update arg names, improve testing of destructive tasks

3e5b18c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AP-5699 Fix incorrect PDA data #7591

AP-5699 Fix incorrect PDA data #7591

naseberry commented Jan 21, 2025 •

edited

Loading

jsugarman Jan 21, 2025 •

edited

Loading

jsugarman Jan 21, 2025 •

edited

Loading

jsugarman left a comment

jsugarman left a comment

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

jsugarman Jan 23, 2025

sonarqubecloud bot commented Jan 27, 2025

agoldstone93 left a comment

agoldstone93 Jan 28, 2025

agoldstone93 Jan 28, 2025

colinbruce Jan 29, 2025

agoldstone93 Jan 28, 2025

colinbruce Jan 29, 2025

agoldstone93 Jan 28, 2025

agoldstone93 Jan 28, 2025

agoldstone93 Jan 28, 2025

colinbruce Jan 29, 2025

colinbruce Jan 29, 2025

		# Transfer provider LLARNI from firm POTTER DERBY LTD to WACE MORGAN SOLICITORS
		Rake::Task["pda:provider_transfer_firm"].invoke("701c89c5-8672-4fd3-997c-a08a08a1cc7c", "2eee972d-4697-480f-be61-29e339bf9945")

	args.with_defaults(dry_run: true)
	args.with_defaults(dry_run: "true")

	Rake::Task["pda:laa_transfer_office"].execute(dry_run: dry_run.to_s, appid: appid, officeid: "c2d4f6df-0a06-4ade-b1c8-1bba1406a874")
	Rake::Task["pda:laa_transfer_office"].execute(dry_run: dry_run, appid: appid, officeid: "c2d4f6df-0a06-4ade-b1c8-1bba1406a874")

	dry_run = args[:dry_run] != "false"
	dry_run = args[:dry_run].to_s.downcase.strip == "true"

		let(:firm) { create(:firm) }
		let(:office) { create(:office) }

	task :provider_transfer_firm, %i[mock providerid firmid] => :environment do \|_task, args\|
	task :provider_transfer_firm, %i[mock provider_id firm_id] => :environment do \|_task, args\|

AP-5699 Fix incorrect PDA data #7591

Are you sure you want to change the base?

AP-5699 Fix incorrect PDA data #7591

Conversation

naseberry commented Jan 21, 2025 • edited Loading

What

Checklist

jsugarman Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

jsugarman Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

jsugarman left a comment

Choose a reason for hiding this comment

jsugarman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 27, 2025

Quality Gate passed

agoldstone93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naseberry commented Jan 21, 2025 •

edited

Loading

jsugarman Jan 21, 2025 •

edited

Loading

jsugarman Jan 21, 2025 •

edited

Loading