Skip to content

LG-11737 Removing providers without integrations#10110

Merged
nprimak merged 3 commits intomainfrom
np/LG-11737/rmvsp-no-integration
Feb 21, 2024
Merged

LG-11737 Removing providers without integrations#10110
nprimak merged 3 commits intomainfrom
np/LG-11737/rmvsp-no-integration

Conversation

@nprimak
Copy link
Contributor

@nprimak nprimak commented Feb 16, 2024

🎫 Ticket

Link to the relevant ticket:
LG-11737

🛠 Summary of changes

Was a little too hasty with #10106 and realized the issue stems not from nil integration_usage needing to be an empty array but for service providers which have no integrations at all

…when removing providers without integrations
@nprimak nprimak requested a review from Sgtpluck February 16, 2024 20:31
Comment on lines +63 to +65
integration_usages.each do |integration_usage|
integration_usage.destroy!
end
Copy link
Contributor

Choose a reason for hiding this comment

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

what if we did this in one pass?

Suggested change
integration_usages.each do |integration_usage|
integration_usage.destroy!
end
integration_usages.destroy_all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its possible for integration_usages to be an empty array

Copy link
Contributor

Choose a reason for hiding this comment

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

looks like this PR undid that by removing the || []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay yes you're right, I added another test for integrations with no integration usages because it looked as though it was returning an empty array in that case even without that bit of code but the test still passed with destroy_all so I think we're good

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the || [] because Integration has_many integration_usages and rails makes that an empty array automatically - see source code

Copy link
Contributor

@Sgtpluck Sgtpluck left a comment

Choose a reason for hiding this comment

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

small, nonblocking suggestion

Co-authored-by: Davida (she/they) <davida.marion@gsa.gov>
@nprimak nprimak merged commit 65353bf into main Feb 21, 2024
@nprimak nprimak deleted the np/LG-11737/rmvsp-no-integration branch February 21, 2024 14:40
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.

4 participants