[PM-8895] Moving the groups controller business logic to a service #55
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| public async Task<GroupResponseModel> Get(string orgId, string id) | ||
| { | ||
| var group = await _groupRepository.GetByIdAsync(new Guid(id)); | ||
| if (group == null || !await _currentContext.ManageGroups(group.OrganizationId)) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| return new GroupResponseModel(group); | ||
| var group = await _groupsControllerService.GetOrganizationGroup(orgId, id); | ||
| return group; | ||
| } |
There was a problem hiding this comment.
style: Consider using a more descriptive variable name than 'group' for clarity
| public async Task<IEnumerable<Guid>> GetUsers(string orgId, string id) | ||
| { | ||
| var idGuid = new Guid(id); | ||
| var group = await _groupRepository.GetByIdAsync(idGuid); | ||
| if (group == null || !await _currentContext.ManageGroups(group.OrganizationId)) | ||
| { | ||
| throw new NotFoundException(); | ||
| } | ||
|
|
||
| var groupIds = await _groupRepository.GetManyUserIdsByIdAsync(idGuid); | ||
| return groupIds; | ||
| var userIds = await _groupsControllerService.GetOrganizationUsers(orgId); | ||
| return userIds; | ||
| } |
There was a problem hiding this comment.
logic: The 'id' parameter is not used in the service method call
| /// <summary> | ||
| /// Gets the basic group information of an organization | ||
| /// </summary> | ||
| /// <param name="orgId">Organization id/param> |
There was a problem hiding this comment.
syntax: Missing closing bracket in XML comment for 'orgId' parameter
| (await _authorizationService.AuthorizeAsync(user, GroupOperations.ReadAll(orgId))).Succeeded; | ||
| if (!authorized) | ||
| { | ||
| throw new NotFoundException(); |
There was a problem hiding this comment.
style: Consider using ForbiddenException instead of NotFoundException for unauthorized access
| .Succeeded; | ||
| if (!authorized) | ||
| { | ||
| throw new NotFoundException("You are not authorized to grant access to these collections."); |
There was a problem hiding this comment.
logic: Use ForbiddenException instead of NotFoundException for unauthorized access
| if (organizationUser != null && !currentGroupUsers.Contains(organizationUser.Id) && model.Users.Contains(organizationUser.Id)) | ||
| { | ||
| throw new BadRequestException("You cannot add yourself to groups."); |
There was a problem hiding this comment.
style: Consider extracting this check into a separate method for better readability
| using Bit.Api.AdminConsole.Models.Request; | ||
| using Bit.Api.AdminConsole.Models.Response; | ||
|
|
||
| namespace Api.AdminConsole.Services; |
There was a problem hiding this comment.
style: Consider using 'Bit.Api.AdminConsole.Services' for consistency with other namespaces
| Task<GroupResponseModel> GetOrganizationGroup(string orgId, string groupId); | ||
| Task<GroupDetailsResponseModel> GetOrganizationGroupDetail(string orgId, string groupId); |
There was a problem hiding this comment.
logic: Inconsistent parameter types: string for orgId and groupId here, but Guid in other methods
| @@ -93,6 +95,9 @@ public void ConfigureServices(IServiceCollection services) | |||
| // BitPay | |||
| services.AddSingleton<BitPayClient>(); | |||
|
|
|||
| // Groups | |||
| services.AddScoped<IGroupsControllerService, GroupsControllerService>(); | |||
There was a problem hiding this comment.
style: Consider adding a comment explaining the purpose of this service
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-8995
📔 Objective
The GroupsController contained a lot of logic that is responsible for creating the objects for the endpoints. Moving to a service allows reusability of the code in the new reports being developed. The controller has repositories injected in. Exposing repos in a controller could open up issues with bypassing logic in services designed to filter. Also a potential issue if the repo gets exposed.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changesGreptile Summary
This PR refactors the GroupsController by moving business logic to a new GroupsControllerService, improving code organization and reusability while maintaining existing functionality.
GroupsControllerServiceto encapsulate group-related operations, separating concerns from the controllerIGroupsControllerServiceinterface to define group management methods, enhancing modularityGroupsControllerto delegate operations to the new service, simplifying its structureStartup.csto registerIGroupsControllerServicefor dependency injection