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

[GH-15906][MM-22844] Redesign welcome and verify email. #16824

Merged
merged 30 commits into from
Feb 26, 2021
Merged

[GH-15906][MM-22844] Redesign welcome and verify email. #16824

merged 30 commits into from
Feb 26, 2021

Conversation

jp0707
Copy link
Contributor

@jp0707 jp0707 commented Jan 28, 2021 •
edited

Summary

Redesign welcome and verify email. Rest of the emails are untouched.

This change adds MJML into the build tool chain. Relevant discussion at https://community.mattermost.com/core/pl/jz9ry3cp97y4bf1jokxff4ui8w

Ticket Link

#15906

@mm-cloud-bot
Copy link

@jp0707: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

@mattermod
Copy link
Contributor

The file Makefile is in the blocklist for external contributors. Hence, these changes are not tested by the CI pipeline active until the build is re-triggered by a core committer or the PR is merged. Please be careful when reviewing it.
/cc @mattermost/core-security @mattermost/core-build-engineers

@mattermod
Copy link
Contributor

Hello @jp0707,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@jp0707
Require mjml as a pre-requisite on dev and CI machines.
ae3796d 
Not 100% sure if the CI side is correct though.
@mattermod
Copy link
Contributor

The files .circleci/config.yml, Makefile, scripts/prereq-check.sh, scripts/prereq-check.sh are in the blocklist for external contributors. Hence, these changes are not tested by the CI pipeline active until the build is re-triggered by a core committer or the PR is merged. Please be careful when reviewing it.
/cc @mattermost/core-security @mattermost/core-build-engineers

@jp0707
Copy link
Contributor Author

jp0707 commented Jan 28, 2021

/release-note-none

@jp0707 jp0707 marked this pull request as ready for review January 28, 2021 23:11
jespino
jespino reviewed Jan 29, 2021
View reviewed changes
app/server.go Outdated Show resolved Hide resolved
jespino
jespino reviewed Jan 29, 2021
View reviewed changes
templates/welcome.mjml Outdated
@@ -0,0 +1,26 @@
<mjml>
Copy link
Member

Choose a reason for hiding this comment

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

I love this, way way way nicer :)

jespino
jespino reviewed Jan 29, 2021
View reviewed changes
utils/html.go Outdated Show resolved Hide resolved
jespino
jespino reviewed Jan 29, 2021
View reviewed changes
app/email.go Outdated Show resolved Hide resolved
jespino
jespino reviewed Jan 29, 2021
View reviewed changes
app/email.go Outdated Show resolved Hide resolved
@agnivade
Copy link
Member

I just finished reviewing a load of PRs and need to get back to other stuff. If this requires quick feedback, then please feel free to assign someone else.

@isacikgoz isacikgoz added 2: Dev Review Requires review by a developer 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review labels Jan 29, 2021
@mm-cloud-bot mm-cloud-bot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed labels Jan 29, 2021
@jespino jespino requested review from jespino and wiggin77 and removed request for agnivade January 29, 2021 09:15
reflog
reflog approved these changes Jan 29, 2021
View reviewed changes
Copy link
Contributor

@reflog reflog left a comment

Choose a reason for hiding this comment

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

Looks good, except for @jespino comments

@wiggin77
Copy link
Member

This looks great @jp0707. One question: when a developer runs make run-server it will build the email templates each time. What happens if they do no have mjml installed? I can see where it gets installed for CI but not developers running locally.

@mattermod
Copy link
Contributor

The files .circleci/config.yml, Makefile, scripts/diff-email-templates.sh, scripts/diff-email-templates.sh are in the blocklist for external contributors. Hence, these changes are not tested by the CI pipeline active until the build is re-triggered by a core committer or the PR is merged. Please be careful when reviewing it.
/cc @mattermost/core-security @mattermost/core-build-engineers

@ogi-m ogi-m added the Setup Cloud Test Server Setup an on-prem test server label Feb 25, 2021
@mm-cloud-bot
Copy link

Creating a new SpinWick test server using Mattermost Cloud.

@mm-cloud-bot
Copy link

Enterprise Edition Image not available in the 30 minutes timeframe, checking the Team Edition Image and if available will use that.

@mm-cloud-bot
Copy link

Mattermost test server created! 🎉

Access here: https://mattermost-server-pr-16824.test.mattermost.cloud

Account Type Username Password
Admin sysadmin Sys@dmin123
User user-1 User-1@123

@mattermod
Copy link
Contributor

The files .circleci/config.yml, Makefile, scripts/diff-email-templates.sh, scripts/diff-email-templates.sh are in the blocklist for external contributors. Hence, these changes are not tested by the CI pipeline active until the build is re-triggered by a core committer or the PR is merged. Please be careful when reviewing it.
/cc @mattermost/core-security @mattermost/core-build-engineers

@mm-cloud-bot
Copy link

New commit detected. SpinWick will upgrade if the updated docker image is available.

@mm-cloud-bot
Copy link

Test server creation failed. See the logs for more information.

ogi-m
ogi-m approved these changes Feb 25, 2021
View reviewed changes
Copy link

@ogi-m ogi-m left a comment

Choose a reason for hiding this comment

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

Tested and passed 👍🏻

@ogi-m ogi-m added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester. May occur at the same time as Dev Review Setup Cloud Test Server Setup an on-prem test server labels Feb 25, 2021
@mm-cloud-bot
Copy link

Test server destroyed

@agnivade
Copy link
Member

/update-branch

@mattermod
Copy link
Contributor

The files .circleci/config.yml, Makefile, scripts/diff-email-templates.sh, scripts/diff-email-templates.sh are in the blocklist for external contributors. Hence, these changes are not tested by the CI pipeline active until the build is re-triggered by a core committer or the PR is merged. Please be careful when reviewing it.
/cc @mattermost/core-security @mattermost/core-build-engineers

@agnivade agnivade added AutoMerge Used by Mattermod to merge PR automatically EETests labels Feb 26, 2021
@mattermod
Copy link
Contributor

Will try to auto merge this PR once all tests and checks are passing. This might take up to an hour.

@mattermod mattermod removed the EETests label Feb 26, 2021
@mattermod
Copy link
Contributor

Trying to auto merge this PR.

@mattermod mattermod merged commit 87f5532 into mattermost:master Feb 26, 2021
@mattermod
Copy link
Contributor

Pull Request successfully merged
SHA: 87f5532

@mattermod mattermod removed the AutoMerge Used by Mattermod to merge PR automatically label Feb 26, 2021
@amyblais amyblais added this to the v5.34.0 milestone Feb 26, 2021
@saturninoabril saturninoabril mentioned this pull request Mar 2, 2021
Merged
@amyblais amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Mar 18, 2021
@kalvdans
Copy link

After this change, Thunderbird warns about external content in the email. Is the exact font in the emails that important?

bild

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reflog reflog reflog approved these changes

@jespino jespino jespino approved these changes

@srkgupta srkgupta srkgupta approved these changes

@matthewbirtch matthewbirtch matthewbirtch approved these changes

@wiggin77 wiggin77 wiggin77 approved these changes

@ogi-m ogi-m ogi-m approved these changes

Assignees
No one assigned
Labels
4: Reviews Complete All reviewers have approved the pull request Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation release-note-none Denotes a PR that doesn't merit a release note.
Projects
None yet
Milestone
v5.34.0
Development

Successfully merging this pull request may close these issues.

None yet