[CPDLP-3484] User migrator find or initialise logic to deal with orphaned ecf user, but return error for all the other scenarios #1705

mooktakim · 2024-09-13T14:20:53Z

Context

Ticket: https://dfedigital.atlassian.net.mcas.ms/browse/CPDLP-3484

Changes proposed in this pull request

Added new find_or_initialize_user(ecf_user) method:
- use ecf_user.id and ecf_user.get_an_identity_id to find users on NPQ
- if users don't exist, initialize new user and return
- if user exists with only ecf_user.id, return user
- if both id's return users they are the same, return user
- if both id's return users, but they are different, raise error
- if only ecf_user.get_an_identity_id return a user, look into ECF with the ecf_id
  - if not found, update NPQ user's ecf_id
  - if ECF user exists, check if its an orphaned user (transferred etc)
    - if yes, update NPQ user's ecf_id
    - if no, raise an error

github-actions · 2024-09-13T14:28:37Z

Review app deployed to https://npq-registration-review-1705-web.test.teacherservices.cloud/

…aned ecf user, but return error for all the other scenarios

…f_user and one for non orphaned ecf_user

sonarcloud · 2024-09-17T10:27:15Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

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

See analysis details on SonarCloud

ethax-ross · 2024-09-18T07:05:57Z

app/services/migration/migrators/user.rb

+        raise ActiveRecord::RecordInvalid, ecf_user
+      end
+
+      ecf_user.errors.add(:base, "End of find_or_initialize_user logic, something weird happened")


Can we cover this with a test?

ethax-ross · 2024-09-18T07:13:12Z

spec/services/migration/migrators/user_spec.rb

+          end
+
+          context "when linked ecf_user is not an orphan" do
+            let(:failure_manager) do


We mock this already in the shared context, so you can test failures similar to this:

https://github.com/DFE-Digital/npq-registration/blob/main/spec/services/migration/migrators/application_state_spec.rb#L36

ethax-ross · 2024-09-18T07:15:57Z

app/services/migration/migrators/user.rb

@@ -54,6 +54,72 @@ def call

  private

+    def find_or_initialize_user(ecf_user)
+      # Find Users with `ecf_user.id`
+      users_with_ecf_id = ::User.where(ecf_id: ecf_user.id)


Instead of loading the whole user model into memory here and in the queries below can we just pluck the id attributes and compare on that? has this been ran on the migration env yet?

ethax-ross · 2024-09-18T07:16:28Z

app/services/migration/migrators/user.rb

@@ -54,6 +54,72 @@ def call

  private

+    def find_or_initialize_user(ecf_user)


This is a bit of a monster function; can we extract several methods to make it easier to follow?

mooktakim temporarily deployed to review September 13, 2024 14:23 — with GitHub Actions Inactive

mooktakim temporarily deployed to staging September 13, 2024 14:25 — with GitHub Actions Inactive

mooktakim force-pushed the 3484-data-cleanup-update-npq-ecf_id-to-match-most-recent-user branch from 0ca8e00 to fc72bf2 Compare September 16, 2024 09:54

mooktakim temporarily deployed to review September 16, 2024 09:57 — with GitHub Actions Inactive

mooktakim temporarily deployed to staging September 16, 2024 09:59 — with GitHub Actions Inactive

mooktakim had a problem deploying to review September 16, 2024 10:26 — with GitHub Actions Failure

mooktakim force-pushed the 3484-data-cleanup-update-npq-ecf_id-to-match-most-recent-user branch 2 times, most recently from fdc3066 to 4d4e858 Compare September 16, 2024 15:41

mooktakim marked this pull request as ready for review September 16, 2024 15:44

mooktakim requested a review from a team as a code owner September 16, 2024 15:44

mooktakim temporarily deployed to review September 16, 2024 15:45 — with GitHub Actions Inactive

mooktakim force-pushed the 3484-data-cleanup-update-npq-ecf_id-to-match-most-recent-user branch from 4d4e858 to 53d78dc Compare September 16, 2024 16:50

mooktakim temporarily deployed to review September 16, 2024 16:53 — with GitHub Actions Inactive

mooktakim temporarily deployed to review September 16, 2024 17:26 — with GitHub Actions Inactive

mooktakim force-pushed the 3484-data-cleanup-update-npq-ecf_id-to-match-most-recent-user branch from 5df35b0 to 7be492d Compare September 16, 2024 21:12

mooktakim had a problem deploying to review September 17, 2024 09:06 — with GitHub Actions Error

mooktakim and others added 3 commits September 17, 2024 10:10

[CPDLP-3484] User migrator find or initialize logic to deal with oprh…

c20ed27

…aned ecf user, but return error for all the other scenarios

[CPDLP-3484] Check for multiple users and tests

a7f485e

[CPDLP-3484] failure_manager will receive multiple errors, one for ec…

1086f53

…f_user and one for non orphaned ecf_user

mooktakim force-pushed the 3484-data-cleanup-update-npq-ecf_id-to-match-most-recent-user branch from 7be492d to 1086f53 Compare September 17, 2024 09:10

mooktakim had a problem deploying to review September 17, 2024 09:11 — with GitHub Actions Failure

cwrw assigned leandroalemao and ethax-ross Sep 17, 2024

[CPDLP-3484] Fixed flakey test

3b73950

mooktakim had a problem deploying to review September 17, 2024 10:23 — with GitHub Actions Failure

mooktakim temporarily deployed to staging September 17, 2024 10:23 — with GitHub Actions Inactive

mooktakim had a problem deploying to review September 17, 2024 10:30 — with GitHub Actions Failure

mooktakim temporarily deployed to review September 17, 2024 10:35 — with GitHub Actions Inactive

ethax-ross reviewed Sep 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CPDLP-3484] User migrator find or initialise logic to deal with orphaned ecf user, but return error for all the other scenarios #1705

[CPDLP-3484] User migrator find or initialise logic to deal with orphaned ecf user, but return error for all the other scenarios #1705

mooktakim commented Sep 13, 2024 •

edited

Loading

github-actions bot commented Sep 13, 2024

sonarcloud bot commented Sep 17, 2024

ethax-ross Sep 18, 2024

ethax-ross Sep 18, 2024

ethax-ross Sep 18, 2024

ethax-ross Sep 18, 2024

		@@ -54,6 +54,72 @@ def call

		private

		def find_or_initialize_user(ecf_user)

[CPDLP-3484] User migrator find or initialise logic to deal with orphaned ecf user, but return error for all the other scenarios #1705

Are you sure you want to change the base?

[CPDLP-3484] User migrator find or initialise logic to deal with orphaned ecf user, but return error for all the other scenarios #1705

Conversation

mooktakim commented Sep 13, 2024 • edited Loading

Context

Changes proposed in this pull request

github-actions bot commented Sep 13, 2024

sonarcloud bot commented Sep 17, 2024

Quality Gate passed

ethax-ross Sep 18, 2024

Choose a reason for hiding this comment

ethax-ross Sep 18, 2024

Choose a reason for hiding this comment

ethax-ross Sep 18, 2024

Choose a reason for hiding this comment

ethax-ross Sep 18, 2024

Choose a reason for hiding this comment

mooktakim commented Sep 13, 2024 •

edited

Loading