fix: Disallow search filter with CreatedAfter greater than CreatedBefore #2019
Conversation
📝 WalkthroughWalkthroughThis pull request introduces a new internal static class Changes
Possibly related PRs
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 0
🧹 Nitpick comments (4)
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/DueAtFilterTests.cs (1)
57-70: Avoid using hardcoded years in testsThe test uses absolute years (2021, 2022) which could make the test brittle over time since these years are already in the past. Consider using relative years based on the current date, similar to how you're doing in the
DueAtTestDatamethod.- var response = await Application.Send(new SearchDialogQuery - { - DueAfter = CreateDateFromYear(2022), - DueBefore = CreateDateFromYear(2021) - }); + var currentYear = DateTimeOffset.UtcNow.Year; + var response = await Application.Send(new SearchDialogQuery + { + DueAfter = CreateDateFromYear(currentYear + 1), + DueBefore = CreateDateFromYear(currentYear) + });tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/VisibleFromFilterTests.cs (2)
14-27: Avoid using hardcoded years in testsSimilar to the DueAtFilterTests, this test uses absolute years (2021, 2022) which could make the test brittle over time. Consider using relative years based on the current date.
- var response = await Application.Send(new SearchDialogQuery - { - VisibleAfter = CreateDateFromYear(2022), - VisibleBefore = CreateDateFromYear(2021) - }); + var currentYear = DateTimeOffset.UtcNow.Year; + var response = await Application.Send(new SearchDialogQuery + { + VisibleAfter = CreateDateFromYear(currentYear + 1), + VisibleBefore = CreateDateFromYear(currentYear) + });
14-16: Inconsistent method namingThe method name has an underscore between "VisibleFrom" and "After" which is inconsistent with the property name in the query (
VisibleAfter). This could lead to confusion.- public async Task Cannot_Filter_On_VisibleFrom_After_With_Value_Greater_Than_VisibleFrom_Before() + public async Task Cannot_Filter_On_VisibleAfter_With_Value_Greater_Than_VisibleBefore()tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/Common.cs (1)
30-52: Consider extracting date constraint logicThe method has embedded knowledge about date constraints (e.g., "Requires CreatedAt to be earlier than UpdatedAt"). If these constraints change, you'll need to update the method. Consider extracting these constraints to make them more explicit and maintainable.
internal static async Task<Guid> CreateDialogWithDateInYear(this DialogApplication application, int year, string dateType) { var date = CreateDateFromYear(year); + var createDialogCommand = CreateDialogCommandWithDate(date, dateType, year); - var createDialogCommand = dateType switch - { - UpdatedAt => DialogGenerator.GenerateFakeCreateDialogCommand( - // Requires CreatedAt to be earlier than UpdatedAt - createdAt: CreateDateFromYear(year - 1), updatedAt: date), - - VisibleFrom => DialogGenerator.GenerateFakeCreateDialogCommand( - // Requires DueAt to be later than VisibleFrom - dueAt: CreateDateFromYear(year + 1), visibleFrom: date), - - DueAt => DialogGenerator.GenerateFakeCreateDialogCommand(dueAt: date), - CreatedAt => DialogGenerator.GenerateFakeCreateDialogCommand(createdAt: date), - _ => throw new ArgumentException("Invalid date type", nameof(dateType)) - }; createDialogCommand.Dto.Party = Party; var createCommandResponse = await application.Send(createDialogCommand); return createCommandResponse.AsT0.DialogId; } + private static DialogGenerator.CreateDialogCommand CreateDialogCommandWithDate(DateTimeOffset date, string dateType, int year) + { + return dateType switch + { + UpdatedAt => DialogGenerator.GenerateFakeCreateDialogCommand( + // Requires CreatedAt to be earlier than UpdatedAt + createdAt: CreateDateFromYear(year - 1), updatedAt: date), + + VisibleFrom => DialogGenerator.GenerateFakeCreateDialogCommand( + // Requires DueAt to be later than VisibleFrom + dueAt: CreateDateFromYear(year + 1), visibleFrom: date), + + DueAt => DialogGenerator.GenerateFakeCreateDialogCommand(dueAt: date), + CreatedAt => DialogGenerator.GenerateFakeCreateDialogCommand(createdAt: date), + _ => throw new ArgumentException("Invalid date type", nameof(dateType)) + }; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/ValidationErrorStrings.cs(1 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQueryValidator.cs(2 hunks)src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQueryValidator.cs(2 hunks)src/Digdir.Tool.Dialogporten.GenerateFakeData/DialogGenerator.cs(6 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/Common.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/Search/CreatedAtFilterTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/Search/DueAtFilterTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/Search/UpdatedAtFilterTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/CreatedAtFilterTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/DueAtFilterTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/UpdatedAtFilterTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/VisibleFromFilterTests.cs(1 hunks)tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/SearchDialogTests.cs(0 hunks)
💤 Files with no reviewable changes (1)
- tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/SearchDialogTests.cs
✅ Files skipped from review due to trivial changes (1)
- src/Digdir.Domain.Dialogporten.Application/Features/V1/Common/ValidationErrorStrings.cs
🔇 Additional comments (26)
src/Digdir.Domain.Dialogporten.Application/Features/V1/ServiceOwner/Dialogs/Queries/Search/SearchDialogQueryValidator.cs (3)
10-10: Good addition of the static import.Using the static import provides cleaner code access to the validation error strings constant.
63-67: Well-implemented validation rule for CreatedAfter/CreatedBefore dates.The validation correctly ensures that CreatedAfter is less than or equal to CreatedBefore, and only applies when both dates are provided. The error message is appropriately set using the constant from ValidationErrorStrings.
68-81: Consistent implementation of date range validations.The validation rules for DueAfter, UpdatedAfter, and VisibleAfter follow the same pattern as CreatedAfter, ensuring consistency across all date filters. This is a good practice that will make the code easier to maintain.
src/Digdir.Domain.Dialogporten.Application/Features/V1/EndUser/Dialogs/Queries/Search/SearchDialogQueryValidator.cs (2)
8-8: Good addition of the static import.Using the static import provides cleaner code access to the validation error strings constant.
57-70: Well-implemented date range validations for EndUser queries.The validation rules for CreatedAfter, DueAfter, and UpdatedAfter follow the same pattern as in the ServiceOwner validator, maintaining consistency across the application. Note that VisibleAfter validation is not included here as it's only applicable for ServiceOwner, which is correct based on the PR objectives.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/UpdatedAtFilterTests.cs (2)
14-51: Well-structured parameterized test for UpdatedAt filtering.The test covers multiple scenarios with different date combinations using InlineData, which is a good approach for testing various filtering conditions. The test correctly verifies both the count of returned items and their specific IDs.
53-66: Good validation test for invalid date ranges.The test correctly verifies that providing an UpdatedAfter date that is greater than UpdatedBefore results in a validation error. This ensures the validation rule works as expected.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/CreatedAtFilterTests.cs (2)
14-51: Well-structured parameterized test for CreatedAt filtering.Similar to the UpdatedAt tests, this test effectively covers multiple scenarios using InlineData and verifies both the count and content of the results. The structure is consistent with other filter tests, which is good for maintainability.
53-66: Good validation test for invalid date ranges.The test properly verifies that providing a CreatedAfter date that is greater than CreatedBefore results in a validation error, ensuring the validation rule works as expected.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/Search/CreatedAtFilterTests.cs (1)
1-68: Well-structured test implementation.The test class is well organized and thoroughly tests the filtering functionality based on created dates. You've implemented both positive tests (using parameterized data) and negative tests to verify validation constraints. The test cases cover essential scenarios: filtering after a date, before a date, and within a date range.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/Search/UpdatedAtFilterTests.cs (1)
1-68: LGTM - Clear and consistent test implementation.This test class follows the same pattern as CreatedAtFilterTests, providing good coverage for filtering based on updated dates. The consistent structure across test files makes the code easier to understand and maintain.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/EndUser/Dialogs/Queries/Search/DueAtFilterTests.cs (3)
13-55: Good approach using relative future dates.Using current year-based offsets for due dates ensures that the tests remain valid regardless of when they're run. This is particularly appropriate for due dates which are typically in the future.
57-71: Validation test for DueAfter > DueBefore looks good.This test effectively verifies that validation prevents setting DueAfter to a date greater than DueBefore, which aligns well with the PR objective.
73-113: Well-documented test data provider.The comments explaining the test data generation are helpful for future maintenance. The test data covers the essential scenarios: filtering after a specific year, before a specific year, and within a year range.
src/Digdir.Tool.Dialogporten.GenerateFakeData/DialogGenerator.cs (6)
38-38: Good enhancement to support visibility date testing.Adding the visibleFrom parameter to the method signature allows for more comprehensive testing of the visibility date filtering functionality.
66-66: Parameter correctly passed through to inner method.The visibleFrom parameter is correctly passed through to the GenerateFakeDialogs method.
91-91: Consistent parameter addition.The visibleFrom parameter is consistently added to all relevant method signatures.
117-117: Parameter correctly passed through.The visibleFrom parameter is correctly passed through from GenerateFakeDialog to GenerateFakeDialogs.
144-144: Parameter added to base method signature.The visibleFrom parameter has been added to the main generating method.
167-167: Property correctly assigned in the rule.The visibleFrom parameter is correctly assigned to the VisibleFrom property using the RuleFor method.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/DueAtFilterTests.cs (2)
14-55: Test implementation looks goodThe parameterized test is well-structured with clear arrange, act, and assert sections. Good use of data-driven testing to cover multiple scenarios.
72-112: Well-structured test data methodThe test data implementation with clear comments explaining the year calculations is excellent for maintainability. The approach of using relative years ensures tests remain valid regardless of when they run.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Features/V1/ServiceOwner/Dialogs/Queries/Search/VisibleFromFilterTests.cs (2)
29-70: Well-structured parameterized testThe test is well-implemented with clear arrange, act, and assert sections. Good use of the theory attribute with data provider method.
72-112: Good test data implementationClear test data method with good explanatory comments. The approach of using relative years ensures the tests remain valid regardless of when they run.
tests/Digdir.Domain.Dialogporten.Application.Integration.Tests/Common/Common.cs (2)
7-13: Well-designed test data classThe
DateFilterTestDataclass is well-structured with clear properties and appropriate use of nullable types for optional values. Therequiredmodifier onExpectedYearsensures this important property is always provided.
15-26: Good use of constants for string valuesUsing constants for string values like
UpdatedAt,VisibleFrom, etc. helps prevent typos and makes refactoring easier.
oskogstad
changed the title
Description
This PR disallows search filtering with
{dateProperty}After > {dateProperty}Beforefor all available date filter types,
Also adding tests to verify these filters work with normal use, and that validation errors are produced when the rule above is broken.
Related Issue(s)
Verification
Documentation
docs-directory, Altinnpedia or a separate linked PR in altinn-studio-docs., if applicable)