-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: Add missing events to dialog subscription #1163
fix: Add missing events to dialog subscription #1163
Conversation
Warning Rate limit exceeded@oskogstad has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 57 seconds before requesting another review. How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughWalkthroughThe pull request introduces significant modifications to the GraphQL schema and related classes for handling dialog events. The Changes
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
|
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.
Actionable comments posted: 3
Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Files selected for processing (7)
- docs/schema/V1/schema.verified.graphql (4 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/Subscriptions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/GraphQL/DialogEventPayload.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/GraphQL/GraphQlSubscriptionConstants.cs (1 hunks)
- tests/Digdir.Domain.Dialogporten.Architecture.Tests/InfrastructureArchitectureTests.cs (1 hunks)
Additional comments not posted (13)
src/Digdir.Domain.Dialogporten.Infrastructure/GraphQL/GraphQlSubscriptionConstants.cs (1)
6-6
: Verify the impact on the GraphQL schema and subscription mechanism.The constant renaming and value change align with the PR objective of renaming the subscription to "dialogEvents" and introducing a new type field. This change indicates a broader scope of events related to dialogs rather than just updates.
Please ensure that the GraphQL schema and subscription mechanism have been updated to reflect this change. Run the following script to verify:
Verification successful
Changes to GraphQL schema and subscription mechanism verified
The renaming from DialogUpdatedTopic to DialogEventsTopic has been successfully implemented across the codebase. The GraphQL schema (docs/schema/V1/schema.verified.graphql) has been updated to include the
dialogEvents
subscription andDialogEventPayload
type. The C# implementation in Subscriptions.cs correctly uses the newDialogEventsTopic
constant, which is defined in GraphQlSubscriptionConstants.cs.Key points:
- GraphQL schema includes
dialogEvents
subscription andDialogEventPayload
type- C# implementation uses the new
DialogEventsTopic
constant- No remnants of the old "DialogUpdated" naming found in the codebase
The changes are consistent and align with the PR objective of renaming the subscription to "dialogEvents" and introducing a new type field.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the GraphQL schema and subscription mechanism have been updated. # Test 1: Search for the old constant usage. Expect: No occurrences. rg --type csharp -i $'DialogUpdatedTopic' # Test 2: Search for the new constant usage. Expect: Occurrences in relevant files. rg --type csharp -i $'DialogEventsTopic' # Test 3: Search for the GraphQL subscription. Expect: Subscription renamed to "dialogEvents". rg --type graphql -i $'subscription DialogUpdated' # Test 4: Search for the GraphQL payload type. Expect: Payload type renamed to "DialogEventPayload". rg --type graphql -i $'type DialogUpdatedPayload'Length of output: 1172
Script:
#!/bin/bash # Description: Verify GraphQL schema and subscription mechanism updates without relying on 'graphql' file type # Test 1: Search for GraphQL subscription definition rg -i 'subscription\s+DialogUpdated' # Test 2: Search for GraphQL subscription definition with new name rg -i 'subscription\s+DialogEvents' # Test 3: Search for old payload type rg -i 'type\s+DialogUpdatedPayload' # Test 4: Search for new payload type rg -i 'type\s+DialogEventPayload' # Test 5: Find potential GraphQL schema files fd -e graphql -e gql # Test 6: If no .graphql or .gql files found, search for GraphQL schema definition in code rg 'ISchema|GraphQLSchema|buildSchema|makeExecutableSchema'Length of output: 533
Script:
#!/bin/bash # Description: Inspect GraphQL schema file and search for subscription-related content # Test 1: Display the contents of the GraphQL schema file cat docs/schema/V1/schema.verified.graphql # Test 2: Search for any subscription-related content in the codebase rg -i 'subscription\s+\w+' --type cs # Test 3: Search for DialogEvents in the codebase (case-insensitive) rg -i 'DialogEvents' --type csLength of output: 10247
src/Digdir.Domain.Dialogporten.Infrastructure/GraphQL/DialogEventPayload.cs (2)
3-7
: LGTM!The
DialogEventPayload
struct provides a clean and standardized structure for representing dialog event payloads. The properties are well-defined and theinternal
access modifier ensures proper encapsulation.
9-13
: LGTM!The
DialogEventType
enum provides a clear and extensible categorization of dialog events. The explicit integer values assigned to the enum members is a good practice. Theinternal
access modifier ensures proper encapsulation.src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/Subscriptions.cs (2)
13-15
: LGTM!The changes to the subscription method align well with the objective to handle both dialog updated and deleted events. Using a separate constant for the topic and changing the parameter type to
DialogEventPayload
improves maintainability and allows passing more information about the event.
19-19
: LGTM!Directly returning the
eventMessage
aligns with the updated method signature and the usage ofDialogEventPayload
. This change looks good.tests/Digdir.Domain.Dialogporten.Architecture.Tests/InfrastructureArchitectureTests.cs (1)
18-18
: LGTM!The removal of the blank line is a cosmetic change and does not affect the logic of the test.
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs (2)
204-208
: LGTM! Remember to update the code that uses this class.The changes to the
DialogEventPayload
class, including the class name change and the addition of theType
property, improve the design and functionality of the dialog event system. These changes allow the payload to represent and categorize different types of dialog events, not just updates.Please ensure that the code that uses this class is updated to reflect these changes.
210-213
: Looks good!The
DialogEventType
enum is a valuable addition to the codebase. It provides a clear way to categorize dialog events into updates and deletions, which enhances the overall functionality of the dialog event system. The enum members are well-named and have appropriate numeric values.The usage of this enum in the
DialogEventPayload
class ensures that the payload can effectively specify the type of event.docs/schema/V1/schema.verified.graphql (4)
135-137
: Breaking change: RenamedDialogUpdatedPayload
toDialogEventPayload
and addedtype
field.The changes look good! Renaming the payload to
DialogEventPayload
and introducing thetype
field allows for capturing a broader range of dialog events, including updates and deletions. This aligns well with the PR objective.Note that this is a breaking change for existing clients. They will need to update their subscription query to use the new payload name and handle the new
type
field.
215-215
: Breaking change: RenameddialogUpdated
subscription todialogEvents
.The subscription renaming aligns perfectly with the new
DialogEventPayload
and the goal of supporting additional event types. Nicely done!Just keep in mind that existing clients will need to update their subscription query to use the new
dialogEvents
name and handle the newDialogEventPayload
format.
300-303
: New feature: AddedDialogEventType
enum.The new
DialogEventType
enum is a great addition! It provides a clear and structured way to categorize dialog events into updates and deletions.Clients will need to handle both the
DIALOG_UPDATED
andDIALOG_DELETED
cases when consuming thedialogEvents
subscription, but that's expected given the expanded scope of the subscription.
364-364
: New feature: AddedUUID
scalar.Adding the
UUID
scalar is a nice touch! It provides a standard and validated way to represent UUIDs throughout the schema.Specifying the scalar with the RFC 4122 URL is perfect, as it clearly communicates the expected format and links to the official specification.
src/Digdir.Domain.Dialogporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs (1)
93-98
: Verify the event type forDialogActivityCreatedDomainEvent
In the
DialogActivityCreatedDomainEvent
case, theType
is set toDialogEventType.DialogUpdated
. Please verify if usingDialogEventType.DialogUpdated
here accurately represents the event, or if a new event type should be introduced to differentiate dialog activity creations.
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/Subscriptions.cs
Outdated
Show resolved
Hide resolved
tests/Digdir.Domain.Dialogporten.Architecture.Tests/InfrastructureArchitectureTests.cs
Show resolved
Hide resolved
...gporten.Infrastructure/DomainEvents/Outbox/ConvertDomainEventsToOutboxMessagesInterceptor.cs
Show resolved
Hide resolved
Quality Gate passedIssues Measures |
Description
The DialogUpdated GraphQL subscription is not triggered when an activity is added, or when the dialog is deleted
Related Issue(s)
Verification
Documentation
docs
-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
DialogEventPayload
structure to handle dialog events, including updates and deletions.Bug Fixes
Chores
Tests