Skip to content

Recurring event fixes#3030

Merged
zomars merged 11 commits intomainfrom
recurring-events-fixes
Jun 10, 2022
Merged

Recurring event fixes#3030
zomars merged 11 commits intomainfrom
recurring-events-fixes

Conversation

@zomars
Copy link
Contributor

@zomars zomars commented Jun 8, 2022

What does this PR do?

  • Incorporates recurring events into calEvent
  • Discovered false positives for recurring events that are treating normal events as recurring

TODO:

Parse all these casted values with the zod schema in zod-utils:

image

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How should this be tested?

  • Test A
  • Test B

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • I haven't performed a self-review of my own code and corrected any misspellings
  • I haven't commented my code, particularly in hard-to-understand areas
  • I haven't checked if my PR needs changes to the documentation
  • I haven't checked if my changes generate no new warnings
  • I haven't added tests that prove my fix is effective or that my feature works
  • I haven't checked if new and existing unit tests pass locally with my changes

@vercel
Copy link

vercel bot commented Jun 8, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
cal ✅ Ready (Inspect) Visit Preview Jun 9, 2022 at 9:57PM (UTC)
3 Ignored Deployments
Name Status Preview Updated
docs ⬜️ Ignored (Inspect) Jun 9, 2022 at 9:57PM (UTC)
swagger ⬜️ Ignored (Inspect) Jun 9, 2022 at 9:57PM (UTC)
ui ⬜️ Ignored (Inspect) Jun 9, 2022 at 9:57PM (UTC)

import { useMutation } from "react-query";
import { Frequency as RRuleFrequency } from "rrule";

import { parseRecurringEvent } from "@calcom/lib";
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Runtime and type safety for an unknown object

Comment on lines +4 to +13
export function isRecurringEvent(obj: unknown): obj is RecurringEvent {
const parsedRecuEvt = recurringEventSchema.safeParse(obj);
return parsedRecuEvt.success;
}

export function parseRecurringEvent(obj: unknown): RecurringEvent | null {
let recurringEvent: RecurringEvent | null = null;
if (isRecurringEvent(obj)) recurringEvent = obj;
return recurringEvent;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This way we ensure the returned data from the DB is valid or null

@zomars zomars marked this pull request as ready for review June 9, 2022 14:50
@zomars
Copy link
Contributor Author

zomars commented Jun 9, 2022

Can you please test the frontend flow and forms an make it mergable if necessary? @leog

Copy link
Contributor Author

@zomars zomars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good progress! @leog

  1. Let's address some comments I've left
  2. Let's address the failing E2E test

`${RRuleFrequency[booking.eventType.recurringEvent.freq]
.toString()
.toLowerCase()}`,
`${Frequency[booking.eventType.recurringEvent.freq].toString().toLowerCase()}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This piece of very long and repeated string throughout the app will be refactored in an util as part of #2750

const [recurringEventFrequency, setRecurringEventFrequency] = useState(
recurringEvent?.freq || RRuleFrequency.WEEKLY
);
const [recurringEventCount, setRecurringEventCount] = useState(recurringEvent?.count || 12);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplifying the logic to just rely on a state object

@leog leog self-requested a review June 9, 2022 21:52
...recurringEventState,
interval: parseInt(event?.target.value),
};
formMethods.setValue("recurringEvent", newVal);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Controller can be used here to simplify, but not needed - this is already a great PR.

reqBody.recurringEventId ? (eventType.recurringEvent as RecurringEvent) : {}
);
await sendOrganizerRequestEmail({ ...evt, additionalNotes });
await sendAttendeeRequestEmail({ ...evt, additionalNotes }, attendeesList[0]);
Copy link
Contributor

@agustif agustif Jun 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of scope for this PR, but made me think about if do we want to allow the booker (attendee[0] to select a checkbox when booking to notify ALL attendees and not him? Since we ask for emails for all attendees too?

Copy link
Contributor

@agustif agustif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work on recurring events @leog and @zomars everything looking great, will look into adopting the new sod types for recurring events into API too.
image

Oh also, left a non-review comment about possible new feature of sending emails to all attendees?

@zomars zomars merged commit b11398f into main Jun 10, 2022
@zomars zomars deleted the recurring-events-fixes branch June 10, 2022 00:32
@zomars zomars mentioned this pull request Jun 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants