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: Mails are sent to the organizer twice #11771

Closed
wants to merge 1 commit into from
Closed

fix: Mails are sent to the organizer twice #11771

wants to merge 1 commit into from

Conversation

dasJ
Copy link

@dasJ dasJ commented Oct 9, 2023

What does this PR do?

The ics package doesn't support setting the SCHEDULE-AGENT parameter, there is a PR (adamgibbons/ics#248), but it is not merged.

This is a workaround that relies on the fact that the ics package does not properly escape the name field of the organizer. In a perfect world, they would merge the PR and create a new release, but this is unfortunately not the world we live in.

Fixes # (issue)

This resolves one of the listed issues in #9485 which also affects NextCloud and SOGo users via WebDav.

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

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • 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?

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 Oct 9, 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 Oct 10, 2023 8:38am

@CLAassistant
Copy link

CLAassistant commented Oct 9, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Oct 9, 2023

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

A member of the Team first needs to authorize it.

@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 2023 •
edited
Loading

Thank you for following the naming conventions! 🙏 Feel free to join our discord and post your PR link to collect XP and win prizes!

@dasJ dasJ mentioned this pull request Oct 9, 2023
Open
@github-actions
Copy link
Contributor

github-actions bot commented Oct 9, 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! 🙌

@alishaz-polymath alishaz-polymath requested review from joeauyeung and a team October 9, 2023 15:21
@neilj
Copy link

neilj commented Oct 10, 2023

It looks like this just adds SCHEDULE-AGENT=CLIENT to the organizer. As per RFC6638, you need to set this on each attendee that the client is handling the scheduling for.

@dasJ
Copy link
Author

dasJ commented Oct 10, 2023

Fair point, I wanted to start with this but it makes sense to also set this to every attendee.

@dasJ dasJ force-pushed the main branch 2 times, most recently from 2754a2d to 659e4c8 Compare October 10, 2023 08:18
@dasJ
fix: Mails are sent to the organizer twice
67d38f3 
The `ics` package doesn't support setting the `SCHEDULE-AGENT`
parameter, there is a PR (adamgibbons/ics#248),
but it is not merged.

This is a workaround that relies on the fact that the `ics` package does
not properly escape the `name` field of the organizer. In a perfect
world, they would merge the PR and create a new release, but this is
unfortunately not the world we live in.

The `SCHEDULE-AGENT` tells the CalDav server that the invitation has
been sent by the client (which is cal in this case), preventing the
CalDav server to not send invitations itself.

refs calcom#9485
@dasJ
Copy link
Author

dasJ commented Oct 10, 2023

Since the question may come up: I tried calling the ics package as-is and replacing the attendee into the resulting string. This did not work as well as I hoped because the ical event has a line limit of 75 so either my regex doesn't match or it may produce lines that are too long.

@PeerRich PeerRich added High priority Created by Linear-GitHub Sync bookings area: bookings, availability, timezones, double booking labels Oct 11, 2023
@PeerRich PeerRich requested review from emrysal and zomars October 11, 2023 23:07
@PeerRich PeerRich added the calendar-apps area: calendar, google calendar, outlook, lark, microsoft 365, apple calendar label Oct 11, 2023
@PeerRich PeerRich requested a review from a team October 11, 2023 23:08
zomars
zomars requested changes Oct 11, 2023
View reviewed changes
Copy link
Member

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

Thank you for your contribution 🙏

Can we come up with a simple test to prevent a future regression?

@dasJ
Copy link
Author

dasJ commented Oct 13, 2023

I'd love to but I don't see how unfortunately. My typescript is not the best (which you may see from the number of pushes I did ;) and the createEvent function seems to call a lot of things that don't seem to be currently mocked.
In fact, the createEvent function (that I modify) is usually mocked in tests.
Do you have any pointers for me how to get started?

@github-actions
Copy link
Contributor

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label Oct 28, 2023
@dasJ
Copy link
Author

dasJ commented Oct 29, 2023

bruh

@alishaz-polymath
Copy link
Member

@dasJ removed from stale state, that wasn't intended I'm sure 😅
But we do need tests for it still.
@Udit-takkar can you please take a look?

@Udit-takkar
Copy link
Contributor

I am not sure about these changes @joeauyeung might be the best person to review this 

@keithwillcode keithwillcode added Medium priority Created by Linear-GitHub Sync and removed High priority Created by Linear-GitHub Sync labels Nov 10, 2023
joeauyeung
joeauyeung requested changes Nov 20, 2023
View reviewed changes
Copy link
Contributor

@joeauyeung joeauyeung left a comment

Choose a reason for hiding this comment

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

Tested this with Mailfence emails are still being sent from the CalDAV provider. Looking at the ics file, it seems like your changes aren't being applied to to the organizer and attendee fields.

ORGANIZER;CN=Free Example:mailto:[email protected]
ATTENDEE;RSVP=TRUE;ROLE=REQ-PARTICIPANT;PARTSTAT=ACCEPTED;CN=Test:mailto:*****@gmail.com

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Dec 5, 2023

This PR is being marked as stale due to inactivity.

@github-actions github-actions bot added the Stale label Dec 5, 2023
@github-actions GitHub Actions
Copy link
Contributor

This PR is being closed due to inactivity. Please reopen if work is intended to be continued.

@github-actions github-actions bot closed this Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zomars zomars zomars requested changes

@joeauyeung joeauyeung joeauyeung requested changes

@alishaz-polymath alishaz-polymath Awaiting requested review from alishaz-polymath

@emrysal emrysal Awaiting requested review from emrysal

Assignees
No one assigned
Labels
bookings area: bookings, availability, timezones, double booking calendar-apps area: calendar, google calendar, outlook, lark, microsoft 365, apple calendar Medium priority Created by Linear-GitHub Sync Stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@dasJ @CLAassistant @neilj @alishaz-polymath @Udit-takkar @zomars @joeauyeung @keithwillcode @PeerRich