Fix handling assignments when removed from role#71
Open
berntjoh wants to merge 8 commits into
Open
Conversation
… into fix_handling_assignments_when_removed_from_role # Conflicts: # src/main/java/no/fintlabs/assignment/flattened/FlattenedAssignmentRepository.java
There was a problem hiding this comment.
Pull request overview
Updates resource fetching and flattened-assignment deletion behavior to better handle cases where a user’s resource access should remain when a role-based assignment is removed.
Changes:
- Add a
resourcefilter(list of resource IDs) to the role resources endpoint and propagate it through the response factory into the JPASpecification. - Extend
ResourceSpecificationBuilderto filter by specific resource IDs. - Adjust flattened-assignment logic/tests to avoid publishing deletion when a direct user assignment still exists for the same user/resource.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/no/fintlabs/resource/AssignmentResourceServiceIntegrationTest.java | Updates ResourceSpecificationBuilder constructor usage and adds role resource filtering tests. |
| src/test/java/no/fintlabs/assignment/flattened/FlattenedAssignmentServiceIntegrationTest.java | Expands deletion-related integration test coverage and verifies producer interactions. |
| src/main/java/no/fintlabs/resource/ResourceSpecificationBuilder.java | Adds resourceIds support to the built Specification. |
| src/main/java/no/fintlabs/resource/ResourceResponseFactory.java | Adds resourceIds parameter and passes it into ResourceSpecificationBuilder. |
| src/main/java/no/fintlabs/resource/ResourceController.java | Adds resourcefilter to role resources endpoint and forwards it to the response factory. |
| src/main/java/no/fintlabs/assignment/flattened/FlattenedAssignmentService.java | Refines direct-assignment lookup and accounts for direct assignments when deciding whether to publish deletions. |
| src/main/java/no/fintlabs/assignment/flattened/FlattenedAssignmentRepository.java | Updates repository method to target direct assignments (assignmentViaRoleRef IS NULL). |
Comments suppressed due to low confidence (1)
src/test/java/no/fintlabs/resource/AssignmentResourceServiceIntegrationTest.java:586
- This test stubs
authorizationUtil.getAllAuthorizedOrgUnitIDs(), butgetResourcesAssignedToRole(...)only uses the passedSpecification/Pageableand does not consultauthorizationUtil. Removing the unused stub will make the test intent clearer.
given(authorizationUtil.getAllAuthorizedOrgUnitIDs()).willReturn(kompavdList);
ResourceSpecificationBuilder builder = new ResourceSpecificationBuilder(null,1L, "ALLTYPES", null, null, null, List.of(1L,2L));
Page<AssignmentResource> rolePage = assignmentResourceService.getResourcesAssignedToRole(1L, builder.build(), Pageable.unpaged());
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+300
to
+320
| //TODO optimize query to check existence only | ||
| private boolean notExistOtherActiveFlattenedAssignmentsWithSameUserRefAndResourceRef(FlattenedAssignment flattenedAssignment) { | ||
|
|
||
| log.info("Checking if other active flattened assignment exists for user {} and resource {}", | ||
| flattenedAssignment.getUserRef(), | ||
| flattenedAssignment.getResourceRef() | ||
| ); | ||
|
|
||
| List<FlattenedAssignment> otherActiveAssignments = flattenedAssignmentRepository.findByAssignmentViaRoleRefNotAndUserRefAndResourceRefAndAssignmentTerminationDateIsNull( | ||
| flattenedAssignment.getAssignmentViaRoleRef(), | ||
| flattenedAssignment.getUserRef(), | ||
| flattenedAssignment.getResourceRef() | ||
| ); | ||
|
|
||
| Optional<FlattenedAssignment> optionalFlattenedAssignment = flattenedAssignmentRepository. | ||
| findByAssignmentViaRoleRefIsNullAndUserRefAndResourceRefAndAssignmentTerminationDateIsNull( | ||
| flattenedAssignment.getUserRef(), | ||
| flattenedAssignment.getResourceRef() | ||
| ); | ||
| optionalFlattenedAssignment.ifPresent(otherActiveAssignments::add); | ||
|
|
Contributor
Author
There was a problem hiding this comment.
@chadaj I think it should be left as it is for now. When the new table for flattened assignments to Entra is in place this code will be changed removed I think. We may have a huddle about this on monday 18.
Comment on lines
+531
to
+532
| given(authorizationUtil.getAllAuthorizedOrgUnitIDs()).willReturn(kompavdList); | ||
|
|
Contributor
Author
There was a problem hiding this comment.
@chadaj I removed the test stubs as suggested by CoPilot.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.