-
Notifications
You must be signed in to change notification settings - Fork 74
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: Delete all Summary 2.0 components that has a reference to a deleted component #14126
Conversation
…elete-all-summary-20-components-that-has-a-reference-to-deleted-element-component-page-or-layout-set
backend/tests/Designer.Tests/Controllers/AppDevelopmentController/SaveFormLayoutTests.cs
Fixed
Show fixed
Hide fixed
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #14126 +/- ##
=======================================
Coverage 95.66% 95.66%
=======================================
Files 1891 1891
Lines 24583 24583
Branches 2823 2823
=======================================
Hits 23517 23517
Misses 805 805
Partials 261 261 ☔ View full report in Codecov by Sentry. |
backend/src/Designer/EventHandlers/LayoutPageDeleted/LayoutPageDeletedHandler.cs
Fixed
Show fixed
Hide fixed
backend/src/Designer/EventHandlers/LayoutSetDeleted/LayoutSetDeletedComponentRefHandler.cs
Fixed
Show fixed
Hide fixed
backend/src/Designer/EventHandlers/LayoutSetDeleted/LayoutSetDeletedComponentRefHandler.cs
Fixed
Show fixed
Hide fixed
…elete-all-summary-20-components-that-has-a-reference-to-deleted-element-component-page-or-layout-set
d881911
to
160d666
Compare
backend/src/Designer/EventHandlers/LayoutPageDeleted/LayoutPageDeletedHandler.cs
Fixed
Show fixed
Hide fixed
…elete-all-summary-20-components-that-has-a-reference-to-deleted-element-component-page-or-layout-set
…mponents-that-has-a-reference-to-deleted-element-component-page-or-layout-set
There was a problem hiding this 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
🧹 Nitpick comments (3)
backend/tests/Designer.Tests/Controllers/AppDevelopmentController/DeleteFormLayoutTests.cs (3)
60-62
: Consider adding more test scenarios.The test only covers one scenario with fixed values. Consider parameterizing the test with more scenarios using
[InlineData]
to test different layouts and components.
74-81
: Extract layout paths to a constant or configuration.The hardcoded array of layout paths could be moved to a constant or configuration file to improve maintainability and reusability. Also, consider using a more flexible collection type instead of an array.
- string[] layoutPaths = [ - "layout/layouts/Side1.json", - "layout/layouts/Side2.json", - "layout2/layouts/Side1.json", - "layout2/layouts/Side2.json", - ]; + private static readonly IReadOnlyList<string> LayoutPaths = new[] + { + "layout/layouts/Side1.json", + "layout/layouts/Side2.json", + "layout2/layouts/Side1.json", + "layout2/layouts/Side2.json" + };
64-66
: Document test data dependencies.Add XML documentation to describe the required structure and content of the test data repositories (
app-with-summary2-components
andapp-with-summary2-components-after-deleting-references
).+ // Test data requirements: + // app-with-summary2-components: Contains layouts with Summary 2.0 components + // app-with-summary2-components-after-deleting-references: Expected state after deletion
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/tests/Designer.Tests/Controllers/AppDevelopmentController/DeleteFormLayoutTests.cs
(2 hunks)backend/tests/Designer.Tests/Controllers/AppDevelopmentController/DeleteLayoutSetTests.cs
(1 hunks)backend/tests/Designer.Tests/Controllers/AppDevelopmentController/SaveFormLayoutTests.cs
(2 hunks)frontend/language/src/nb.json
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/tests/Designer.Tests/Controllers/AppDevelopmentController/DeleteLayoutSetTests.cs
- backend/tests/Designer.Tests/Controllers/AppDevelopmentController/SaveFormLayoutTests.cs
- frontend/language/src/nb.json
⏰ 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 integration tests against actual gitea and db
- GitHub Check: Run dotnet build and test (ubuntu-latest)
🔇 Additional comments (2)
backend/tests/Designer.Tests/Controllers/AppDevelopmentController/DeleteFormLayoutTests.cs (2)
2-2
: LGTM!The addition of the
System.Linq
namespace is appropriate as it's required for theToList()
extension method used in the test.
71-72
: Verify async operation handling.Consider adding a timeout to the HTTP request and ensure proper resource cleanup:
- using var response = await HttpClient.SendAsync(httpRequestMessage); + using var cts = new CancellationTokenSource(TimeSpan.FromSeconds(30)); + using var response = await HttpClient.SendAsync(httpRequestMessage, cts.Token);Also, verify that no other async operations are running that could affect the test:
backend/tests/Designer.Tests/Controllers/AppDevelopmentController/DeleteFormLayoutTests.cs
Show resolved
Hide resolved
…mponents-that-has-a-reference-to-deleted-element-component-page-or-layout-set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested in dev - works smooth. 🚀
Description
Delete all Summary 2.0 components that has a reference to a deleted component
Related Issue(s)
Verification
Summary by CodeRabbit
Based on the comprehensive summary, here are the updated release notes:
New Features
Bug Fixes
Documentation
Refactor
Style