feat: Add new endpoint to update task navigation #15062
Conversation
📝 WalkthroughWalkthroughThis pull request introduces functionality to update task navigation groups by adding a new Changes
Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 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
CodeRabbit Configuration File (
|
mlqn
changed the title
github-actions
bot
added
solution/studio/designer
mlqn
force-pushed
the
14929-be-add-new-task-to-task-navigation
branch
from
5f40fa5 to
a0adca6
Compare
mlqn
force-pushed
the
14929-be-add-new-task-to-task-navigation
branch
from
a0adca6 to
0b9f065
Compare
github-actions
bot
added
the
quality/testing
…-to-task-navigation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #15062 +/- ##
=======================================
Coverage 95.96% 95.96%
=======================================
Files 2036 2036
Lines 26138 26138
Branches 2948 2948
=======================================
Hits 25083 25083
Misses 790 790
Partials 265 265 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
mlqn
changed the title
mlqn
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
backend/src/Designer/Helpers/Extensions/EnumExtensions.cs (1)
9-27: 💡 Verification agent🧩 Analysis chain
Method renamed and behavior changed to throw exception.
The method has been renamed from
GetJsonStringEnumMemberNametoToStringValueand now throws an exception instead of returning null when the attribute is not found. This change in behavior could break existing code that expects null to be returned.
🏁 Script executed:
#!/bin/bash # Check for usages of the old method name that might need to be updated echo "Checking for usages of GetJsonStringEnumMemberName that need to be updated..." rg "GetJsonStringEnumMemberName" --type csharp # Check for places that might be expecting null returns (catching Exception might indicate handling of the new exception) echo "Checking for potential places expecting null returns from the renamed method..." rg -A 3 -B 3 "\.ToStringValue\(\)" --type csharpLength of output: 1855
Critical Behavior Change Detected in Enum Extension
- The method has been renamed from
GetJsonStringEnumMemberNametoToStringValueand now throws an exception if theJsonStringEnumMemberNameAttributeis missing.- Verification shows no remaining usages of the old name; however, in backend/src/Designer/Mappers/TaskNavigationGroupMapper.cs, the method is invoked directly as
receipt.Type.ToStringValue()without any exception handling.- This change may lead to runtime exceptions in parts of the code that expect a
nullreturn value when the attribute is not present.Please review the affected usages and add appropriate null checks or exception handling to safeguard against potential breakage.
🧹 Nitpick comments (3)
backend/src/Designer/Helpers/Extensions/EnumExtensions.cs (1)
29-41: New ToEnumValue method could be optimized.The method correctly converts string to enum values but iterates through all enum values for each conversion. For better performance, consider caching the mappings in a static dictionary.
+ private static readonly Dictionary<Type, Dictionary<string, object>> _enumCache = new(); public static TEnum ToEnumValue<TEnum>(this string name) where TEnum : struct, Enum { + Type enumType = typeof(TEnum); + + // Initialize cache for this enum type if needed + if (!_enumCache.TryGetValue(enumType, out var nameToValueMap)) + { + nameToValueMap = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase); + foreach (TEnum value in Enum.GetValues<TEnum>()) + { + try + { + string jsonName = value.ToStringValue(); + nameToValueMap[jsonName] = value; + } + catch (ArgumentException) + { + // Skip values without the attribute + } + } + _enumCache[enumType] = nameToValueMap; + } + + // Look up the value in the cache + if (nameToValueMap.TryGetValue(name, out var enumValue)) + { + return (TEnum)enumValue; + } - foreach (TEnum value in Enum.GetValues<TEnum>().Cast<TEnum>()) - { - string jsonName = value.ToStringValue(); - if (string.Equals(jsonName, name, StringComparison.OrdinalIgnoreCase)) - { - return value; - } - } throw new ArgumentException($"'{name}' is not a valid {typeof(TEnum).Name} JSON enum name."); }backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs (2)
60-91: Test for invalid data update could be more comprehensive.While the test correctly verifies that invalid data returns a BadRequest response, it could be enhanced by:
- Testing multiple invalid data scenarios
- Asserting that the error response contains meaningful information
- Verifying proper validation handling
[Theory] [InlineData("ttd", "app-with-groups-and-taskNavigation", "testUser")] public async Task UpdateTaskNavigation_WhenInvalidData_ReturnsBadRequest(string org, string app, string developer) { string targetRepository = TestDataHelper.GenerateTestRepoName(); await CopyRepositoryForTest(org, app, developer, targetRepository); string url = VersionPrefix(org, targetRepository); IEnumerable<TaskNavigationGroupDto> taskNavigationGroupDtoList = [new () { TaskType = "test", }]; using var httpRequestMessage = new HttpRequestMessage(HttpMethod.Post, url) { Content = new StringContent(JsonSerializer.Serialize(taskNavigationGroupDtoList), Encoding.UTF8, MediaTypeNames.Application.Json) }; using var response = await HttpClient.SendAsync(httpRequestMessage); Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + + // Verify error response content + var responseContent = await response.Content.ReadAsStringAsync(); + Assert.Contains("Unknown", responseContent); string relativePath = "App/ui/layout-sets.json"; string expectedFile = TestDataHelper.GetFileFromRepo(org, app, developer, relativePath); JsonNode expectedData = JsonNode.Parse(expectedFile)["uiSettings"]["taskNavigation"]; string savedFile = TestDataHelper.GetFileFromRepo(org, targetRepository, developer, relativePath); JsonNode savedData = JsonNode.Parse(savedFile)["uiSettings"]["taskNavigation"]; Assert.True(JsonUtils.DeepEquals(expectedData.ToJsonString(), savedData.ToJsonString())); } +[Theory] +[InlineData("ttd", "app-with-groups-and-taskNavigation", "testUser")] +public async Task UpdateTaskNavigation_WhenEmptyData_ReturnsBadRequest(string org, string app, string developer) +{ + string targetRepository = TestDataHelper.GenerateTestRepoName(); + await CopyRepositoryForTest(org, app, developer, targetRepository); + + string url = VersionPrefix(org, targetRepository); + + // Empty task navigation list + IEnumerable<TaskNavigationGroupDto> taskNavigationGroupDtoList = []; + + using var httpRequestMessage = new HttpRequestMessage(HttpMethod.Post, url) + { + Content = new StringContent(JsonSerializer.Serialize(taskNavigationGroupDtoList), Encoding.UTF8, MediaTypeNames.Application.Json) + }; + + using var response = await HttpClient.SendAsync(httpRequestMessage); + Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + + // Rest of verification code +}
20-20: Consider adding tests for additional edge cases.The current tests cover the basic happy and error paths, but they could be enhanced with tests for additional scenarios such as:
- Cancellation token being triggered
- Concurrent updates
- Malformed JSON input
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/src/Designer/Controllers/TaskNavigationController.cs(3 hunks)backend/src/Designer/Helpers/Extensions/EnumExtensions.cs(2 hunks)backend/src/Designer/Mappers/TaskNavigationGroupMapper.cs(1 hunks)backend/src/Designer/Services/Implementation/TaskNavigationService.cs(1 hunks)backend/src/Designer/Services/Interfaces/ITaskNavigationService.cs(2 hunks)backend/tests/Designer.Tests/Controllers/TaskNavigationController/GetTaskNavigationTests.cs(1 hunks)backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
backend/src/Designer/Mappers/TaskNavigationGroupMapper.cs (3)
backend/src/Designer/Helpers/Extensions/EnumExtensions.cs (1)
ToStringValue(9-27)backend/src/Designer/Converters/TaskNavigationGroupJsonConverter.cs (1)
TaskNavigationGroup(10-26)backend/src/Designer/Models/LayoutSets.cs (2)
TaskNavigationTask(60-64)TaskNavigationReceipt(66-70)
backend/tests/Designer.Tests/Controllers/TaskNavigationController/GetTaskNavigationTests.cs (3)
backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs (3)
VersionPrefix(22-22)Theory(24-59)Theory(60-91)backend/src/Designer/Services/Implementation/TaskNavigationService.cs (2)
Task(16-25)Task(36-47)backend/src/Designer/Models/Dto/TaskNavigationGroupDto.cs (1)
TaskNavigationGroupDto(5-18)
backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs (5)
backend/tests/Designer.Tests/Controllers/TaskNavigationController/GetTaskNavigationTests.cs (1)
VersionPrefix(16-16)backend/src/Designer/Services/Interfaces/ITaskNavigationService.cs (3)
Task(17-17)Task(33-33)IEnumerable(25-25)backend/src/Designer/Services/Implementation/TaskNavigationService.cs (3)
Task(16-25)Task(36-47)IEnumerable(27-34)backend/tests/Designer.Tests/Utils/TestDataHelper.cs (2)
TestDataHelper(19-470)GetFileFromRepo(317-326)backend/src/Designer/Mappers/TaskNavigationGroupMapper.cs (1)
TaskNavigationGroupDto(10-27)
🔇 Additional comments (11)
backend/src/Designer/Mappers/TaskNavigationGroupMapper.cs (1)
22-22: Method call updated for consistency.The call has been updated to use
ToStringValue()instead of what was likelyGetJsonStringEnumMemberName()before, aligning with the renamed method inEnumExtensions.cs.backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs (1)
24-59: Test for valid data update looks good.The test properly checks the happy path scenario for updating task navigation with valid data. It verifies both the HTTP response and the actual data saved to the repository.
backend/src/Designer/Services/Interfaces/ITaskNavigationService.cs (2)
16-16: LGTM: Documentation improvement for consistency.Adding a period at the end of the return documentation is a small but meaningful improvement for documentation consistency.
27-33: LGTM: Well-defined interface method for updating task navigation.The new method signature is clear, follows the same pattern as the existing methods, and aligns with the PR's objective of adding an endpoint to update task navigation.
One minor observation: unlike
GetTaskNavigation, this method doesn't provide a default value for thecancellationTokenparameter. While this is a minor inconsistency, it won't affect functionality.backend/tests/Designer.Tests/Controllers/TaskNavigationController/GetTaskNavigationTests.cs (4)
12-14: LGTM: Nice modernization using C# primary constructor syntax.The refactoring to use primary constructor syntax makes the code more concise while maintaining the same functionality.
16-16: LGTM: Well-structured helper method.This helper method provides a clean way to construct the API endpoint URL and is consistently used across the tests.
18-50: LGTM: Comprehensive test for the existing task navigation retrieval.The test correctly verifies that when task navigation exists, the API returns the expected task navigation array with a 200 OK status code.
52-68: LGTM: Good negative test case for empty task navigation.This test appropriately verifies the behavior when no task navigation exists, ensuring the API returns an empty array with a 200 OK status code.
backend/src/Designer/Controllers/TaskNavigationController.cs (3)
1-1: LGTM: Necessary import for exception handling.The addition of the System namespace import is required for using ArgumentException in the new method.
32-32: LGTM: Documentation consistency improvement.Adding a period at the end of the return documentation is consistent with the interface update.
47-71:✅ Verification successful
LGTM: Well-implemented endpoint for updating task navigation.
The new endpoint is well-structured and follows established patterns:
- Proper HTTP method (POST) for update operations
- Consistent parameter naming with other endpoints
- Appropriate error handling for invalid input
- Returns 204 No Content on success (standard for updates without returning data)
I do have a couple of observations:
The error handling only catches ArgumentException. Consider if there are other exceptions that might need to be handled specifically (like IOException for file operations).
There's no explicit validation of the input taskNavigationGroupDtoList in the controller method. While validation might be happening in the service layer, consider if additional validation might be needed at the controller level.
🏁 Script executed:
#!/bin/bash # Check the implementation of UpdateTaskNavigation in the service layer to understand how validation is performed rg -A 10 "UpdateTaskNavigation" --glob "*.cs" | grep -v "interface"Length of output: 8101
LGTM – The endpoint is well-implemented overall.
The updated controller demonstrates a clean design with proper HTTP semantics, consistent naming, and clear segregation of concerns. In reviewing the underlying service implementation and tests, I have the following suggestions to consider:
Error Handling:
Currently, the controller only catches anArgumentException. Since the service layer performs file operations (e.g., fetching and saving layout sets), consider whether additional exception types (for example,IOException) should be caught or whether a higher-level global exception handler already covers these scenarios.Input Validation:
The validation appears to be handled downstream (possibly during the DTO-to-domain conversion or within the service layer itself). You might want to double-check if any supplemental validation of thetaskNavigationGroupDtoListat the controller level would provide earlier feedback or improve robustness.These points are optional refinements and don't impede the current functionality as confirmed by the tests. Please verify that these considerations align with your broader error handling and validation strategy across controllers.
backend/src/Designer/Services/Implementation/TaskNavigationService.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/src/Designer/Services/Implementation/TaskNavigationService.cs (1)
37-55:⚠️ Potential issueNew method to update task navigation lacks exception handling.
The method correctly implements the update functionality but doesn't include try-catch blocks for handling potential exceptions from repository operations or null parameter checks. This could lead to unhandled exceptions when saving to the repository fails.
Apply this diff to add proper parameter validation and exception handling:
public async Task UpdateTaskNavigation(AltinnRepoEditingContext altinnRepoEditingContext, IEnumerable<TaskNavigationGroup> taskNavigationGroupList, CancellationToken cancellationToken) { cancellationToken.ThrowIfCancellationRequested(); + if (altinnRepoEditingContext == null) + { + throw new ArgumentNullException(nameof(altinnRepoEditingContext)); + } + if (taskNavigationGroupList == null) + { + throw new ArgumentNullException(nameof(taskNavigationGroupList)); + } + AltinnAppGitRepository altinnAppGitRepository = altinnGitRepositoryFactory.GetAltinnAppGitRepository(altinnRepoEditingContext.Org, altinnRepoEditingContext.Repo, altinnRepoEditingContext.Developer); LayoutSets layoutSets = await altinnAppGitRepository.GetLayoutSetsFile(cancellationToken); if (taskNavigationGroupList.Any()) { layoutSets.UiSettings ??= new(); layoutSets.UiSettings.TaskNavigation = taskNavigationGroupList; } else if (layoutSets.UiSettings != null) { layoutSets.UiSettings.TaskNavigation = null; } + try + { await altinnAppGitRepository.SaveLayoutSets(layoutSets); + } + catch (Exception ex) + { + // Log the exception or wrap it in a more specific exception type + throw new InvalidOperationException($"Failed to save layout sets: {ex.Message}", ex); + } }
🧹 Nitpick comments (1)
backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs (1)
86-117: Comprehensive error handling test.This test properly validates the error case by:
- Sending an invalid payload (only TaskType without Name)
- Verifying the API returns BadRequest
- Confirming the original data remains unchanged
Consider adding an assertion to verify the error response message contains helpful details about what validation failed.
using var response = await HttpClient.SendAsync(httpRequestMessage); Assert.Equal(HttpStatusCode.BadRequest, response.StatusCode); + // Verify error response contains helpful validation details + string responseContent = await response.Content.ReadAsStringAsync(); + Assert.Contains("validation failed", responseContent, StringComparison.OrdinalIgnoreCase);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/src/Designer/Services/Implementation/TaskNavigationService.cs(2 hunks)backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs (5)
backend/tests/Designer.Tests/Controllers/TaskNavigationController/GetTaskNavigationTests.cs (1)
VersionPrefix(16-16)backend/src/Designer/Services/Implementation/TaskNavigationService.cs (3)
Task(16-25)Task(36-47)IEnumerable(27-34)backend/src/Designer/Services/Interfaces/ITaskNavigationService.cs (3)
Task(17-17)Task(33-33)IEnumerable(25-25)backend/tests/Designer.Tests/Utils/TestDataHelper.cs (2)
TestDataHelper(19-470)GetFileFromRepo(317-326)backend/src/Designer/Mappers/TaskNavigationGroupMapper.cs (1)
TaskNavigationGroupDto(10-27)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run integration tests against actual gitea and dbs
- GitHub Check: Analyze
🔇 Additional comments (5)
backend/src/Designer/Services/Implementation/TaskNavigationService.cs (2)
2-2: Added LINQ dependency for the new method.This import is correctly added to support the
.Any()method used in the newUpdateTaskNavigationmethod.
44-52: Good handling of empty task navigation lists.The conditional logic correctly handles both cases:
- When the list has items, it ensures UiSettings is initialized
- When the list is empty, it sets TaskNavigation to null (but only if UiSettings exists)
This approach properly maintains the data structure integrity.
backend/tests/Designer.Tests/Controllers/TaskNavigationController/UpdateTaskNavigationGroupTests.cs (3)
20-22: Test class is well-structured.The class follows best practices by inheriting from the appropriate base class and using IClassFixture. The helper method for generating the URL prefix is consistent with other similar test files.
24-59: Well-designed "happy path" test.This test thoroughly validates the successful case by:
- Setting up test data with a copy of the repository
- Creating a valid payload with both task and receipt types
- Verifying the response status code is NoContent
- Confirming the data was correctly saved to the repository
Good use of deep equality comparison to validate the saved JSON structure matches expectations.
61-84: Good edge case test for empty lists.This test properly verifies that:
- An empty list payload is handled correctly
- The API returns NoContent
- The taskNavigation property is correctly set to null in the saved file
Testing this edge case ensures the API handles clearing navigation groups properly.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Description
Related Issue(s)
Verification
Summary by CodeRabbit
New Features
Bug Fixes