fix(webAPI): Allow front channel embeds on TransmissionContent #1276
Conversation
oskogstad
force-pushed
the
fix/add-maincontent-to-transmission
branch
from
49c1f71 to
66d8b64
Compare
oskogstad
changed the title
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces a new field Changes
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range and nitpick comments (16)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241011202807_AddMainContentReferenceOnTransmissionContent.cs (1)
10-17: LGTM: TheUpmethod correctly adds the new content type.The insertion of the new
MainContentReferencecontent type aligns with the PR objective. The values set for each column are appropriate, particularly theAllowedMediaTypeswhich specifically allows for front channel embeds.Consider adding a comment explaining the significance of the
Idvalue 3 and why this specific media type is being added. This would improve the code's self-documentation:migrationBuilder.InsertData( table: "DialogTransmissionContentType", columns: new[] { "Id", "AllowedMediaTypes", "MaxLength", "Name", "Required" }, values: new object[] { 3, // Assuming 1 and 2 are used for existing content types new[] { "application/vnd.dialogporten.frontchannelembed+json;type=markdown" }, // Allows for front channel embeds 1023, "MainContentReference", false });tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (4)
19-39: LGTM: Well-structured test for creating a simple transmission.The test follows the Arrange-Act-Assert pattern and effectively validates the creation of a simple transmission. It checks both the response and the database state, which is crucial for integration tests.
Consider adding assertions for other properties of the created transmission, such as
EndUtc,StartUtc, or any other relevant fields to ensure all aspects of the transmission are correctly persisted.
41-77: LGTM: Comprehensive test for creating a transmission with embeddable content.The test effectively validates the creation of a transmission with embeddable content, checking both the response and the database state. It ensures the correct association between the transmission and its content.
Consider adding assertions to verify the actual content of the
DialogTransmissionContententity, such as checking theValueproperty to ensure it matches the expected URL. This would provide a more thorough validation of the content persistence.
79-103: LGTM: Good negative test case for URL validation.This test effectively ensures that the system rejects non-HTTPS URLs for embeddable content, which is an important security consideration.
Consider adding more test cases to cover edge cases:
- A URL with uppercase "HTTP" to ensure case-insensitive validation.
- A URL without a protocol (e.g., "example.com") to check how the system handles incomplete URLs.
- A URL with a valid HTTPS protocol but an invalid domain to ensure full URL validation.
These additional cases would provide more comprehensive coverage of the URL validation logic.
1-104: Overall: Well-structured and comprehensive test suite.The test file provides good coverage of transmission creation functionality, including both positive and negative scenarios. The use of fake data generation and helper methods enhances maintainability.
To further improve the test suite:
- Consider adding tests for edge cases, such as creating transmissions with minimum and maximum allowed content sizes.
- Add tests for concurrent transmission creation to ensure thread safety if applicable.
- Consider testing the behavior when creating transmissions for non-existent dialogs.
- If not covered elsewhere, add tests for different localization scenarios in the content.
These additions would provide a more exhaustive test suite, covering a wider range of potential issues and use cases.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1)
97-101: LGTM! Consider enhancing the comment slightly.The addition of the
MainContentReferenceproperty is well-implemented and aligns with the PR objective. The property is appropriately nullable, and its placement within theGetDialogTransmissionContentDtoclass is logical.Consider slightly enhancing the summary comment to provide more context:
/// <summary> /// Used to dynamically embed content in the frontend from an external URL. /// This allows for flexible, front-channel content embedding within the TransmissionContent. /// Allowed media types: application/vnd.dialogporten.frontchannelembed+json;type=markdown /// </summary> public ContentValueDto? MainContentReference { get; set; }This addition clarifies the purpose within the context of TransmissionContent, which aligns more closely with the PR objectives.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1)
103-107: LGTM! Consider adding validation and an example.The addition of the
MainContentReferenceproperty is well-implemented and aligns with the PR objectives. The property type, nullability, and naming convention are consistent with the existing code structure.Some suggestions for further improvement:
- Consider adding runtime validation for the allowed media types to ensure the constraint is enforced.
- It might be helpful to include an example in the summary comment to illustrate the expected format of the content reference.
Here's an example of how you might enhance the summary comment:
/// <summary> /// Used to dynamically embed content in the frontend from an external URL. /// Allowed media types: application/vnd.dialogporten.frontchannelembed+json;type=markdown /// </summary> /// <example> /// { /// "url": "https://example.com/content", /// "mimeType": "application/vnd.dialogporten.frontchannelembed+json;type=markdown" /// } /// </example> public ContentValueDto? MainContentReference { get; set; }For runtime validation, you might want to implement a custom validator or use attributes if you're using a validation framework.
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs (1)
123-123: LGTM! Consider adding a documentation comment.The addition of the
MainContentReferenceproperty to theTransmissionContentclass is consistent with the structure in theContentclass and aligns with the PR objective of enabling front channel embeds. The nullableContentValue?type provides flexibility for optional usage.Consider adding a brief XML documentation comment to explain the purpose and usage of this property:
/// <summary> /// Gets or sets the optional reference to the main content for front channel embeds. /// </summary> public ContentValue? MainContentReference { get; set; }docs/schema/V1/schema.verified.graphql (1)
256-256: LGTM! Consider adding a description for the new field.The addition of the
mainContentReferencefield to theTransmissionContenttype is a good enhancement that aligns with the PR objective of enabling front channel embeds. It provides more flexibility in content representation and is consistent with other fields in the type.To improve documentation, consider adding a description for the new field. This will help API consumers understand its purpose and proper usage. You can add a description like this:
type TransmissionContent { title: ContentValue! summary: ContentValue! "Reference to the main content of the transmission, such as a front channel embed." mainContentReference: ContentValue }src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs (1)
172-177: LGTM! Consider enhancing the comment for clarity.The addition of the
MainContentReferenceproperty aligns well with the PR objective to enable front channel embeds on TransmissionContent. The property is correctly implemented as nullable, making it optional.Consider slightly modifying the comment to enhance clarity:
/// <summary> /// Used to dynamically embed content in the frontend from an external URL. - /// Allowed media types: application/vnd.dialogporten.frontchannelembed+json;type=markdown + /// Allowed media type: application/vnd.dialogporten.frontchannelembed+json;type=markdown /// </summary> public ContentValueDto? MainContentReference { get; set; }This change emphasizes that there's only one allowed media type, which could help prevent potential misunderstandings.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (1)
259-264: LGTM! Consider enhancing the documentation.The addition of the
MainContentReferenceproperty aligns well with the PR objective to enable front channel embeds within the TransmissionContent component. The implementation is consistent with the existing structure and follows good practices.Consider adding a brief example of the expected JSON structure for the "application/vnd.dialogporten.frontchannelembed+json;type=markdown" media type in the XML documentation. This would provide clearer guidance for developers implementing this feature.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogDto.cs (1)
344-349: LGTM! Consider enhancing the comment.The addition of the
MainContentReferenceproperty toGetDialogDialogTransmissionContentDtois well-implemented and aligns with the PR objective. The property is correctly defined as nullable, and the comment provides clear guidance on its purpose and allowed media type.Consider enhancing the comment to include a brief explanation of how this property relates to front channel embeds, as mentioned in the PR title. This would provide more context for developers working with this code in the future. For example:
/// <summary> -/// Used to dynamically embed content in the frontend from an external URL. +/// Used to dynamically embed front channel content in the frontend from an external URL. /// Allowed media types: application/vnd.dialogporten.frontchannelembed+json;type=markdown +/// This property enables the integration of external content directly into the transmission display. /// </summary>src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (1)
178-197: Explicitly handle theNullabilityState.UnknowncaseIn the
switchstatement forpropMetadata.NullabilityInfo.WriteState, theNullabilityState.Unknowncase is currently empty. Consider handling this case explicitly or adding a comment to clarify the intended behavior. This will improve code readability and ensure that any future changes account for this state appropriately.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (3)
246-265: HandleNullabilityState.Unknownexplicitly or remove itIn the switch statement, the
NullabilityState.Unknowncase is currently empty. If this state is expected, consider handling it explicitly. Otherwise, you can remove the case to simplify the code.If
NullabilityState.Unknownis not anticipated:- switch (propMetadata.NullabilityInfo.WriteState) + switch (propMetadata.NullabilityInfo.WriteState) { case NullabilityState.NotNull: // handle NotNull case break; case NullabilityState.Nullable: // handle Nullable case break; - case NullabilityState.Unknown: - break; default: break; }
253-253: Remove unnecessary null-forgiving operatorAt line 253, the null-forgiving operator
!is used afternew ContentValueDtoValidator(...)!. Sincenewexpressions are non-nullable, the operator is unnecessary and can be removed to improve code clarity.Apply this change:
-.SetValidator(new ContentValueDtoValidator( - DialogTransmissionContentType.Parse(propertyName))!); +.SetValidator(new ContentValueDtoValidator( + DialogTransmissionContentType.Parse(propertyName)));
258-258: Remove unnecessary null-forgiving operatorSimilarly, at line 258, the
!operator is unnecessary after thenewexpression.Apply this change:
-.SetValidator(new ContentValueDtoValidator( - DialogTransmissionContentType.Parse(propertyName))!); +.SetValidator(new ContentValueDtoValidator( + DialogTransmissionContentType.Parse(propertyName)));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
- docs/schema/V1/schema.verified.graphql (1 hunks)
- docs/schema/V1/swagger.verified.json (15 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/Contents/DialogTransmissionContentType.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241011202807_AddMainContentReferenceOnTransmissionContent.Designer.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241011202807_AddMainContentReferenceOnTransmissionContent.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs (2 hunks)
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs
🧰 Additional context used
🔇 Additional comments (35)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241011202807_AddMainContentReferenceOnTransmissionContent.cs (3)
1-9: LGTM: Class declaration and namespace are correctly structured.The migration class is properly defined within the correct namespace, inherits from
Migration, and follows the naming convention for EF Core migrations. The use ofpartialand#nullable disableis appropriate for generated migration files.
19-26: LGTM: TheDownmethod correctly reverts the changes.The
Downmethod properly removes the row added in theUpmethod by targeting the correctId. This ensures that the migration can be rolled back if needed.
1-28: Overall, the migration file is well-implemented and achieves the PR objective.The
AddMainContentReferenceOnTransmissionContentmigration correctly adds support for front channel embeds in theTransmissionContentby introducing a new content type. BothUpandDownmethods are properly implemented, ensuring the change can be applied and reverted as needed.The only suggestion is to add a brief comment explaining the
Idvalue and the purpose of the new media type for better self-documentation.Great job on implementing this feature!
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/Contents/DialogTransmissionContentType.cs (3)
12-13: LGTM: New enum value added correctly.The new enum value
MainContentReference = 3has been added correctly. It follows the existing naming convention and uses the next available integer value.
35-40: LGTM: New case added for MainContentReference, but clarification needed.The new case for
Values.MainContentReferencehas been added correctly to theMapValuemethod. However, there are some differences in the property values compared to the other cases:
Requiredis set tofalse, while it'struefor other cases.MaxLengthis set to 1023, instead ofConstants.DefaultMaxStringLength.AllowedMediaTypesusesMediaTypes.EmbeddableMarkdowninstead ofMediaTypes.PlainText.Could you please provide the rationale behind these differences? This will help ensure that the new content type behaves as intended within the system.
Line range hint
1-40: Verify impact on wider system and update documentation if necessary.The introduction of the new
MainContentReferencecontent type with its unique properties may have implications for other parts of the system. Please ensure that:
- Any code that relies on the
DialogTransmissionContentTypeenum or class is updated to handle the new value.- The system correctly handles the different properties of
MainContentReference(e.g., not required, longer max length, different media type).- Documentation, including API docs and user guides, is updated to reflect this new content type and its usage.
Additionally, consider adding unit tests to verify the behavior of the new content type if they don't already exist.
To help verify the usage of
DialogTransmissionContentTypeacross the codebase, you can run the following script:This script will help identify areas of the code that might need to be updated to accommodate the new content type.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (1)
1-17: LGTM: Imports and class declaration are well-structured.The import statements, namespace declaration, and class definition are appropriate for the integration tests being implemented. The
Collectionattribute and constructor are correctly set up.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (2)
Line range hint
1-101: Summary: Well-implemented addition of MainContentReference propertyThe changes in this file are focused and align well with the PR objectives. The addition of the
MainContentReferenceproperty to theGetDialogTransmissionContentDtoclass enables front channel embeds on TransmissionContent as intended. The property is well-documented and consistent with the existing structure of the class.Key points:
- The new property is appropriately nullable and of type
ContentValueDto.- The summary comment provides clear guidance on its usage and constraints.
- The change is minimal and doesn't introduce any apparent inconsistencies.
Overall, this change appears to be a solid implementation of the required feature. Ensure that related components (e.g., mapping profiles, serialization logic) are updated as necessary to fully integrate this new property.
96-101: Verify usage of the new MainContentReference property.The addition of the
MainContentReferenceproperty toGetDialogTransmissionContentDtois consistent with the existing structure. To ensure full integration:
- Verify that any serialization/deserialization logic (if present) has been updated to include this new property.
- Check if any mapping logic (e.g., AutoMapper profiles) needs to be updated.
- Ensure that any consumers of
GetDialogTransmissionContentDtoare aware of and can handle this new property.Run the following script to check for potential usage and necessary updates:
src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs (1)
Line range hint
1-200: Overall, the change looks good. Consider adding unit tests.The addition of the
MainContentReferenceproperty to theTransmissionContentclass is the only change in this file, and it aligns well with the PR objectives. The implementation is consistent with the existing codebase structure.To ensure the robustness of this change, consider adding unit tests for the
TransmissionContentclass, particularly focusing on the newMainContentReferenceproperty. This will help validate its behavior and prevent potential regressions in the future.Run the following script to check for existing tests and identify potential locations for new tests:
If no suitable test file is found, consider creating a new test file for
TransmissionContentin an appropriate test directory.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogDto.cs (1)
354-359: New property added for front channel embed support.The addition of the
MainContentReferenceproperty toGetDialogDialogTransmissionContentDtoallows for dynamic embedding of content from an external URL in the frontend. This is a good enhancement for flexibility in content presentation.However, there are a few points to consider:
- The property is nullable, which is appropriate for optional content.
- The comment specifies the allowed media type, which is good for maintaining consistency.
To ensure consistency across the codebase, let's check if similar properties exist in related DTOs:
✅ Verification successful
Verified: 'MainContentReference' property is consistently added across DTO classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar properties in other DTO classes rg --type csharp 'class.*Dto' -A 20 | rg 'MainContentReference|ContentReference'Length of output: 1525
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogDto.cs (2)
Line range hint
1-649: LGTM! File changes are well-implemented and contained.The addition of the
MainContentReferenceproperty toGetDialogDialogTransmissionContentDtois the only change in this file. The modification is well-implemented and doesn't introduce any apparent issues or inconsistencies with the rest of the file. The existing code structure and naming conventions are maintained throughout.
344-349: Verify intentional difference and check for related updatesThe addition of
MainContentReferencetoGetDialogDialogTransmissionContentDtointroduces a slight inconsistency withGetDialogContentDto, which doesn't have this property. While this might be intentional due to different requirements at the dialog and transmission levels, it's worth confirming.Please confirm:
- Is the absence of
MainContentReferenceinGetDialogContentDtointentional?- Are there any other parts of the codebase that need to be updated to handle this new property in transmission content?
To help verify this, you can run the following script to check for usage of these DTOs:
This script will help identify areas of the codebase that might need attention due to this change.
✅ Verification successful
Difference between GetDialogDialogTransmissionContentDto and GetDialogContentDto is intentional
The inconsistency between
GetDialogDialogTransmissionContentDtoandGetDialogContentDtoregarding theMainContentReferenceproperty is intentional and reflects the different requirements for dialog-level and transmission-level content. Here's why:
- The
DialogTransmissionContentTypeenum includes aMainContentReferencevalue, whileDialogContentTypedoes not, indicating a deliberate design choice.- Separate mapping profiles, input/output converters, and database tables exist for dialog content and transmission content, supporting their distinct structures.
- This design allows for flexibility in handling specific requirements for transmission-level content that may not apply to dialog-level content.
No further action is needed unless there are specific requirements to align these DTOs that we're not aware of. The current implementation appears to be consistent with the intended design.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of GetDialogDialogTransmissionContentDto and GetDialogContentDto echo "Searching for GetDialogDialogTransmissionContentDto usage:" rg --type csharp "GetDialogDialogTransmissionContentDto" -A 5 echo "\nSearching for GetDialogContentDto usage:" rg --type csharp "GetDialogContentDto" -A 5 echo "\nSearching for potential places that might need updates for MainContentReference:" rg --type csharp "TransmissionContent|DialogContent" -A 5Length of output: 519375
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs (3)
Line range hint
1-1031: Summary of changes in DialogDbContextModelSnapshotThis migration snapshot includes two main changes:
- Updated Entity Framework Core version from 8.0.8 to 8.0.10.
- Added a new DialogTransmissionContentType for "MainContentReference" to support front channel embeds.
These changes align with the PR objective and should enable the desired functionality for front channel embeds in TransmissionContent.
To ensure these changes are fully integrated and don't introduce any issues:
#!/bin/bash # Check for any TODOs or FIXMEs that might have been added rg -i "TODO|FIXME" src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs # Verify that there are no unexpected changes in this file git diff --stat src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs
1023-1030: New DialogTransmissionContentType added: MainContentReferenceA new entry has been added to the
DialogTransmissionContentTypeentity's data seeding section. This new content type, "MainContentReference", allows for front channel embeds with markdown content, which aligns with the PR objective.Key details:
- Id: 3
- Name: "MainContentReference"
- AllowedMediaTypes: ["application/vnd.dialogporten.frontchannelembed+json;type=markdown"]
- MaxLength: 1023
- Required: false
To ensure this change is properly integrated:
- Check if related documentation has been updated:
#!/bin/bash # Search for documentation files that might need updating fd -e md -e txt | xargs rg -i "dialogtransmissioncontent|frontchannelembed"
- Verify that the application code handles this new content type correctly:
#!/bin/bash # Search for relevant code that might need updating rg -t cs "DialogTransmissionContent|frontchannelembed"
- Ensure that appropriate validation is in place for this new content type:
#!/bin/bash # Search for validation logic related to DialogTransmissionContent rg -t cs "DialogTransmissionContent.*Valid|frontchannelembed.*Valid"
20-20: Entity Framework Core version update approved.The
ProductVersionhas been updated from "8.0.8" to "8.0.10". This minor version update is good for keeping the project up-to-date with the latest bug fixes and improvements.It's recommended to check the Entity Framework Core changelog for any breaking changes or new features that might be relevant to this project. You can run the following command to view the changelog:
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241011202807_AddMainContentReferenceOnTransmissionContent.Designer.cs (6)
1-11: LGTM: File header and imports look good.The file header and import statements are correctly set up for an Entity Framework Core migration file.
12-17: LGTM: Migration class definition is correct.The migration class is properly defined with the correct naming convention and attributes.
640-694: New MainContentReference content type added correctly.The new MainContentReference content type has been added to the DialogContentType entity with the following properties:
- Id: 6
- Name: "MainContentReference"
- AllowedMediaTypes: ["application/vnd.dialogporten.frontchannelembed+json;type=markdown"]
- MaxLength: 1023
- OutputInList: false
- Required: false
This addition aligns with the PR objective of enabling front channel embeds within the TransmissionContent component.
1010-1034: MainContentReference added to DialogTransmissionContentType.The MainContentReference has also been added to the DialogTransmissionContentType entity with similar properties:
- Id: 3
- Name: "MainContentReference"
- AllowedMediaTypes: ["application/vnd.dialogporten.frontchannelembed+json;type=markdown"]
- MaxLength: 1023
- Required: false
This ensures consistency between DialogContent and DialogTransmissionContent.
18-2006: LGTM: Overall migration structure and entity definitions.The migration file correctly defines the database schema with proper relationships between entities. The addition of the MainContentReference type is consistent across relevant entities. No issues were found in the existing entity definitions or their relationships.
1-2006: Overall assessment: Migration implements required changes correctly.This migration file successfully adds the MainContentReference content type to both DialogContentType and DialogTransmissionContentType entities. The changes are consistent with the PR objective of enabling front channel embeds on TransmissionContent. The migration maintains proper relationships and consistency across the database schema. No issues or areas for improvement were identified.
docs/schema/V1/swagger.verified.json (9)
621-629: New feature: Dynamic content embedding for transmissionsThe addition of the
mainContentReferenceproperty to theCreateDialogDialogTransmissionContentDtoschema enables dynamic embedding of content from external URLs. This feature enhances the flexibility of the API by allowing front-channel embedded content.The property is correctly implemented as nullable and includes clear documentation on its usage and allowed media types.
1989-1997: Consistent implementation: Dynamic content retrieval for transmissionsThe addition of the
mainContentReferenceproperty to theGetDialogDialogTransmissionContentDtoschema is consistent with the changes in the Create DTO. This allows for retrieving dynamically embedded content in the API responses.The property is correctly implemented as nullable, maintaining backwards compatibility, and includes clear documentation on its usage and allowed media types.
2020-2028: Consistent implementation for service owners: Dynamic content retrievalThe addition of the
mainContentReferenceproperty to theGetDialogDialogTransmissionContentDtoSOschema maintains consistency across DTOs, including the service owner (SO) version. This allows service owners to retrieve dynamically embedded content in their API responses.The property is correctly implemented as nullable, maintaining backwards compatibility, and includes clear documentation on its usage and allowed media types.
Line range hint
4046-4065: Improved documentation: SearchDialogTransmissionAttachmentDtoThe additions to the
SearchDialogTransmissionAttachmentDtoschema enhance the API documentation by providing clear descriptions for existing properties. These changes improve the clarity for API consumers without altering the structure of the DTO.Key improvements:
- Added description for
displayNameproperty- Added description for
idproperty- Added description for
urlspropertyThese documentation enhancements will help developers better understand the purpose and usage of each property.
Line range hint
4071-4090: Consistent documentation improvement: SearchDialogTransmissionAttachmentDtoSOThe additions to the
SearchDialogTransmissionAttachmentDtoSOschema enhance the API documentation for service owners, consistent with the improvements made to the non-SO version. These changes provide clear descriptions for existing properties, improving clarity without altering the DTO structure.Key improvements:
- Added description for
displayNameproperty- Added description for
idproperty- Added description for
urlspropertyThis consistency in documentation across both regular and service owner DTOs ensures a uniform understanding of the API for all consumers.
4096-4119: Enhanced property descriptions: SearchDialogTransmissionAttachmentUrlDtoThe
SearchDialogTransmissionAttachmentUrlDtoschema has been updated with improved descriptions for its properties. These enhancements provide more context and clarity for API consumers without changing the structure of the DTO.Notable improvements:
- Added description for
consumerTypeproperty- Added description for
idproperty- Enhanced description for
mediaTypeproperty, including examples- Expanded description for
urlproperty, including information about authorizationThese detailed descriptions will help developers better understand the purpose and potential values of each property, leading to more effective use of the API.
4127-4150: Consistent documentation enhancement: SearchDialogTransmissionAttachmentUrlDtoSOThe
SearchDialogTransmissionAttachmentUrlDtoSOschema has been updated with improved descriptions for its properties, maintaining consistency with the non-SO version. These enhancements provide more context and clarity for service owner API consumers without altering the DTO structure.Key improvements:
- Added description for
consumerTypeproperty- Added description for
idproperty- Enhanced description for
mediaTypeproperty, including examples- Expanded description for
urlproperty, including information about authorizationThis consistency in documentation between regular and service owner DTOs ensures a uniform understanding of the API across all consumer types, facilitating easier integration and usage.
4157-4180: Consistent feature implementation: Dynamic content in SearchDialogTransmissionContentDtoThe
SearchDialogTransmissionContentDtoschema has been updated to include themainContentReferenceproperty, maintaining consistency with the changes made to other DTOs. This addition enables searching and retrieving dynamically embedded content from external URLs.Key points:
- The new
mainContentReferenceproperty is nullable, ensuring backwards compatibility.- Clear description and allowed media types are provided, guiding proper usage.
- This change aligns the search functionality with the create and get operations for transmission content.
The consistent implementation across DTOs ensures a uniform API experience for consumers, whether they're creating, retrieving, or searching for dialog transmissions.
4188-4211: Consistent feature for service owners: Dynamic content in SearchDialogTransmissionContentDtoSOThe
SearchDialogTransmissionContentDtoSOschema has been updated to include themainContentReferenceproperty, aligning with changes made to other DTOs, including the non-SO version. This addition enables service owners to search and retrieve dynamically embedded content from external URLs.Key points:
- The new
mainContentReferenceproperty is nullable, ensuring backwards compatibility.- The implementation is consistent with other DTOs, maintaining a uniform API structure.
- The description focuses specifically on "Front-channel embedded content," which slightly differs from the non-SO version but provides clarity for service owner use cases.
This change ensures that service owners have the same capabilities for handling dynamic content as regular API consumers, while the description tailors the explanation to their specific context.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs (4)
63-64:⚠️ Potential issueFix improper list initialization for
AttachmentsThe property
Attachmentsis initialized with=[], which is not valid in C#. You should initialize lists usingnew List<T>().Apply this diff to correct the initialization:
-public List<SearchDialogTransmissionAttachmentDto> Attachments { get; set; } = []; +public List<SearchDialogTransmissionAttachmentDto> Attachments { get; set; } = new List<SearchDialogTransmissionAttachmentDto>();Likely invalid or redundant comment.
118-119:⚠️ Potential issueFix improper list initialization for
DisplayNameThe property
DisplayNameis initialized with=[], which is not valid in C#. Initialize it usingnew List<LocalizationDto>().Apply this diff:
-public List<LocalizationDto> DisplayName { get; set; } = []; +public List<LocalizationDto> DisplayName { get; set; } = new List<LocalizationDto>();Likely invalid or redundant comment.
149-151: 🛠️ Refactor suggestionRemove unnecessary null-forgiving operator in nullable
MediaTypeThe property
MediaTypeis declared as nullable (string?), so assigning= null!;is unnecessary. You can remove the initialization or assign it tonull.Apply this diff:
-public string? MediaType { get; set; } = null!; +public string? MediaType { get; set; }Likely invalid or redundant comment.
123-124:⚠️ Potential issueFix improper list initialization for
UrlsThe property
Urlsis initialized with=[], which is not valid in C#. Initialize it usingnew List<SearchDialogTransmissionAttachmentUrlDto>().Apply this diff:
-public List<SearchDialogTransmissionAttachmentUrlDto> Urls { get; set; } = []; +public List<SearchDialogTransmissionAttachmentUrlDto> Urls { get; set; } = new List<SearchDialogTransmissionAttachmentUrlDto>();Likely invalid or redundant comment.
...Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs
Outdated
Show resolved
Hide resolved
...Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs
Outdated
Show resolved
Hide resolved
...Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (7)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (2)
Line range hint
242-246: LGTM! Consider adding a validation attribute for the HTTPS requirement.The updated documentation for
MainContentReferenceproperty clearly specifies the HTTPS requirement and the supported media type. This is a good improvement for security and clarity.Consider adding a validation attribute to enforce the HTTPS requirement programmatically. For example:
[HttpsOnly] public ContentValueDto? MainContentReference { get; set; }You would need to implement the
HttpsOnlyAttributeclass to perform the validation.
259-264: LGTM! Consider aligning the wording with the previous class for consistency.The addition of the
MainContentReferenceproperty toCreateDialogDialogTransmissionContentDtois consistent with the changes made toCreateDialogContentDto. The documentation clearly specifies the HTTPS requirement and the allowed media type.For consistency, consider aligning the wording of the documentation with the
CreateDialogContentDtoclass:/// <summary> /// Front-channel embedded content. Used to dynamically embed content in the frontend from an external URL. Must be HTTPS. /// Supported media types: application/vnd.dialogporten.frontchannelembed+json;type=markdown /// </summary> public ContentValueDto? MainContentReference { get; set; }This change would make the documentation consistent across both classes.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogDto.cs (2)
338-341: LGTM! Consider enhancing the documentation.The addition of the
MainContentReferenceproperty is well-implemented and properly documented. It aligns with the PR objective of enabling front channel embeds within the TransmissionContent component.Consider adding an example URL in the documentation to illustrate the expected format for the
MainContentReferenceproperty. This would provide clearer guidance for developers implementing this feature.
355-360: LGTM! Consider enhancing the documentation.The addition of the
MainContentReferenceproperty inGetDialogDialogTransmissionContentDtois well-implemented and properly documented. It's consistent with the changes made inGetDialogContentDtoand aligns with the PR objective.As suggested for
GetDialogContentDto, consider adding an example URL in the documentation to illustrate the expected format for theMainContentReferenceproperty. This would provide clearer guidance for developers implementing this feature.docs/schema/V1/swagger.verified.json (3)
Line range hint
1162-1171: Consistent addition of mainContentReference with a minor suggestionThe addition of the
mainContentReferenceproperty to theGetDialogContentDtoschema aligns with the changes made to the create operations, maintaining consistency across the API.However, there's a minor discrepancy in the property description:
Consider updating the description to include the HTTPS requirement, as seen in the
CreateDialogContentDto:- "description": "Front-channel embedded content. Used to dynamically embed content in the frontend from an external URL.", + "description": "Front-channel embedded content. Used to dynamically embed content in the frontend from an external URL. Must be HTTPS.",This will ensure consistency in documentation and reinforce the security requirement across all related DTOs.
4188-4196: Consistent addition of mainContentReference for SO search with a minor suggestionThe addition of the
mainContentReferenceproperty to theSearchDialogTransmissionContentDtoSOschema aligns with the changes made to other DTOs, maintaining consistency across the API for Service Owner operations.However, there's a minor discrepancy in the property description:
Consider updating the description to include the HTTPS requirement and allowed media types, as seen in other DTOs:
- "description": "Front-channel embedded content. Used to dynamically embed content in the frontend from an external URL.", + "description": "Front-channel embedded content. Used to dynamically embed content in the frontend from an external URL. Must be HTTPS. Allowed media types: application/vnd.dialogporten.frontchannelembed+json;type=markdown",This will ensure consistency in documentation across all related DTOs and provide complete information for Service Owners.
Line range hint
4501-4508: Addition of mainContentReference for update operations with a suggestionThe addition of the
mainContentReferenceproperty to theUpdateDialogContentDtoschema extends the front-channel embedded content feature to update operations, maintaining consistency across the API.However, the property description could be more comprehensive:
Consider updating the description to include information about allowed media types, as seen in other DTOs:
- "description": "Front-channel embedded content. Used to dynamically embed content in the frontend from an external URL. Must be HTTPS.", + "description": "Front-channel embedded content. Used to dynamically embed content in the frontend from an external URL. Must be HTTPS. Allowed media types: application/vnd.dialogporten.frontchannelembed+json;type=markdown",This will provide complete information for API consumers performing update operations and maintain consistency with other DTOs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- docs/schema/V1/swagger.verified.json (18 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogDto.cs (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs
🧰 Additional context used
🔇 Additional comments (8)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (1)
Line range hint
1-724: Overall, the changes look good and align with the PR objectives.The additions to both
CreateDialogContentDtoandCreateDialogDialogTransmissionContentDtoclasses consistently implement support for front-channel embedded content. The HTTPS requirement and specific media type are clearly documented, which enhances security and provides clear guidelines for usage.To further improve the implementation, consider:
- Adding validation attributes to enforce the HTTPS requirement.
- Ensuring that the handling of this new property is implemented consistently throughout the application, including any necessary changes to controllers, services, and database operations.
- Updating relevant unit tests to cover the new property and its constraints.
docs/schema/V1/swagger.verified.json (7)
Line range hint
317-329: New feature: Front-channel embedded content supportThe addition of the
mainContentReferenceproperty to theCreateDialogContentDtoschema enhances the API's capabilities by allowing dynamic embedding of content from external URLs. This feature provides more flexibility in dialog content presentation.Key points:
- Requires HTTPS for security
- Supports specific media types (application/vnd.dialogporten.frontchannelembed+json;type=markdown)
- Nullable, indicating it's an optional feature
This change is well-documented and aligns with modern web practices for dynamic content embedding.
621-629: Consistent implementation: Front-channel embedded content for transmissionsThe addition of the
mainContentReferenceproperty to theCreateDialogDialogTransmissionContentDtoschema is consistent with the changes made to theCreateDialogContentDto. This ensures that the front-channel embedded content feature is available for both dialog and transmission content.Key points:
- Maintains consistency across related DTOs
- Extends the dynamic content embedding feature to transmissions
- Uses the same requirements and media types as the dialog content
This change promotes a uniform approach to content handling within the API.
1989-1997: Well-documented addition of mainContentReference for transmission contentThe addition of the
mainContentReferenceproperty to theGetDialogDialogTransmissionContentDtoschema is consistent with previous changes and includes a comprehensive description.Key points:
- Explicitly mentions the HTTPS requirement
- Specifies the allowed media types
- Maintains consistency with other DTOs
This change is well-documented and provides clear guidance for API consumers. It serves as a good example for how other similar properties should be described.
2020-2028: Consistent implementation for Service Owner DTOThe addition of the
mainContentReferenceproperty to theGetDialogDialogTransmissionContentDtoSOschema mirrors the changes made to theGetDialogDialogTransmissionContentDto. This ensures consistency between the regular and Service Owner (SO) versions of the DTO.Key points:
- Identical property description, including HTTPS requirement and allowed media types
- Maintains parity between regular and SO DTOs
- Ensures that Service Owners have access to the same front-channel embedded content feature
This change promotes a uniform API experience across different user roles and maintains the integrity of the data model.
4157-4165: Extension of front-channel embedded content to search operationsThe addition of the
mainContentReferenceproperty to theSearchDialogTransmissionContentDtoschema extends the front-channel embedded content feature to search operations. This ensures that the feature is consistently available across create, get, and search operations.Key points:
- Consistent property description, including HTTPS requirement and allowed media types
- Enhances search functionality by including embedded content information
- Maintains a uniform data model across different API operations
This change improves the overall coherence of the API and ensures that clients can access embedded content information in various contexts, including search results.
4840-4848: Well-documented addition of mainContentReference for transmission content updatesThe addition of the
mainContentReferenceproperty to theUpdateDialogDialogTransmissionContentDtoschema is consistent with previous changes and includes a comprehensive description.Key points:
- Explicitly mentions the HTTPS requirement
- Specifies the allowed media types
- Maintains consistency with other DTOs
- Provides complete information for update operations on transmission content
This change is well-documented and provides clear guidance for API consumers performing update operations on dialog transmission content. It serves as a good example for how other similar properties should be described across the API.
Line range hint
1-5424: Summary: Comprehensive addition of front-channel embedded content supportThe changes made to this OpenAPI specification file significantly enhance the Dialogporten API by introducing support for front-channel embedded content. This feature has been consistently implemented across various DTOs, covering create, get, search, and update operations for both dialogs and transmissions.
Key observations:
- The
mainContentReferenceproperty has been added to relevant DTOs, allowing for dynamic embedding of content from external URLs.- Most implementations include proper documentation, specifying the HTTPS requirement and allowed media types.
- The feature is available for both regular and Service Owner (SO) operations, maintaining API consistency.
Suggestions for improvement:
- Ensure all instances of the
mainContentReferenceproperty description include the HTTPS requirement and allowed media types for consistency.- Review and update any related documentation or guides to inform API consumers about this new feature and its proper usage.
Overall, these changes provide a valuable addition to the API, offering more flexibility in content presentation while maintaining security through the HTTPS requirement. The consistent implementation across different operations and DTOs will help ensure a smooth integration experience for API consumers.
oskogstad
force-pushed
the
fix/add-maincontent-to-transmission
branch
from
44843fe to
1edc34b
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241012192435_AddContentReferenceOnTransmissions.cs (1)
5-9: LGTM! Consider enhancing the XML documentation.The class declaration looks good. It follows C# naming conventions and correctly inherits from
Migration. The use of apartialclass is appropriate for generated migration files.Consider enhancing the XML documentation to provide more information about the purpose of this migration, e.g.:
/// <summary> /// Adds support for content reference on transmissions by inserting a new row into the DialogTransmissionContentType table. /// </summary>src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/Contents/DialogTransmissionContentType.cs (1)
12-13: LGTM. Consider adding XML documentation.The addition of the
ContentReferenceenum value is consistent with the PR objectives and follows the existing naming convention.Consider adding XML documentation to describe the purpose and usage of this new enum value, which will improve code readability and maintainability.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (1)
Line range hint
187-223: LGTM! Consider extracting common nullability logic.The changes improve the validation by considering nullability states. This allows for more precise and flexible validation rules. However, there's similar logic in
CreateDialogDialogTransmissionContentDtoValidator. Consider extracting the common nullability handling into a base class or helper method to reduce code duplication and improve maintainability.Here's a potential refactoring approach:
- Create a base class:
public abstract class NullabilityAwareValidator<T> : AbstractValidator<T> { protected static readonly NullabilityInfoContext Context = new(); protected static readonly Dictionary<string, PropertyInfoWithNullability> SourcePropertyMetaDataByName; static NullabilityAwareValidator() { SourcePropertyMetaDataByName = typeof(T) .GetProperties() .Select(x => new PropertyInfoWithNullability(x, Context.Create(x))) .ToDictionary(x => x.Property.Name, StringComparer.InvariantCultureIgnoreCase); } protected void AddNullabilityRules(Func<string, object> contentTypeParser, Func<object, IValidator> validatorFactory) { foreach (var (propertyName, propMetadata) in SourcePropertyMetaDataByName) { var contentType = contentTypeParser(propertyName); var propertySelector = propMetadata.Property; switch (propMetadata.NullabilityInfo.WriteState) { case NullabilityState.NotNull: RuleFor(x => propertySelector.GetValue(x) as ContentValueDto) .NotNull() .WithMessage($"{propertyName} must not be empty.") .SetValidator(validatorFactory(contentType)!); break; case NullabilityState.Nullable: RuleFor(x => propertySelector.GetValue(x) as ContentValueDto) .SetValidator(validatorFactory(contentType)!) .When(x => propertySelector.GetValue(x) is not null); break; } } } }
- Update
CreateDialogContentDtoValidator:internal sealed class CreateDialogContentDtoValidator : NullabilityAwareValidator<CreateDialogContentDto?> { public CreateDialogContentDtoValidator(IUser? user) { AddNullabilityRules( DialogContentType.Parse, contentType => new ContentValueDtoValidator((DialogContentType)contentType, user) ); } }
- Update
CreateDialogDialogTransmissionContentDtoValidatorsimilarly.This approach centralizes the nullability logic, making it easier to maintain and extend in the future.
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241012192435_AddContentReferenceOnTransmissions.Designer.cs (1)
1-1992: Auto-Generated Code NoticeThis file is auto-generated. Manual changes may be overwritten. Ensure that any necessary customizations are implemented in partial classes or separate migration files.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (19)
- docs/schema/V1/schema.verified.graphql (1 hunks)
- docs/schema/V1/swagger.verified.json (19 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs (1 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 (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogDto.cs (3 hunks)
- src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/Contents/DialogTransmissionContentType.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241012192435_AddContentReferenceOnTransmissions.Designer.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241012192435_AddContentReferenceOnTransmissions.cs (1 hunks)
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs (1 hunks)
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (13)
- docs/schema/V1/schema.verified.graphql
- docs/schema/V1/swagger.verified.json
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Get/GetDialogTransmissionDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogDto.cs
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogDto.cs
- src/Digdir.Domain.Dialogporten.GraphQL/EndUser/DialogById/ObjectTypes.cs
- src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Transmissions/Commands/CreateTransmissionTests.cs
🧰 Additional context used
🔇 Additional comments (16)
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241012192435_AddContentReferenceOnTransmissions.cs (3)
19-26: LGTM! TheDownmethod correctly reverts the migration.The
Downmethod is implemented correctly to revert the changes made by theUpmethod. It precisely targets the inserted row by itsId, ensuring a clean rollback of the migration if needed.
1-28: Overall, the migration looks good and aligns with the PR objectives.This migration file correctly implements the addition of a new content type for front channel embeds in transmissions. The
UpandDownmethods are properly implemented, ensuring that the changes can be applied and reverted as needed.A few suggestions for improvement:
- Enhance the class-level XML documentation to provide more context about the purpose of this migration.
- Verify the
MaxLengthvalue of 1023 characters for the new content type and consider adding a comment explaining this limit if it's based on a specific requirement.These minor improvements would enhance the maintainability and clarity of the code without affecting its functionality.
10-17: LGTM! Verify theMaxLengthvalue.The
Upmethod correctly inserts a new row into theDialogTransmissionContentTypetable to support front channel embeds, which aligns with the PR objectives. The method signature and implementation are appropriate for an EF Core migration.Please verify if the
MaxLengthof 1023 characters is sufficient for all use cases of front channel embeds. If this limit is based on a specific requirement or constraint, consider adding a comment explaining the reason for this particular value.To check for any existing comments or documentation about this limit, you can run:
✅ Verification successful
Verified! The
MaxLengthvalue of 1023 is consistently applied throughout the codebase, aligning with existing implementations. No issues detected regarding its usage.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any comments or documentation about the MaxLength limit rg -i "maxlength.*1023|1023.*maxlength" --type csLength of output: 27458
src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/Transmissions/Contents/DialogTransmissionContentType.cs (2)
Line range hint
1-52: Overall implementation looks good.The changes in this file successfully introduce support for front channel embeds on transmission content, aligning well with the PR objectives. The new
ContentReferenceenum value and its corresponding case in theMapValuemethod are implemented correctly.A few minor suggestions:
- Consider adding XML documentation for the new enum value.
- Provide clarification on the choice of 1023 as the max length for
ContentReference.These changes enhance the flexibility of the
DialogTransmissionContentTypeclass while maintaining consistency with the existing code structure.
35-40: LGTM. Please clarify the max length choice.The new case for
Values.ContentReferencein theMapValuemethod is well-implemented and aligns with the PR objectives. The use ofMediaTypes.EmbeddableMarkdownsupports front channel embeds as intended.Could you please clarify the rationale behind choosing 1023 as the
MaxLengthforContentReference? This specific value seems arbitrary and might benefit from a brief explanation or justification.To verify the usage of
MediaTypes.EmbeddableMarkdown, let's run the following script:This will help us understand if
MediaTypes.EmbeddableMarkdownis consistently used across the codebase for similar purposes.✅ Verification successful
LGTM. Please clarify the max length choice.
The new case for
Values.ContentReferencein theMapValuemethod is well-implemented and aligns with the PR objectives. The use ofMediaTypes.EmbeddableMarkdownsupports front channel embeds as intended.Could you please clarify the rationale behind choosing 1023 as the
MaxLengthforContentReference? This specific value seems arbitrary and might benefit from a brief explanation or justification.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of MediaTypes.EmbeddableMarkdown in the codebase # Test: Search for MediaTypes.EmbeddableMarkdown usage rg --type csharp -A 5 "MediaTypes\.EmbeddableMarkdown"Length of output: 4840
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs (1)
97-100: Approved: New feature for front-channel embedded content.The addition of the
ContentReferenceproperty allows for dynamic content embedding from external URLs. This is a valuable feature that enhances the flexibility of the content delivery system.src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommandValidator.cs (3)
163-196: LGTM! Consider optimizing the property metadata initialization.The new approach using
NullabilityInfoContextandPropertyInfoWithNullabilityprovides more precise validation based on property nullability. This is a good improvement as it allows for different validation rules for nullable and non-nullable properties.However, consider moving the initialization of
SourcePropertyMetaDataByNameto a static constructor to ensure it's only computed once when the class is first used, which could slightly improve performance.Here's a suggested optimization:
internal sealed class UpdateDialogDialogTransmissionContentDtoValidator : AbstractValidator<UpdateDialogDialogTransmissionContentDto> { private static readonly NullabilityInfoContext Context = new(); - private static readonly Dictionary<string, PropertyInfoWithNullability> SourcePropertyMetaDataByName = typeof(UpdateDialogDialogTransmissionContentDto) - .GetProperties() - .Select(x => - { - var nullabilityInfo = Context.Create(x); - return new PropertyInfoWithNullability(x, nullabilityInfo); - }) - .ToDictionary(x => x.Property.Name, StringComparer.InvariantCultureIgnoreCase); + private static readonly Dictionary<string, PropertyInfoWithNullability> SourcePropertyMetaDataByName; + + static UpdateDialogDialogTransmissionContentDtoValidator() + { + SourcePropertyMetaDataByName = typeof(UpdateDialogDialogTransmissionContentDto) + .GetProperties() + .Select(x => new PropertyInfoWithNullability(x, Context.Create(x))) + .ToDictionary(x => x.Property.Name, StringComparer.InvariantCultureIgnoreCase); + } public UpdateDialogDialogTransmissionContentDtoValidator() { // ... rest of the constructor } }To ensure these changes don't have unintended consequences, let's verify the usage of
UpdateDialogDialogTransmissionContentDtoValidator:✅ Verification successful
Verification Successful: No Additional Usages Found
The
UpdateDialogDialogTransmissionContentDtoValidatorclass is only used withinUpdateDialogCommandValidator.cs. Moving the initialization ofSourcePropertyMetaDataByNameto a static constructor is safe and will improve performance without affecting other parts of the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of UpdateDialogDialogTransmissionContentDtoValidator rg --type csharp -A 5 "UpdateDialogDialogTransmissionContentDtoValidator"Length of output: 2207
Line range hint
238-271: LGTM! Consider optimizing and reducing duplication.The changes in
UpdateDialogContentDtoValidatorare consistent with those made inUpdateDialogDialogTransmissionContentDtoValidatorand provide the same benefits of more precise validation based on property nullability.
- Apply the same optimization suggestion as for
UpdateDialogDialogTransmissionContentDtoValidator:internal sealed class UpdateDialogContentDtoValidator : AbstractValidator<UpdateDialogContentDto> { private static readonly NullabilityInfoContext Context = new(); - private static readonly Dictionary<string, PropertyInfoWithNullability> SourcePropertyMetaDataByName = - typeof(UpdateDialogContentDto).GetProperties() - .Select(x => - { - var nullabilityInfo = Context.Create(x); - return new PropertyInfoWithNullability(x, nullabilityInfo); - }) - .ToDictionary(x => x.Property.Name, StringComparer.InvariantCultureIgnoreCase); + private static readonly Dictionary<string, PropertyInfoWithNullability> SourcePropertyMetaDataByName; + + static UpdateDialogContentDtoValidator() + { + SourcePropertyMetaDataByName = typeof(UpdateDialogContentDto) + .GetProperties() + .Select(x => new PropertyInfoWithNullability(x, Context.Create(x))) + .ToDictionary(x => x.Property.Name, StringComparer.InvariantCultureIgnoreCase); + } public UpdateDialogContentDtoValidator(IUser? user) { // ... rest of the constructor } }
- Consider extracting the common logic for property metadata initialization and validation into a base class or helper method to reduce duplication between
UpdateDialogDialogTransmissionContentDtoValidatorandUpdateDialogContentDtoValidator.Let's check for other similar validators that might benefit from this refactoring:
Line range hint
1-471: Overall improvements with room for further refactoringThe changes in this file significantly improve the validation logic by considering property nullability, which allows for more precise validation rules. This approach is consistently applied across multiple validator classes, particularly in
UpdateDialogDialogTransmissionContentDtoValidatorandUpdateDialogContentDtoValidator.While these changes are beneficial, there's an opportunity for further improvement:
The similar logic in
UpdateDialogDialogTransmissionContentDtoValidatorandUpdateDialogContentDtoValidatorsuggests that this pattern might be reusable. Consider creating a base class or a generic helper method to encapsulate this common logic, which could reduce duplication and make it easier to apply this pattern to other validators in the future.The
NullabilityInfoContextand property metadata initialization logic could be moved to a separate utility class, making it easier to reuse across different parts of the application.Here's a high-level suggestion for refactoring:
Create a base class
NullabilityAwareValidator<T>that encapsulates the common logic for initializing property metadata and applying nullability-aware validation rules.Modify
UpdateDialogDialogTransmissionContentDtoValidatorandUpdateDialogContentDtoValidatorto inherit from this base class.Create a utility class
NullabilityHelperto handle the creation and caching ofNullabilityInfoContextand property metadata.This refactoring would improve code organization, reduce duplication, and make it easier to apply this nullability-aware validation pattern to other DTOs in the future.
To assess the potential impact of this refactoring, let's check for other validator classes that might benefit from this approach:
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommandValidator.cs (1)
228-264: LGTM! Remember to consider the refactoring suggestion.The changes in this class mirror those in
CreateDialogContentDtoValidator, providing improved validation based on nullability states. This implementation is correct and follows good practices.As mentioned in the previous comment, consider refactoring both this class and
CreateDialogContentDtoValidatorto use a common base class or helper method. This would reduce code duplication and improve maintainability.src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241012192435_AddContentReferenceOnTransmissions.Designer.cs (6)
15-16: Migration Class is Properly DefinedThe migration class
AddContentReferenceOnTransmissionsis correctly named and annotated, ensuring it reflects the purpose of the migration.
981-1030: Addition ofContentReferenceinDialogTransmissionContentTypeThe new
ContentReferencecontent type is appropriately added with the correct properties:
- Id:
3- Name:
"ContentReference"- AllowedMediaTypes:
new[] { "application/vnd.dialogporten.frontchannelembed+json;type=markdown" }- MaxLength:
1023- Required:
falseThis aligns with the PR objective to support front channel embeds on transmission content.
608-690: Addition ofMainContentReferenceinDialogContentTypeThe new
MainContentReferencecontent type is correctly introduced with the following specifications:
- Id:
6- Name:
"MainContentReference"- AllowedMediaTypes:
new[] { "application/vnd.dialogporten.frontchannelembed+json;type=markdown" }- MaxLength:
1023- OutputInList:
false- Required:
falseThis ensures that the main content reference is properly integrated into the dialog content types.
674-680: Consistency inDialogContentTypeEntriesEnsure that all content types within
DialogContentTypemaintain consistent formatting and definitions. The previous entry forExtendedStatusis correctly formatted.
1006-1030: Data Seeding forDialogTransmissionContentTypeThe seeding data for
DialogTransmissionContentTypeis appropriately updated to include the new content type. This is crucial for the application to recognize and handle the new content type without additional manual setup.
715-718: Optional PropertyExpiresAtWithout Default ValueThe
ExpiresAtproperty inDialogEntityis nullable, which is acceptable. Ensure that business logic correctly handles cases whereExpiresAtisnull.Consider adding tests to confirm that dialogs without an expiration date behave as expected.
...n/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs
Show resolved
Hide resolved
...n/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs
Show resolved
Hide resolved
...n/Features/V1/ServiceOwner/DialogTransmissions/Queries/Search/SearchDialogTransmissionDto.cs
Show resolved
Hide resolved
...ructure/Persistence/Migrations/20241012192435_AddContentReferenceOnTransmissions.Designer.cs
Show resolved
Hide resolved
oskogstad
changed the title
|
🤖 I have created a release *beep* *boop* --- ## [1.23.2](v1.23.1...v1.23.2) (2024-10-14) ### Bug Fixes * **webAPI:** Allow front channel embeds on TransmissionContent ([#1276](#1276)) ([c87e8f4](c87e8f4)) --- 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
Summary by CodeRabbit
New Features
contentReferenceto theTransmissionContenttype, allowing for additional content references.contentReferenceand improved descriptions for clarity.Bug Fixes
mainContentReferenceto ensure compliance with specified constraints.Tests
Createcommand in the ServiceOwner feature to validate transmission creation functionality.