LG-16075: Duplicate submission error handling at sign up completion by jmdembe · Pull Request #12273 · 18F/identity-idp

jmdembe · 2025-06-18T19:30:02Z

🎫 Ticket

🛠 Summary of changes

This PR seeks to stop the ActiveRecord::RecordNotUnique from happening when a user submits the "Agree and Continue" button on the sign_up/completed screen.

changelog: Bug Fixes, sign up completion, fix double submission at sign in completion

mitchellhenke · 2025-06-18T19:37:01Z

Could the checks be moved closer to where the database call is raising an exception, I think it's somewhere here based on the ticket description.

This is potentially also a case for create_or_find_by, we addressed a similar issue with it in #6524.

jmdembe · 2025-06-23T15:28:28Z

Could the checks be moved closer to where the database call is raising an exception, I think it's somewhere here based on the ticket description.

This is potentially also a case for create_or_find_by, we addressed a similar issue with it in #6524.

I see. We are doing a create_or_find_by in one method where one does a create and the other does a find. Maybe this would be a time to refactor?

mitchellhenke · 2025-06-23T18:53:44Z

app/services/agency_identity_linker.rb

-    ai = AgencyIdentity.find_by(uuid: @sp_identity.uuid)
-    return ai if ai
-    sp = ServiceProvider.find_by(issuer: @sp_identity.service_provider)
-    return unless agency_id(sp)
-    AgencyIdentity.find_by(agency_id: agency_id, user_id: @sp_identity.user_id)


If I'm reading this right, this method:

Tries to find an AgencyIdentity based only on the ServiceProviderIdentity uuid

If there isn't one, and the service provider has an agency association, it tries to find an AgencyIdentity based on the agency_id and user_id

The above find_agency_identity method that replaces it doesn't have the same behavior, I'm not sure if that will cause issues.

I pushed up commit e2208d9 with a new test that fails with the current changes.

…ound

mitchellhenke · 2025-06-24T15:17:47Z

app/services/agency_identity_linker.rb

      uuid: uuid,
-    )
+    ) do |ai|
+      ai.uuid = uuid


Does this set the UUID to the same UUID it would have otherwise had? If so, can it be deleted?

Yes it does. Deleted in b6e2cfe.

mitchellhenke · 2025-06-24T17:17:20Z

app/services/agency_identity_linker.rb


    uuid = spi&.uuid || SecureRandom.uuid
-    AgencyIdentity.create(
+    AgencyIdentity.find_or_create_by(


I'm not sure that this is the right method to be changing, the error was in sign up, but this method is only called in an Attempts API class: https://github.com/18F/identity-idp/blob/2fc19c9/app/services/attempts_api/tracker.rb#L98-L102

So are you saying to do the call at the point of sign up? Because if we are to do that, we would have to do the same logic that is in the above link, no?
Also, wouldn't this also reduce the chance of a race condition in the Attempts API class? Or does that not matter?

Sorry, I was intending to say that the exception occurred during sign up, which calls find_or_create_agency_identity further down, and that's probably where any changes would need to occur.

can u change this back to just create please.

My mistake. Changed in 2f448c0.

mitchellhenke · 2025-06-26T18:05:20Z

app/services/agency_identity_linker.rb

-    AgencyIdentity.find_by(agency_id: agency_id, user_id: @sp_identity.user_id)
+
+    begin
+      AgencyIdentity.create(


This is effectively create_or_find_by, could we use that?

When I wrote this, I did not use create_or_find_by because Agency.create requires three arguments while Agency.find_by requires two. Adding the UUID to the find makes the links identities from 2 sps test fail, so that is why I went back to begin...rescue

I think he is saying u can use create or find_by in this line here instead of the begin.

I believe I changed that in 995b5e2. There are tests failing now that didn't when I used begin...create that I am struggling to understand.

jmdembe · 2025-06-27T16:50:40Z

For 1776946, is this a viable solution, and if so, how can a test be written for this case?

mitchellhenke · 2025-06-30T14:56:33Z

For 1776946, is this a viable solution, and if so, how can a test be written for this case?

I'm not sure that putting it in a transaction is sufficient in this case.

add rescue to prevent RecordNotUnique error

687a390

changelog: Bug Fixes, sign up completion, fix double submission at sign in completion

jmdembe requested review from mdiarra3 and vrajmohan June 18, 2025 19:30

jmdembe added 5 commits June 23, 2025 13:34

change course, create_or_find_by agency id

d26b50b

add service provider check

c60e991

raise exception of agency identity is not found

25b814d

make variables make sense

277e76e

fix next_setup_path

8b47d12

mitchellhenke reviewed Jun 23, 2025

View reviewed changes

Mitchell Henke and others added 2 commits June 23, 2025 13:58

add test for multiple SP identities

e2208d9

use begin...rescue in service

a17c544

jmdembe force-pushed the jd/LG-16075-duplicate_signup_handling branch from a8f54bf to a17c544 Compare June 23, 2025 20:09

jmdembe added 2 commits June 24, 2025 09:58

look for agency identity first, then create if multiple records are f…

9540150

…ound

do find_or_create_by, search using uuid

7845957

jmdembe requested a review from mitchellhenke June 24, 2025 15:04

mitchellhenke reviewed Jun 24, 2025

View reviewed changes

get rid of do statement

b6e2cfe

jmdembe requested a review from mitchellhenke June 24, 2025 16:48

mitchellhenke reviewed Jun 24, 2025

View reviewed changes

fix logic for create and find

f6479d9

jmdembe force-pushed the jd/LG-16075-duplicate_signup_handling branch from 815ab28 to f6479d9 Compare June 26, 2025 16:54

jmdembe requested a review from mitchellhenke June 26, 2025 17:58

mitchellhenke reviewed Jun 26, 2025

View reviewed changes

replace to create

2f448c0

jmdembe requested a review from mitchellhenke June 30, 2025 13:34

mdiarra3 approved these changes Jun 30, 2025

View reviewed changes

mdiarra3 self-requested a review June 30, 2025 14:57

use

995b5e2

jmdembe force-pushed the jd/LG-16075-duplicate_signup_handling branch from 1776946 to 995b5e2 Compare June 30, 2025 17:30

restore find or create agency identity and refactor

3014843

mdiarra3 approved these changes Jul 1, 2025

View reviewed changes

re-start build

07d4b18

jmdembe merged commit e649880 into main Jul 1, 2025
1 check passed

jmdembe deleted the jd/LG-16075-duplicate_signup_handling branch July 1, 2025 13:04

mitchellhenke mentioned this pull request Jul 1, 2025

Deploy RC 492 to Production #12306

Merged

Conversation

jmdembe commented Jun 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

Uh oh!

mitchellhenke commented Jun 18, 2025

Uh oh!

jmdembe commented Jun 23, 2025

Uh oh!

mitchellhenke Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdembe Jun 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jmdembe commented Jun 27, 2025

Uh oh!

mitchellhenke commented Jun 30, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jmdembe commented Jun 18, 2025 •

edited

Loading

mitchellhenke Jun 23, 2025 •

edited

Loading

jmdembe Jun 24, 2025 •

edited

Loading