Skip to content

Collapse profile creation to single method#6681

Merged
aduth merged 4 commits intomainfrom
aduth-init-complete-profile
Aug 8, 2022
Merged

Collapse profile creation to single method#6681
aduth merged 4 commits intomainfrom
aduth-init-complete-profile

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Aug 2, 2022

Context: #6634 (comment)

Why: So that deactivation_reason can be assigned in initial creation of the profile, rather than as an immediate follow-on transaction.

**Why**: So that deactivation_reason can be assigned in initial creation of the profile, rather than as an immediate follow-on transaction.

changelog: Internal, Optimization, Optimize identity verification profile database creation
@aduth
Copy link
Contributor Author

aduth commented Aug 2, 2022

I guess this could go one step further and try to eliminate the complete_profile methods as well 🤔

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, big fan!

1. Applicant is now assigned in `before` block of the top-level method `describe`
2. Avoid calling create_profile_from_applicant_with_password twice
@aduth
Copy link
Contributor Author

aduth commented Aug 3, 2022

I guess this could go one step further and try to eliminate the complete_profile methods as well 🤔

Looking at this closer, the activation of a profile is responsible for more than just setting the active flag on the profile, so while my instinct was to change active: false to active: deactivation_reason.blank?, it seems like it's probably best to keep that separate.

@zachmargolis
Copy link
Contributor

I guess this could go one step further and try to eliminate the complete_profile methods as well 🤔

Looking at this closer, the activation of a profile is responsible for more than just setting the active flag on the profile, so while my instinct was to change active: false to active: deactivation_reason.blank?, it seems like it's probably best to keep that separate.

What if we just made active: an explicit, required param and push that up to the call sites? It would be clearer! (unsure how many callsites that is), and no more guessing inside the method what the callers want?

So activation is centralized to the ProfileMaker and callsites must make an explicit choice

#6681 (comment)
@aduth
Copy link
Contributor Author

aduth commented Aug 5, 2022

What if we just made active: an explicit, required param and push that up to the call sites? It would be clearer! (unsure how many callsites that is), and no more guessing inside the method what the callers want?

Is 8ae135e what you had in mind? It doesn't help with regards to consolidating database calls, but it does help centralize and make the flag more explicit.

def save_profile
profile = Profile.new(user: user, active: false)
def save_profile(active:, deactivation_reason: nil)
profile = Profile.new(user: user, active: false, deactivation_reason: deactivation_reason)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: This must be active: false and not active: active, since the activate call below would try to find an existing profile with active: true and incorrectly catch the profile which had just been created here.

@zachmargolis
Copy link
Contributor

Is 8ae135e what you had in mind? It doesn't help with regards to consolidating database calls, but it does help centralize and make the flag more explicit.

Yup! Love it

@aduth aduth merged commit d25fcc6 into main Aug 8, 2022
@aduth aduth deleted the aduth-init-complete-profile branch August 8, 2022 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants