fix: accept no options-folder when fetching usage #14409
Conversation
standeren
added
skip-releasenotes
📝 WalkthroughWalkthroughThe pull request introduces changes to the error handling and repository interaction in the Altinn App Development system. The modifications primarily focus on the Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
solution/studio/designer
![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 (2)
backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (1)
801-801: Consider documenting the behavior changeThe method now returns an empty array instead of throwing a
NotFoundExceptionwhen the options folder doesn't exist. This is a breaking change that should be documented in the method's XML documentation to inform callers of the new behavior./// <summary> /// Gets a list of file names from the Options folder representing the available options lists. /// </summary> + /// <remarks> + /// If the options folder doesn't exist, returns an empty array instead of throwing an exception. + /// </remarks> /// <returns>A list of option list names.</returns> public string[] GetOptionsListIds()backend/tests/Designer.Tests/Controllers/OptionsController/GetOptionListsReferencesTests.cs (1)
92-111: LGTM! Well-structured test for the new behaviorThe test effectively validates the API's behavior when no options are present. Consider extracting the JSON layout string to a constant for better readability.
+ private const string EmptyLayout = @"{ ""data"": {""layout"": []}}"; + [Fact] public async Task GetOptionListsReferences_Returns200Ok_WithEmptyOptionsReferences_WhenAppDoesNotHaveOptions() { string targetRepository = TestDataHelper.GenerateTestRepoName(); await CopyRepositoryForTest("ttd", RepoWithoutOptions, "testUser", targetRepository); string repoPath = TestDataHelper.GetTestDataRepositoryDirectory("ttd", targetRepository, "testUser"); - string exampleLayout = @"{ ""data"": {""layout"": []}}"; string filePath = Path.Combine(repoPath, "App/ui/form/layouts"); Directory.CreateDirectory(filePath); - await File.WriteAllTextAsync(Path.Combine(filePath, "exampleLayout.json"), exampleLayout); + await File.WriteAllTextAsync(Path.Combine(filePath, "exampleLayout.json"), EmptyLayout);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/src/Designer/Controllers/AppDevelopmentController.cs(1 hunks)backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs(1 hunks)backend/tests/Designer.Tests/Controllers/OptionsController/GetOptionListsReferencesTests.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Analyze
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Run integration tests against actual gitea and db
🔇 Additional comments (1)
backend/src/Designer/Controllers/AppDevelopmentController.cs (1)
550-557: LGTM! Clean implementation of the empty options caseThe implementation correctly handles the case where no options are available by returning a 204 No Content response.
standeren
force-pushed
the
500-from-usage-when-app-lacks-option-lists
branch
from
6b734e8 to
8df70a0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/tests/Designer.Tests/Controllers/OptionsController/GetOptionListsReferencesTests.cs (1)
92-111: Good test coverage for the empty options scenario.The test effectively validates the behavior when an app has no options, which directly addresses the PR objective.
Consider these improvements:
- Add cleanup code to remove the test repository after the test completes
- Move the hardcoded layout JSON to a constant or test data file
- Add a test case for invalid repository scenario
Example cleanup implementation:
public async Task GetOptionListsReferences_Returns200Ok_WithEmptyOptionsReferences_WhenAppDoesNotHaveOptions() { string targetRepository = TestDataHelper.GenerateTestRepoName(); + try + { await CopyRepositoryForTest("ttd", RepoWithoutOptions, "testUser", targetRepository); string repoPath = TestDataHelper.GetTestDataRepositoryDirectory("ttd", targetRepository, "testUser"); string exampleLayout = @"{ ""data"": {""layout"": []}}"; string filePath = Path.Combine(repoPath, "App/ui/form/layouts"); Directory.CreateDirectory(filePath); await File.WriteAllTextAsync(Path.Combine(filePath, "exampleLayout.json"), exampleLayout); string apiUrl = $"/designer/api/ttd/{targetRepository}/options/usage"; using HttpRequestMessage httpRequestMessage = new(HttpMethod.Get, apiUrl); using HttpResponseMessage response = await HttpClient.SendAsync(httpRequestMessage); string responseBody = await response.Content.ReadAsStringAsync(); Assert.Equal(StatusCodes.Status200OK, (int)response.StatusCode); Assert.Equivalent("[]", responseBody); + } + finally + { + // Clean up the test repository + string repoPath = TestDataHelper.GetTestDataRepositoryDirectory("ttd", targetRepository, "testUser"); + if (Directory.Exists(repoPath)) + { + Directory.Delete(repoPath, recursive: true); + } + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/src/Designer/Controllers/AppDevelopmentController.cs(1 hunks)backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs(1 hunks)backend/tests/Designer.Tests/Controllers/OptionsController/GetOptionListsReferencesTests.cs(3 hunks)backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnAppGitRepositoryTests.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/src/Designer/Controllers/AppDevelopmentController.cs
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (macos-latest)
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run integration tests against actual gitea and db
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (3)
backend/tests/Designer.Tests/Infrastructure/GitRepository/AltinnAppGitRepositoryTests.cs (1)
299-306: LGTM! Test correctly validates the new behavior.The test has been properly updated to verify that
GetOptionsListIdsreturns an empty array when no options folder exists, which aligns with the PR's objective of handling missing options gracefully.backend/src/Designer/Infrastructure/GitRepository/AltinnAppGitRepository.cs (1)
801-801: LGTM! Implementation correctly handles missing options folder.The change to return an empty array instead of throwing an exception when the options folder doesn't exist is a good improvement in error handling. This aligns with the principle that the absence of options is a valid state, not an error condition.
backend/tests/Designer.Tests/Controllers/OptionsController/GetOptionListsReferencesTests.cs (1)
19-19: LGTM! Clear and consistent constant naming.The constant follows the existing naming pattern and clearly indicates its purpose.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14409 +/- ##
=======================================
Coverage 95.66% 95.66%
=======================================
Files 1880 1880
Lines 24446 24446
Branches 2810 2810
=======================================
Hits 23386 23386
Misses 800 800
Partials 260 260 ☔ View full report in Codecov by Sentry. |
Description
If an app does not have any option lists yet, and opening the content library, an toast error will occur. This error is triggered by backend responding with 500 when fetching usages of code lists.
This happens since the code that finds usage references fetches all option ids. When the app does not have any option lists in the options-folder, this method throws
DirectoryNotFound exceptionwhich the usages endpoint does not handle.To fix the issue, this PR returns an empty list from the
GetOptionsListIdsmethod instead of throwing an error. Then the method that iterates over the layouts to find usage will be able to handle apps that has no option lists.Related Issue(s)
Verification
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
The changes enhance the robustness of option list retrieval, providing a more consistent and predictable API response when no options are available.