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

3540: Fix bad user roles #3550

Merged
merged 2 commits into from
Apr 28, 2023
Merged

3540: Fix bad user roles #3550

merged 2 commits into from
Apr 28, 2023

Conversation

dorner
Copy link
Collaborator

@dorner dorner commented Apr 21, 2023

Resolves #3540

Description

This finds roles that don't have corresponding resources, and tries its darndest to update them so they do.

Type of change

  • Data fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Local data


bad_roles.each do |role|
UsersRole.where(role_id: role.id).destroy_all
role.destroy
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we do

Suggested change
role.destroy
role.destroy!

so if something goes wrong it gets angrier?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm... no, I don't think so. We can verify the data afterwards if something failed to delete, but I wouldn't want it to hold up the rest of the migration.

@awwaiid
Copy link
Collaborator

awwaiid commented Apr 23, 2023

Should we change this in the Role model

  belongs_to :resource,
    polymorphic: true,
    optional: true

to not be optional?

Copy link
Collaborator

@awwaiid awwaiid left a comment

Choose a reason for hiding this comment

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

Looks fine; we'll probably want to remove the allow-null later

@cielf
Copy link
Collaborator

cielf commented Apr 26, 2023

FWIW. I checked the latest version against local data too -- have the right numbers of bank roles and partner roles afterwards -- as expected.

@cielf cielf merged commit c61eeee into main Apr 28, 2023
@cielf cielf deleted the 3540-fix-bad-roles branch April 28, 2023 21:59
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.

[Investigation] roles without resources
3 participants