Skip to content

Dynamic Links re-integrate with availability logic#3687

Merged
kodiakhq[bot] merged 54 commits intomainfrom
fix/dynamic-link-relaunch
Aug 12, 2022
Merged

Dynamic Links re-integrate with availability logic#3687
kodiakhq[bot] merged 54 commits intomainfrom
fix/dynamic-link-relaunch

Conversation

@alishaz-polymath
Copy link
Copy Markdown
Member

@alishaz-polymath alishaz-polymath commented Aug 4, 2022

What does this PR do?

This PR re-integrates the Dynamic Group booking feature with the new availability Logic, allowing us to use dynamic group bookings again 🚀

Fixes #3409

Environment: Staging(main branch)

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?

  • Try out the dynamic booking all the way through

@vercel
Copy link
Copy Markdown

vercel Bot commented Aug 4, 2022

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

Name Status Preview Updated
cal ❌ Failed (Inspect) Aug 12, 2022 at 7:29PM (UTC)
cal-com ❌ Failed (Inspect) Aug 12, 2022 at 7:29PM (UTC)
nightly-cal ❌ Failed (Inspect) Aug 12, 2022 at 7:29PM (UTC)
1 Ignored Deployment
Name Status Preview Updated
swagger ⬜️ Ignored (Inspect) Aug 12, 2022 at 7:29PM (UTC)

@alishaz-polymath alishaz-polymath added this to the v.1.9 milestone Aug 5, 2022
@alishaz-polymath alishaz-polymath added the 🧹 Improvements Improvements to existing features. Mostly UX/UI label Aug 5, 2022
@alishaz-polymath alishaz-polymath self-assigned this Aug 5, 2022
@alishaz-polymath
Copy link
Copy Markdown
Member Author

@alishaz-polymath I think it would be a good idea to add jest tests for getSchedule for dynamic logic. https://github.com/calcom/cal.com/blob/5165c511f5b060fff617f85ba2a6ca3d2042ae3c/apps/web/test/lib/getSchedule.test.ts

I agree. I have that planned as a follow up which speeds up the re-integration of dynamic links while allowing time for the edge-case availability issues to be resolved in the mean time.

Copy link
Copy Markdown
Contributor

@emrysal emrysal left a comment

Choose a reason for hiding this comment

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

Just minor comments, looking good 👍

Comment thread apps/web/pages/api/book/event.ts Outdated
Comment thread apps/web/pages/api/book/event.ts Outdated
Comment thread packages/trpc/server/routers/viewer/slots.tsx Outdated
@alishaz-polymath
Copy link
Copy Markdown
Member Author

Just minor comments, looking good 👍

All make sense. I had missed them out somehow, had put them in place to narrow-down the typesafety issues 😅
resolved them now 🙏

Copy link
Copy Markdown
Contributor

@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.

Nice work so far @alishaz-polymath have some nitpicks and Qs here and there. Will block merge in the meantime. 🙏

Comment thread apps/web/pages/api/book/event.ts Outdated
Comment thread packages/core/getBusyTimes.ts Outdated
Comment thread packages/lib/defaultEvents.ts Outdated
Comment thread packages/lib/defaultEvents.ts Outdated
Comment thread packages/trpc/server/routers/viewer/slots.tsx Outdated
Comment thread packages/trpc/server/routers/viewer/slots.tsx Outdated
Comment thread packages/trpc/server/routers/viewer/slots.tsx Outdated
const users = await ctx.prisma.user.findMany({
where: {
username: {
in: usernameList as string[],
Copy link
Copy Markdown
Contributor

@zomars zomars Aug 11, 2022

Choose a reason for hiding this comment

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

Can this be inferred? I see that it's coming from input which should be already typed from a zod schema

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since the usernameList can be a null array as well, if we don't do this, it'll throw a type error stating Type 'null' is not assignable to type 'string', even though it'll only ever be called when usernameList isn't null. This just clears out that type error.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I see that it's coming from input which should be already typed from a zod schema

So yeah, and the zod schema has it as nullable array, hence the typecasting

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

But... do we accept null arrays as inputs? If we don't let's make it not optional or non-nullable. That's the point of validating. We need to check or filter our nulls anyways don't we?

Comment thread packages/trpc/server/routers/viewer/slots.tsx Outdated
Comment thread packages/trpc/server/routers/viewer/slots.tsx Outdated
}, [telemetry]);

// get dynamic user list here
const userList = eventType.users.map((user) => user.username).filter(notEmpty);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Filtered out empty usernames instead of accepting null values

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge ♻️ autoupdate tells kodiak to keep this branch up-to-date core area: core, team members only 🧹 Improvements Improvements to existing features. Mostly UX/UI

Projects

No open projects
Status: Done

Development

Successfully merging this pull request may close these issues.

Integrate dynamic group links with the new availability

5 participants