feat(webAPI): Add legacy HTML support for MainContentReference #1256
Conversation
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the 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: 1
🧹 Outside diff range and nitpick comments (4)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (3)
355-383: LGTM! Consider adding a comment explaining the test's purpose.The test case is well-structured and follows the existing patterns in the file. It correctly verifies that title content cannot use the
LegacyEmbeddableHtmlmedia type, even with the appropriate scope.Consider adding a brief comment at the beginning of the test method to explain why title content should not allow embeddable HTML, as this might not be immediately obvious to other developers.
385-411: LGTM! Consider adding a verification step.The test case is well-structured and correctly verifies that main content reference can use the
LegacyEmbeddableHtmlmedia type when the user has the appropriate scope.Consider adding a verification step to ensure that the created dialog actually contains the expected main content reference with the
LegacyEmbeddableHtmlmedia type. This could be done by retrieving the created dialog and asserting its properties.
355-411: Overall, the new tests look good. Consider additional scenarios.The two new test methods effectively cover the creation of dialogs with embeddable HTML content for both title and main content reference. They align well with the existing test structure and naming conventions.
To ensure comprehensive coverage of the new feature:
- Consider adding tests for other content types (e.g.,
AdditionalInfo) with embeddable HTML.- Add tests to verify behavior when a user doesn't have the correct scope for embeddable HTML.
- If not already present, ensure there are tests for the actual rendering or processing of the embeddable HTML content in the main application code.
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (1)
Line range hint
28-31: Ensure consistent use ofStringComparisoninStartsWithmethodThere is an inconsistency in the
StringComparisonparameter used in theStartsWithmethod within the two constructors:
- In the first constructor,
StringComparison.OrdinalIgnoreCaseis used.- In the second constructor,
StringComparison.InvariantCultureIgnoreCaseis used.For consistency and to prevent potential bugs due to different string comparison behaviors, consider using the same
StringComparisonoption in both constructors.Here's how you can unify the
StringComparisonparameter:Option 1: Use
StringComparison.OrdinalIgnoreCasein both constructors.// In the second constructor: - && x.MediaType.StartsWith(MediaTypes.EmbeddablePrefix, StringComparison.InvariantCultureIgnoreCase), + && x.MediaType.StartsWith(MediaTypes.EmbeddablePrefix, StringComparison.OrdinalIgnoreCase),Option 2: Use
StringComparison.InvariantCultureIgnoreCasein both constructors.// In the first constructor: - && x.MediaType.StartsWith(MediaTypes.EmbeddablePrefix, StringComparison.OrdinalIgnoreCase), + && x.MediaType.StartsWith(MediaTypes.EmbeddablePrefix, StringComparison.InvariantCultureIgnoreCase),Choose the option that aligns with your application's standards for string comparisons.
Also applies to: 55-58
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (2 hunks)
- src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs (1 hunks)
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2 hunks)
🧰 Additional context used
📓 Learnings (1)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs (2)
Learnt from: oskogstad PR: digdir/dialogporten#1210 File: tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:282-282 Timestamp: 2024-10-08T15:39:24.789Z Learning: Using square brackets `[]` for collection initialization is acceptable in the codebase's C# code.Learnt from: oskogstad PR: digdir/dialogporten#1210 File: tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Commands/CreateDialogTests.cs:282-282 Timestamp: 2024-10-01T17:03:49.291Z Learning: Using square brackets `[]` for collection initialization is acceptable in the codebase's C# code.
🔇 Additional comments (2)
src/Digdir.Domain.Dialogporten.Domain/MediaTypes.cs (1)
7-7: LGTM! New constant for legacy HTML support added correctly.The addition of
LegacyEmbeddableHtmlconstant aligns well with the PR objective of adding legacy HTML support for MainContentReference. The implementation follows the existing naming convention and structure, maintaining consistency within theMediaTypesclass.To ensure proper integration, let's verify the usage of this new constant in the codebase:
This will help identify if the new constant is being used appropriately throughout the codebase and if there are any places where the string literal is used directly instead of referencing the constant.
✅ Verification successful
Verified! The
LegacyEmbeddableHtmlconstant is consistently used across the codebase, and no hard-coded string literals matching its value were found. This ensures maintainability and consistency in handling legacy HTML media types.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new LegacyEmbeddableHtml constant # Search for direct usage of the constant echo "Searching for direct usage of LegacyEmbeddableHtml:" rg --type csharp "MediaTypes\.LegacyEmbeddableHtml" # Search for string literal that matches the constant value echo "Searching for string literal matching the constant value:" rg --type csharp "application/vnd\.dialogporten\.frontchannelembed\+json;type=html"Length of output: 1149
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs (1)
72-82: LGTM! Implementation correctly extends allowed media types based on user scopeThe changes appropriately extend the allowed media types for
DialogContentType.Values.AdditionalInfoandDialogContentType.Values.MainContentReferencewhen the user has theLegacyHtmlScope. The use of a switch expression simplifies the logic and enhances code readability.
...igdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (4)
212-213: LGTM! Consider clarifying if HTML is supported.The updated documentation clarifies that text/plain is a supported media type for the Title property. This change aligns with the PR objective of adding legacy HTML support.
Consider explicitly mentioning whether HTML is also supported for this property, to align fully with the PR's objective of adding legacy HTML support.
218-219: LGTM! Consider clarifying if HTML is supported.The updated documentation clarifies that text/plain is a supported media type for the Summary property. This change is consistent with the PR objective of adding legacy HTML support.
Consider explicitly mentioning whether HTML is also supported for this property, to fully align with the PR's objective of adding legacy HTML support.
230-231: LGTM! Consider clarifying HTML support.The updated documentation for the AdditionalInfo property now includes support for both text/plain and text/markdown. This is a positive change that allows for richer text formatting.
To fully align with the PR's objective of adding legacy HTML support, consider clarifying whether HTML is also supported for this property or if markdown is intended as an alternative to HTML for rich text formatting.
236-237: LGTM! Consider clarifying if HTML is supported.The updated documentation clarifies that text/plain is a supported media type for the ExtendedStatus property. This change is consistent with the PR objective of adding legacy HTML support.
Consider explicitly mentioning whether HTML is also supported for this property, to fully align with the PR's objective of adding legacy HTML support.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
- docs/schema/V1/swagger.verified.json (6 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDto.cs (0 hunks)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (1 hunks)
💤 Files with no reviewable changes (1)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDto.cs
🧰 Additional context used
🔇 Additional comments (9)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs (2)
225-225: LGTM! Documentation update is consistent.The updated documentation for the SenderName property maintains that text/plain is the supported media type, which is consistent with the previous requirement.
243-243: LGTM! Please clarify the relationship to legacy HTML support.The updated documentation for the MainContentReference property introduces support for a JSON-based media type with markdown content. This is a significant change that allows for rich text formatting.
Could you please clarify how this change relates to the PR objective of adding legacy HTML support? Is the markdown-in-JSON approach intended as an alternative to direct HTML support, or will HTML support be added in a different way?
To verify the implementation of this new media type, please run the following script:
docs/schema/V1/swagger.verified.json (7)
Line range hint
5-24: New enum schemas added: ActorType_Values and AttachmentUrlConsumerType_ValuesThese new enum schemas provide more structure and clarity to the API. They define specific values for actor types and attachment URL consumer types, which should improve consistency and reduce errors in API usage.
Line range hint
299-349: Updates to CreateDialogContentDto schemaThe CreateDialogContentDto schema has been updated with more detailed descriptions for its properties. These changes improve the API documentation and provide clearer guidance for API consumers.
Key improvements:
- More specific media type information for each property.
- Clearer descriptions of the purpose and usage of each field.
Line range hint
4476-4627: New path operations for dialog activities, transmissions, and seen logsSeveral new path operations have been added to the API, including:
- GET, POST operations for dialog activities
- GET, POST operations for dialog transmissions
- GET operations for dialog seen logs
These additions expand the API's functionality and provide more granular control over dialog-related data. The new operations follow a consistent pattern and include proper authentication and authorization checks.
Line range hint
4628-4731: Updates to existing path operationsExisting path operations have been updated with more detailed descriptions, additional query parameters, and improved response documentation. These changes enhance the API's usability and provide clearer guidance for API consumers.
Notable improvements:
- More detailed descriptions of operation behavior and requirements.
- Additional query parameters for filtering and customizing responses.
- Clearer documentation of response structures and possible status codes.
Line range hint
6469-6475: Addition of JWT Bearer authentication schemeA JWT Bearer authentication scheme has been added to the API specification. This improves the API's security model by requiring a valid JWT token for authentication.
The security scheme is well-defined and includes a clear description of its purpose and usage.
Line range hint
1-6476: Consistent application of security requirementsAfter reviewing the entire file, it's evident that the JWT Bearer authentication scheme is consistently applied across all path operations that require authentication. This ensures a uniform security model throughout the API.
Each protected endpoint includes the following security requirement:
"security": [ { "JWTBearerAuth": [] } ]This consistent application of security requirements is a good practice and helps prevent unauthorized access to sensitive operations.
Line range hint
1-6476: Overall improvements to the Dialogporten API specificationThis update to the Dialogporten API specification includes several significant improvements:
- New enum schemas (ActorType_Values and AttachmentUrlConsumerType_Values) that provide more structure and clarity.
- Updates to existing schemas with more detailed descriptions and clearer documentation.
- New path operations for managing dialog activities, transmissions, and seen logs, expanding the API's functionality.
- Improvements to existing path operations with more detailed descriptions, additional query parameters, and better response documentation.
- Addition of a JWT Bearer authentication scheme and consistent application of security requirements across protected endpoints.
These changes enhance the API's functionality, improve its documentation, and strengthen its security model. The updates follow API best practices and maintain consistency throughout the specification.
...Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogDto.cs
Show resolved
Hide resolved
...igdir.Domain.Dialogporten.Application/Features/V1/Common/Content/ContentValueDtoValidator.cs
Outdated
Show resolved
Hide resolved
|
oskogstad
deleted the
feat/add-legacy-html-support-for-main-content-ref
branch
🤖 I have created a release *beep* *boop* --- ## [1.23.0](v1.22.0...v1.23.0) (2024-10-10) ### Features * **infra:** upgrade postgresql SKU in test ([#1257](#1257)) ([5a751af](5a751af)) * **webAPI:** Add legacy HTML support for MainContentReference ([#1256](#1256)) ([482b38a](482b38a)) ### Bug Fixes * Add missing return types for Transmissions and Activities in OpenAPI spec ([#1244](#1244)) ([972870d](972870d)) * **graphQL:** Missing MediaType on dialog attachment url ([#1264](#1264)) ([3919343](3919343)) * Refactor probes and add more health checks ([#1159](#1159)) ([6889a96](6889a96)) * **webapi:** ensure correct health checks are used in probes ([#1249](#1249)) ([f951152](f951152)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Description
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation