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.

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

improve the recape email design #10195

Merged
merged 27 commits into from
Jan 14, 2025
Merged

Conversation

KaleemNeslit
Copy link
Collaborator

@KaleemNeslit KaleemNeslit commented Dec 10, 2024

Link to Issue

Closes: #10139

Description of Changes

  • Updated the email subject
  • Remove hardcoded rows from Your Discussions and Governance sections
  • Make sections only appear if more than 2 notifications/rows exist for the section
  • Update date format for readability
  • created the new page and route to unsubscribe the emails
  • show the modal with unsubscribe options
    **Backend **
  • created the new command [UnSubscribeEmail.command.ts]
    -command are unSecure as user maynot be login when unsubscribe the emails
    -Updated the email_notifications_enabled key|Filed from models.SubscriptionPreference
  • created the UUID for every user so it will always be unique .
    -update the [GetRecapEmailData.query.ts] also sending the user email_notifications_enabled Preference in response
    -added the new condition in Know workflow for email_notifications_enabled

"How We Fixed It"

  • Updated the email subject
  • Remove hardcoded rows from Your Discussions and Governance sections
  • Make sections only appear if more than 2 notifications/rows exist for the section
  • Update date format for readability
  • created the new page and route to unsubscribe the emails
  • show the modal with unsubscribe options
    **Backend **
  • created the UUID for every user so it will always be unique .
  • created the new command [UnSubscribeEmail.command.ts]
    -command are unSecure as user maynot be login when unsubscribe the emails
    -Updated the email_notifications_enabled key|Filed from models.SubscriptionPreference
    -update the [GetRecapEmailData.query.ts] also sending the user email_notifications_enabled Preference in response
    -added the new condition in Know workflow for email_notifications_enabled

**Notes **
Currently every user has email_notifications_enabled false in DB maybe we need to update that to true

Test Plan

pnpm -F commonwealth start
Run RabbitMQ in Docker
pnpm -F commonwealth start-message-relayer
pnpm -F commonwealth start-knock
Use 2 accounts to trigger thread/comment notifications
Use pnpm -F commonwealth emit-event snapshot to emit snapshot notifications
Trigger the Workflow:
Select the Email Recap workflow
Click Run a test in the top right
Select a recipient (same user ID as the user who received all the triggered notifications above)
Leave all other fields empty and hit Run test
Note that the Email Recap workflow has a condition where the email will not be sent if there are 5 or fewer notifications i.e. you need to trigger 6 notifications or the email will not be sent.

To test in development you will need to run ngrok and then update the common_api_url variable on Knock. You also need to ensure that the KNOCK_AUTH_TOKEN variable on Knock matches your local .env.

here is tested SC
image
image

@KaleemNeslit KaleemNeslit marked this pull request as draft December 10, 2024 15:15
@KaleemNeslit KaleemNeslit marked this pull request as ready for review December 11, 2024 12:29
Copy link
Contributor

@masvelio masvelio left a comment

Choose a reason for hiding this comment

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

FE code looks good to me, didnt test it tho

Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

Nice first foray into the backend! Left a few comments about things that you couldn't be expected to know since they are mainly "domain" knowledge about our system. Let me know if you want to huddle for better explanations.

Will test once we resolve all the issues.

libs/core/src/integration/email.schemas.ts Outdated Show resolved Hide resolved
libs/model/src/emails/GetRecapEmailData.query.ts Outdated Show resolved Hide resolved
libs/model/src/subscription/UnSubscribeEmail.command.ts Outdated Show resolved Hide resolved
libs/model/src/subscription/UnSubscribeEmail.command.ts Outdated Show resolved Hide resolved
libs/model/src/subscription/UnSubscribeEmail.command.ts Outdated Show resolved Hide resolved
libs/model/src/utils/generateUnsubscribeLink.ts Outdated Show resolved Hide resolved
libs/schemas/src/commands/subscription.schemas.ts Outdated Show resolved Hide resolved
libs/schemas/src/events/index.ts Outdated Show resolved Hide resolved
libs/schemas/src/events/index.ts Outdated Show resolved Hide resolved
@KaleemNeslit
Copy link
Collaborator Author

Hi @Rotorsoft,
I noticed you often work with test/user/user-lifecycle.spec.ts. I'm currently facing some test failures in this file after adding a new field, unsubscribe_link, to the User model.

Could you please guide me on how to address these failures? Any pointers or suggestions would be greatly appreciated.

KaleemNeslit added 2 commits December 16, 2024 22:22
Copy link
Collaborator

@timolegros timolegros left a comment

Choose a reason for hiding this comment

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

Lets update UnSubscribeEmail to UnsubscribeEmail everywhere (no need for S capitalization).

Also please merge master into this branch - this may help resolve the unit test issues.

libs/core/src/integration/email.schemas.ts Show resolved Hide resolved
libs/model/src/utils/generateUnsubscribeLink.ts Outdated Show resolved Hide resolved
libs/model/src/utils/generateUnsubscribeLink.ts Outdated Show resolved Hide resolved
libs/schemas/src/commands/subscription.schemas.ts Outdated Show resolved Hide resolved
libs/model/src/models/user.ts Show resolved Hide resolved
@KaleemNeslit
Copy link
Collaborator Author

Lets update UnSubscribeEmail to UnsubscribeEmail everywhere (no need for S capitalization).

Also please merge master into this branch - this may help resolve the unit test issues.

@timolegros still unit test is failing for me

@timolegros
Copy link
Collaborator

Just fix conflict + build + the small pending change and then good to merge.

@KaleemNeslit KaleemNeslit merged commit 4ca02f1 into master Jan 14, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Recap Email Template
5 participants