Skip to content
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

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Merge accounts #1683

wants to merge 27 commits into from

Conversation

fherreazcue
Copy link
Collaborator

Adds methods and tests to be able to merge two accounts, transferring all the relevant associations without duplication.

Resolves #1666.

@fherreazcue fherreazcue linked an issue Nov 28, 2023 that may be closed by this pull request
Copy link
Member

@stuzart stuzart left a 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

lib/seek/merging/person_merge.rb Outdated Show resolved Hide resolved
lib/seek/merging/person_merge.rb Outdated Show resolved Hide resolved
test/unit/person_test.rb Outdated Show resolved Hide resolved
test/unit/person_test.rb Outdated Show resolved Hide resolved
test/unit/person_test.rb Show resolved Hide resolved
test/unit/person_test.rb Outdated Show resolved Hide resolved
test/unit/person_test.rb Show resolved Hide resolved
lib/seek/merging/person_merge.rb Show resolved Hide resolved
Comment on lines +74 to +85
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
Copy link
Contributor

Choose a reason for hiding this comment

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

I did a search for belongs_to :user and found a few other associations:
image

The auth lookup tables will be too slow to update, but the various log tables are probably worth updating

Copy link
Member

@stuzart stuzart Nov 30, 2023

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

@fbacall
Copy link
Contributor

fbacall commented Nov 30, 2023

Maybe useful:
List of tables that reference user_id:

api_tokens
assay_auth_lookup
asset_doi_logs
collection_auth_lookup
data_file_auth_lookup
document_auth_lookup
event_auth_lookup
favourite_groups
favourites
file_template_auth_lookup
identities
investigation_auth_lookup
model_auth_lookup
moderatorships
oauth_sessions
placeholder_auth_lookup
presentation_auth_lookup
publication_auth_lookup
resource_publish_logs
sample_auth_lookup
saved_searches
sop_auth_lookup
strain_auth_lookup
study_auth_lookup
template_auth_lookup
workflow_auth_lookup

List of tables that reference person_id:

admin_defined_role_programmes
admin_defined_role_projects
disciplines_people
favourite_group_memberships
group_memberships
project_subscriptions
publication_authors
roles
subscriptions
users

@fherreazcue
Copy link
Collaborator Author

Just some notes on the comment above:
Person:
The roles, admin_defined_role_programmes, admin_defined_role_projects, group_memberships, subsriptions and users are already included.
For favourite_group_memberships, project_subscriptions and publication_authors, this needs to be added in the main merge method:

          merge_associations(other_person, 'favourite_group_memberships',
                             %i[favourite_group_id access_type], { person_id: id })
          merge_associations(other_person, 'project_subscriptions',
                             %i[project_id unsubscribed_types frequency], { person_id: id })
          merge_associations(other_person, 'publication_authors',
                             %i[first_name last_name publication_id author_index], { person_id: id })

with accompanying tests.
For disciplines_people I am unsure... the method person.disciplines bounces off the Disciplines table...

User:
Identities are already included.
For api_tokens, favourite_groups and oauth_sessions this can be added in the merge_user method:

        merge_user_associations(other_person, 'api_tokens',
                                %i[title], { user_id: user.id })
        merge_user_associations(other_person, 'favourite_groups',
                                %i[name], { user_id: user.id })
        merge_user_associations(other_person, 'oauth_sessions',
                                %i[provider], { user_id: user.id })

with accompanying tests.
For asset_doi_logs, favourites, moderatorships, resource_publish_logs and saved_searches I am not sure.

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.

Merge accounts
3 participants