feat: email unsubscribe and settings link#6340
Conversation
✅ Deploy Preview for partners-bloom-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for bloom-angelopolis canceled.
|
✅ Deploy Preview for bloom-public-seeds ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for bloom-exygy-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Some things I'm noticing, not all of which are caused by these changes, so happy to split these up if we need to! Specific to this PR, it's only the below, otherwise the redirect looks great:
- The string is
Unsubscribe and manage email settingbut it should be pluralsettings
The below are unrelated to this change, but matter for the notifications feature as a whole. I'm 100% okay with making these into a separate ticket if you would prefer.
- Locally if I create a new account and update notification settings, I'm unable to get any emails to send unless I remove
userPreferences: { sendEmailNotifications: true }from the emailUsers query. It doesn't look like that is being set anywhere - not sure if it can be removed or if it's having some side effect I'm not aware of? - I'm not able to get notifications for waitlist emails. We have a check for lottery, we also need one for waitlist.
- I'm not able to get notifications for regions in Angelopolis. I think this is because we're checking the
regionfield, but LA is usingconfigurableRegion(confusing I know). We should be fine to check both likeconst listingRegion = listing?.configurableRegion || listing?.region; - The listing template needs to show region (configurableRegion) if the value exists which was part of this ticket, but it's not displaying.
- The listing template is always showing accessible marketing flyer even if one does not exist on the listing.
- We're not using consistent naming for the accessibility types. The email should match what is in the form, which is to use
Hearing/Vision. The email should not sayHearing and Vision.
| }; | ||
| }); | ||
|
|
||
| if (listing?.region) { |
There was a problem hiding this comment.
issue: This still has the configurable region issue. An LA listing will have a null listing.region but may have a value in listing.configurableRegion.
| sendEmailNotifications: true, | ||
| }, | ||
| }, | ||
| // { TODO: consider enabling this when this flag will be available to update from user account page |
There was a problem hiding this comment.
question: Is there a future state where we expect a new flag to be available on accounts?
emilyjablonski
left a comment
There was a problem hiding this comment.
Tysm for all the updates!! It's just the remaining configurable region filtering piece.
There is also an unrelated issue I made a new ticket for, where on an open listing, if you just make a change and save it, it sends out the notification email again as if a new listing has been published.
This PR addresses #5954
Description
How Can This Be Tested/Reviewed?
setupscript to apply new migrationsUserNotificationPreferencessetUnsubscribe and manage email settingredirectUrl=/account/notificationsURL param attachedAuthor Checklist:
yarn generate:clientand/or created a migration when requiredReview Process: