feat(webapi): Return 410 GONE for notification checks on deleted dialogs #1387
Conversation
📝 WalkthroughWalkthroughThe changes involve updates to the Changes
Possibly related PRs
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 using PR comments)
Other keywords and placeholders
Documentation and Community
|
…for-soft-deleted-dialogs
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionQuery.cs (1)
Line range hint
26-57: Consider standardizing deleted resource handlingThe introduction of
EntityDeletedand the pattern of returning 410 GONE for deleted resources is a good practice. Consider applying this pattern consistently across other endpoints that might encounter deleted resources.tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs (1)
89-110: LGTM: Good test coverage for deleted dialog scenarioThe test effectively verifies the new 410 GONE response requirement. Consider enhancing it with additional test cases:
- Verify behavior when attempting to delete an already deleted dialog
- Test with different activity types to ensure consistent behavior
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionQuery.cs(3 hunks)src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/NotificationCondition/NotificationConditionEndpoint.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs(4 hunks)
🔇 Additional comments (8)
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/ServiceOwner/DialogActivities/NotificationCondition/NotificationConditionEndpoint.cs (1)
31-32: LGTM! Good use of HTTP status codes.
The implementation correctly distinguishes between "not found" (404) and "gone" (410) states, following REST best practices. This provides clear feedback to API consumers about whether a dialog doesn't exist or has been deleted.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionQuery.cs (3)
26-26: LGTM: Good addition of EntityDeleted to OneOf type
The addition of EntityDeleted to the union type is well-placed and aligns with the PR objective to handle deleted dialogs differently.
44-44: Consider documenting the IgnoreQueryFilters usage
While the addition of IgnoreQueryFilters() is necessary to detect deleted dialogs, it bypasses all global query filters. Consider adding a code comment explaining why this is needed.
Let's check for other global filters that might be affected:
#!/bin/bash
# Description: Check for other global query filters that might be affected
# Expected: Find any HasQueryFilter calls on DbSet<DialogEntity>
# Search for query filter definitions
ast-grep --pattern 'modelBuilder.Entity<DialogEntity>().HasQueryFilter($_)'53-57: Verify the dialog deletion mechanism
The deletion check is implemented correctly. However, let's ensure the Deleted property is properly maintained.
Let's verify the dialog deletion implementation:
✅ Verification successful
Based on the search results, I can now provide a final response about the dialog deletion mechanism:
Dialog deletion is properly implemented with soft delete pattern
The codebase shows a robust implementation of dialog deletion:
- The
Deletedproperty is part ofISoftDeletableEntityinterface with proper soft delete functionality - Deletion is handled through
SoftDelete()method which setsDeleted = trueandDeletedAttimestamp - The deletion state is consistently checked across queries and commands
- Tests verify that deletion sets
DeletedAttimestamp correctly - Domain events (
DialogDeletedDomainEvent) are raised when a dialog is deleted
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify dialog deletion implementation
# Expected: Find where Deleted property is set
# Search for dialog deletion logic
ast-grep --pattern 'dialog.Deleted = $_'
# Search for soft delete configuration
ast-grep --pattern 'modelBuilder.Entity<DialogEntity>().$_Delete$_'
Length of output: 171
Script:
#!/bin/bash
# Let's try different approaches to find dialog deletion related code
# Search for dialog deletion in method implementations
ast-grep --pattern 'void Delete($$$) { $$$dialog$$$Deleted$$$}'
# Search for Deleted property definition
ast-grep --pattern 'public bool Deleted { get; set; }'
# Search for any references to dialog deletion using ripgrep
rg -i "dialog.*delet" -A 2 -B 2
# Look for soft delete configurations in DbContext
rg -i "softdelete|deletefilter" -A 2 -B 2
Length of output: 76949
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/NotificationCondition/NotificationConditionTests.cs (4)
3-3: LGTM: Import addition is appropriate
The added import for DeleteDialogCommand is necessary for the new test case and follows the existing import organization pattern.
43-43: LGTM: Good refactoring to reduce code duplication
The extraction of query creation logic into a shared method improves maintainability and follows the DRY principle.
78-78: LGTM: Consistent refactoring application
The test maintains its behavior while benefiting from the shared query creation method.
112-120: LGTM: Well-structured helper method
The CreateNotificationConditionQuery method is well-designed with sensible defaults and improves code maintainability.
oskogstad
deleted the
feat/notification-condition-return-410-gone-for-soft-deleted-dialogs
branch
🤖 I have created a release *beep* *boop* --- ## [1.28.0](v1.27.1...v1.28.0) (2024-11-05) ### Features * update swagger name generation ([#1350](#1350)) ([94c5544](94c5544)) * **webapi:** Add ExternalReference to dialog search result ([#1384](#1384)) ([431fe16](431fe16)) * **webapi:** Return 410 GONE for notification checks on deleted dialogs ([#1387](#1387)) ([198bebd](198bebd)) ### Bug Fixes * Add system user id to identifying claims ([#1362](#1362)) ([16f160d](16f160d)) * **e2e:** Use pagination in sentinel ([#1372](#1372)) ([a1df0ff](a1df0ff)) * fixed placement of referenced workflow-file ([#1365](#1365)) ([49c1d80](49c1d80)) * workaround for github number error in dispatch workflow ([#1367](#1367)) ([06ee356](06ee356)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Tests