feat: added id to attachments, ApiActions and GuiActions in DialogCreate #1670
Conversation
Validators and tests updated
📝 WalkthroughWalkthroughThe pull request introduces comprehensive changes to the Dialogporten system, focusing on adding UUIDv7 identifier support across various components. The modifications span schema definitions, application logic, data transfer objects, and test scenarios. The primary objective is to enhance entity identification by implementing a consistent method for generating and validating unique identifiers, particularly for dialog-related entities such as attachments, API actions, and GUI actions. These changes improve the system's ability to handle idempotent operations and provide more robust data management. Changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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
|
|
![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
🧹 Nitpick comments (3)
tests/k6/tests/serviceowner/all-tests.js (1)
Line range hint
17-32: Consider adding threshold tests for ID-related operations.Given that this PR adds ID attributes to Attachments, ApiActions, and GuiActions, consider:
- Adding specific threshold tests for concurrent operations with user-defined IDs
- Validating performance impact of ID validation and uniqueness checks
Would you like me to help create additional test scenarios for these cases?
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (1)
149-165: Consider optimizing database queries for ID existence checks.The implementation correctly validates the uniqueness of IDs for Attachments, GuiActions, and ApiActions. However, consider combining these database queries into a single operation to reduce the number of database roundtrips.
- var existingAttachmentIds = await _db.GetExistingIds(dialog.Attachments, cancellationToken); - if (existingAttachmentIds.Count != 0) - { - _domainContext.AddError(DomainFailure.EntityExists<DialogAttachment>(existingAttachmentIds)); - } - - var existingGuiActionIds = await _db.GetExistingIds(dialog.GuiActions, cancellationToken); - if (existingGuiActionIds.Count != 0) - { - _domainContext.AddError(DomainFailure.EntityExists<DialogGuiAction>(existingGuiActionIds)); - } - - var existingApiActionIds = await _db.GetExistingIds(dialog.ApiActions, cancellationToken); - if (existingApiActionIds.Count != 0) - { - _domainContext.AddError(DomainFailure.EntityExists<DialogApiAction>(existingApiActionIds)); - } + var (attachmentIds, guiActionIds, apiActionIds) = await Task.WhenAll( + _db.GetExistingIds(dialog.Attachments, cancellationToken), + _db.GetExistingIds(dialog.GuiActions, cancellationToken), + _db.GetExistingIds(dialog.ApiActions, cancellationToken) + ); + + if (attachmentIds.Count != 0) + _domainContext.AddError(DomainFailure.EntityExists<DialogAttachment>(attachmentIds)); + if (guiActionIds.Count != 0) + _domainContext.AddError(DomainFailure.EntityExists<DialogGuiAction>(guiActionIds)); + if (apiActionIds.Count != 0) + _domainContext.AddError(DomainFailure.EntityExists<DialogApiAction>(apiActionIds));tests/k6/tests/serviceowner/dialogCreatePatchDelete.js (1)
34-54: Consider adding test cases for GuiActions and ApiActions.The test cases thoroughly cover attachment ID scenarios. However, since the PR objective includes adding IDs to GuiActions and ApiActions, consider adding similar test cases for these entities:
- Dialog creation with specified GuiAction ID
- Dialog creation with specified ApiAction ID
- Error handling for duplicate GuiAction and ApiAction IDs
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/schema/V1/swagger.verified.json(4 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs(3 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs(3 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/GetDialogTests.cs(1 hunks)tests/k6/tests/enduser/all-tests.js(1 hunks)tests/k6/tests/serviceowner/all-tests.js(3 hunks)tests/k6/tests/serviceowner/dialogCreatePatchDelete.js(3 hunks)
🧰 Additional context used
📓 Learnings (2)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/GetDialogTests.cs (1)
Learnt from: elsand
PR: digdir/dialogporten#1529
File: tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:29-32
Timestamp: 2024-11-25T18:06:11.188Z
Learning: The end-to-end tests cover the `serviceprovider.admin-scope` scenario for dialog creation, so additional unit tests are not necessary.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (1)
Learnt from: elsand
PR: digdir/dialogporten#1529
File: tests/Digdir.Domain.Dialogporten.Application.Unit.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:29-32
Timestamp: 2024-11-25T18:06:11.188Z
Learning: The end-to-end tests cover the `serviceprovider.admin-scope` scenario for dialog creation, so additional unit tests are not necessary.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Dry run deploy apps / Deploy service to test
- GitHub Check: build / build-and-test
🔇 Additional comments (16)
tests/k6/tests/serviceowner/all-tests.js (2)
5-5: Verify the new test modules exist and follow conventions.The new imports follow the established pattern, but let's verify that the modules exist and maintain consistent testing patterns.
Also applies to: 15-15
✅ Verification successful
Imports and test structure verified successfully
The new test modules follow the established k6 testing patterns with proper configuration, thresholds, and implementation separation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence and structure of the new test modules # Check if the new test files exist fd "createDialogWithThresholds.js|serviceOwnerSearchWithThresholds.js" tests/k6/tests/serviceowner/ # Verify consistent testing patterns across modules rg -l "export default function" tests/k6/tests/serviceowner/ | \ xargs rg "import \{ check \}|import http from 'k6/http'"Length of output: 351
Script:
#!/bin/bash # Description: Check the content of new test files for k6 patterns # Display the content of both files to verify k6 imports and structure echo "=== createDialogWithThresholds.js ===" cat tests/k6/tests/serviceowner/createDialogWithThresholds.js echo -e "\n=== serviceOwnerSearchWithThresholds.js ===" cat tests/k6/tests/serviceowner/serviceOwnerSearchWithThresholds.jsLength of output: 1769
21-21: Consider test execution order and dependencies.The new threshold tests are positioned at different points in the execution sequence. Please verify:
- Whether
createDialogWithThresholdsneeds to run before other dialog-related tests- If
serviceOwnerSearchWithThresholdshas any dependencies on previous test resultsAlso applies to: 31-31
✅ Verification successful
Tests are properly isolated, execution order is not critical
After analyzing the test implementations:
- Each test is self-contained with its own configuration and implementation
- No shared state or dependencies between
createDialogWithThresholdsandserviceOwnerSearchWithThresholds- Tests can run independently in any order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential dependencies between tests # Look for shared resources or state between tests rg -A 5 "export const dialogId|export const searchId" tests/k6/tests/serviceowner/Length of output: 84
Script:
#!/bin/bash # Get the content of the test file cat tests/k6/tests/serviceowner/all-tests.js # Also search for test group declarations and test functions rg "group\(|export function" tests/k6/tests/serviceowner/all-tests.jsLength of output: 1759
Script:
#!/bin/bash # Check the content of the threshold-related test files cat tests/k6/tests/serviceowner/createDialogWithThresholds.js cat tests/k6/tests/serviceowner/serviceOwnerSearchWithThresholds.jsLength of output: 1581
tests/k6/tests/enduser/all-tests.js (1)
5-5: LGTM!The addition of the
enduserSearchWithThresholdstest module follows the established pattern of importing and invoking test modules.Also applies to: 12-12
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/GetDialogTests.cs (1)
52-58: LGTM!The test assertions have been correctly updated to exclude ID properties from comparison for Attachments, ApiActions, and GuiActions. This aligns with the PR objective of adding ID attributes to these entities.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (1)
12-12: LGTM!The addition of the Actions namespace is necessary for the new entity types.
tests/k6/tests/serviceowner/dialogCreatePatchDelete.js (2)
18-20: LGTM!The addition of variables to track dialog IDs and attachment ID is well-structured and necessary for the new test cases.
217-222: LGTM!The cleanup section ensures proper test data management by purging all created dialogs.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (3)
272-274: LGTM! Proper validation for attachment Id.The validation rules correctly ensure that the attachment Id is a valid UUIDv7 with a timestamp in the past, which is essential for idempotent operations.
342-344: LGTM! Proper validation for GUI action Id.The validation rules correctly ensure that the GUI action Id is a valid UUIDv7 with a timestamp in the past, which is essential for idempotent operations.
374-376: LGTM! Proper validation for API action Id.The validation rules correctly ensure that the API action Id is a valid UUIDv7 with a timestamp in the past, which is essential for idempotent operations.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (3)
318-322: LGTM! Well-documented Id property for API actions.The Id property is properly defined as nullable with clear documentation explaining its purpose for merging existing data, including handling of unknown IDs.
402-406: LGTM! Well-documented Id property for GUI actions.The Id property is properly defined as nullable with clear documentation explaining its purpose for merging existing data, including handling of unknown IDs.
470-473: LGTM! Well-documented Id property for attachments.The Id property is properly defined as nullable with clear documentation explaining its purpose as a unique identifier in UUIDv7 format.
docs/schema/V1/swagger.verified.json (3)
2097-2103: LGTM! Proper schema definition for API action Id.The schema correctly defines the Id property as a nullable GUID with appropriate description and example, matching the DTO definition.
2171-2176: LGTM! Proper schema definition for attachment Id.The schema correctly defines the Id property as a nullable GUID with appropriate description and example, matching the DTO definition.
2448-2454: LGTM! Proper schema definition for GUI action Id.The schema correctly defines the Id property as a nullable GUID with appropriate description and example, matching the DTO definition.
Fargekritt
changed the title
...Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs
Outdated
Show resolved
Hide resolved
...Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs
Show resolved
Hide resolved
...ten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/GetDialogTests.cs
Outdated
Show resolved
Hide resolved
…achments. Updated Doc comments on CreateDialogDto and UpdateDialogDto
Description
Added Id to Attachment, ApiActions and GuiActions in DialogCreate
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)