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

Add ability to send transactional emails #3272

Merged
merged 1 commit into from
Jul 11, 2024

Conversation

mathjazz
Copy link
Collaborator

@mathjazz mathjazz commented Jul 3, 2024

Fix #3268. Add ability to send transactional emails (spec).

For now, the only difference between transactional and non-transactional emails is that the Unsubscribe footer is appended to the latter.

I've made the "Transactional" flag only visible when the Email option is checked. This patterns prevents selecting the Transactional option when Email option is not checked and makes the extra validation required by the spec redundant.

Deployed to stage for testing:
https://mozilla-pontoon-staging.herokuapp.com/messaging/

@mathjazz mathjazz requested review from eemeli and bcolsson July 3, 2024 20:17
Copy link
Member

@eemeli eemeli left a comment

Choose a reason for hiding this comment

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

Changes here look fine.

I had not looked at the UI for this before, is this intended as final?
Screenshot 2024-07-04 at 11 32 05

Some comments:

  • It's not at all obvious that the 🚫 is a checkbox indicator; to me this seems to be saying "Sending notifications and emails is not available". Having some different indicator for an unselected option could be much clearer.
  • The #1 and #2 parts of the title don't seem to mean anything?
  • Is the "Message editor" title necessary?
@mathjazz
Copy link
Collaborator Author

mathjazz commented Jul 4, 2024

I had not looked at the UI for this before, is this intended as final?

There are massive UI changes coming in the subsequent patches (especially user filters), so I'd revisit the UI when we have the entire functionality in place.

@mathjazz
Copy link
Collaborator Author

@bcolsson Gentle ping for the review. I'd be especially interested in your take on this deviation from the spec:

I've made the "Transactional" flag only visible when the Email option is checked. This patterns prevents selecting the Transactional option when Email option is not checked and makes the extra validation required by the spec redundant.

Copy link
Collaborator

@bcolsson bcolsson left a comment

Choose a reason for hiding this comment

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

Thanks for the ping. The proposed pattern makes sense, so I'm ok with the PR as proposed.

@mathjazz mathjazz merged commit 434a77c into mozilla:main Jul 11, 2024
4 checks passed
@mathjazz mathjazz deleted the 3268-transactional-emails branch July 11, 2024 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants