PRO-42 Create Program Endpoints by nickpalladino · Pull Request #3 · Breeding-Insight/bi-api

nickpalladino · 2020-03-11T21:50:04Z

Includes cards PRO-42 Create program endpoints, INF-85 Create GET Species endpoints, and INF-83 Create program user endpoints. The project should build, the API documentation should be verified to be updated and in sync with the implemented endpoints, and unit tests should exist and pass that cover the implemented endpoints.

Here are the added endpoints:

GET /programs
POST /programs
GET /programs/{id}
PUT /programs/{id}
DELETE /programs/{id}

GET /species
GET /species/{id}

GET /programs/{id}/users
POST /programs/{id}/users
GET /programs/{id}/users/{userId}
PUT /programs/{id}/users/{userId}
DELETE /programs/{id}/users/{userId}

…o be able to resolve dependencies at github.

timparsons · 2020-03-16T21:46:58Z

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

+    public Species getById(UUID speciesId) throws DoesNotExistException{
+
+        SpeciesEntity speciesEntity = dao.fetchOneById(speciesId);
+
+        if (speciesEntity == null){
+            throw new DoesNotExistException("Species does not exist");
+        }
+
+        Species species = new Species(speciesEntity);
+        return species;
+    }
+
+    public Optional<SpeciesEntity> getByIdOptional(UUID speciesId) {
+
+        SpeciesEntity species = dao.fetchOneById(speciesId);
+
+        if (species == null) {
+            return Optional.empty();
+        }
+
+        return Optional.of(species);
+    }


I'd suggest combining these methods and returning Optional<Species>

pushed changes

timparsons · 2020-03-16T21:47:49Z

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

    public User getById(UUID userId) throws DoesNotExistException {

+        Optional<User> user = getByIdOptional(userId);
+
+        if (user.isEmpty()) {
+            throw new DoesNotExistException("UUID for user does not exist");
+        }
+
+        return user.get();
+    }
+
+    public Optional<User> getByIdOptional(UUID userId) {
+
        // User has been authenticated against orcid, check they have a bi account.
-        BiUser biUser = dao.fetchOneById(userId);
+        BiUserEntity biUser = dao.fetchOneById(userId);

        if (biUser == null) {
-            throw new DoesNotExistException("UUID for user does not exist");
+            return Optional.empty();
        }

-        return new User(biUser);
+        return Optional.of(new User(biUser));
    }


I'd suggest combining these methods and returning Optional<User>

Pushed changes

timparsons · 2020-03-16T21:51:25Z

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

+        return created.get();
+    }
+
+    public Optional<User> createOptional(UserRequest user) {


I'd suggest making this method private given that the create method throws an AlreadyExistsException if the user is not created, otherwise it always returns the created user.

pushed changes

nickpalladino · 2020-03-16T15:39:44Z

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

+        return species;
+    }
+
+    public Optional<SpeciesEntity> getByIdOptional(UUID speciesId) {


Should we have this return an Optional<Species> for consistency at the service interface? That was the approach I took with the ProgramUser service.

I'd say "Yes"

I think this was fixed in the commit from this morning

nickpalladino · 2020-03-17T17:22:12Z

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

+    @Secured(SecurityRule.IS_AUTHENTICATED)
+    public HttpResponse<Response<ProgramUser>> updateProgramUser(Principal principal, @PathVariable UUID programId, @PathVariable UUID userId,
+                                                                 @Valid @Body ProgramUserRequest programUserRequest) {
+        /* Add a user to a program. Create the user if they don't exist. */


Agree, will update or remove and indicate when changes have been pushed.

nickpalladino · 2020-03-17T17:24:15Z

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

+import static org.breedinginsight.dao.db.Tables.*;
+
+@Singleton
+public class ProgramDao extends org.breedinginsight.dao.db.tables.daos.ProgramDao {


ok, don't have the same problem with the ProgramUserDao but I can rename for consistency. Will indicate when changes are pushed.

nickpalladino · 2020-03-17T17:25:59Z

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

+    @Inject
+    DSLContext dsl;
+    @Inject
+    public ProgramDao(Configuration config) {
+        super(config);
+    }


ok, I'll update and indicate when changes have been pushed.

nickpalladino · 2020-03-17T17:32:55Z

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

+        SelectOnConditionStep<Record> query = dsl.select()
+                .from(PROGRAM)
+                .join(SPECIES).on(PROGRAM.SPECIES_ID.eq(SPECIES.ID))
+                .leftJoin(createdByUser).on(PROGRAM.CREATED_BY.eq(createdByUser.ID))


@ctucker3 should be fine, let me know when the db is updated and I can update mine.

nickpalladino · 2020-03-17T17:42:37Z

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

+                user.setUser(User.parseSQLRecord(records.get(0)));
+
+                // populate roles
+                for (Record record : records) {


@timparsons What is the rationale for splitting into two database calls?

nickpalladino · 2020-03-17T17:47:59Z

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

+        programEntity.setAbbreviation(programRequest.getAbbreviation());
+        programEntity.setObjective(programRequest.getObjective());
+        programEntity.setDocumentationUrl(programRequest.getDocumentationUrl());
+        programEntity.setUpdatedAt(OffsetDateTime.now());


sounds good

nickpalladino · 2020-03-17T18:01:57Z

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

+                    optUser = userService.createOptional(userRequest);
+                    if (optUser.isEmpty()) {
+                        throw new AlreadyExistsException("Cannot create new user, email already exists");
+                    } else {
+                        user = optUser.get();
+                    }


Was trying to avoid having exceptions thrown that would make the code harder to follow but in this case it's not bad since there's no need for any flow control based on the exception generation, just not as explicit where the exception is coming from without looking at the called function.

nickpalladino · 2020-03-17T18:08:19Z

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

+        List<UUID> roleIds = programUserRequest.getRoles().stream().map(role -> role.getId()).collect(Collectors.toList());
+
+        if (hasDuplicatesRoleIds(roleIds)) {
+            throw new AlreadyExistsException("Duplicate roles specified, must be unique");
+        }


So you don't think we should send an error status back when the client makes a request with duplicate roles, just silently handle that as ok? If that is the case, I agree about the set, but thought that should be an error condition.

…ptions

timparsons · 2020-03-24T21:30:24Z

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

+            String orcid = principal.getName();
+            User user = userService.getByOrcid(orcid);
+            Program program = programService.create(programRequest, user);
+            Response<Program> response = new Response(program);
+            return HttpResponse.ok(response);
+        } catch (DoesNotExistException e){
+            log.info(e.getMessage());
+            return HttpResponse.status(HttpStatus.NOT_FOUND, e.getMessage());


For the future, it would make the code cleaner to split out the try/catch blocks for service calls from different services. In this example, having the try/catch for fetching the User by their ORCID iD as a separate block from creating the program would make it clearer that the DoesNotExistException is thrown by UserService#getByOrcid and not by ProgramService#create

1 suggested change (having UserService#fetchByOrcid return Optional<User>) that should be addressed before merge. Left a few other comments for general discussion, but not blocking of approval of the merge. I'm going to approve now even with the suggested change.

Added card as discussed.

timparsons · 2020-03-24T21:36:05Z

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

-    private UserDao dao;
+    private UserDAO dao;

    public User getByOrcid(String orcid) throws DoesNotExistException {


I think this should return Optional<User> just as getById does

Added card as discussed.

timparsons · 2020-03-24T21:42:58Z

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

+    public List<Role> getRolesByIds(List<UUID> roleIds) {
+        List<RoleEntity> roleEntities = dao.fetchById(roleIds.stream().toArray(UUID[]::new));
+
+        List<Role> roles = new ArrayList<>();
+        for (RoleEntity roleEntity: roleEntities){
+            roles.add(new Role(roleEntity));
+        }
+        return roles;
+    }


What if a role id doesn't exist? Should the service verify that and notify the caller if the list of roles returned from the DB doesn't contain all the roles requested by ID?

This is currently checked by the only caller in ProgramUserService#validateAndGetRoles. In the future if this method is used by other callers we could evaluate whether the logic should be moved into the RoleService or by the callers.

timparsons · 2020-03-24T21:46:50Z

src/main/resources/db/migration/V0.5.2__populate-user-data.sql

-insert into bi_user (orcid, name, email) values ('0000-0002-7156-4503', 'Nick Palladino', 'nicksandbox@mailinator.com');
-insert into bi_user (orcid, name) values ('0000-0003-0437-8310', 'BI-DEV Admin');
-insert into bi_user (id, orcid, name, email) values ('74a6ebfe-d114-419b-8bdc-2f7b52d26172', '1111-2222-3333-4444', 'Test User', 'test@test.com');
+insert into bi_user (id, orcid, name, created_by, updated_by)


Is it worth having a "system user" that's not a real person? I.e. "BI-Admin"

Yeah good question. For now it is a placeholder, we haven't really discussed what the start up of the application looks like for other groups, and I imagine that this starting user is going to be in that discussion.

I could see an admin user that is the first user in any system that gets spun up. In the test isolation task this user is made configurable by making the admin orcid an environmental variable. We will need at least one user in the system with system admin permissions starting out to get your programs and users created.

timparsons

1 suggested change (having UserService#fetchByOrcid return Optional<User>) that should be addressed before merge. Left a few other comments for general discussion, but not blocking of approval of the merge. I'm going to approve now even with the suggested change.

PRO-31 Log in via ORCID + table lookup Approved-by: Liz Woods

Chris T and others added 30 commits February 28, 2020 13:53

Created required empty endpoint methods for programs

6cf66ba

scaffold out program controller and service

ad1f5f7

Created program unit tests.

3a722fb

switch to jooq <model>Entity naming

456d1aa

committing work in progress

cf12b14

Work on program endpoints api docs

e46a4ee

query program object with foreign objects

bfc6cd1

make timestamps format correctly with jackson

4285be7

Work on program locations api docs

4713cec

Changed to postgis image for databases

a88ba59

Work on add program user

32c4a97

Added optional functions into service

74a1e44

progress update to program endpoints

451d62d

added create and edit user to program endpoints

2416893

finish up program endpoints

03c61ff

species endpoints

410d0cf

Added roles list and delete work

0df017e

Add code collapse to program tests

08b68a1

Make test objects dynamic

bd3b77a

Added valid role to tests

a951d8c

new program integration tests

2f0b16c

Split off ProgramUserService from ProgramService

521e2ce

fixing test breaking exception. Plus other program tests.

fe24c1b

Removed extraneous post programs test

b1ede5c

adding more program tests

bf42cf6

fix broken program unit tests

a896d2e

Update api docs for program endpoints

f08b559

program api doc touch ups. Program controller touch ups

f93cb6d

Changing to "pluginRepository" in order for the JOOQ codegen plugin t…

cbb7751

…o be able to resolve dependencies at github.

Work on PUT/GET program user endpoints

8a4a03e

timparsons reviewed Mar 16, 2020

View reviewed changes

Chris T added 2 commits March 17, 2020 09:27

fixed some changes from pull request comments

d42c2d0

add unprocessable exception to program endpoints.

8b81a9a

nickpalladino commented Mar 17, 2020

View reviewed changes

nickpalladino and others added 14 commits March 17, 2020 14:43

Uncommented test case

f06f605

Added handling for invalid user id in post and test case

a31ceb0

Fixed test case and added checks for program id

bcfc913

Removed inaccurate comment

c4e0ffd

Renamed DAOs to match new naming convention

714bd98

Make created_by and updated_by columns NOT NULL

fd4d112

made created_by and updated_by foreign keys in tables

5e9192c

update get id controller request to process optionals instead of exce…

4c66423

…ptions

Updated dsl injection

b76f949

fixes to program and species controllers

c90070f

Optional refactor program user by id

bf30ed6

Optional refactor for user by id

ed92e90

Allow duplicate roles

74ac47e

User creation review action

1e590ab

ctucker3 approved these changes Mar 24, 2020

View reviewed changes

timparsons reviewed Mar 24, 2020

View reviewed changes

timparsons approved these changes Mar 24, 2020

View reviewed changes

nickpalladino merged commit 6f2a522 into develop Mar 25, 2020

nickpalladino deleted the feature/PRO-42 branch March 25, 2020 16:20

nickpalladino added the feature label Aug 24, 2020

timparsons pushed a commit that referenced this pull request Oct 9, 2020

Merged in PRO-31 (pull request #3)

eefd2ce

PRO-31 Log in via ORCID + table lookup Approved-by: Liz Woods

Conversation

nickpalladino commented Mar 11, 2020

Uh oh!

timparsons Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timparsons Mar 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

timparsons left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

timparsons Mar 16, 2020 •

edited

Loading

timparsons Mar 16, 2020 •

edited

Loading