feat(designer): endpoint to get UserOrgPermissions #14389
Conversation
github-actions
bot
added
solution/studio/designer
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
github-actions
bot
added
the
quality/testing
framitdavid
changed the title
framitdavid
changed the title
framitdavid
changed the title
📝 WalkthroughWalkthroughThe pull request introduces a comprehensive enhancement to the user permission and organization repository creation functionality in the Altinn Studio Designer backend. The changes include adding a new 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
backend/src/Designer/Services/Implementation/UserService.cs (1)
34-35:⚠️ Potential issueUse short-circuit logic to prevent unnecessary API calls.
The current implementation might evaluate both conditions even when the first one is true. Use short-circuit logic with
||instead of|.
🧹 Nitpick comments (7)
backend/src/Designer/Models/Dto/UserOrgPermission.cs (1)
3-6: Add XML documentation for the DTO.Since this DTO is part of the API contract, please add XML documentation to describe the class and its property:
+/// <summary> +/// Represents user permissions for organization repository operations. +/// </summary> public class UserOrgPermission { + /// <summary> + /// Gets or sets a value indicating whether the user can create repositories for the organization. + /// </summary> public bool CanCreateOrgRepo { get; set; } }backend/src/Designer/Services/Interfaces/IUserService.cs (1)
7-10: Add XML documentation and support for cancellation.
- Add XML documentation for the interface and method.
- Consider adding CancellationToken parameter for proper async operation cancellation support.
+/// <summary> +/// Service for managing user-related operations. +/// </summary> public interface IUserService { + /// <summary> + /// Gets the user's permissions for an organization. + /// </summary> + /// <param name="altinnOrgContext">The organization context.</param> + /// <param name="cancellationToken">A token to cancel the async operation.</param> + /// <returns>The user's permissions for the organization.</returns> - public Task<UserOrgPermission> GetUserOrgPermission(AltinnOrgContext altinnOrgContext); + public Task<UserOrgPermission> GetUserOrgPermission(AltinnOrgContext altinnOrgContext, CancellationToken cancellationToken = default); }backend/src/Designer/RepositoryClient/Model/Team.cs (1)
13-14: Add XML documentation and improve property organization.The new property should follow the existing documentation pattern and be properly organized with other properties.
- public bool CanCreateOrgRepo { get; set; } + /// <summary> + /// Gets or sets a value indicating whether the team can create organization repositories. + /// </summary> + public bool CanCreateOrgRepo { get; set; }backend/src/Designer/Models/AltinnOrgContext.cs (1)
5-23: Add XML documentation and strengthen input validation.
- Add XML documentation for the class, properties, and methods.
- Consider adding input validation in the factory method for defensive programming.
+/// <summary> +/// Represents the context for Altinn organization operations. +/// </summary> public class AltinnOrgContext { + /// <summary> + /// Gets the organization identifier. + /// </summary> public string Org { get; } + + /// <summary> + /// Gets the developer's username. + /// </summary> public string DeveloperName { get; } private AltinnOrgContext(string org, string developerName) { Guard.AssertValidateOrganization(org); Org = org; Guard.AssertArgumentNotNullOrWhiteSpace(developerName, nameof(developerName)); DeveloperName = developerName; } + /// <summary> + /// Creates a new instance of the <see cref="AltinnOrgContext"/> class. + /// </summary> + /// <param name="org">The organization identifier.</param> + /// <param name="developerName">The developer's username.</param> + /// <returns>A new instance of <see cref="AltinnOrgContext"/>.</returns> + /// <exception cref="ArgumentException">Thrown when inputs are invalid.</exception> public static AltinnOrgContext FromOrg(string org, string developerName) { + if (string.IsNullOrWhiteSpace(org)) + throw new ArgumentException("Organization cannot be null or empty.", nameof(org)); + if (string.IsNullOrWhiteSpace(developerName)) + throw new ArgumentException("Developer name cannot be null or empty.", nameof(developerName)); + return new AltinnOrgContext(org, developerName); } }backend/src/Designer/Services/Implementation/UserService.cs (1)
11-18: Add XML documentation for public members.Add XML documentation for the class and constructor to improve API documentation.
+/// <summary> +/// Service for managing user-related operations. +/// </summary> public class UserService : IUserService { private readonly IGitea _giteaApi; + /// <summary> + /// Initializes a new instance of the <see cref="UserService"/> class. + /// </summary> + /// <param name="giteaApi">The Gitea API client.</param> public UserService(IGitea giteaApi) { _giteaApi = giteaApi; }backend/tests/Designer.Tests/Helpers/GuardTests.cs (1)
55-63: Add more test cases for organization name validation.Consider adding test cases for:
- Organization names with spaces (e.g., "Organization Name")
- Organization names with hyphens (e.g., "org-name")
- Organization names with numbers (e.g., "org123")
[Theory] [InlineData(null)] [InlineData("")] [InlineData(" ")] [InlineData("Invalid@OrgName")] +[InlineData("Organization Name")] +[InlineData("org name")] +[InlineData("org-name")] +[InlineData("123org")] public void AssertValidateOrganization_InvalidOrg_ShouldThrowException(string org)backend/src/Designer/Services/Implementation/GiteaAPIWrapper/GiteaAPIWrapper.cs (1)
Line range hint
1-1000: Consider standardizing error handling across all Gitea API operations.The codebase shows inconsistent error handling patterns across different methods. Some return null, others return empty collections, and some include status codes in the response. Given this service's critical role in permission management, consider:
- Implementing a consistent error handling strategy
- Using custom exception types for different error scenarios
- Including relevant context in error responses
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
backend/src/Designer/Controllers/UserController.cs(3 hunks)backend/src/Designer/Helpers/Guard.cs(1 hunks)backend/src/Designer/Infrastructure/ServiceRegistration.cs(1 hunks)backend/src/Designer/Models/AltinnOrgContext.cs(1 hunks)backend/src/Designer/Models/AltinnRepoContext.cs(2 hunks)backend/src/Designer/Models/Dto/UserOrgPermission.cs(1 hunks)backend/src/Designer/RepositoryClient/Model/Team.cs(1 hunks)backend/src/Designer/Services/Implementation/GiteaAPIWrapper/GiteaAPIWrapper.cs(1 hunks)backend/src/Designer/Services/Implementation/UserService.cs(1 hunks)backend/src/Designer/Services/Interfaces/IUserService.cs(1 hunks)backend/tests/Designer.Tests/GiteaIntegrationTests/UserControllerGiteaIntegrationTests.cs(4 hunks)backend/tests/Designer.Tests/Helpers/GuardTests.cs(3 hunks)backend/tests/Designer.Tests/Services/UserServiceTests.cs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: Run dotnet build and test (macos-latest)
backend/src/Designer/Controllers/UserController.cs
[warning] 30-30:
Parameter 'userService' has no matching param tag in the XML comment for 'UserController.UserController(IGitea, IAntiforgery, IUserService)' (but other parameters do)
[warning] 30-30:
Parameter 'userService' has no matching param tag in the XML comment for 'UserController.UserController(IGitea, IAntiforgery, IUserService)' (but other parameters do)
🪛 GitHub Check: Run integration tests against actual gitea and db
backend/src/Designer/Controllers/UserController.cs
[warning] 30-30:
Parameter 'userService' has no matching param tag in the XML comment for 'UserController.UserController(IGitea, IAntiforgery, IUserService)' (but other parameters do)
[warning] 30-30:
Parameter 'userService' has no matching param tag in the XML comment for 'UserController.UserController(IGitea, IAntiforgery, IUserService)' (but other parameters do)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Build environment and run e2e test
- GitHub Check: Run dotnet build and test (windows-latest)
- GitHub Check: Run dotnet build and test (ubuntu-latest)
- GitHub Check: Analyze
🔇 Additional comments (5)
backend/src/Designer/Models/AltinnOrgContext.cs (1)
12-12: Verify the organization validation logic.The
Guard.AssertValidateOrganizationmethod is used for validation, but its implementation is not visible in the provided context.✅ Verification successful
Organization validation logic is properly implemented
- The validation uses a regex pattern to ensure valid organization names
- Includes null/empty checks with appropriate error messages
- Has comprehensive test coverage including valid and invalid cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Guard.AssertValidateOrganization implementation ast-grep --pattern 'AssertValidateOrganization($_) { $$$ }'Length of output: 61
Script:
#!/bin/bash # Search for AssertValidateOrganization method using ripgrep rg -l "AssertValidateOrganization" # Search for Guard class definition ast-grep --pattern 'class Guard { $$$ }' # Search for files containing validation logic fd -t f "(Guard|Validate|Organization)"Length of output: 2038
Script:
#!/bin/bash # Get the Guard class implementation rg -A 10 "AssertValidateOrganization" backend/src/Designer/Helpers/Guard.cs # Get the test cases rg -A 10 "AssertValidateOrganization" backend/tests/Designer.Tests/Helpers/GuardTests.cs # Get usage context from AltinnRepoContext rg -A 5 "AssertValidateOrganization" backend/src/Designer/Models/AltinnRepoContext.csLength of output: 1336
backend/src/Designer/Models/AltinnRepoContext.cs (1)
23-27: LGTM! Good refactoring of validation logic.Moving organization validation to the Guard class promotes code reuse and consistency across the codebase.
backend/src/Designer/Infrastructure/ServiceRegistration.cs (1)
57-57: LGTM!The UserService is correctly registered with a scoped lifetime, consistent with other similar services.
backend/src/Designer/Services/Implementation/GiteaAPIWrapper/GiteaAPIWrapper.cs (2)
81-87: LGTM! Improved JSON deserialization with proper property naming policy.The change to use
JsonSerializerOptionswithSnakeCaseLowernaming policy ensures correct mapping between Gitea API's snake_case response and the Team model's PascalCase properties.
91-91: LGTM! Fixed typo in error message.Corrected the error message from "Cold" to "Could".
backend/tests/Designer.Tests/GiteaIntegrationTests/UserControllerGiteaIntegrationTests.cs
Show resolved
Hide resolved
backend/tests/Designer.Tests/GiteaIntegrationTests/UserControllerGiteaIntegrationTests.cs
Show resolved
Hide resolved
backend/src/Designer/Services/Implementation/GiteaAPIWrapper/GiteaAPIWrapper.cs
Show resolved
Hide resolved
framitdavid
added
the
skip-manual-testing
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
I have tested it locally, and it will undergo full testing with the frontend PR afterwards as well. 😊 |
framitdavid
deleted the
feat/newEndpointToFetchUserRepositoryPermissions
branch
Description
This PR introduces a new backend endpoint to verify if a user has the necessary permissions to create a new app/repository for their organization. The endpoint will be used by the frontend to check the user's access rights before enabling the "Submit" button for repository/app creation. This proactive validation helps prevent errors and ensures a smoother user experience.
Related Issue(s)
Verification
Manual testing
Manual tested that I cannot create repos for orgs I don't belong to
Manual tested that I can create repos for orgs I belongs to
Manual tested that I can create repo if the owner is myself.
Frontend
The frontend changes and video of how it looks can be seen within the PR description of the following PR: #14395
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests