fix: Provide event rescheduling option before the event end time #15150
Conversation
|
@anikdhabal is attempting to deploy a commit to the cal Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
bot
added
booking-page
|
Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link. |
📦 Next.js Bundle Analysis for @calcom/webThis analysis was generated by the Next.js Bundle Analysis action. 🤖 Eighty-six Pages Changed SizeThe following pages changed size from the code in this PR compared to its base branch:
DetailsOnly the gzipped size is provided here based on an expert tip. First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If Any third party scripts you have added directly to your app using the The "Budget %" column shows what percentage of your performance budget the First Load total takes up. For example, if your budget was 100kb, and a given page's first load size was 10kb, it would be 10% of your budget. You can also see how much this has increased or decreased compared to the base branch of your PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this. If you see "+/- <0.01%" it means that there was a change in bundle size, but it is a trivial enough amount that it can be ignored. |
dosubot
bot
added
bookings
Graphite Automations"Add community label" took an action on this PR • (06/04/24)1 label was added to this PR based on Keith Williams's automation. "Add consumer team as reviewer" took an action on this PR • (06/04/24)1 reviewer was added to this PR based on Keith Williams's automation. |
There was a problem hiding this comment.
Wow amazing PR @anikdhabal ! Given that there is another attempt #15304 to resolve the issue I have following thoughts:
-
I see you added re-schedule policy
Only allow reschedule up to <time> before start time, but I think it's not necessary - in event-type settings -> Limits we already have "Minimum notice: N hours". So I think that if someone wants to re-schedule, then what matters is that there is N hours before the booking time so the booked person has enough time to prepare. -
For the "Allow reschedule within a grace period of after start time" - does the current code set grace period respecting event-type length, or is it fixed? Aka does 1 hour event-type has 1 hour grace period by default like in the screenshot 30 minute event has 30? Just like I noted in feat: Stop users from rescheduling ongoing bookings #15304 PR review comment, I think we should focus on not whether or not meeting already has started, but whether or not it has ended, and if it has not yet ended allow re-schedule.
Personally, I prefer PR #15304 because the linked issue's core problem is people re-scheduling booking days or weeks after it's end time, and adding grace period for re-schedule is a feature possibly in search of a problem that does not exist and only adds to the code complexity - users have not requested this, they just don't want to be re-scheduled for bookings that have expired for days not minutes or seconds.
@SomayChauhan @shirazdole what do you think?
PS Anik, once again, really amazing job on the PR itself!
|
agree 100%, |
|
Hey @supalarry, great explanation. Firstly, in this PR, I added two reschedule options as @shirazdole mentioned in the issue. One is flexible, and the other is strict. From your explanation, I understand what you want, and I will update my PR accordingly today. You mentioned that you personally prefer the other PR, but could you give this PR a chance for an updated and ready version, since I submitted this PR before the other? What do you think? Should I update the PR according to your preferences, or are you ready to proceed with it? |
Thanks for the fast response! First, I would like to get your thoughts too - I believe in what I wrote, but do you agree that the simpler approach in this case is better or that it's better to ship extra features like you have now? Shiraz wrote that we "may" have to do it, but that was not a strong YES from his side and we have to prioritize solving core issue of customers instead of shipping 100 features. If you agree, the author of PR #15304 still has not replied, so if you implement the feedback faster let's move with your PR. Once again, thanks for all the efforts they are appreciated! |
Yes, I agree with you. Thanks for your always kind words and great explanations. Let me quickly update the code so you can proceed. |
anikdhabal
changed the title
There was a problem hiding this comment.
We currently would show "Meeting is scheduled" for a meeting in the past - would be good if we can show a message "This event is expired" or something:
because now we only hide the reschedule and cancel buttons so the heading would be "This meeting is scheduled"
Do you think you can add that? Would be nice user experience improvement.
|
That would be great. And one thing that needs to be fixed is that currently, users can reschedule past bookings directly from the email. So, we should show something in the Date picker indicating that 'Reschedule is not possible or the event is expired'. |
|
@supalarry all things are up. |
There was a problem hiding this comment.
Great work again @anikdhabal !! I have left a few suggestions. Let me know if those make sense.
Functionally it is working fine but as it involves booker which is a critical piece, so we have to be extra careful with its complexity.
I am okay merging the PR without that as well. But blocking merge till you acknowledge so that it isn't merged before you see the feedback.
keithwillcode
added
the
community-interns
|
Hey @hariombalhara I will follow up to address these or make any necessary updates. |
…com#15150) * feat: Provide event rescheduling option * update * update * add checker function * refactor and update * update * fix type error * Update bookings-single-view.tsx --------- Co-authored-by: Somay Chauhan <[email protected]>
What does this PR do?
Mandatory Tasks (DO NOT REMOVE)
How should this be tested?
Checklist