[BI-1366] Subscribe Ontology by ctucker3 · Pull Request #175 · Breeding-Insight/bi-api

ctucker3 · 2022-04-05T13:54:50Z

Description

Story: https://breedinginsight.atlassian.net/jira/software/c/projects/BI/boards/1?modal=detail&selectedIssue=BI-1366

Adds subscribe endpoints to the backend OntologyController. See bi-web for the different check conditions.

Includes tests for all scenarios.

Dependencies

biweb: BI-1366

Testing

See biweb.

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>

timparsons · 2022-04-07T18:36:08Z

src/main/java/org/breedinginsight/api/v1/controller/OntologyController.java

-        return HttpResponse.ok(response);
+        try {
+            List<SharedProgram> sharedPrograms = ontologyService.getSharedOntology(programId, shared);
+            List<org.breedinginsight.api.model.v1.response.metadata.Status> metadataStatus = new ArrayList<>();


I think you can reduce the generic in List to just List<Status>

timparsons · 2022-04-07T18:36:55Z

src/main/java/org/breedinginsight/api/v1/controller/OntologyController.java

-        Response<DataResponse<SharedProgram>> response = new Response(metadata, new DataResponse<>(sharedPrograms));
-        return HttpResponse.ok(response);
+        try {
+            List<SharedProgram> sharedPrograms = ontologyService.getSharedOntology(programId, shared);


Could the name of both the class and this variable be changed to SharedOntology? SharedProgram doesn't make as much sense to me.

timparsons · 2022-04-07T18:39:12Z

src/main/java/org/breedinginsight/api/v1/controller/OntologyController.java

+            ontologyService.unsubscribeOntology(programId, sharingProgramId);
+            return HttpResponse.ok();
+        } catch (UnprocessableEntityException e) {
+            return HttpResponse.status(HttpStatus.UNPROCESSABLE_ENTITY, e.getMessage());


Can you add an error log statement, please?

timparsons · 2022-04-07T18:39:25Z

src/main/java/org/breedinginsight/api/v1/controller/OntologyController.java

+            SubscribedProgram shareRequest = ontologyService.subscribeOntology(programId, sharingProgramId);
+            Response<SubscribedProgram> response = new Response(shareRequest);
+            return HttpResponse.ok(response);
+        } catch (UnprocessableEntityException e) {


Can you add an error log statement, please?

timparsons · 2022-04-07T18:41:05Z

src/main/java/org/breedinginsight/daos/ProgramOntologyDAO.java


 import javax.inject.Inject;
 import javax.inject.Singleton;
+import javax.swing.text.html.Option;


I think this can be removed

timparsons · 2022-04-07T20:00:14Z

src/main/java/org/breedinginsight/model/SubscribedProgram.java

+@NoArgsConstructor
+@AllArgsConstructor
+@Builder
+public class SubscribedProgram {


Can this change to SubscribedOntology?

timparsons · 2022-04-07T20:02:36Z

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

+        // Check that shared program exists
+        Optional<ProgramSharedOntologyEntity> optionalSharedOntology = programOntologyDAO.getSharedOntologyById(sharingProgramId, programId);
+        if (optionalSharedOntology.isEmpty()) {
+            throw new DoesNotExistException("Shared ontology between specified programs was not found.");


Thoughts on this throwing more of an illegal state type of exception?

Let's talk more about this over a slack huddle if that works for you? Did you have a specific status in mind? The only other one I can think of is UnprocessableEntityException but the DoesNotExistException seems to fit better since the user is requesting a specific resource and it doesn't exist.

The other UnprocessableEntityException errors are thrown when the shared record is found, but there is something else that is preventing the action.

Talked with Chris, and I'm fine for it to stay as is

timparsons · 2022-04-07T20:04:20Z

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

+        // Check that shared program exists
+        Optional<ProgramSharedOntologyEntity> optionalSharedOntology = programOntologyDAO.getSharedOntologyById(sharingProgramId, programId);
+        if (optionalSharedOntology.isEmpty()) {
+            throw new DoesNotExistException("Shared ontology between specified programs was not found.");


Same as above...thoughts on this throwing some sort of illegal state type of exception?

timparsons · 2022-04-13T20:20:34Z

src/main/java/org/breedinginsight/api/v1/controller/OntologyController.java

            HttpResponse response = HttpResponse.status(HttpStatus.UNPROCESSABLE_ENTITY).body(e.getErrors());
            return response;
        } catch (UnprocessableEntityException e) {
+            log.error(e.getMessage());


Sorry to be a pain, but for these error messages, can you change this to log.error(e.getMessage(), e);, please? That way the error stack gets printed to the logs too.

Thanks for catching that! Added.

github-actions bot added the feature label Apr 5, 2022

ctucker3 force-pushed the feature/BI-1366 branch from c74dd84 to 4088d20 Compare April 6, 2022 15:21

ctucker3 marked this pull request as ready for review April 6, 2022 15:36

ctucker3 requested review from dmeidlin and timparsons April 6, 2022 15:38

ctucker3 assigned dmeidlin Apr 6, 2022

timparsons requested changes Apr 7, 2022

View reviewed changes

dmeidlin approved these changes Apr 8, 2022

View reviewed changes

ctucker3 requested a review from timparsons April 11, 2022 14:27

timparsons requested changes Apr 13, 2022

View reviewed changes

ctucker3 requested a review from timparsons April 19, 2022 13:49

timparsons approved these changes Apr 19, 2022

View reviewed changes

Chris T added 8 commits April 19, 2022 16:06

shared ontology subscribe: subscribe and unsubscribe from ontology

f329cf6

subscribe ontology: change unsubscribe to DELETE call

4744f4b

subscribe ontology: change permissions to breeder

bcc8ee7

subscribe ontology: endpoints and fixes

0ffd4ba

subscribe ontology: add editable trait to subscribe options endpoint

697649e

subscribe ontology: rename models

ddfa722

subscribe ontology: PR changes

10cba57

subscribe ontology: add error to logging

dae5604

ctucker3 force-pushed the feature/BI-1366 branch from 664ed2f to dae5604 Compare April 19, 2022 20:06

ctucker3 merged commit 3a51d92 into develop Apr 19, 2022

ctucker3 deleted the feature/BI-1366 branch April 19, 2022 20:36

Conversation

ctucker3 commented Apr 5, 2022

Description

Dependencies

Testing

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants