-
Notifications
You must be signed in to change notification settings - Fork 1
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
Admin UI: tier quota definition is not deleted from the database after removing it from a tier #519
Conversation
Important Auto Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Modules/Quotas/src/Quotas.Infrastructure/Persistence/Repository/TiersRepository.cs
Outdated
Show resolved
Hide resolved
Modules/Quotas/src/Quotas.Infrastructure/Persistence/Repository/TiersRepository.cs
Show resolved
Hide resolved
@NikolaVetnic The title of the PR is a bit messed up.
|
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.
Did you try to write a test for this? With the db contexts of the FakeDbContextFactory
this should be possible, I think.
So:
- Create fake db contexts
- Use the
actContext
to create a repository - Use the repository to add a Tier with a quota
- Delete the quota from the Tier
- Use the repository to delete the Tier
- Use the
assertionContext
to check whether the TierQuotaDefinition was deleted as well.
Modules/Quotas/src/Quotas.Application/Infrastructure/Persistence/Repository/ITiersRepository.cs
Outdated
Show resolved
Hide resolved
Modules/Quotas/src/Quotas.Infrastructure/Persistence/Repository/TiersRepository.cs
Outdated
Show resolved
Hide resolved
Modules/Quotas/src/Quotas.Infrastructure/Persistence/Repository/TiersRepository.cs
Outdated
Show resolved
Hide resolved
Modules/Quotas/src/Quotas.Infrastructure/Persistence/Repository/TiersRepository.cs
Outdated
Show resolved
Hide resolved
Modules/Quotas/test/Quotas.Application.Tests/TestDoubles/AddMockTiersRepository.cs
Outdated
Show resolved
Hide resolved
...s/Quotas/test/Quotas.Application.Tests/TestDoubles/FindTierQuotaDefinitionsStubRepository.cs
Show resolved
Hide resolved
Modules/Quotas/test/Quotas.Application.Tests/TestDoubles/FindTiersStubRepository.cs
Show resolved
Hide resolved
…ackbone into tierquotadefinition-deletion-bug
…ackbone into tierquotadefinition-deletion-bug
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.
Question: did you do a manual test?
Literally the first thing I did :) I'll do it once again just to be on the safe side. |
Readiness checklist
Description
Prior to fixing it the attempt at deleting the tier quota definition in the Admin UI would imply orphan it, i.e. set its
TierId
column value tonull
, making it in effect "orphaned" (not connected to anyTier
entity). Database operation trigger is introduced that is activated after updating tier quota definition and which deletes the said definition should it turn out to be orphaned.