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

fix: remove newline sign from email subject #2576

Merged
merged 2 commits into from
Jul 6, 2022
Merged

fix: remove newline sign from email subject #2576

merged 2 commits into from
Jul 6, 2022

Conversation

mszekiel
Copy link
Contributor

@mszekiel mszekiel commented Jul 6, 2022

Removed empty lines in email.subject.gotmpl and removed .trim() in tests, which prevented from spotting that bug.

Related issue(s)

Fixes #2491

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change introduces a new feature.
  • I am following the contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security vulnerability.
    If this pull request addresses a security. vulnerability,
    I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added or changed the documentation.

Further Comments

I also considered that we could use the trim() in EmailSubject/LoadText functions, but then also the same is being used for body, so not sure if this could break more. For now, this PR is the simplest solution.

Copy link
Member

@zepatrik zepatrik left a comment

Choose a reason for hiding this comment

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

Good catch 👍

I would suggest that we rather trim all subjects in the EmailSubject functions, because this is something users (and tools) might get wrong regularly (e.g. prettier ensures files end with a newline).

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #2576 (ba41384) into master (c058e23) will increase coverage by 0.02%.
The diff coverage is n/a.

❗ Current head ba41384 differs from pull request most recent head 627ee4c. Consider uploading reports for the commit 627ee4c to get more accurate results

@@            Coverage Diff             @@
##           master    #2576      +/-   ##
==========================================
+ Coverage   76.59%   76.62%   +0.02%     
==========================================
  Files         321      321              
  Lines       17961    17961              
==========================================
+ Hits        13758    13762       +4     
+ Misses       3252     3249       -3     
+ Partials      951      950       -1     
Impacted Files Coverage Δ
persistence/sql/persister_courier.go 89.70% <0.00%> (ø)
session/test/persistence.go 100.00% <0.00%> (+1.38%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e746c33...627ee4c. Read the comment docs.

@mszekiel mszekiel requested a review from zepatrik July 6, 2022 18:48
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Awesome, thank you! 🎉 Your contribution makes Ory better :)

@aeneasr aeneasr merged commit ca3d9c2 into ory:master Jul 6, 2022
@mszekiel mszekiel deleted the fix-email-subject-newline branch July 6, 2022 20:24
@vinckr
Copy link
Member

vinckr commented Jul 7, 2022

Hello @mszekiel
Congrats on merging your first PR in Ory 🎉 !
Your contribution will soon be helping secure millions of identities around the globe 🌏.
As a small token of appreciation we send all our first time contributors a gift package to welcome them to the community.
Please drop me an email and I will forward you the form to claim your Ory swag!

peturgeorgievv pushed a commit to senteca/kratos-fork that referenced this pull request Jun 30, 2023
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.

Default email templates as well as custom ones add '\n' at the end of subject
4 participants