Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: weekly limits #10404

Merged
merged 3 commits into from
Jul 27, 2023
Merged

fix: weekly limits #10404

merged 3 commits into from
Jul 27, 2023

Conversation

nicktrn
Copy link
Contributor

@nicktrn nicktrn commented Jul 26, 2023 •
edited
Loading

What does this PR do?

This addresses two bugs related to weekly events, relating to:

  1. All non-variable length event types
  2. Weeks spanning two months

Fixes #10361
Fixes #10402

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How should this be tested?

  1. Fixed length events
  • Limit a 30min event to 30min per week
  • Book once
  • See that the entire week is blocked
  1. Week in two months
  • Limit any event to 1 booking (or 1 length of event) per week
  • Book in week that ends in next month, e.g. Mon 31 Jul
  • See that you are unable to book 1-5 Aug

Availability and booking page tests are passing, but might make sense to add a new test to prevent regressions. Bug (2) was probably always there, but not (1).

Mandatory Tasks

  • Make sure you have self-reviewed the code. A decent size PR without self-review might be rejected.

Checklist

  • I haven't added tests that prove my fix is effective or that my feature works

@vercel
Copy link

vercel bot commented Jul 26, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 27, 2023 11:08am

@vercel
Copy link

vercel bot commented Jul 26, 2023

@nicktrn is attempting to deploy a commit to the cal Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added bookings area: bookings, availability, timezones, double booking High priority Created by Linear-GitHub Sync linear Sync Github Issue from community members to Linear.app 🐛 bug Something isn't working labels Jul 26, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2023 •
edited
Loading

Thank you for following the naming conventions! 🙏

nicktrn
nicktrn commented Jul 26, 2023
View reviewed changes
Copy link
Contributor Author

@nicktrn nicktrn left a comment

Choose a reason for hiding this comment

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

Ready for review

packages/core/getUserAvailability.ts
Comment on lines -168 to +171
startTime: dateFrom.toISOString(),
endTime: dateTo.toISOString(),
// needed to correctly apply limits (weeks can be part of two months)
startTime: dateFrom.startOf("week").toISOString(),
endTime: dateTo.endOf("week").toISOString(),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this affects performance negatively, it would have to be pulled into both limit functions separately, or called with startOf/endOf only if any weekly limits are present.

@nicktrn nicktrn changed the title Fix weekly limits fix: weekly limits Jul 26, 2023
@nicktrn nicktrn marked this pull request as ready for review July 26, 2023 16:09
@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2023 •
edited
Loading

📦 Next.js Bundle Analysis for @calcom/web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

This PR introduced no changes to the JavaScript bundle! 🙌

@nicktrn nicktrn mentioned this pull request Jul 26, 2023
Closed
Udit-takkar
Udit-takkar reviewed Jul 27, 2023
View reviewed changes
Copy link
Contributor

@Udit-takkar Udit-takkar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Screenshot 2023-07-27 at 5 20 15 PM

If i have availability on all days then if i book on 13th july then sunday to saturday(of next week) gets blocked. I expected monday to sunday getting blocked ( 7th july - 13th July).

@nicktrn
Copy link
Contributor Author

nicktrn commented Jul 27, 2023

If i have availability on all days then if i book on 13th july then sunday to saturday(of next week) gets blocked. I expected monday to sunday getting blocked ( 7th july - 13th July).

Seems unintuitive, but I think that was the case before. AFAIK, start of week is timezone dependent in dayjs. Is there a custom start of week option for users?

Udit-takkar
Udit-takkar approved these changes Jul 27, 2023
View reviewed changes
Copy link
Contributor

@Udit-takkar Udit-takkar left a comment

Choose a reason for hiding this comment

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

LGTM

@nicktrn i just checked that i had set start of the week to sunday in settings http://localhost:3000/settings/my-account/general. so yeah this works fine

@Udit-takkar Udit-takkar requested a review from emrysal July 27, 2023 12:07
@emrysal emrysal merged commit 1d5b383 into calcom:main Jul 27, 2023
28 of 29 checks passed
@nicktrn nicktrn deleted the fix-weekly-limits branch July 27, 2023 16:55
@qu8n
Copy link

qu8n commented Jul 27, 2023

Thanks, @nicktrn 🙏. I assume that this fix has been deployed to cal.com?

@nicktrn
Copy link
Contributor Author

nicktrn commented Jul 27, 2023

@qu8n no worries, glad I could help!

I assume that this fix has been deployed to cal.com?

Not quite yet, but keep an eye on releases, should only be a few days max. In the meantime, limiting to 1 booking per week (not via duration) should work fine for weeks in the middle of the month.

sean-brydon pushed a commit that referenced this pull request Jul 31, 2023
@nicktrn @Udit-takkar @sean-brydon
fix: weekly limits (#10404)
bbcbadb 
* fix: correctly apply duration limits

* fix: weekly limits for weeks in two months

---------

Co-authored-by: Udit Takkar <[email protected]>
@CarinaWolli CarinaWolli mentioned this pull request Jul 31, 2023
Closed
2 tasks
@nicktrn nicktrn mentioned this pull request Aug 2, 2023
Merged
3 tasks
emrysal added a commit that referenced this pull request Aug 2, 2023
@emrysal
Revert "fix: weekly limits (#10404)"
83678df 
This reverts commit 1d5b383.
emrysal added a commit that referenced this pull request Aug 2, 2023
@emrysal
Revert "fix: weekly limits (#10404)"
ae3c169 
This reverts commit 1d5b383.
emrysal added a commit that referenced this pull request Aug 2, 2023
@emrysal
Revert "Revert "fix: weekly limits (#10404)""
bf050b4 
This reverts commit 83678df.
emrysal added a commit that referenced this pull request Aug 2, 2023
@emrysal
Revert "Revert "fix: weekly limits (#10404)""
7dae176 
This reverts commit 83678df.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Udit-takkar Udit-takkar Udit-takkar approved these changes

@emrysal emrysal Awaiting requested review from emrysal

Assignees
No one assigned
Labels
bookings area: bookings, availability, timezones, double booking 🐛 bug Something isn't working High priority Created by Linear-GitHub Sync linear Sync Github Issue from community members to Linear.app
Projects
None yet
Milestone
No milestone
4 participants
@nicktrn @qu8n @Udit-takkar @emrysal