BI-2258 - Endpoint filtering for Experimental Collaborator role by mlm483 · Pull Request #385 · Breeding-Insight/bi-api

mlm483 · 2024-08-09T17:56:18Z

Description

Story: https://breedinginsight.atlassian.net/browse/BI-2258

Added filtering-by-authorized-experiments to two endpoints that Experimental Collaborators are allowed to access. Filtering only happens for users in the Experimental Collaborator role, for other users, no filtering is applied. The following methods were updated.
- BrAPIStudiesController::getStudies
- BrAPITrialsController::getExperiments
Added integration tests for this filtering.
Explicitly implmented /seasons GET endpoint instead of relying on BrAPIV2Controller::getCatchall. The idea here is that Experimental Collaborators are allowed to access this endpoint, but not every GET endpoint that was handled by the catchall method.

Testing

See acceptance criteria on Jira.
The testing steps on for BI-2283, in PR #393 test this story as well.

Checklist:

I have performed a self-review of my own code
I have tested my code and ensured it meets the acceptance criteria of the story
I have tested that my code works with both the brapi-java-server and BreedBase
I have create/modified unit tests to cover this change
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to documentation
I have run TAF: <please include a link to TAF run>

davedrp

Passes Test

dmeidlin · 2024-08-26T21:45:52Z

src/main/java/org/breedinginsight/brapi/v2/BrAPIStudiesController.java

+            Optional<ProgramUser> experimentalCollaborator = programUserService.getIfExperimentalCollaborator(programId, securityService.getUser().getId());
+            // If the program user is an experimental collaborator, filter results.
+            if (experimentalCollaborator.isPresent()) {
+                Optional<Program> program = programService.getById(programId);
+                if (program.isEmpty()) {
+                    return HttpResponse.notFound();
+                }
+
+                List<UUID> experimentIds = experimentalCollaboratorService.getAuthorizedExperimentIds(experimentalCollaborator.get().getId());
+                studies = studyService.getStudiesByExperimentIds(program.get(), experimentIds)
+                        .stream()
+                        .peek(this::setDbIds)
+                        .collect(Collectors.toList());
+            } else {
+                studies = studyService.getStudies(programId)
+                        .stream()
+                        .peek(this::setDbIds)
+                        .collect(Collectors.toList());
+            }


Optional methods could be used to refactor the fetching logic.

Suggested change

Optional<ProgramUser> experimentalCollaborator = programUserService.getIfExperimentalCollaborator(programId, securityService.getUser().getId());

// If the program user is an experimental collaborator, filter results.

if (experimentalCollaborator.isPresent()) {

Optional<Program> program = programService.getById(programId);

if (program.isEmpty()) {

return HttpResponse.notFound();

}

List<UUID> experimentIds = experimentalCollaboratorService.getAuthorizedExperimentIds(experimentalCollaborator.get().getId());

studies = studyService.getStudiesByExperimentIds(program.get(), experimentIds)

.stream()

.peek(this::setDbIds)

.collect(Collectors.toList());

} else {

studies = studyService.getStudies(programId)

.stream()

.peek(this::setDbIds)

.collect(Collectors.toList());

}

Optional<HttpResponse> httpResponse = programUserService.getIfExperimentalCollaborator(programId, securityService.getUser().getId())

.flatMap(experimentalCollaborator -> { //If the program user is an experimental collaborator, filter results.

Optional<Program> program = programService.getById(programId);

return program.map(p -> {

List<UUID> experimentIds = experimentalCollaboratorService.getAuthorizedExperimentIds(experimentalCollaborator.getId());

List<Study> studies = studyService.getStudiesByExperimentIds(p, experimentIds)

.stream()

.peek(this::setDbIds)

.collect(Collectors.toList());

return HttpResponse.ok(studies);

});

})

.orElseGet(() -> { //Otherwise, fallback to fetching studies without considering collaborator status

List<Study> studies = studyService.getStudies(programId)

.stream()

.peek(this::setDbIds)

.collect(Collectors.toList());

return HttpResponse.ok(studies);

});

return httpResponse.orElse(HttpResponse.notFound());

I tried to test this and the BrAPITrialsController suggestions locally, they both resulted in build errors. Are there problems in my code as written that you would like me to address?

dmeidlin · 2024-08-26T21:52:14Z

src/main/java/org/breedinginsight/brapi/v2/BrAPITrialsController.java

+            Optional<ProgramUser> experimentalCollaborator = programUserService.getIfExperimentalCollaborator(programId, securityService.getUser().getId());
+            // If the program user is an experimental collaborator, filter results.
+            if (experimentalCollaborator.isPresent()) {
+                Optional<Program> program = programService.getById(programId);
+                if (program.isEmpty()) {
+                    return HttpResponse.notFound();
+                }
+                List<UUID> experimentIds = experimentalCollaboratorService.getAuthorizedExperimentIds(experimentalCollaborator.get().getId());
+                experiments = experimentService.getTrialsByExperimentIds(program.get(), experimentIds).stream().peek(this::setDbIds).collect(Collectors.toList());
+            } else {
+                experiments = experimentService.getExperiments(programId).stream().peek(this::setDbIds).collect(Collectors.toList());
+            }
+


Suggested change

Optional<ProgramUser> experimentalCollaborator = programUserService.getIfExperimentalCollaborator(programId, securityService.getUser().getId());

// If the program user is an experimental collaborator, filter results.

if (experimentalCollaborator.isPresent()) {

Optional<Program> program = programService.getById(programId);

if (program.isEmpty()) {

return HttpResponse.notFound();

}

List<UUID> experimentIds = experimentalCollaboratorService.getAuthorizedExperimentIds(experimentalCollaborator.get().getId());

experiments = experimentService.getTrialsByExperimentIds(program.get(), experimentIds).stream().peek(this::setDbIds).collect(Collectors.toList());

} else {

experiments = experimentService.getExperiments(programId).stream().peek(this::setDbIds).collect(Collectors.toList());

}

Optional<HttpResponse> httpResponse = programUserService.getIfExperimentalCollaborator(programId, securityService.getUser().getId())

.flatMap(experimentalCollaborator ->

programService.getById(programId)

.map(program ->

experimentalCollaboratorService.getAuthorizedExperimentIds(experimentalCollaborator.getId())

.stream()

.flatMap(experimentId -> experimentService.getTrialsByExperimentIds(program, List.of(experimentId)).stream())

.peek(this::setDbIds)

.collect(Collectors.toList())))

.or(() ->

Optional.of(experimentService.getExperiments(programId)

.stream()

.peek(this::setDbIds)

.collect(Collectors.toList()))

.filter(experiments -> !experiments.isEmpty())

.map(HttpResponse::ok)

.orElse(HttpResponse.notFound());

return httpResponse.orElse(HttpResponse.notFound());

See my comment on the BrAPIStudiesController.java suggestion.

dmeidlin · 2024-08-26T22:03:53Z

src/main/java/org/breedinginsight/services/ProgramUserService.java

+    public Optional<ProgramUser> getIfExperimentalCollaborator(UUID programId, UUID userId) throws DoesNotExistException {
+        Optional<ProgramUser> programUser = getProgramUserbyId(programId, userId);
+        if (programUser.isEmpty()) {
+            throw new DoesNotExistException("Program User does not exist");
+        }
+        boolean isExperimentalCollaborator = programUser.get().getRoles().stream().anyMatch(x -> ProgramSecuredRole.getEnum(x.getDomain()).equals(ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR));
+        if (isExperimentalCollaborator) {
+            return programUser;
+        }
+        return Optional.empty();
+    }


Could refactor here using Optional methods

Suggested change

public Optional<ProgramUser> getIfExperimentalCollaborator(UUID programId, UUID userId) throws DoesNotExistException {

Optional<ProgramUser> programUser = getProgramUserbyId(programId, userId);

if (programUser.isEmpty()) {

throw new DoesNotExistException("Program User does not exist");

}

boolean isExperimentalCollaborator = programUser.get().getRoles().stream().anyMatch(x -> ProgramSecuredRole.getEnum(x.getDomain()).equals(ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR));

if (isExperimentalCollaborator) {

return programUser;

}

return Optional.empty();

}

public Optional<ProgramUser> getIfExperimentalCollaborator(UUID programId, UUID userId) throws DoesNotExistException {

return getProgramUserbyId(programId, userId)

.filter(programUser -> programUser.getRoles().stream()

.anyMatch(x -> ProgramSecuredRole.getEnum(x.getDomain()).equals(ProgramSecuredRole.EXPERIMENTAL_COLLABORATOR)))

.or(() -> {

throw new RuntimeException(new DoesNotExistException("Program User does not exist"));

});

}

Thank you for the suggestion. I tested it locally and it caused tests to fail because returning Optional.empty() is an important case which conceptually indicates that the program user exists, but is not in the "Experimental Collaborator" role.

mlm483 added 14 commits August 8, 2024 16:27

[BI-2258] - added authorization to datasetExport

60ffbf0

[BI-2258] - merged develop into feature/BI-2258

af93ba4

[BI-2258] - added filtering for Experimental Collaborator

86e0892

[BI-2258] - merge develop into feature/BI-2258

7de97f9

[BI-2258] - implemented /seasons GET endpoint

be4ba6f

[BI-2258] - added integration test for studies filtering

17c6098

[BI-2258] - added integration test for trials filtering

ea4c341

[BI-2258] - documented tests.

d9e9e95

Merge branch 'develop' into feature/BI-2258

dbdaa78

[BI-2258] - added notice

b03360e

[BI-2258] - added method documentation

e2534b4

[BI-2258] - added documentation

de49a17

[BI-2258] - reduced code duplication

4b13e24

[BI-2258] - added notice

d3d5929

mlm483 marked this pull request as ready for review August 19, 2024 20:58

mlm483 requested review from a team, davedrp and dmeidlin and removed request for a team August 19, 2024 21:35

mlm483 assigned davedrp Aug 19, 2024

davedrp approved these changes Aug 20, 2024

View reviewed changes

[BI-2258] - merged develop into feature/BI-2258

d8e0935

rob-ouser-bi force-pushed the develop branch from 400a09e to 37b75c4 Compare August 26, 2024 15:10

dmeidlin reviewed Aug 26, 2024

View reviewed changes

mlm483 requested a review from dmeidlin August 27, 2024 18:46

dmeidlin approved these changes Aug 27, 2024

View reviewed changes

mlm483 added 2 commits August 27, 2024 16:23

Merge branch 'develop' into feature/BI-2258

328e542

Merge branch 'develop' into feature/BI-2258

9b11eb8

mlm483 added 2 commits August 29, 2024 12:26

[BI-2258] - merged develop into feature/BI-2258

bb57b96

[BI-2258] - removed duplicate SQL in test file

4418f21

mlm483 merged commit a910b89 into develop Aug 29, 2024

mlm483 deleted the feature/BI-2258 branch August 29, 2024 19:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BI-2258 - Endpoint filtering for Experimental Collaborator role#385

BI-2258 - Endpoint filtering for Experimental Collaborator role#385
mlm483 merged 19 commits intodevelopfrom
feature/BI-2258

mlm483 commented Aug 9, 2024 •

edited

Loading

Uh oh!

davedrp left a comment

Uh oh!

dmeidlin Aug 26, 2024

Uh oh!

mlm483 Aug 27, 2024

Uh oh!

dmeidlin Aug 26, 2024

Uh oh!

mlm483 Aug 27, 2024

Uh oh!

dmeidlin Aug 26, 2024

Uh oh!

mlm483 Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

mlm483 commented Aug 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Checklist:

Uh oh!

davedrp left a comment

Choose a reason for hiding this comment

Uh oh!

dmeidlin Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

mlm483 Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

dmeidlin Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

mlm483 Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

dmeidlin Aug 26, 2024

Choose a reason for hiding this comment

Uh oh!

mlm483 Aug 27, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mlm483 commented Aug 9, 2024 •

edited

Loading