Skip to content
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

feat: basic label implementation to hide dialogs #1192

Merged
merged 40 commits into from
Oct 3, 2024

Conversation

Fargekritt
Copy link
Contributor

@Fargekritt Fargekritt commented Sep 25, 2024

Description

  • Added systemlabel (Bin/Archive) to hide dialogs
  • Added post enpoints for endusers to change labels
  • Added LabelAssignmentLogs

Related Issue(s)

#841

Verification

  • Your code builds clean without any errors or warnings
  • Manual testing done (required)
  • Relevant automated test added (if you find this hard, leave it and we'll help out)

Documentation

  • Documentation is updated (either in docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)

Summary by CodeRabbit

  • New Features

    • Introduced a new mutation for setting system labels, enhancing dialog management capabilities.
    • Added new API endpoints for retrieving and setting dialog labels, improving user interaction with dialog states.
    • Implemented logging for label assignments, allowing users to track changes effectively.
    • Enhanced search functionality to filter dialogs based on display state.
    • Added support for handling system labels through a structured command and validation process.
    • Introduced new data structures and interfaces for improved dialog label assignment and error handling.
  • Bug Fixes

    • Enhanced validation for dialog labels to ensure correct formatting and values.
  • Documentation

    • Updated schema and API documentation to reflect new features and endpoints.
  • Chores

    • Modified project structure for better organization of dialog label features.
    • Added support for pre-release versions in the SDK configuration.

Copy link
Contributor

coderabbitai bot commented Sep 25, 2024

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

The pull request introduces a new mutation for setting system labels in a dialog management context, along with related data structures and API endpoints. It enhances functionality for managing dialog display states through the setSystemLabel mutation and updates the schema, Swagger documentation, and various application components to support these changes. Additionally, it modifies the database context and introduces new query and command classes for handling label assignments and logs.

Changes

File Change Summary
docs/schema/V1/schema.verified.graphql Added setSystemLabel mutation, SetSystemLabelInput, SetSystemLabelPayload, SetSystemLabelError interface, and SystemLabel enum.
docs/schema/V1/swagger.verified.json Introduced systemLabel property, new data structures for logging, and endpoints for managing dialog labels.
global.json Added allowPrerelease property under the sdk object.
src/Digdir.Domain.Dialogporten.Application/Externals/IDialogDbContext.cs Added properties for DialogEndUserContexts and LabelAssignmentLogs to the database context.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/MappingProfile.cs Created MappingProfile for mapping between entities and DTOs related to label assignment logs.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchDialogLabelAssignmentLogDto.cs Introduced DTOs for search results related to label assignment logs.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchDialogLabelAssignmentLogQuery.cs Added query class for retrieving label assignment logs.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelCommand.cs Created command class for setting dialog labels with concurrency management.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelDto.cs Defined DTO for setting dialog labels, including dialog ID and label properties.

Possibly related issues

Possibly related PRs

  • feat: basic label implementation to hide dialogs #1192: This PR introduces the setSystemLabel mutation and related types, which directly aligns with the changes in the main PR that also adds the setSystemLabel mutation and the SystemLabel enum, enhancing the schema for managing dialog labels.

Suggested reviewers

  • MagnusSandgren

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@Fargekritt Fargekritt force-pushed the feature/basic-label-implementation branch from bbb8981 to d4ef702 Compare September 27, 2024 08:45
@Fargekritt Fargekritt changed the title Feature/basic label implementation feaat: basic label implementation to hide dialogs Sep 27, 2024
@Fargekritt Fargekritt changed the title feaat: basic label implementation to hide dialogs feat: basic label implementation to hide dialogs Sep 27, 2024
@Fargekritt Fargekritt marked this pull request as ready for review September 27, 2024 09:00
@Fargekritt Fargekritt requested a review from a team as a code owner September 27, 2024 09:01
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 49

🧹 Outside diff range comments (2)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (1)

Line range hint 1-199: Summary: SystemLabel filtering implemented, tests needed

The changes introduce a new filtering option for dialogs based on system labels, which is a valuable addition to the search functionality. The implementation is mostly correct, with a minor logical error that has been addressed in a previous comment.

To ensure the reliability and maintainability of this new feature, consider adding the following:

  1. Unit tests for the SearchDialogQuery class to verify the behavior of the new SystemLabel property.
  2. Integration tests for the SearchDialogQueryHandler to confirm that the filtering based on SystemLabel works as expected.
  3. Update any existing tests that may be affected by this change.

Would you like assistance in creating these tests or opening a GitHub issue to track this task?

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (1)

Line range hint 178-201: LGTM: Well-implemented security measure for unauthorized URLs.

The ReplaceUnauthorizedUrls method effectively prevents unauthorized access to sensitive URLs:

  • It covers all types of actions (GUI and API) and transmissions.
  • Using Constants.UnauthorizedUri for replacement is a good practice.

Minor optimization suggestion:
Consider using LINQ's ForEach method for a more concise implementation, especially for the transmission attachments. For example:

dialogTransmission.Attachments
    .SelectMany(a => a.Urls)
    .ToList()
    .ForEach(url => url.Url = Constants.UnauthorizedUri);

This change would make the code slightly more readable and maintainable.

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between f20a738 and c94424a.

📒 Files selected for processing (20)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssigmentLog/Queries/Search/MappingProfile.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssigmentLog/Queries/Search/SearchDialogLabelAssignmentLogQuery.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabels/Commands/Set/SetDialogLabelCommand.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabels/Commands/Set/SetDialogLabelCommandValidator.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabels/Commands/Set/SetDialogLabelDto.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (3 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (3 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQueryValidator.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (4 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQueryValidator.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/DialogEndUserContext.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/LabelAssignmentLog.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/SystemLabel.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (4 hunks)
  • src/Digdir.Domain.Dialogporten.GraphQL/EndUser/MutationTypes/Mutations.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs (2 hunks)
  • src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20240925075200_AddSystemLabel.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssigmentLogs/Search/SearchDialogLabelAssigmentLogEndpoint.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabels/Set/SetDialogLabelEndpoint.cs (1 hunks)
🔇 Additional comments (24)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssigmentLog/Queries/Search/MappingProfile.cs (2)

7-10: LGTM: Class structure is well-defined.

The MappingProfile class is correctly defined as sealed and inherits from Profile. The constructor is properly implemented. This structure follows good practices for AutoMapper profile classes.

Also applies to: 14-14


11-13: Verify complete property mappings.

The mapping configurations look correct for basic property mapping. However, ensure that all relevant properties from the source entities are properly mapped to their corresponding DTO properties.

To verify the completeness of the mappings, run the following script:

Compare the output to ensure all necessary properties are mapped.

src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/LabelAssignmentLog.cs (2)

6-17: LGTM: Well-structured class for logging label assignments.

The LabelAssignmentLog class is well-designed for its purpose. It implements IImmutableEntity, which is appropriate for logging scenarios where data should not be modified after creation. The properties cover essential information for tracking label assignments, including the action performed, the context, and who performed the action.


19-23: LGTM: Well-structured class for representing label assignment actors.

The LabelAssignmentLogActor class is well-designed to represent the actor who performed a label assignment action. It appropriately inherits from Actor and implements IImmutableEntity, maintaining consistency with the LabelAssignmentLog class.

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabels/Commands/Set/SetDialogLabelCommandValidator.cs (2)

1-4: LGTM: Imports and namespace are appropriate.

The imports and namespace declaration are correct and follow the expected structure for this validator class.


6-7: LGTM: Class declaration follows best practices.

The class is correctly named, inherits from the appropriate base class, and is sealed to prevent inheritance. This follows best practices for command validators using FluentValidation.

src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/SystemLabel.cs (1)

6-20: LGTM: Well-structured SystemLabel class implementation.

The SystemLabel class is well-designed:

  • Sealed class prevents unintended inheritance.
  • Enum values start from 1, avoiding potential issues with default 0 value.
  • MapValue method creates a new instance, promoting immutability.
  • Overall structure is clean and follows good design principles.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssigmentLogs/Search/SearchDialogLabelAssigmentLogEndpoint.cs (2)

1-7: LGTM: Import statements are appropriate and relevant.

The import statements cover all necessary namespaces for the implementation, including diagnostics, application features, authorization, extensions, FastEndpoints, and MediatR.


18-25: LGTM: Configure method is well-structured and follows best practices.

The Configure method effectively sets up the endpoint:

  • Correctly specifies a GET request at "dialogs/{dialogId}/labellog".
  • Applies an authorization policy for end users, enhancing security.
  • Groups the endpoint under EndUserGroup, improving organization.
  • Sets up Swagger description, which is great for API documentation.

These practices contribute to a well-organized and secure API structure.

src/Digdir.Domain.Dialogporten.Domain/Dialogs/Entities/DialogEntity.cs (3)

3-3: LGTM: New using statement added correctly.

The new using statement for Digdir.Domain.Dialogporten.Domain.DialogEndUserContexts.Entities is correctly placed and is necessary for the new DialogEndUserContext property.


101-104: LGTM: Improved code formatting.

The indentation changes in the LINQ query within the UpdateSeenAt method enhance code readability without altering functionality.


Line range hint 1-145: Verify complete implementation of the labeling system.

The addition of the DialogEndUserContext property aligns with the PR objectives of implementing a labeling system for dialogs. However, to ensure a comprehensive implementation:

  1. Verify that other necessary changes have been made across the codebase to support this new property.
  2. Ensure that the DialogEndUserContext is properly initialized and used in relevant parts of the application.
  3. Check if any database migrations are required for this change.

To assist in verifying the implementation, you can run the following script:

This script will help identify related changes and potential areas that might need attention to fully implement the labeling system.

✅ Verification successful

Complete Implementation of the Labeling System Verified

The DialogEndUserContext and related label functionalities have been fully integrated across the codebase. All necessary usages, initializations, and database migrations are in place to support the labeling system as intended.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for usages of DialogEndUserContext and related classes/methods

# Search for DialogEndUserContext usage
echo "Searching for DialogEndUserContext usage:"
rg --type csharp "DialogEndUserContext"

# Search for label-related classes or interfaces
echo "\nSearching for label-related classes or interfaces:"
rg --type csharp "class.*Label|interface.*Label"

# Search for methods related to label operations
echo "\nSearching for methods related to label operations:"
rg --type csharp "method.*(Add|Remove|Update|Get).*Label"

# Search for potential database migration files
echo "\nSearching for potential database migration files:"
fd -e cs -e sql "migration|label"

Length of output: 26558

src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/DialogDbContext.cs (1)

47-48: Verify database schema updates and migrations.

The new properties DialogEndUserContexts and LabelAssignmentLogs have been added correctly to the DialogDbContext. To ensure full implementation:

  1. Confirm that corresponding entity configurations exist (e.g., in separate configuration classes).
  2. Verify that a new migration has been created to update the database schema.
  3. Ensure that the migration has been applied to all relevant environments.

To check for entity configurations and migrations, run:

✅ Verification successful

Database schema updates and migrations are properly implemented.

The new properties DialogEndUserContexts and LabelAssignmentLogs have been correctly added to the DialogDbContext with corresponding entity configurations and migration files present in the codebase. To ensure full implementation:

  1. Entity Configurations:

    • Confirm that DialogEndUserContextConfiguration and LabelAssignmentLogConfiguration classes are correctly set up.
  2. Migrations:

    • Verify that the 20240925075200_AddSystemLabel migration has been created and includes the necessary schema changes.
    • Ensure that the migration has been applied to all relevant environments (development, staging, production).
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check for entity configurations
echo "Searching for DialogEndUserContext configuration:"
rg -i "dialogenduser.*configure" --type csharp

echo "Searching for LabelAssignmentLog configuration:"
rg -i "labelassignment.*configure" --type csharp

# Check for migrations
echo "Searching for migrations mentioning the new entities:"
rg -i "dialogenduser|labelassignment" --type csharp ./Migrations

Length of output: 550


Script:

#!/bin/bash
# Search for Entity Framework migration commands in the codebase
echo "Searching for migration commands in the codebase:"
rg -i "Add-Migration|Update-Database" --type csharp

# Search for migration classes or configurations using ast-grep
echo "Searching for migration classes:"
ast-grep --lang csharp --pattern $'public class $_ : Migration { $$$ }'

# Search for any DbContext configurations related to the new entities
echo "Searching for DbContext configurations for DialogEndUserContext and LabelAssignmentLog:"
rg -i "modelBuilder.Entity<DialogEndUserContext>" --type csharp
rg -i "modelBuilder.Entity<LabelAssignmentLog>" --type csharp

Length of output: 625


Script:

#!/bin/bash

# Search the entire codebase for mentions of DialogEndUserContext and LabelAssignmentLog
echo "Searching for mentions of DialogEndUserContext and LabelAssignmentLog in the codebase:"
rg -i "DialogEndUserContext|LabelAssignmentLog" --type csharp

# Search for any classes that inherit from Migration
echo "Searching for migration classes in the codebase:"
ast-grep --lang csharp --pattern $'public class $_ : Migration { $$$ }'

# Search for OnModelCreating methods that configure the new entities
echo "Searching for OnModelCreating configurations for DialogEndUserContext and LabelAssignmentLog:"
rg -i "OnModelCreating" --type csharp | rg -i "DialogEndUserContext|LabelAssignmentLog"

Length of output: 29492

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (1)

11-11: LGTM: Import statement added for DialogEndUserContexts.Entities

The added import statement is appropriate and necessary for the new SystemLabel property introduced in this file.

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Get/GetDialogQuery.cs (3)

81-82: LGTM: Improved query efficiency and added necessary context.

The changes to the SeenLog filtering and the inclusion of DialogEndUserContext are well-implemented:

  1. Filtering SeenLog entries to those created after the dialog's last update improves query efficiency.
  2. Including DialogEndUserContext aligns with the PR objective of implementing a labeling system for dialogs.

These changes enhance the query's performance and functionality.

Also applies to: 84-84


155-155: LGTM: Improved readability of authorization conditions.

The reformatting of the conditions for setting IsAuthorized on actions enhances code readability:

  • The logic remains unchanged.
  • The separation of cases for null and non-null authorization attributes is now more explicit.

This change contributes to better code maintainability.

Also applies to: 164-164


Line range hint 1-201: Verify alignment with PR objectives: "Bin/Archive" label implementation.

The changes in this file align well with the PR objectives of implementing a basic label system for dialogs:

  • The inclusion of DialogEndUserContext suggests that labels are now part of the dialog context.
  • Enhanced authorization checks and URL replacement improve security.

However, I couldn't find an explicit implementation of the "Bin/Archive" system label mentioned in the PR objectives. Could you please clarify:

  1. Where is the "Bin/Archive" label logic implemented?
  2. How does it interact with the GetDialogQuery to hide dialogs?

This information will help ensure that all PR objectives are met and properly reviewed.

src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabels/Set/SetDialogLabelEndpoint.cs (2)

13-20: Configuration Method is Well-Structured

The Configure method correctly sets up the endpoint with the appropriate route, authorization policies, grouping, and Swagger documentation. This ensures that the endpoint is properly exposed and secured.


9-11: ⚠️ Potential issue

Potential Syntax Error with Primary Constructor in Class Declaration

The use of a primary constructor in the class declaration may not be supported in the current C# version you are targeting. Primary constructors are typically available for records, not classes. To ensure compatibility and avoid potential syntax errors, consider refactoring the class to use a standard constructor.

Apply this diff to fix the issue:

-public sealed class SetDialogLabelEndpoint(ISender sender) : Endpoint<SetDialogLabelCommand>
-{
-    private readonly ISender _sender = sender ?? throw new ArgumentNullException(nameof(sender));
+public sealed class SetDialogLabelEndpoint : Endpoint<SetDialogLabelCommand>
+{
+    private readonly ISender _sender;
+
+    public SetDialogLabelEndpoint(ISender sender)
+    {
+        _sender = sender ?? throw new ArgumentNullException(nameof(sender));
+    }

Likely invalid or redundant comment.

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssigmentLog/Queries/Search/SearchDialogLabelAssignmentLogQuery.cs (2)

19-19: Confirm the necessity of the partial keyword in SearchDialogLabelAssignmentLogResult

The class SearchDialogLabelAssignmentLogResult is declared as public sealed partial class.

Typically, the partial keyword is used when a class is split across multiple files or when code generation tools expand the class definition.

Please verify if the partial keyword is required due to the [GenerateOneOf] attribute.

If not, consider removing partial to avoid confusion.


43-46: Differentiate between not found and deleted entities

Currently, if the dialog is not found, you return an EntityNotFound.

Later, if the dialog is marked as deleted, you return an EntityDeleted.

Ensure that the client can distinguish between a dialog that does not exist and one that has been deleted for better error handling on the client side.

src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Get/GetDialogQuery.cs (1)

86-86: ⚠️ Potential issue

Verify the impact of including DialogEndUserContext with .IgnoreQueryFilters()

Including DialogEndUserContext using .Include(x => x.DialogEndUserContext) after invoking .IgnoreQueryFilters() may result in loading entities that are normally filtered out by global query filters, potentially exposing data that should remain hidden. Please ensure that this inclusion does not unintentionally bypass any security measures, and that appropriate authorization checks are applied.

Run the following script to check if DialogEndUserContext has any global query filters applied:

✅ Verification successful

No global query filters found on DialogEndUserContext

Including DialogEndUserContext with .Include(x => x.DialogEndUserContext) after invoking .IgnoreQueryFilters() does not bypass any global query filters since none are applied. Therefore, there is no risk of unintentionally exposing restricted data.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Identify any global query filters applied to DialogEndUserContext.

# Search for query filters configured on DialogEndUserContext entities.
rg --type csharp 'HasQueryFilter' -A 5 -B 2 | rg 'DialogEndUserContext'

# List properties of DialogEndUserContext to assess for sensitive data.
rg --type csharp 'class DialogEndUserContext' -A 20

Length of output: 4605

src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (2)

11-11: Import statement is appropriate.

The added using directive brings in DialogEndUserContext entities, which are necessary for the new implementation.


187-187: Approve the addition of the Process filter.

The WhereIf clause correctly adds a condition to filter dialogs based on the Process field using a case-insensitive LIKE comparison, enhancing the query's flexibility.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 44

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between c94424a and 97f8277.

📒 Files selected for processing (17)
  • docs/schema/V1/swagger.verified.json (12 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/MappingProfile.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchDialogLabelAssignmentLogDto.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/SearchDialogLabelAssignmentLogQuery.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelCommand.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelCommandValidator.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelDto.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (3 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (4 hunks)
  • src/Digdir.Domain.Dialogporten.GraphQL/EndUser/MutationTypes/MappingProfile.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.GraphQL/EndUser/MutationTypes/Mutations.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssigmentLogs/Search/SearchDialogLabelAssigmentLogEndpoint.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssigmentLogs/Search/SearchDialogLabelAssignmentSwaggerConfig.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogSystemLabels/Set/SetDialogSystemLabelEndpoint.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogSystemLabels/Set/SetDialogSystemLabelSwaggerConfig.cs (1 hunks)
  • tests/k6/common/testimports.js (2 hunks)
  • tests/k6/tests/enduser/dialogSystemLabelLog.js (1 hunks)
🧰 Additional context used
🪛 Biome
tests/k6/tests/enduser/dialogSystemLabelLog.js

[error] 22-22: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 24-24: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 36-36: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 38-38: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 47-47: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 49-49: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 57-57: Template literals are preferred over string concatenation.

Unsafe fix: Use a template literal.

(lint/style/useTemplate)


[error] 11-11: This let declares a variable that is only assigned once.

'd' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)


[error] 12-12: This let declares a variable that is only assigned once.

'r' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)


[error] 19-19: This let declares a variable that is only assigned once.

'body' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)


[error] 33-33: This let declares a variable that is only assigned once.

'body' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)


[error] 57-57: This let declares a variable that is only assigned once.

'response' is never reassigned.

Safe fix: Use const instead.

(lint/style/useConst)

🔇 Additional comments (37)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelCommandValidator.cs (1)

1-3: LGTM: Appropriate namespace and import.

The use of FluentValidation and the well-structured namespace are appropriate for this validator class.

src/Digdir.Domain.Dialogporten.GraphQL/EndUser/MutationTypes/MappingProfile.cs (2)

1-2: LGTM: Imports are appropriate and concise.

The imports are relevant to the functionality being implemented. AutoMapper is used for the mapping profile, and the command import is necessary for the mapping configuration.


4-4: LGTM: Namespace is appropriate and consistent.

The namespace Digdir.Domain.Dialogporten.GraphQL.EndUser.MutationTypes accurately reflects the file's location and purpose within the project structure. It's consistent with the GraphQL and end-user related functionality mentioned in the PR objectives.

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogLabelAssignmentLog/Queries/Search/MappingProfile.cs (2)

5-5: LGTM: Namespace declaration is clear and well-structured.

The namespace accurately reflects the location and purpose of the class within the application structure, following a clear and organized naming convention.


7-8: LGTM: Class declaration follows best practices.

The MappingProfile class is well-defined:

  1. The name clearly indicates its purpose.
  2. It's sealed, which prevents inheritance and can improve performance.
  3. Inheriting from Profile is correct for creating an AutoMapper profile.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogSystemLabels/Set/SetDialogSystemLabelSwaggerConfig.cs (1)

1-6: LGTM: Namespace and class declaration.

The namespace follows the file path structure, and the class is appropriately declared as public and sealed. The class name is descriptive and follows the Pascal case convention.

tests/k6/common/testimports.js (3)

8-8: LGTM: Addition of postEU export aligns with PR objectives.

The inclusion of postEU in the exports is consistent with the PR's goal of adding post endpoints for end users to modify labels. This addition follows the existing naming convention and is appropriately positioned within the EU-related exports.


33-33: LGTM: Addition of setSystemLabel export supports new labeling feature.

The inclusion of setSystemLabel in the dialog-related exports aligns with the PR's objective of implementing a system label for hiding dialogs. This addition is consistent with the existing naming convention and is appropriately positioned within the dialog-related exports.


Line range hint 1-33: Consider adding automated tests for the new labeling feature.

While the additions to this file provide the necessary tools for testing the new labeling feature, the PR objectives mention that relevant automated tests have not been added. To ensure the reliability and correctness of the new feature, it would be beneficial to implement automated tests using these newly exported functions.

Would you like assistance in creating automated tests for the new labeling feature using postEU and setSystemLabel?

src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssigmentLogs/Search/SearchDialogLabelAssignmentSwaggerConfig.cs (2)

7-10: LGTM: Class declaration and OperationId property are well-implemented.

The class is correctly sealed and implements the ISwaggerConfig interface. The static OperationId property provides a consistent identifier for the operation, which is a good practice for Swagger configurations.


1-17: LGTM: Overall structure and consistency are good.

The file is well-structured and follows C# conventions. The namespace is consistent with the file path, and the class name matches the file name. The implementation effectively configures Swagger for the search endpoint related to dialog label assignments.

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelDto.cs (5)

6-10: LGTM: SetDialogSystemLabelDto implementation

The SetDialogSystemLabelDto class is well-structured and aligns with the PR objectives. It provides a clear way to associate a system label with a specific dialog.


12-13: Clarify the necessity of SetDialogSystemLabelLabelDto

There's a comment questioning if this class is still needed. Could you please clarify the purpose and necessity of this class in the context of the current implementation? If it's no longer required, consider removing it to keep the codebase clean.


20-35: LGTM: SetDialogSystemLabelLabelDto constructor

The constructor is well-implemented with proper input validation for both parameters. It correctly sets the Label, Namespace, and FullName properties.


64-67: LGTM: Parse method implementation

The Parse method is well-implemented. It correctly uses the TryParse method and throws an appropriate exception when parsing fails, following the common pattern for Parse methods in .NET.


1-68: Overall assessment: Good implementation with minor suggestions

The implementation of SetDialogSystemLabelDto and SetDialogSystemLabelLabelDto aligns well with the PR objectives of managing dialog visibility through labels. The code is generally well-structured and follows good practices.

Key points:

  1. The necessity of SetDialogSystemLabelLabelDto needs clarification.
  2. Consider simplifying the TryParse method for better readability.
  3. Add a comment explaining the commented-out code in the TryParse method.

These minor improvements will enhance the overall quality and maintainability of the code.

src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQuery.cs (3)

11-11: LGTM: Appropriate using statement added

The addition of the using Digdir.Domain.Dialogporten.Domain.DialogEndUserContexts.Entities statement is appropriate for the new SystemLabel feature implementation.


179-179: LGTM: Correct SystemLabel filtering condition implemented

The filtering condition for SystemLabel has been correctly implemented. It now applies the filter only when SystemLabel is provided (not null or empty), which addresses the logical error mentioned in the previous review.

.WhereIf(!request.SystemLabel.IsNullOrEmpty(), x => request.SystemLabel!.Contains(x.DialogEndUserContext.SystemLabelId))

This implementation ensures that dialogs are filtered based on the SystemLabelId only when the SystemLabel list is not null or empty.


Line range hint 1-197: Overall assessment: SystemLabel feature well-implemented

The changes in this file successfully implement the SystemLabel feature for filtering dialogs. The new property and filtering condition are correctly added, and the implementation addresses the issues mentioned in previous reviews. The code is clean, follows good practices, and aligns well with the PR objectives.

A minor suggestion was made to enhance the summary comment for the SystemLabel property, but otherwise, the implementation looks good and ready for merging.

docs/schema/V1/swagger.verified.json (13)

264-272: New property "systemLabel" added to CreateDialogCommand schema

The "systemLabel" property has been added to the CreateDialogCommand schema. This new property allows setting the system label of a dialog, which appears to be for migration purposes.

The implementation looks correct and consistent with other properties in the schema. However, consider the following suggestions:

  1. The description mentions "Migration purposes". It might be beneficial to provide more context or documentation about when and how this property should be used.
  2. Consider adding an example value to help API consumers understand the expected format.

2105-2112: New property "systemLabel" added to GetDialogDto schema

The "systemLabel" property has been added to the GetDialogDto schema, representing the current display state of a dialog.

The implementation is consistent with the CreateDialogCommand schema. However, consider the following suggestions:

  1. The description "Current display state." could be more informative. Consider expanding it to explain the purpose and possible values of this property.
  2. Add an example value to help API consumers understand the expected format and possible values.

2274-2281: New property "systemLabel" added to GetDialogDtoSO schema

The "systemLabel" property has been added to the GetDialogDtoSO schema, which is likely used for service owner operations.

The implementation is consistent with the GetDialogDto schema. However, consider the following suggestions:

  1. Ensure that the description and any additional documentation for this property are consistent across all schemas where it appears.
  2. If there are any differences in how this property should be used or interpreted by service owners, make sure to highlight those in the description or additional documentation.

3220-3227: New property "systemLabel" added to SearchDialogDto schema

The "systemLabel" property has been added to the SearchDialogDto schema, representing the current display state in search results.

The implementation is consistent with other schemas. However, consider the following suggestions:

  1. Ensure that the description and any additional documentation for this property are consistent across all schemas where it appears.
  2. If there are any specific search-related behaviors or considerations for this property, make sure to highlight those in the description or additional documentation.

3337-3344: New property "systemLabel" added to SearchDialogDtoSO schema

The "systemLabel" property has been added to the SearchDialogDtoSO schema, which is likely used for service owner search operations.

The implementation is consistent with the SearchDialogDto schema. However, consider the following suggestions:

  1. Ensure that the description and any additional documentation for this property are consistent across all schemas where it appears.
  2. If there are any differences in how this property should be used or interpreted by service owners in search operations, make sure to highlight those in the description or additional documentation.

3469-3482: New enum "SystemLabel_Values" added

A new enum "SystemLabel_Values" has been added to define the possible values for the systemLabel property.

The enum definition looks correct and includes three values: "Default", "Bin", and "Archive". However, consider the following suggestions:

  1. Add a description for each enum value to clarify their meanings and use cases.
  2. Consider adding a more detailed description for the enum itself, explaining its purpose and when it should be used.

3455-3468: New schema "SetDialogSystemLabelCommand" added

A new schema "SetDialogSystemLabelCommand" has been added, likely for setting the system label of a dialog.

The schema looks correct and includes properties for optimistic concurrency control ("ifMatchDialogRevision") and the new label. However, consider the following suggestions:

  1. Add a description for the schema itself, explaining its purpose and when it should be used.
  2. Consider adding examples or more detailed descriptions for each property to guide API consumers on their proper usage.

2579-2590: New schema "LabelAssignmentLogActorDto" added

A new schema "LabelAssignmentLogActorDto" has been added, likely for logging label assignment actions.

The schema looks correct and includes properties for actorId and actorName. However, consider the following suggestions:

  1. Add a description for the schema itself, explaining its purpose and when it should be used.
  2. Consider adding descriptions for each property to clarify their meanings and expected values.
  3. If there are any constraints or format requirements for actorId or actorName, make sure to document them.

3360-3378: New schema "SearchDialogLabelAssignmentLogDto" added

A new schema "SearchDialogLabelAssignmentLogDto" has been added, likely for searching and retrieving label assignment logs.

The schema looks correct and includes properties for action, createdAt, name, and performedBy. However, consider the following suggestions:

  1. Add a description for the schema itself, explaining its purpose and when it should be used.
  2. Consider adding descriptions for each property to clarify their meanings and expected values.
  3. For the "createdAt" property, specify the expected date-time format.
  4. For the "action" property, consider using an enum if there's a fixed set of possible actions.

4334-4347: New query parameter "systemLabel" added to GetDialogList endpoint

A new query parameter "systemLabel" has been added to the GetDialogList endpoint, allowing filtering by display state.

The implementation looks correct and consistent with other query parameters. However, consider the following suggestions:

  1. Expand the description to provide more context on how this filter works, especially if multiple values can be provided.
  2. If there are any limitations or special considerations when using this filter, make sure to document them in the description.

5157-5170: New query parameter "systemLabel" added to GetDialogListSO endpoint

A new query parameter "systemLabel" has been added to the GetDialogListSO endpoint, allowing filtering by display state for service owners.

The implementation is consistent with the GetDialogList endpoint. However, consider the following suggestions:

  1. Ensure that the description and any additional documentation for this parameter are consistent with the GetDialogList endpoint.
  2. If there are any differences in how this filter should be used or interpreted by service owners, make sure to highlight those in the description.

4583-4639: New endpoint "/api/v1/enduser/dialogs/{dialogId}/labellog" added

A new GET endpoint for retrieving the label assignment log for a dialog has been added.

The endpoint implementation looks correct and includes appropriate response codes. However, consider the following suggestions:

  1. Add a description for the endpoint, explaining its purpose and when it should be used.
  2. Consider adding more detailed descriptions for the possible response codes, especially for error cases.
  3. If there are any query parameters for filtering or pagination, make sure to document them.

4761-4850: New endpoint "/api/v1/enduser/dialogs/{dialogId}/systemlabels" added

A new PUT endpoint for setting the system label of a dialog has been added.

The endpoint implementation looks correct and includes appropriate request body and response codes. However, consider the following suggestions:

  1. Add a description for the endpoint, explaining its purpose and when it should be used.
  2. Consider adding more detailed descriptions for the possible response codes, especially for error cases.
  3. Provide more information about the expected format and possible values for the request body.
  4. If there are any specific authorization requirements or limitations for this operation, make sure to document them.
src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogLabelAssigmentLogs/Search/SearchDialogLabelAssigmentLogEndpoint.cs (3)

8-10: Typo in namespace and class name persists

The namespace and class name contain a typo: "Assigment" should be "Assignment". This issue was previously flagged and is still present. Please correct the spelling to ensure consistency throughout the codebase.


18-25: Endpoint configuration is correct

The Configure method properly sets up the endpoint route, authorization policies, group, and description. The use of Get("dialogs/{dialogId}/labellog") and the application of AuthorizationPolicy.EndUser are appropriate.


26-33: Consider adding a catch-all case for unexpected results

As previously suggested, consider adding a catch-all case to handle unexpected results in the Match expression. This ensures that any unforeseen result types are handled gracefully, improving the robustness of the endpoint.

src/Digdir.Domain.Dialogporten.WebApi/Endpoints/V1/EndUser/DialogSystemLabels/Set/SetDialogSystemLabelEndpoint.cs (1)

9-11: ⚠️ Potential issue

Fix invalid class constructor syntax

The class declaration uses invalid syntax. In C#, constructor parameters cannot be declared directly in class declarations unless it's a record type. Since SetDialogSystemLabelEndpoint is a class, you should define the constructor explicitly.

Apply this diff to correct the syntax:

-public sealed class SetDialogSystemLabelEndpoint(ISender sender) : Endpoint<SetDialogSystemLabelCommand>
-{
-    private readonly ISender _sender = sender ?? throw new ArgumentNullException(nameof(sender));
+public sealed class SetDialogSystemLabelEndpoint : Endpoint<SetDialogSystemLabelCommand>
+{
+    private readonly ISender _sender;
+
+    public SetDialogSystemLabelEndpoint(ISender sender)
+    {
+        _sender = sender ?? throw new ArgumentNullException(nameof(sender));
+    }

Likely invalid or redundant comment.

src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQuery.cs (1)

11-11: [Approved] Addition of necessary using directive

The added using Digdir.Domain.Dialogporten.Domain.DialogEndUserContexts.Entities; directive is appropriate as it enables the use of DialogEndUserContext entities later in the code.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 17

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 97f8277 and 0d4ad1c.

📒 Files selected for processing (8)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelCommand.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelDto.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (4 hunks)
  • src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (7 hunks)
  • src/Digdir.Domain.Dialogporten.Domain/DialogEndUserContexts/Entities/DialogEndUserContext.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241002134750_AddSystemLabel.Designer.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241002134750_AddSystemLabel.cs (1 hunks)
  • src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs (6 hunks)
🔇 Additional comments (20)
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/DialogSystemLabels/Commands/Set/SetDialogSystemLabelCommand.cs (2)

65-66: Verify the concurrency check is applied to the correct entity

Currently, the concurrency check is enabled on dialog.DialogEndUserContext using request.IfMatchDialogRevision. Confirm that IfMatchDialogRevision corresponds to the revision token of DialogEndUserContext. If the concurrency token is associated with dialog itself, consider applying the concurrency check to dialog instead.


61-61: ⚠️ Potential issue

Handle potential null currentUserInformation

The method GetCurrentUserInformation may return null. Accessing currentUserInformation.UserId without checking for null could lead to a NullReferenceException. Ensure that you check for null before using currentUserInformation.

Apply this diff to add a null check:

var currentUserInformation = await _userRegistry.GetCurrentUserInformation(cancellationToken);
+ if (currentUserInformation == null)
+ {
+     return new DomainError("User information could not be retrieved.");
+ }

Likely invalid or redundant comment.

src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Create/CreateDialogCommand.cs (3)

5-5: Addition of necessary using directives

The added using statements are appropriate and necessary for the new functionality.

Also applies to: 8-8, 11-11


35-35: Injection of IUser dependency into CreateDialogCommandHandler

Introducing the IUser dependency enables access to user-specific information required for creating the dialog's end-user context. The dependency is correctly injected and assigned.

Also applies to: 38-38, 46-46


72-72: Invocation of CreateDialogEndUserContext method

Calling the CreateDialogEndUserContext method integrates the end-user context creation into the dialog handling process appropriately.

src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241002134750_AddSystemLabel.cs (4)

52-73: Review nullable constraints and foreign keys in DialogEndUserContext

  • The DialogId column is nullable, but there's a foreign key constraint with onDelete: ReferentialAction.SetNull. Verify if this is the intended behavior.
  • Ensure that records in DialogEndUserContext are meaningful when DialogId is NULL.
  • The foreign key on SystemLabelId uses ReferentialAction.Restrict. Confirm if this aligns with the desired cascade behavior.

97-110: Assess the impact of updating existing DialogStatus data

Modifying the Name values from "Signing" to "Draft" and "Processing" to "Sent" could affect existing logic that relies on these statuses.

  • Ensure that all application logic and business processes are updated to reflect these changes.
  • Consider data migration strategies to handle any dependencies.

148-155: Verify nullable foreign key constraint on LabelAssignmentLogId

The Actor table has a nullable LabelAssignmentLogId, but the foreign key enforces onDelete: ReferentialAction.Cascade. Ensure that this is appropriate and doesn't lead to unintended deletions when a LabelAssignmentLog is removed.


170-217: Ensure Down migration properly reverses all changes

Review the Down method to confirm that it accurately and completely reverses all operations performed in the Up method, including:

  • Dropping all added columns and tables.
  • Reverting data updates made to the DialogStatus table.
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Commands/Update/UpdateDialogCommand.cs (2)

Line range hint 38-54: Proper Implementation of Dependency Injection

The addition of the IUser dependency and its assignment in the constructor follow best practices for dependency injection. The null check ensures robustness.


152-152: Ensure Errors from UpdateLabel Are Handled Before Saving

Ensure that any domain errors added within UpdateLabel are appropriately handled before proceeding to save changes.

src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/DialogDbContextModelSnapshot.cs (7)

195-232: Entity Definition for DialogEndUserContext Is Correct

The DialogEndUserContext entity is properly defined with all the necessary properties, primary key, indices, and table mapping. The use of HasDefaultValueSql for generating UUIDs and timestamps ensures consistent default values.


266-296: SystemLabel Entity and Data Seeding

The SystemLabel entity is correctly set up with required properties and primary key. The seeding of initial data with IDs and Names (Default, Bin, Archive) aligns with the requirements to manage dialog visibility through labeling.


1321-1333: Inheritance Implementation for LabelAssignmentLogActor

The LabelAssignmentLogActor entity correctly inherits from the Actor base class. The unique index on LabelAssignmentLogId ensures a one-to-one relationship, which is appropriate for tracking who performed the label assignment action.


1511-1527: Relationship Configuration for DialogEndUserContext

The relationships are properly configured:

  • The one-to-one relationship between DialogEndUserContext and DialogEntity uses a nullable foreign key with OnDelete(DeleteBehavior.SetNull), which is suitable if a dialog can exist without an associated end-user context.
  • The mandatory relationship with SystemLabel ensures that every DialogEndUserContext is associated with a SystemLabel, enforcing data integrity.

1529-1539: Cascade Delete Behavior for LabelAssignmentLog

LabelAssignmentLog has a required relationship with DialogEndUserContext with OnDelete(DeleteBehavior.Cascade). This setup implies that when a DialogEndUserContext is deleted, all associated LabelAssignmentLog entries will also be deleted, which is appropriate for maintaining data consistency.


1757-1767: Association Between LabelAssignmentLog and LabelAssignmentLogActor

The one-to-one relationship between LabelAssignmentLog and LabelAssignmentLogActor is correctly established with a cascade delete behavior. This ensures that when a LabelAssignmentLog is deleted, the associated actor record is also removed.


1896-1906: Navigation Properties for DialogEndUserContext and LabelAssignmentLog

The navigation properties are appropriately configured:

  • DialogEndUserContext includes a collection of LabelAssignmentLogs, allowing access to all label assignments related to the context.
  • LabelAssignmentLog has a required navigation to PerformedBy, ensuring that every label assignment action is associated with an actor.
src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241002134750_AddSystemLabel.Designer.cs (2)

1515-1530: Confirm OnDelete Behavior for Foreign Keys

The OnDelete(DeleteBehavior.SetNull) on the DialogId foreign key means that when a Dialog is deleted, the DialogId in DialogEndUserContext will be set to null. Similarly, OnDelete(DeleteBehavior.Restrict) on SystemLabelId prevents deletion of a SystemLabel if it's referenced.

Ensure this behavior aligns with the domain requirements. If orphaned DialogEndUserContext records are undesirable, consider changing the delete behavior.

// Change DeleteBehavior if needed:
- .OnDelete(DeleteBehavior.SetNull);
+ .OnDelete(DeleteBehavior.Cascade);

283-299: Verify Data Seeding for SystemLabel

The migration seeds the SystemLabel table with IDs 1, 2, and 3 for "Default", "Bin", and "Archive". Ensure that these IDs do not conflict with existing data, especially in databases that may already have entries with these IDs.

Run the following script to check for existing IDs:

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE

📥 Commits

Files that changed from the base of the PR and between 0d4ad1c and 26d4ede.

📒 Files selected for processing (1)
  • src/Digdir.Domain.Dialogporten.Infrastructure/Persistence/Migrations/20241002134750_AddSystemLabel.cs (1 hunks)

@Fargekritt Fargekritt force-pushed the feature/basic-label-implementation branch from 26d4ede to 51213a7 Compare October 3, 2024 08:07
MagnusSandgren
MagnusSandgren previously approved these changes Oct 3, 2024
Copy link

sonarqubecloud bot commented Oct 3, 2024

@Fargekritt Fargekritt merged commit ee90c68 into main Oct 3, 2024
21 checks passed
@Fargekritt Fargekritt deleted the feature/basic-label-implementation branch October 3, 2024 08:39
oskogstad pushed a commit that referenced this pull request Oct 4, 2024
🤖 I have created a release *beep* *boop*
---


##
[1.21.0](v1.20.2...v1.21.0)
(2024-10-03)


### Features

* basic label implementation to hide dialogs
([#1192](#1192))
([ee90c68](ee90c68))


### Bug Fixes

* **webAPI:** Broken mapping when creating Transmissions in an Update
([#1221](#1221))
([6e7dfe4](6e7dfe4))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
@coderabbitai coderabbitai bot mentioned this pull request Oct 7, 2024
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants