Skip to content
This repository has been archived by the owner on Jun 9, 2023. It is now read-only.

chore: cancel event email content #1517

Merged
merged 20 commits into from
Nov 28, 2022

Conversation

Sboonny
Copy link
Member

@Sboonny Sboonny commented Sep 9, 2022

  • I have read Chapter's contributing guidelines.
  • My pull request has a descriptive title (not a vague title like Update README.md).
  • My pull request targets the main branch of Chapter.

Ref #100

Currently, It isn't possible to run cypress locally, and it doesn't work on Gitpod, so I will delay adding test until then.

How it looks

image

the email content

The Upcoming Event for Heidenreich, Koch and Kreiger is canceled.

View Upcoming Events for Abshire - Barrows: [Abshire - Barrows chapter](https://3000-sboonny-chapter-0ge8coe9jt5.ws-eu64.gitpod.io/chapters/4).
----------------------------

- Stop receiving upcoming event notifications for Abshire - Barrows. you can do it here: https://3000-sboonny-chapter-0ge8coe9jt5.ws-eu64.gitpod.io/events/15?emaillink=true.
- More about Abshire - Barrows or to unfollow this chapter: https://3000-sboonny-chapter-0ge8coe9jt5.ws-eu64.gitpod.io/chapters/4.

----------------------------
You received this email because you subscribed to Heidenreich, Koch and Kreiger Event.

See the options above to change your notifications.

@Sboonny Sboonny requested a review from a team September 9, 2022 12:57
@gitpod-io
Copy link

gitpod-io bot commented Sep 9, 2022

@ghost
Copy link

ghost commented Sep 9, 2022

👇 Click on the image for a new way to code review
  • Make big changes easier — review code in small groups of related files

  • Know where to start — see the whole change at a glance

  • Take a code tour — explore the change with an interactive tour

  • Make comments and review — all fully sync’ed with github

    Try it now!

Review these changes using an interactive CodeSee Map

Legend

CodeSee Map Legend

server/src/controllers/Events/resolver.ts Outdated Show resolved Hide resolved
View Upcoming Events for ${event.chapter.name}: <a href='${chapterURL}'>${event.chapter.name} chapter</a>.<br />
----------------------------<br />
<br />
- Stop receiving upcoming event notifications for ${event.chapter.name}. you can do it here: <a href="${eventURL}">${eventURL}</a>.<br />
Copy link
Member

Choose a reason for hiding this comment

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

From event page, only further event notifications for that event can be changed. Seems that either wording or url needs to be changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated it to this

- Stop receiving event notifications for ${event.name}. You can do it here: <a href="${eventURL}">${eventURL}</a>.<br />

But it maybe redundant, should we delete that line?

Sboonny and others added 3 commits September 9, 2022 21:46
Co-authored-by: Krzysztof G. <60067306+gikf@users.noreply.github.com>
@gikf
Copy link
Member

gikf commented Sep 10, 2022

Out of curiosity, what's the join and leave function mentioned in comment?

@Sboonny
Copy link
Member Author

Sboonny commented Sep 10, 2022

In issue #274, there is this a mentioning of join button in this comment. It should act as indicator that you are planning to attend the Event, as far as I know

So Event Card could have a join button

@gikf
Copy link
Member

gikf commented Sep 10, 2022

That sounds like joining and leaving chapter. Joining chapter is implemented as a standalone action and also is implicitly performed when user is RSVPing for an event from not joined chapter.

Leaving chapter is not implemented.

server/src/controllers/Events/resolver.ts Outdated Show resolved Hide resolved
Comment on lines 867 to 878
const cancelEventEmail = `The Upcoming Event ${event.name} is canceled.<br />
<br />
View Upcoming Events for ${event.chapter.name}: <a href='${chapterURL}'>${event.chapter.name} chapter</a>.<br />
----------------------------<br />
<br />
- Stop receiving event notifications for ${event.name}. You can do it here: <a href="${eventURL}">${eventURL}</a>.<br />
- More about ${event.chapter.name} or to unfollow this chapter: <a href="${chapterURL}">${chapterURL}</a>.<br />
<br />
----------------------------<br />
You received this email because you Subscribed to ${event.name} Event.<br />
<br />
See the options above to change your notifications.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const cancelEventEmail = `The Upcoming Event ${event.name} is canceled.<br />
<br />
View Upcoming Events for ${event.chapter.name}: <a href='${chapterURL}'>${event.chapter.name} chapter</a>.<br />
----------------------------<br />
<br />
- Stop receiving event notifications for ${event.name}. You can do it here: <a href="${eventURL}">${eventURL}</a>.<br />
- More about ${event.chapter.name} or to unfollow this chapter: <a href="${chapterURL}">${chapterURL}</a>.<br />
<br />
----------------------------<br />
You received this email because you Subscribed to ${event.name} Event.<br />
<br />
See the options above to change your notifications.
const cancelEventEmail = `The upcoming event ${event.name} has been canceled.<br />
<br />
View upcoming events for ${event.chapter.name}: <a href='${chapterURL}'>${event.chapter.name} chapter</a>.<br />
----------------------------<br />

I think this flows better. I deleted the notifications text because it should be possible to DRY it out.

We have two types of emails

  • Ones sent in response to a user action
  • Ones sent because a user is subscribed

All emails should have the same unsubscribe text

To manage notifications about this event, go to <a href="${eventURL}">${eventURL}</a>.<br />
To manage notifications from this chapter about new events, go to <a href="${chapterURL}">${chapterURL}</a>.<br />

Emails that are sent because the user is subscribed to an event should explain that. Similarly emails that are sent because of a chapter subscription should explain that.

Finally emails that are sent in response to a user action need no explanation.

@Sboonny could you handle this in this PR? I think makes sense to handle them all together, so we can make sure they're consistent.

Copy link
Member Author

@Sboonny Sboonny Oct 24, 2022

Choose a reason for hiding this comment

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

in 0620983

I have tried to DRY it using unsubscribeOption, but the resolver didn't use user.id.

So before I add them, do we need user.id in cancel events email? although it doesn't make sense to unfollow canceled events, people may want to cancel their chapter membership.

So do we add it? or just link it?

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems simplest to always provide both of the unsubscribe links. We don't need to customise them for the users current subscription state.

Unfollowing cancelled events should mostly be pointless, but conceivably events could be uncancelled. It's safer to let users unsubscribe and respect their wishes.

Copy link
Member Author

@Sboonny Sboonny Oct 25, 2022

Choose a reason for hiding this comment

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

Sure, random question 😅, the 0620983 should have failed cypress because there were no user.id, but it succeeded. should we worry about that?

Nvm it checked for 4 tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that the tests didn't run, I'm not worried!

Copy link
Member Author

Choose a reason for hiding this comment

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

sorry for the confusion, the function meant to run without user.id, which is interesting 🤔

@Sboonny Sboonny marked this pull request as draft October 25, 2022 11:21
@Sboonny Sboonny marked this pull request as ready for review November 22, 2022 08:48
Copy link
Contributor

@ojeytonwilliams ojeytonwilliams left a comment

Choose a reason for hiding this comment

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

Hey @Sboonny I hope the suggestions make sense, but give me a shout if they don't.

server/src/util/eventEmail.ts Outdated Show resolved Hide resolved
server/src/util/eventEmail.ts Outdated Show resolved Hide resolved
server/src/util/eventEmail.ts Outdated Show resolved Hide resolved
server/src/controllers/Events/resolver.ts Outdated Show resolved Hide resolved
server/src/controllers/Events/resolver.ts Outdated Show resolved Hide resolved
Comment on lines 804 to 807
const unsubScribeOptions = NotificationContextText({
linkToEvent: eventURL,
linkToChapter: chapterURL,
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use getEventUnsubscribeOptions (and be unsubscribeOptions)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is complicated because this cancel RSVP without userId, so if we are using getEventUnsubscibeOptions, we will force user to log in

Copy link
Member Author

Choose a reason for hiding this comment

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

Should we move the text to the resolver with comment of why we are doing this?

Copy link
Contributor

Choose a reason for hiding this comment

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

We can't force users to login when they want to unsubscribe from email notifications. Fortunately, there's no need to. We have access to the userId via event_users. Since each email is different (different unsubscribe tokens) you can use batchSender to send them all. There's an example of that in sendEventInvite.

server/src/controllers/Events/resolver.ts Outdated Show resolved Hide resolved
server/src/controllers/Events/resolver.ts Outdated Show resolved Hide resolved
server/src/controllers/Events/resolver.ts Outdated Show resolved Hide resolved
@Sboonny
Copy link
Member Author

Sboonny commented Nov 28, 2022

I have merged this, if there is an issue ping me and if it's breaking the site revert it and ping me to fix it 👍

@Sboonny Sboonny deleted the chore/cancel-event-email-content branch November 28, 2022 19:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants