Skip to content

feat: Add full names in the description of dynamic events.#10251

Merged
PeerRich merged 21 commits into
calcom:mainfrom
jyellapu:jyellapu/feat/add-full-names-in-the-description
Jul 27, 2023
Merged

feat: Add full names in the description of dynamic events.#10251
PeerRich merged 21 commits into
calcom:mainfrom
jyellapu:jyellapu/feat/add-full-names-in-the-description

Conversation

@jyellapu

Copy link
Copy Markdown
Contributor

What does this PR do?

Updates description of only dynamic events with full names of the attendees.

Sample screenshots:

  • Dynamic events:
    With two users:
    image

With more than two users:
image

*Normal event:
image

Fixes # (issue)
Fixes #10194
/claim #10194

Requirement/Documentation

  • If there is a requirement document, please, share it here.
  • If there is ab UI/UX design document, please, share it here.

Type of change

  • New feature (non-breaking change which adds functionality)

How should this be tested?

Create dynamic events with 2 or more attendees, the description on booker should have full names of the attendees, if number of attendees is more than 2 then it should show first two attendees full names with more at the end.

  • Are there environment variables that should be set?
  • What are the minimal test data to have?
  • What is expected (happy path) to have (input and output)?
  • Any other important info that could help to test that PR

Mandatory Tasks

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

Checklist

  • I haven't read the contributing guide
  • My code doesn't follow the style guidelines of this project
  • 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

vercel Bot commented Jul 19, 2023

Copy link
Copy Markdown

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 8:46am

@vercel

vercel Bot commented Jul 19, 2023

Copy link
Copy Markdown

@jyellapu 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 commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

Thank you for following the naming conventions! 🙏

@github-actions github-actions Bot added event-types area: event types, event-types ui area: UI, frontend, button, form, input ⚡ Quick Wins A collection of quick wins/quick fixes that are less than 30 minutes of work ✨ feature New feature or request 💎 Bounty A bounty on Algora.io labels Jul 19, 2023
…u/feat/add-full-names-in-the-description
@github-actions

github-actions Bot commented Jul 19, 2023

Copy link
Copy Markdown
Contributor

📦 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! 🙌

Comment thread packages/features/bookings/Booker/components/EventMeta.tsx Outdated
@PeerRich

Copy link
Copy Markdown
Member

would this also reflect in the calendar and emails correctly?

@PeerRich

Copy link
Copy Markdown
Member

can you make a test booking and show the email template and calendar entry?

@PeerRich PeerRich added the Low priority Created by Linear-GitHub Sync label Jul 19, 2023

@alishaz-polymath alishaz-polymath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for raising this PR 🙏

There are still places which contain the old description. The changes look to be only counting towards the booking availability page.

For instance, even the dynamic event list, and the success page have the old description. Potentially, even emails and events created in connected calendars would have the old description.

image

image

@jyellapu

Copy link
Copy Markdown
Contributor Author

Sure, I'm running mailhog locally but getting 'SEND_BROKEN_INTEGRATION_ERROR Error: Missing credentials for "PLAIN"'
error, debugging the setup.

Bookings page:
image

image

@alishaz-polymath

Copy link
Copy Markdown
Member

We have a guide for Setting up Mailtrap for local email debugging. You can use that if that helps. Ideally, we want the connected calendars & emails to show event name as the new description. That's where it matters the most.

@jyellapu

Copy link
Copy Markdown
Contributor Author

@alishaz-polymath In the screenshots you shared above, it is event title which we are displaying.

@jyellapu

Copy link
Copy Markdown
Contributor Author

@alishaz-polymath I'm going through the code, the issue seems to be since dynamic events doesn't have eventId but they just have slug, thus whenever eventId is missing in the code it is getting default event with that event's slug, as default events are predefined in the code, thus the title we see above is coming from predefined default event.
Ref: https://github.com/calcom/cal.com/blob/main/packages/features/bookings/lib/handleNewBooking.ts#L692

Could you confirm once again, what is expected in the email templates and in the calendar events?

@PeerRich

PeerRich commented Jul 19, 2023

Copy link
Copy Markdown
Member

@alishaz-polymath i have another idea, what if we make a mandatory input called "title of this call" that has a max. char limit and we use that input as the title for the meeting?
image

IMO we can even have this empty here:

image

and only focus on title in calendar and confirmation email (based on the user input)

@alishaz-polymath

Copy link
Copy Markdown
Member

@alishaz-polymath i have another idea, what if we make a mandatory input called "title of this call" that has a max. char limit and we use that input as the title for the meeting? image

IMO we can even have this empty here:

image

and only focus on title in calendar and confirmation email (based on the user input)

I agree. That looks a lot better. ♥️♥️

@jyellapu

Copy link
Copy Markdown
Contributor Author

@alishaz-polymath i have another idea, what if we make a mandatory input called "title of this call" that has a max. char limit and we use that input as the title for the meeting? image

IMO we can even have this empty here:

image

and only focus on title in calendar and confirmation email (based on the user input)

Yes it was good one, that way user was given flexibility to describe the booking.

@alishaz-polymath alishaz-polymath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor changes requested. 🙏
Also, please ensure the Type Checks don't fail.

Comment thread apps/web/pages/booking/[uid].tsx Outdated
Comment thread apps/web/pages/booking/[uid].tsx
Comment thread packages/features/bookings/Booker/components/BookEventForm/BookEventForm.tsx Outdated
@alishaz-polymath

Copy link
Copy Markdown
Member

@PeerRich Are we happy with no description? The PR essentially allows booker to create a title for the event being booked, but the description is empty.

@PeerRich

PeerRich commented Jul 24, 2023

Copy link
Copy Markdown
Member

@PeerRich Are we happy with no description? The PR essentially allows booker to create a title for the event being booked, but the description is empty.

yes i think no description is fine for the booking page as long as we have a required title and the title of the calendar entry looks good

@alishaz-polymath

Copy link
Copy Markdown
Member

Hey @jyellapu just FYI, I'll review this today 🙏

@emrysal

emrysal commented Jul 26, 2023

Copy link
Copy Markdown
Contributor

@alishaz-polymath I merged #9931 which also removes the description; it condenses all dynamic events into one with multiple durations.

@alishaz-polymath

Copy link
Copy Markdown
Member

@alishaz-polymath I merged #9931 which also removes the description; it condenses all dynamic events into one with multiple durations.

I see. Let me test after update locally to see how it changes things for this PR (if it does). Thanks for the update @emrysal 🙏

@alishaz-polymath alishaz-polymath left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me, works well 🚀
Thank you for the PR @jyellapu 🙏

@PeerRich

Copy link
Copy Markdown
Member

misclicked. merging!

@PeerRich PeerRich enabled auto-merge (squash) July 27, 2023 08:42
@PeerRich PeerRich merged commit 98a1563 into calcom:main Jul 27, 2023
sean-brydon pushed a commit that referenced this pull request Jul 31, 2023
* Add full names in the descripiton of dynamic events.

* Initialising dynamic event desc after the check.

* Revert "Initialising dynamic event desc after the check."

This reverts commit ddd89c4.

* Revert "Add full names in the descripiton of dynamic events."

This reverts commit ce232d6.

* Added booking title for dynamic events.

* Update apps/web/public/static/locales/en/common.json

* Fix typo.

* Fix lint error.

* Fix redundant booking information.

* Fix type script lint.

* Addressed review suggestions.

---------

Co-authored-by: Peer Richelsen <peer@cal.com>
Co-authored-by: Syed Ali Shahbaz <52925846+alishaz-polymath@users.noreply.github.com>
Co-authored-by: Peer Richelsen <peeroke@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

💎 Bounty A bounty on Algora.io event-types area: event types, event-types ✨ feature New feature or request Low priority Created by Linear-GitHub Sync ⚡ Quick Wins A collection of quick wins/quick fixes that are less than 30 minutes of work ui area: UI, frontend, button, form, input

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-2189] change: "Dynamic Collective 60min Event" to "Firstname, Firstname & Firstname"

4 participants