feat(recurrence): Added advanced recurrence settings #7585

BartoszJarocki · 2022-12-16T08:48:56Z

Description

Fixes #6922, #7497, #7496

Changes introduced in this PR reflect technical spec discussed here.

Removed startRecurrence and stopRecurrence mutation
Introduced updateRecurrenceSettings mutation that handles both scenarios
Migrated startTeamPrompt mutation to SDL
Introduced RRule gql scalar type
A lot of UI

TODO:

Handle tzid issue
Migration for making duration nullable - will be handled in separate PR

Demo

https://www.loom.com/share/09082094a1e14e168e6af28fe3d1d12a

Testing scenarios

Enabling recurrence from the new meeting view
- Open new meeting modal
- Select standup
- Enable recurrence by selecting various options from the Recurrence Settings dropdown
- Start the meeting, see if recurrence is active with correct options
Updating recurrence from the existing meeting view
- Prepare a meeting with recurrence enabled (first scenario)
- Open meeting options menu in the top right corner, select Edit Recurrence Settings
- Edit the recurrence settings, try different options
- Click update, see if recurrence is correctly updated
- Make sure a snackbar is displayed after updating recurrence settings
Disabling recurrence from the existing meeting view
- Prepare a meeting with recurrence enabled (first scenario)
- Open meeting options menu in the top right corner, select Edit Recurrence Settings
- Uncheck all the days
- Click update, see if recurrence was correctly disabled
- Make sure a snackbar is displayed after disabling recurrence
Enable recurrence from the meeting view
- Start new standup meeting without recurrence enabled
- Open meeting options menu in the top right corner, select Start Recurrence
- Select the recurrence settings
- Click update, see if recurrence was correctly started
- Make sure a snackbar is displayed after updating recurrence settings

Final checklist

I checked the code review guidelines
I have added Metrics Representative as reviewer(s) if my PR invovles metrics/data/analytics related changes
I have performed a self-review of my code, the same way I'd do it for any other team member
I have tested all cases I listed in the testing scenarios and I haven't found any issues or regressions
Whenever I took a non-obvious choice I added a comment explaining why I did it this way
I added the label One Review Required if the PR introduces only minor changes, does not contain any architectural changes or does not introduce any new patterns and I think one review is sufficient'
PR title is human readable and could be used in changelog

BartoszJarocki · 2022-12-16T09:39:46Z

packages/server/graphql/private/mutations/processRecurrence.ts

  const meeting = await safeCreateTeamPrompt(teamId, facilitatorId, r, dataLoader, {
    name: `${meetingSeries.title} - ${formattedDate}`,
-    scheduledEndTime: new Date(startTime.getTime() + ms(`${meetingSeries.duration}m`)),
+    scheduledEndTime: nextMeetingStartDate,


I changed the way we calculate the next meeting start date because now, as we support multiple recurrence configurations, we can't rely on fixed meeting duration. Imagine the following scenario:

The meeting is set to recur weekly on Fridays and Saturdays. IMO it's not ideal to assume that the meeting will last only 24h, and I'd expect the meeting to last the whole period before the next one starts. this means Saturday's meeting will last almost a week and will end just before Friday's meeting starts. Friday's meeting will last only 24h and will end on Saturday (just before Saturday's meeting starts)

@jmtaber129 @enriquesanchez wdyt?

I'd expect the meeting to last the whole period before the next one starts

This sounds right to me 👍

… to start team prompt mutation

… recurrence

github-actions · 2023-01-27T12:28:21Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

github-actions · 2023-01-27T14:28:34Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

github-actions · 2023-02-01T12:22:39Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

github-actions · 2023-02-07T16:22:59Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

github-actions · 2023-02-07T16:38:39Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

BartoszJarocki · 2023-02-08T14:20:26Z

@jmtaber129 except for the timezone issue this PR is ready to be reviewed. Migration to make duration nullable will be a separate PR, no need to make this one even bigger. Do you mind taking a look? thanks!

edit: I didn't notice you already approved it! sorry!

github-actions · 2023-02-08T14:24:58Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

BartoszJarocki · 2023-02-08T14:29:50Z

packages/client/components/TeamPrompt/Recurrence/RecurrenceSettings.tsx

+            //TODO: this causes rrule to provide 'Invalid Date' for the next occurrences - see https://github.com/jakubroztocil/rrule/pull/547
+            tzid: timeZone


Seems like the RRule library isn't actively maintained anymore. I propose to fork and maintain our own version of it. @mattkrick @Dschoordsch do you have any concerns or thoughts about that? this issue is the only blocker of this PR.

There was a comment yesterday from what seems to be one of the maintainers, so I wouldn't consider this unmaintained. I see no issues having our own fork with this fix if we need it right now. Maintaining a date handling library long term though sounds quite frightening to me.

yeah, will wait a little bit longer and see what happens.

jmtaber129 · 2023-02-08T14:48:17Z

packages/client/components/TeamPrompt/Recurrence/RecurrenceDayCheckbox.tsx

-export type Day = {name: DayFullName; short: DayShortName; rruleVal: Weekday; intVal: number}
+export type Day = {
+  name: DayFullName
+  med: string


+1 The name med is somewhat ambiguous. Maybe we call this shortName, and rename short to abbreviation?

I like that! will do.

github-actions · 2023-02-08T14:54:12Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

BartoszJarocki · 2023-02-08T14:59:06Z

@mattkrick do you mind taking a look? thanks!

mattkrick

all pretty minor stuff. lookin ggggreat!

mattkrick · 2023-02-08T20:54:24Z

packages/client/components/TeamPrompt/Recurrence/RecurrenceSettings.tsx

+  const [recurrenceInterval, setRecurrenceInterval] = React.useState(
+    recurrenceRule ? recurrenceRule.options.interval : 1
+  )
+  const [recurrenceDays, setRecurrenceDays] = React.useState<Day[]>(


+1 if we're suggesting 6:00AM tomorrow down below, should we pick a default day for them here? If we only want to select a single day, perhaps we pick today's date & time & assume they'll want to run the same meeting in a week?

we're suggesting 6:00 AM, not necessarily tomorrow. setting next day in code is a safeguard to not create any meeting automatically the same day user starts the meeting manually (ie. via start new meeting modal while setting up recurrence).

current recurrence behavior is the following, new meeting is started immediately after clicking, a new one will be started according to recurrence rule, but not faster than tomorrow. ie. we don't want to start a meeting today, and also automatically start a new meeting on the same day. couple of examples below

It's 11 PM and user starts a meeting with recurrence enabled for all days with time 6:00 AM selected - new meeting starts immediately, next one will be started automatically at 6:00 AM tomorrow (and the previous one automatically closed)

It's Monday, 11:00 PM and user starts a meeting with recurrence enabled for Fridays with 6:00 AM selected - new meeting starts immediately, next one will be started automatically on Friday at 6:00 AM (and the previous one closed)

@matt do you have any concerns about this behavior? isn't it clear how it works when you look at the UI for the first time? if not, how would you improve that?

mattkrick · 2023-02-08T20:55:59Z

packages/client/components/TeamPrompt/Recurrence/RecurrenceSettings.tsx

+      ? recurrenceRule.options.dtstart
+      : dayjs()
+          .add(1, 'day')
+          .set('hour', 6)


+1 Please let's not encourage 6AM meetings 😅 Maybe 9AM, or whatever their current time is rounded to the nearest 30 mins? We do something similar here when we suggest adding the meeting to their calendar

parabol/packages/client/utils/makeCalendarInvites.ts

Line 12 in c70d47b

const newTime = roundDateToNearestHalfHour(new Date(createdAt.getTime() + ms('7d')))

EDIT: ohhh i think this is when the meeting opens, not necessary when it starts! If so, could we add a little note about that? If people set it for their actual meeting start time, it'd be a shame if no one can join 15 seconds early

yeah, standups are meant to be async first so there's no such thing as start time. 6AM is suggested as it's a pretty safe bet so everyone can have time to add responses. if that causes any tensions for our users, we'll improve but imo should be good as v1

mattkrick · 2023-02-08T21:01:24Z

packages/client/components/TeamPrompt/Recurrence/RecurrenceTimePicker.tsx

+  const {menuProps, onClick} = props
+  const startOfToday = new Date().setHours(0, 0, 0, 0)
+  return (
+    <Menu {...menuProps} ariaLabel={'6:00 AM'}>


-1 set ariaLabel to a description

mattkrick · 2023-02-08T21:03:19Z

packages/client/components/TeamPrompt/Recurrence/RecurrenceTimePicker.tsx

+  const {menuProps, onClick} = props
+  const startOfToday = new Date().setHours(0, 0, 0, 0)
+  return (
+    <Menu {...menuProps} ariaLabel={'6:00 AM'}>


-1 can we add a defaultActiveIdx on & scroll to the current value? For example, if the value is currently 6AM, when i open the menu that item should be highlighted & in the center so I can quickly pick 5:30 or 6:30

added defaultActiveIdx. seems like it should automatically scroll to the element using this code. but it doesn't seem to work. I played with it for a while but I can't make it work. would you mind if I handle this as a separate issue?

mattkrick · 2023-02-08T21:09:45Z

packages/server/graphql/public/mutations/updateRecurrenceSettings.ts

+
+  // TODO: now, as new meeting series could be created from a new meeting view, let's grab the first part ie. "Standup - Jan 1" will be "Standup"
+  // TODO: this is a temporary solution, we should have a better way to handle this
+  const [cleanMeetingName] = meetingName.includes('-') ? meetingName.split('-') : [meetingName]


+1 no need for the ternary, meetingName.split('-') works, or meetingName.slice(0, meetingName.indexOf('-')) plays better with typescript so you don't need to add a !

changed to using just split and TS is good with the output, no need to add a !

mattkrick · 2023-02-08T21:17:29Z

packages/client/components/TeamPrompt/Recurrence/TimeLeftBadge.tsx

@@ -25,7 +26,7 @@ export const TimeLeftBadge = (props: Props) => {
      <TeamPromptBadge onMouseEnter={openTooltip} onMouseLeave={closeTooltip} ref={originRef}>
        {fromNow} left
      </TeamPromptBadge>
-      {tooltipPortal(`Ends at ${meetingEndTimeDate.toLocaleString()}`)}
+      {tooltipPortal(`Restarts at ${dayjs(meetingEndTimeDate).format('MMM D, YYYY h:mm A')}`)}


+1 Since the day immediately follows the preposition, grammatically I believe it's "restarts on" and then ideally have "at" preceding the time.

unfortunately dayjs doesn't support custom formats so I had to call format twice, but looks much better, thank you

mattkrick · 2023-02-08T21:19:48Z

packages/client/components/TeamPrompt/Recurrence/TimeLeftBadge.tsx

@@ -25,7 +26,7 @@ export const TimeLeftBadge = (props: Props) => {
      <TeamPromptBadge onMouseEnter={openTooltip} onMouseLeave={closeTooltip} ref={originRef}>


+1 badges shouldn't show the caret when hovered. maybe user-select: none or change the cursor?

YES, I don't see anything better than cursor help which isn't ideal. I'd go with user-select: none

github-actions · 2023-02-10T13:54:21Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

github-actions · 2023-02-10T14:33:03Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

BartoszJarocki · 2023-02-10T14:38:02Z

@mattkrick fixed all the comments, would you mind taking a look? I added DO NOT MERGE as we still need to wait for final decision here

github-actions · 2023-02-11T08:41:56Z

This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR will be delayed and might be rejected due to its size.

BartoszJarocki · 2023-02-11T08:42:24Z

@mattkrick everything resolved, rrule library updated, unless you have other comments, it's ready to go 🚀

github-actions bot added the size/m label Dec 16, 2022

BartoszJarocki commented Dec 16, 2022

View reviewed changes

BartoszJarocki added 3 commits January 11, 2023 15:39

feat: added basic recurrence settings ui component; added rrule param…

96dfb24

… to start team prompt mutation

code cleanup

6b88e72

feat: next meeting start date based on rrule not fixed value

cceb2f8

BartoszJarocki force-pushed the feat/6922/recurrence-settings branch from 25f7536 to cceb2f8 Compare January 11, 2023 14:39

BartoszJarocki added 3 commits January 16, 2023 16:35

feat: added recurrence start time selector

13b8680

chore: code clenaup

70ef01d

feat: added start time dropdown label

6f6c36c

github-actions bot added size/l and removed size/m labels Jan 16, 2023

BartoszJarocki added 9 commits January 16, 2023 16:59

fix: recurrence start time menu fix

55618c2

code cleanup

b8aafc5

fix: recurrence settings init

e2534ef

fix: styles

55fa549

chore: use dayjs for utc conversion

9bfb088

feat: added update recurrence settings modal

c6ead99

fix: provide correct id and parent id for new modal

03112c0

fix: not ideal solution for not hardcoding meeting series name

7a2f127

feat: added update recurrence settings mutation instead of start/stop…

34acb03

… recurrence

github-actions bot added size/xl and removed size/l labels Jan 27, 2023

feat: show snackbar after updating recurrence settings

bb02452

This was linked to issues Jan 27, 2023

Recurrence: Show confirmation snackbar after enabling recurrence from the meeting view #7496

Closed

Recurrence: Add an option to edit recurrence settings from the meeting view #7497

Closed

Merge branch 'master' into feat/6922/recurrence-settings

ee0ff83

chore: updated update recurrence settings mutation description

afe0f8e

code cleanup

700113a

BartoszJarocki marked this pull request as ready for review February 8, 2023 14:18

fix: rrule validation

9e1fe0b

BartoszJarocki commented Feb 8, 2023

View reviewed changes

jmtaber129 approved these changes Feb 8, 2023

View reviewed changes

chore: renamed 'med' to 'shortName' and 'short' to 'abbreviation'

67a93a5

BartoszJarocki requested a review from mattkrick February 8, 2023 14:59

mattkrick requested changes Feb 8, 2023

View reviewed changes

chore: fixed aria label, set default menu item in recurrence time picker

1f4ae32

BartoszJarocki added 2 commits February 10, 2023 15:19

chore: removed ternary when creating recurring meeting name

f39dfbb

fix: language, added user select none to time left badge

efa578b

BartoszJarocki changed the title ~~feat(recurrence): Added advanced recurrence settings~~ [DO NOT MERGE] feat(recurrence): Added advanced recurrence settings Feb 10, 2023

BartoszJarocki added 2 commits February 11, 2023 09:30

fix: time left badge time format

3fb303d

chore: updated rrule library

4939a15

BartoszJarocki changed the title ~~[DO NOT MERGE] feat(recurrence): Added advanced recurrence settings~~ feat(recurrence): Added advanced recurrence settings Feb 11, 2023

Merge branch 'master' into feat/6922/recurrence-settings

510ffb9

BartoszJarocki requested a review from mattkrick February 13, 2023 13:50

mattkrick merged commit 3f9bdd8 into master Feb 13, 2023

mattkrick deleted the feat/6922/recurrence-settings branch February 13, 2023 18:16

		//TODO: this causes rrule to provide 'Invalid Date' for the next occurrences - see https://github.com/jakubroztocil/rrule/pull/547
		tzid: timeZone

		@@ -25,7 +26,7 @@ export const TimeLeftBadge = (props: Props) => {
		<TeamPromptBadge onMouseEnter={openTooltip} onMouseLeave={closeTooltip} ref={originRef}>

feat(recurrence): Added advanced recurrence settings #7585

feat(recurrence): Added advanced recurrence settings #7585

Conversation

BartoszJarocki commented Dec 16, 2022 • edited Loading

Description

Demo

Testing scenarios

Final checklist

BartoszJarocki Dec 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 27, 2023

github-actions bot commented Jan 27, 2023

github-actions bot commented Feb 1, 2023

github-actions bot commented Feb 7, 2023

github-actions bot commented Feb 7, 2023

BartoszJarocki commented Feb 8, 2023 • edited Loading

github-actions bot commented Feb 8, 2023

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

github-actions bot commented Feb 8, 2023

BartoszJarocki commented Feb 8, 2023

mattkrick 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 10, 2023

github-actions bot commented Feb 10, 2023

BartoszJarocki commented Feb 10, 2023

github-actions bot commented Feb 11, 2023

BartoszJarocki commented Feb 11, 2023

BartoszJarocki commented Dec 16, 2022 •

edited

Loading

BartoszJarocki Dec 16, 2022 •

edited

Loading

BartoszJarocki commented Feb 8, 2023 •

edited

Loading