Added Role and links to AdminUser/Portal #13
Conversation
| public class AdminUserController implements AdminUserApi { | ||
|
|
||
| private AdminUserService adminUserService; | ||
|
|
||
| public AdminUserController(AdminUserService adminUserService) { | ||
| private PortalAdminUserRoleService portalAdminUserRoleService; |
There was a problem hiding this comment.
nit: four words is right about when I start looking to abbreviate these to keep code more readable. e.g. paUserRoleService
There was a problem hiding this comment.
I agree with the dislike of the long name. I also don't love paUserRoleService and can't think of a shortened form that I like. One thought is that we could drop the word "User" which is redundant anyway, and we don't have to worry about conflicting with reserved words once we add other words to the compound name.
| @@ -31,4 +48,11 @@ public ResponseEntity<AdminUserDto> get(UUID id) { | |||
| }); | |||
| return ResponseEntity.of(adminUserDtoOpt); | |||
| } | |||
|
|
|||
| // TODO: return something useful here... but what? PortalAdminUserRoles? Role names? | |||
There was a problem hiding this comment.
I'd go with returning the same type of thing you were passed. So yes, a list of string role names
| responses: | ||
| '204': | ||
| description: roles updated | ||
| '500': |
There was a problem hiding this comment.
we could use some yaml & sugar to DRY out this '500' response, similar to how we handle *idColumn in our migration scripts
There was a problem hiding this comment.
I'm seeing people having trouble with both the OpenAPI tools (OpenAPITools/openapi-generator#1593) and Swagger (go-swagger/go-swagger#1193) when dealing with anchors. Also, I'm not too fond of how it works in practice with the definitions inlined in our migration scripts. At least with Swagger, it seems like you can have a separate definitions section (https://swagger.io/docs/specification/2-0/enums/). However, OpenAPI also provides the components section for defining common elements, so I'm in favor of using that facility, especially for the API specification that could be consumed by other clients.
| * Together with the code generated from openapi.yml, a Controller is responsible for: * | ||
| * implementing *Api methods * calling *Service methods * translating Service results (including | ||
| * exceptions) into HTTP responses | ||
| */ |
There was a problem hiding this comment.
This comment should be in a package-level README
There was a problem hiding this comment.
Oh, right... I forgot about this. It was just a reflection of my thinking and kept getting reformatted on me. 😠 I'll move it to a README, which should solve both problems.
| } | ||
|
|
||
| @Test | ||
| public void oldTestSetRolesReturnsBadRequestForNonExistentRole() throws Exception { |
There was a problem hiding this comment.
why is this test called 'oldTest...'?
There was a problem hiding this comment.
Oops... leftover from when I was experimenting with some different test styles. I'll either remove the word "old" or remove the test if I find that it's duplicating another test that I prefer the style of.
There was a problem hiding this comment.
Yes, this the old test for behavior that is now covered by testValidationExceptionReturnsBadRequest.
| return portalAdminUserRoleDao.findByPortalAdminUserId(adminUserId); | ||
| } | ||
|
|
||
| // TODO: Should this take a PortalAdminUser ID or an AdminUserId and a Portal ID (or shortcode)? |
There was a problem hiding this comment.
I think the way you have it is the right place to start, and then we can see if we need the other formulation later.
| return portalAdminUserDao.create(portalAdminUser); | ||
| } | ||
|
|
||
| public Optional<PortalAdminUser> findByPortalAdminUserId(UUID portalAdminUserId) { |
There was a problem hiding this comment.
BLOCKER: this should be called findOne to match the general Java/spring convention that the default find-by-id in a service is just called 'find' or 'findOne'.
There was a problem hiding this comment.
You're right. This is a holdover name from before I moved it here from AdminUserService.
| package bio.terra.pearl.core.service.exception; | ||
|
|
||
| import lombok.Getter; | ||
|
|
There was a problem hiding this comment.
This seems like overkill to make a separate exception class for every class that might not be found. Can we just make a general "NotFoundException"? It could even that take a class argument, so that in the rare cases where it's ambiguous what wasn't found, the callee could interrogate it
There was a problem hiding this comment.
Potentially, yes. The reason for the distinction is that different types can be thrown from the same method and the tests use the distinct exception types to verify that the exception was thrown for the right reason.
| - column: *idColumn | ||
| - column: *createdAt | ||
| - column: *lastUpdatedAt | ||
| - column: { name: name, type: text } |
There was a problem hiding this comment.
BLOCKER: name should have a unique constraint on it.
We should also have "display_name" and "description" text columns so that we can display user-friendly text and description in the UX. But that doesn't need to be in this PR
| @Autowired | ||
| private RoleService roleService; | ||
|
|
||
| private Matcher<PortalAdminUserRole> matchingPortalAdminUserRole(PortalAdminUserRole portalAdminUserRole) { |
There was a problem hiding this comment.
nice, I like the use of matchers here
…ntroller for an imagined role management UI.
breilly2
force-pushed
the
br-admin-roles
branch
from
a761f15 to
fd67165
Compare
|
Addressed feedback. Also rebased on top of the current development branch and force pushed, so if you have a local copy of the branch, be sure to blow it away before pulling down the changes. |
... including services and a controller for an imagined role management UI.
To test... well... run the tests. 😄 After running the core app to apply liquibase changes, of course.
There are a few TODOs here, at least some of which I plan to do (specifically adding a couple of DAO tests). Others may be helped by some conversation to make decisions.