feat: add dialogOpened activitytype #1110
Conversation
...Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs
Outdated
Show resolved
Hide resolved
Fargekritt
force-pushed
the
feature/add-dialogOpened-activitytype
branch
from
480e770 to
b4af999
Compare
Fargekritt
force-pushed
the
feature/add-dialogOpened-activitytype
branch
from
a4a787d to
16d54f1
Compare
...erviceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionValidator.cs
Outdated
Show resolved
Hide resolved
Fargekritt
force-pushed
the
feature/add-dialogOpened-activitytype
branch
from
8015d7d to
832e2f4
Compare
...Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs
Outdated
Show resolved
Hide resolved
Added DialogOpened to CloudEventTypes
Fixed CreateDialogCommandValidator check for TransmissionOpened
…Owner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs Co-authored-by: Ole Jørgen Skogstad <skogstad@softis.net>
Fargekritt
force-pushed
the
feature/add-dialogOpened-activitytype
branch
from
410c337 to
577f286
Compare
|
@coderabbitai full review |
Actions performedFull review triggered. |
WalkthroughThe changes introduce a new enumeration value, Changes
Poem
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
tests/k6/common/dialog.js (1)
143-149: Nitpick: Consider settingactivitiestoundefinedinstead of usingdelete.The static analysis tool suggests avoiding the
deleteoperator for performance reasons. As an alternative, you can set theactivitiesproperty toundefinedinstead of deleting it:export function setActivities(dialog, activities) { if (activities == null) { - delete dialog.activities; + dialog.activities = undefined; return; } dialog.activities = activities; }However, this is a low-priority suggestion and the performance impact is likely negligible in most scenarios. The current implementation is correct and can be left as is.
Let me know if you'd like me to submit a PR with this change.
Tools
Biome
[error] 145-145: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (19)
- docs/schema/V1/schema.verified.graphql (1 hunks)
- docs/schema/V1/swagger.verified.json (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Events/CloudEventTypes.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionValidator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Activities/DialogActivityType.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Common/ObjectTypes.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20240910063003_AddDialogActivityDialogOpened.Designer.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20240910063003_AddDialogActivityDialogOpened.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs (1 hunks)
- src/Digdir.Tool.Dialogporten.GenerateFakeData/DialogGenerator.cs (1 hunks)
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/Common/Events/DomainEventsTests.cs (1 hunks)
- tests/k6/common/dialog.js (1 hunks)
- tests/k6/common/testimports.js (1 hunks)
- tests/k6/tests/serviceowner/all-tests.js (1 hunks)
- tests/k6/tests/serviceowner/dialogCreateActivity.js (1 hunks)
- tests/k6/tests/serviceowner/dialogCreatePatchDelete.js (6 hunks)
- tests/k6/tests/serviceowner/dialogUpdateActivity.js (1 hunks)
Additional context used
Biome
tests/k6/common/dialog.js
[error] 145-145: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (29)
tests/k6/common/testimports.js (1)
27-30: LGTM! Verify the usage of the modified exports.The changes to the exports from
dialog.jslook good. The addition ofsetActivitiesandaddActivityenhances the module's capability to handle activities, while the removal ofsetProcesssuggests a shift away from that functionality.These changes reflect an evolution in the module's interface to align with current needs. However, ensure that the code importing these functions is updated accordingly.
Run the following script to verify the usage of the modified exports:
Verification successful
To further investigate the inconsistency with the
setProcessfunction, we need to gather more information. Let's run additional scripts to check for other files importingsetProcessand look for recent changes that might explain this inconsistency.
Approval confirmed. Cleanup of
setProcessreferences recommended.The removal of
setProcessfrom the exports intests/k6/common/testimports.jsis verified and appears to be part of an intentional cleanup process. The function is not being imported or actively used in other parts of the codebase, as evidenced by the lack of imports and recent activity related to it.However, there are still some remnants of
setProcessin other files that should be addressed:
- tests/k6/common/testimports.js
- tests/k6/common/dialog.js
- tests/k6/tests/enduser/dialogSearch.js
- tests/k6/tests/serviceowner/dialogSearch.js
Recommendation: Create a follow-up task to remove these remaining references to
setProcessto maintain consistency across the codebase.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the modified exports from dialog.js. # Test 1: Search for the usage of the added exports. Expect: At least one occurrence of each. rg --type js -A 5 $'setActivities|addActivity' # Test 2: Search for the usage of the removed export. Expect: No occurrences. rg --type js -A 5 $'setProcess'Length of output: 10732
Script:
#!/bin/bash # Description: Check for other files importing setProcess and look for recent changes # Test 1: Search for imports of setProcess from testimports.js echo "Files importing setProcess from testimports.js:" rg --type js "import.*setProcess.*from '.*testimports\.js'" -l # Test 2: Check recent commits mentioning setProcess echo -e "\nRecent commits mentioning setProcess:" git log -n 5 --grep="setProcess" --pretty=format:"%h - %s" --since="2 weeks ago" # Test 3: Check if there are any open pull requests mentioning setProcess echo -e "\nOpen pull requests mentioning setProcess:" gh pr list --search "setProcess in:title,body" --state open --limit 5 --json number,title,urlLength of output: 545
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20240910063003_AddDialogActivityDialogOpened.cs (1)
20-26: LGTM!The
Downmethod correctly reverts the changes made by theUpmethod by deleting the inserted record based on theIdvalue. This ensures that the migration can be rolled back safely.tests/k6/tests/serviceowner/all-tests.js (3)
5-5: LGTM!The import statement for
dialogCreateActivityis correctly added, aligning with the list of alterations provided in the summary.
11-11: LGTM!The import statement for
dialogUpdateActivityis correctly added, aligning with the list of alterations provided in the summary.
17-23: LGTM!The function calls to
dialogCreateActivityanddialogUpdateActivityare correctly added to the main exported function, aligning with the AI-generated summary.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogActivities/Queries/NotificationCondition/NotificationConditionValidator.cs (1)
25-29: LGTM!The new validation rule for
TransmissionIdis implemented correctly. It enforces the requirement forTransmissionIdto be null whenActivityTypeisDialogOpened.The custom error message clearly communicates the validation condition, making it easier for consumers to understand and fix any validation errors. This aligns with the best practice discussed in the past review comments.
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Activities/DialogActivityType.cs (1)
40-45: LGTM!The addition of the new enum member
DialogOpenedwith a value of 7 is a non-breaking change that enhances the functionality of theDialogActivityTypeenum. The summary comment provides a clear description of the purpose of the enum member.src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Events/CloudEventTypes.cs (1)
23-23: LGTM!The new mapping for the
DialogOpenedevent type is added correctly to theCloudEventTypesclass. It follows the existing pattern and naming convention, allowing the system to recognize and handle the event appropriately.src/Digdir.Domain.Dialogporten.GraphQL/EndUser/Common/ObjectTypes.cs (1)
72-75: LGTM!The addition of the
DialogOpenedenum member to theActivityTypeenum is a valuable enhancement. It allows the system to represent and track the activity of opening a dialog, which is consistent with the existing enum members. The assigned value maintains the sequential numbering, and theGraphQLDescriptionattribute provides a clear description of the enum member's purpose, improving the clarity of the GraphQL API for developers.tests/k6/tests/serviceowner/dialogUpdateActivity.js (1)
1-115: LGTM!The test suite for dialog update activities is well-structured and comprehensive. It covers various scenarios, including both valid and invalid cases, ensuring that the dialog update functionality behaves as expected.
The use of
describeblocks helps in organizing the tests, making the code more readable and maintainable. The cleanup phase at the end is a good practice to ensure that the test environment remains clean for future test runs.Overall, the tests enhance the reliability of the service owner interface.
tests/k6/tests/serviceowner/dialogCreateActivity.js (1)
1-119: Excellent test coverage for dialog creation with activity types!The test suite covers a good range of scenarios for dialog creation with various activity types. It ensures that the API handles both valid and invalid activity types correctly. The test cases follow a clear Arrange-Act-Assert pattern, making them easy to understand and maintain.
Some suggestions for improvement:
- Consider extracting the common setup code into a
beforeEachhook to avoid duplication.- Consider using more descriptive names for the test cases to clarify the scenario being tested.
- Consider adding more test cases for edge cases, such as missing required fields or invalid data types.
Overall, the test suite looks great and provides good coverage for the dialog creation functionality.
tests/k6/tests/serviceowner/dialogCreatePatchDelete.js (5)
1-13: LGTM!The new imports
uuidv7andsetActivitiesare necessary for the added test cases and utility functions. The imports look good.
Line range hint
27-41: Great job adding more assertions!The new assertions for
createdAt,updatedAt, andrevisionproperties improve the test coverage and ensure the correctness of the dialog object after creation. The logic and format checks are accurate.
Line range hint
54-67: Good update to ensure unique URLs!Using
uuidv4()to generate a unique URL for the API action endpoint in each test run is a good practice. It prevents potential conflicts and makes the test more robust.
Line range hint
75-88: Excellent additions to the test case!The new assertions for
updatedAtandrevisionproperties after patching the dialog improve the test coverage and ensure the correctness of the dialog object. Checking thatupdatedAtis different fromcreatedAtand verifying the update of therevisionproperty are important validations.
89-185: Comprehensive test cases for dialog creation with activity types!The new test cases cover various scenarios for creating dialogs with different activity types, including both valid and invalid cases. The use of
uuidv7()for generating unique activity IDs is a good practice.The assertions check for the expected status codes and response structures based on the activity type, ensuring the correct behavior of the API. The test cases are well-structured and follow a consistent pattern of setup, act, and assert, making them readable and maintainable.
Overall, these test cases significantly improve the test coverage for dialog creation with activity types. Great job!
docs/schema/V1/schema.verified.graphql (1)
279-280: LGTM!The new enum value
DIALOG_OPENEDis a valid addition to theActivityTypeenum. It follows the naming convention and includes a clear comment describing its purpose. This change enhances the schema's ability to represent and track user interactions with dialogs.tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/Common/Events/DomainEventsTests.cs (2)
40-43: LGTM!The code segment correctly links the
transmissionOpenedActivityto a transmission by setting itsTransmissionIdproperty. This is necessary to ensure that the activity is correctly associated with a transmission when creating a dialog.
50-51: LGTM!The code segment correctly adds the generated fake transmission to the
createDialogCommand. This is necessary to ensure that the dialog is created with the correct transmission data.src/Digdir.Tool.Dialogporten.GenerateFakeData/DialogGenerator.cs (1)
253-262: Temporary exclusion ofTransmissionOpenedactivity type.The changes temporarily exclude the
TransmissionOpenedactivity type from the list of possible types for random picking when generating fake dialog activities. This is indicated by the comment referencing a GitHub issue, suggesting that further revisions to the generator are planned.Consider monitoring the referenced issue (#1123) for updates on the planned revisions to the generator, which may address any limitations or concerns related to the current implementation.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (2)
399-406: LGTM!The new validation rules for
TransmissionIdbased on theTypeproperty are implemented correctly:
- When
TypeisDialogActivityType.Values.DialogOpened,TransmissionIdmust benull. This ensures that a dialog opened activity does not reference any transmission.- When
TypeisDialogActivityType.Values.TransmissionOpened,TransmissionIdmust not be empty. This mandates that a transmission opened activity must reference a valid transmission.These rules enhance the validation logic and improve data integrity by enforcing constraints on
TransmissionIdbased on the activity type.
Line range hint
1-398: Skipped review.The
UpdateDialogCommandValidatorclass has no code changes to review.Also applies to: 407-500
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (2)
135-135: Looks good!Adding an empty line here improves the readability by separating the validation rules for different properties.
415-422: LGTM!The new validation rules for the
TransmissionIdproperty based on theTypeof the dialog activity are implemented correctly. They ensure that:
- For a
DialogOpenedactivity, theTransmissionIdmust be null, indicating that a dialog opened event should not reference any transmission.- For a
TransmissionOpenedactivity, theTransmissionIdmust not be empty, ensuring that a transmission opened event must reference a valid transmission.These rules improve data integrity by enforcing the expected behavior of the
TransmissionIdproperty based on the activity type.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs (1)
461-465: Looks good! Verify if existing activity handling code needs updates.The new
DialogOpenedactivity type has been added correctly to theDialogActivityTypeenum.To ensure the new activity type is handled correctly throughout the codebase, run the following script:
If the script highlights any code that needs to be updated, please make the necessary changes. Let me know if you would like me to suggest the required code changes.
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20240910063003_AddDialogActivityDialogOpened.Designer.cs (2)
434-469: LGTM!The new
DialogOpenedactivity type is being added correctly to theDialogActivityTypeseed data. TheIdandNamevalues are consistent with the existing entries.
379-418: Looks good!The new
DialogIdproperty in theDialogActivityentity is defined correctly. It establishes the necessary relationship between activities and dialogs.docs/schema/V1/swagger.verified.json (2)
855-856: Looks good!The addition of the "SignatureProvided" enum value expands the set of possible activity types that the application can handle. This is a non-breaking change that enhances the functionality.
865-866: LGTM!The addition of the "DialogOpened" enum value expands the set of possible activity types, allowing the application to handle events related to dialog opening. This non-breaking change enhances the functionality, which aligns with the PR objective.
...porten.Infrastructure/Persistence/Migrations/20240910063003_AddDialogActivityDialogOpened.cs
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
tests/k6/common/dialog.js (1)
143-149: Consider an alternative approach to remove theactivitiesproperty.The function correctly handles setting the
activitiesproperty of the dialog object based on the providedactivitiesargument.However, the static analysis tool has flagged the usage of the
deleteoperator on line 145 as a potential performance issue. While the impact may be negligible in this specific scenario, consider an alternative approach to remove theactivitiesproperty without using thedeleteoperator.Apply this diff to remove the
activitiesproperty by assigningundefined:export function setActivities(dialog, activities) { if (activities == null) { - delete dialog.activities; + dialog.activities = undefined; return; } dialog.activities = activities; }Tools
Biome
[error] 145-145: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- tests/k6/common/dialog.js (1 hunks)
Additional context used
Biome
tests/k6/common/dialog.js
[error] 145-145: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
Additional comments not posted (1)
tests/k6/common/dialog.js (1)
151-157: LGTM!The
addActivityfunction is implemented correctly and handles adding an activity to the dialog'sactivitiesarray appropriately. The null checks and array initialization are handled properly.
🤖 I have created a release *beep* *boop* --- ## [1.18.0](v1.17.0...v1.18.0) (2024-09-16) ### Features * add dialogOpened activitytype ([#1110](#1110)) ([711fa6d](711fa6d)) * Add process and precedingProcess to dialog as optional fields ([#1092](#1092)) ([2bf0d30](2bf0d30)) ### Bug Fixes * Allow setting UpdatedAt when creating Dialog ([#1105](#1105)) ([481e907](481e907)) * Authorize access to dialog details for any mainresource action ([#1122](#1122)) ([a7e769a](a7e769a)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Added dialogOpened as an activityType, and added validation for dialogOpened
Related Issue(s)
#1078
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
TransmissionIdbased on activity type.Tests