Create Shift CRUD Methods and GraphQL Resolvers #55

lambo-liu · 2021-11-27T20:08:49Z

Ticket link

Closes #51

Implementation description

Implemented the following endpoints & CRUD methods for shifts:

getShifts
getShift
createShifts (creates shifts of a posting by bulk)
updateShift (updates a single shift)
updateShifts (updates shifts of a posting by bulk)
deleteShift (deletes a single shift)
deleteShifts (deletes all shifts of a posting)

Steps to test

Create a dummy posting using SQL. Here are the following SQL queries to run:
insert into branches (id, name) values (1, 'test branch');
insert into postings (id, branch_id, type, title, description, auto_closing_date, start_date, end_date) values (1, 1, 'GROUP', 'a', 'a', '2022-01-09T21:33:51.302Z', '2022-01-09T21:33:51.302Z', '2022-01-09T21:33:51.302Z');
Go to localhost:5000/graphql, and test the individual queries & resolvers.

Examples:
Create shifts by bulk, using a biweekly recurrence interval
mutation { createShifts(shifts: { postingId: "1", times: [ { date: "2021-12-16" startTime: "01:30" endTime: "04:30" }, { date: "2021-12-18" startTime: "12:00" endTime: "16:00" } ], endDate: "2022-01-15", recurrenceInterval: BIWEEKLY }) { id postingId startTime endTime } }

Update a single shift
mutation { updateShift(id: "1", shift: { startTime: "2021-12-16 04:30", endTime: "2021-12-16 06:30" }) { postingId startTime endTime } }

What should reviewers focus on?

There are lots of validations to be checked. For bulk create / update, make sure that users can only input time blocks within a 1-week timeframe.
If duplicate DateTimes persist, they should be neglected in the bulk creation process.
Create/Update procedures should only work with a valid postingId
Times should be inputted in the HH:mm format, and DateTimes should be inputted in the YYYY-MM-DD HH:mm format. Any other format should throw an error.
Check that the functionality checks out and the code is clean!
Have a good day 🌈

Checklist

My PR name is descriptive and in imperative tense
My commit messages are descriptive and in imperative tense. My commits are atomic and trivial commits are squashed or fixup'd into non-trivial commits
I have run the appropriate linter(s)
I have requested a review from the PL, as well as other devs who have background knowledge on this PR or who will be building on top of this PR

sherryhli

Great progress so far! 😄 This is a big set of changes so I'll be doing another round of review after these comments are addressed

backend/services/implementations/shiftService.ts

backend/services/interfaces/IShiftService.ts

lambo-liu · 2021-12-04T20:11:42Z

@sherryhli updated changes! Feel free to review again. Thanks! 💯

sherryhli

Almost there! I noticed an edge case when creating shifts with no recurrence interval, otherwise just some minor things.

backend/graphql/index.ts

backend/graphql/resolvers/shiftResolvers.ts

backend/graphql/types/shiftType.ts

backend/services/implementations/shiftService.ts

backend/services/interfaces/IShiftService.ts

sherryhli

One more minor comment!

lambo-liu · 2021-12-18T20:39:48Z

@sherryhli fixed! Let me know if there's anything else 👍

sherryhli

Sorry for the delay reviewing! A few very minor things left, which I can take of

backend/yarn.lock

backend/Dockerfile

backend/services/implementations/shiftService.ts

sherryhli · 2022-01-07T00:09:08Z

backend/services/implementations/shiftService.ts

+    return shiftTimes;
+  }
+
+  async checkConflictingShifts(


@LenaNguyen some context on why we check for conflicts :)

We want to avoid creating duplicate shifts (i.e. 2 shifts associated with the same posting that have the exact same start time and end time. This validation is pretty costly but unless there are significant performance issues, I think it's worthwhile to keep. We wouldn't want the database to be spammed with lots of duplicate entries (either accidentally or maliciously), it'd hurt performance and waste our row count quota in Heroku.

Not sure what is more efficient but we can also add an index to the database so that postingId, startTime, and endTime are unique.

Also, overlapping shift times are okay?

I asked product peops last month and they were OK with overlapping shift times 👍

Not sure what is more efficient but we can also add an index to the database so that postingId, startTime, and endTime are unique.

I like this idea. My feeling is that even with the write overhead of indexing, it might be more performant as it wouldn't require querying all shifts of a posting and then comparing start/end times for every shift write.

Would suggest merging this current implementation first though and doing some experiments before committing to the index approach.

LenaNguyen · 2022-01-09T17:58:08Z

backend/services/implementations/shiftService.ts

+  }
+
+  buildTimeBlocks(shifts: ShiftBulkRequestDTO): TimeBlock[] {
+    const endDate: Moment = moment(shifts.endDate, dateFormat, STRICT_MODE).add(


why do we add a day to the endDate?

This offset is made so that when we compare startTime to endDate, any time block that starts and ends on the original endDate will be added to shiftTimes. For example, if our end date is 2022-01-01, a time block that starts on that same day (2022-01-01) will be added as it passes the start < endDate check.

LenaNguyen · 2022-01-09T20:51:57Z

backend/services/implementations/shiftService.ts

+    return shiftTimes;
+  }
+
+  async checkConflictingShifts(


Not sure what is more efficient but we can also add an index to the database so that postingId, startTime, and endTime are unique.

LenaNguyen · 2022-01-09T20:54:06Z

backend/services/implementations/shiftService.ts

+    return shiftTimes;
+  }
+
+  async checkConflictingShifts(


Also, overlapping shift times are okay?

LenaNguyen · 2022-01-09T21:00:47Z

backend/services/implementations/shiftService.ts

+      const recurringShifts = [];
+      if (duration) {
+        for (
+          let start = startTime.clone(), end = endTime;


why do we copy startTime but not endTime?

I think it's safe to not clone startTime but I wasn't 100% sure, @lambo-liu can you confirm?

LenaNguyen

Create a dummy posting using SQL. Here are the following SQL queries to run:
insert into branches (id, name) values (1, 'test branch');
insert into postings (id, branch_id, type, title, purpose, responsibilitie, tasks, desired_qualities, mandatory_activities, benefits, working_conditions, num_volunteers) values (1, 1, 'GROUP', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 0);
Go to localhost:5000/graphql, and test the individual queries & resolvers.

The insert for postings in this PR description is out of date right? I tried running it and the table no longer has some of these colums.

lambo-liu · 2022-01-09T21:09:52Z

Create a dummy posting using SQL. Here are the following SQL queries to run:
insert into branches (id, name) values (1, 'test branch');
insert into postings (id, branch_id, type, title, purpose, responsibilitie, tasks, desired_qualities, mandatory_activities, benefits, working_conditions, num_volunteers) values (1, 1, 'GROUP', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 'a', 0);
Go to localhost:5000/graphql, and test the individual queries & resolvers.

The insert for postings in this PR description is out of date right? I tried running it and the table no longer has some of these colums.

Sorry Lena! Forgot to update these after Brian & Albert changed the schema for postings. Will update this now.

sherryhli · 2022-01-11T18:45:51Z

The insert for postings in this PR description is out of date right? I tried running it and the table no longer has some of these colums.

@LenaNguyen this is the updated insert:

INSERT INTO postings (branch_id, type, title, description, num_volunteers, auto_closing_date, start_date, end_date) VALUES (1, 'GROUP', 'Title', 'Description', 5, '2021-01-01', '2021-01-11', '2021-01-15');

Could you take a final pass at this PR, I think it's good to go :)

sherryhli

LGTM but let's hold off on merging until Lena approves

LenaNguyen

Looks good :))

Lambert Liu added 7 commits November 2, 2021 21:59

update shift schema model

81832ab

create CRUD shift services

5177f3b

Merge branch 'main' into shift-crud-resolvers

e13ba5b

create resolvers & fix bugs in service

895439c

fix promise & date duration check

3d86f38

wrap up shift service

f659e08

add moment validations

a819d67

lambo-liu requested review from sherryhli, albertlai431 and briantu November 27, 2021 20:09

Co-authored-by: Brian Tu <brianptu2@gmail.com>

dc71fe9

sherryhli requested changes Nov 28, 2021

View reviewed changes

fix PR review comments

9553342

lambo-liu requested a review from sherryhli December 4, 2021 20:11

sherryhli requested changes Dec 6, 2021

View reviewed changes

sherryhli mentioned this pull request Dec 7, 2021

Add VolunteerUser CRUD Methods #63

Merged

4 tasks

sherryhli reviewed Dec 8, 2021

View reviewed changes

lambo-liu force-pushed the shift-crud-resolvers branch from 37e1d33 to 9553342 Compare December 18, 2021 18:56

Lambert Liu added 3 commits December 18, 2021 14:21

Merge branch 'main' into shift-crud-resolvers

f6c8a95

Update package.json

17ce0c7

fix PR comments for shifts

33486f1

lambo-liu requested a review from sherryhli December 18, 2021 20:39

sherryhli reviewed Jan 4, 2022

View reviewed changes

sherryhli added 4 commits January 4, 2022 18:00

Remove extraneous dependencies from backend/yarn.lock

8901445

Remove libssl-dev install from backend/Dockerfile

aaf1972

Cleanup ShiftService::buildTimeBlocks

1272d81

Run bulk shift creations and updates sequentially

77d3475

sherryhli force-pushed the shift-crud-resolvers branch from 77d3475 to 08b3564 Compare January 6, 2022 23:56

sherryhli force-pushed the shift-crud-resolvers branch from 08b3564 to 77d3475 Compare January 6, 2022 23:58

sherryhli requested a review from LenaNguyen January 7, 2022 00:02

sherryhli reviewed Jan 7, 2022

View reviewed changes

LenaNguyen reviewed Jan 9, 2022

View reviewed changes

Merge branch 'main' into shift-crud-resolvers

5a7c1f9

sherryhli approved these changes Jan 11, 2022

View reviewed changes

LenaNguyen approved these changes Jan 11, 2022

View reviewed changes

lambo-liu merged commit 6763199 into main Jan 11, 2022

lambo-liu deleted the shift-crud-resolvers branch January 11, 2022 21:20

hujoseph99 mentioned this pull request Jan 16, 2022

Built out shift signup endpoints #90

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Shift CRUD Methods and GraphQL Resolvers #55

Create Shift CRUD Methods and GraphQL Resolvers #55

lambo-liu commented Nov 27, 2021 •

edited

sherryhli left a comment

lambo-liu commented Dec 4, 2021

sherryhli left a comment

sherryhli left a comment

lambo-liu commented Dec 18, 2021

sherryhli left a comment

sherryhli Jan 7, 2022

LenaNguyen Jan 9, 2022

LenaNguyen Jan 9, 2022

lambo-liu Jan 9, 2022

sherryhli Jan 11, 2022

LenaNguyen Jan 9, 2022

lambo-liu Jan 9, 2022

LenaNguyen Jan 9, 2022

LenaNguyen Jan 9, 2022

LenaNguyen Jan 9, 2022

sherryhli Jan 11, 2022

LenaNguyen left a comment

lambo-liu commented Jan 9, 2022

sherryhli commented Jan 11, 2022

sherryhli left a comment

LenaNguyen left a comment

Create Shift CRUD Methods and GraphQL Resolvers #55

Create Shift CRUD Methods and GraphQL Resolvers #55

Conversation

lambo-liu commented Nov 27, 2021 • edited

Ticket link

Implementation description

Steps to test

What should reviewers focus on?

Checklist

sherryhli left a comment

Choose a reason for hiding this comment

lambo-liu commented Dec 4, 2021

sherryhli left a comment

Choose a reason for hiding this comment

sherryhli left a comment

Choose a reason for hiding this comment

lambo-liu commented Dec 18, 2021

sherryhli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LenaNguyen left a comment

Choose a reason for hiding this comment

lambo-liu commented Jan 9, 2022

sherryhli commented Jan 11, 2022

sherryhli left a comment

Choose a reason for hiding this comment

LenaNguyen left a comment

Choose a reason for hiding this comment

lambo-liu commented Nov 27, 2021 •

edited