-
Notifications
You must be signed in to change notification settings - Fork 52
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merge accounts #1683
base: main
Are you sure you want to change the base?
Merge accounts #1683
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of minor comments on the main code, and the need to merge annotations the person has made.
Main concern is that the tests aren't robust enough, needs expanding a bit and more explicit checking. For this it is more important that the tests are really robust
def merge_user(other_person) | ||
return unless user && other_person.user | ||
|
||
merge_user_associations(other_person, 'identities', | ||
%i[provider uid], { user_id: user.id }) | ||
merge_user_associations(other_person, 'oauth_applications', | ||
%i[redirect_uri scopes], { owner_id: user.id }) | ||
merge_user_associations(other_person, 'access_tokens', | ||
%i[application_id scopes], { resource_owner_id: user.id }) | ||
merge_user_associations(other_person, 'access_grants', | ||
%i[application_id redirect_uri scopes], { resource_owner_id: user.id }) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the auth lookup table should update automatically. rows will be removed when the user is destroyed (another case where we can enhance the delete_all). After it's merged I also plan to check what jobs get created, or whether it just needs a job firing at the end
Maybe useful:
List of tables that reference
|
Just some notes on the comment above:
with accompanying tests. User:
with accompanying tests. |
Adds methods and tests to be able to merge two accounts, transferring all the relevant associations without duplication.
Resolves #1666.