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

New emails for sharebymail #4307

Merged
merged 9 commits into from
Apr 12, 2017
Merged

New emails for sharebymail #4307

merged 9 commits into from
Apr 12, 2017

Conversation

MorrisJobke
Copy link
Member

  • I added a new method to allow adding a single button with no width constraints
  • escape the texts of the heading, body and button text with htmlspecialchars (is this enough, @LukasReschke ) - this was needed to show also file names in there and not cause code injection (the escaping is only done in the HTML emails and not in the text emails
  • migrate share email and the password notification email

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews enhancement labels Apr 11, 2017
@MorrisJobke MorrisJobke added this to the Nextcloud 12.0 milestone Apr 11, 2017
@mention-bot
Copy link

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @schiessle, @jancborchardt and @LukasReschke to be potential reviewers.

@MorrisJobke
Copy link
Member Author

Followup to #4304 and #4244

@MorrisJobke
Copy link
Member Author

Before:

bildschirmfoto 2017-04-11 um 16 25 44

After:

bildschirmfoto 2017-04-11 um 16 26 35

bildschirmfoto 2017-04-11 um 16 25 49

@MorrisJobke
Copy link
Member Author

@nextcloud/designers I'm also open for new wordings of the emails, because it's a lot of duplicate text. Maybe you have better ideas. cc @Espina2

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Awesome stuff. Lets do this!

@LukasReschke
Copy link
Member

Rebased upon master.

Copy link
Member

@schiessle schiessle left a comment

Choose a reason for hiding this comment

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

The HTML mail looks awesome! But the text mail looks strange:

admin shared »welcome.txt« with you

admin shared »welcome.txt« with you. Click the button below to open it.

Open »welcome.txt«: http://localhost/master/index.php/s/RPGlwrxTJr2lrm5
--
Nextcloud - a safe home for all your data
This is an automatically generated email, please do not reply.

Can we fix this?

@nickvergessen
Copy link
Member

i will fix

@LukasReschke
Copy link
Member

escape the texts of the heading, body and button text with htmlspecialchars (is this enough, @LukasReschke ) - this was needed to show also file names in there and not cause code injection (the escaping is only done in the HTML emails and not in the text emails

Not entirely sure about that. For example in the activity app we would have an email that uses something like:

$emailTemplate->addBodyText($l->t('There was some activity at %s', [$this->urlGenerator->getAbsoluteURL('/')]));

In this case having HTML allowed here for URLs (and a separate text for plain text) may be very useful. Also I think sometimes some stuff like <strong> may be useful as well. Thoughts?

@LukasReschke
Copy link
Member

Also HTML may be useful when embedding icons like the activity state icons. Just a thought though :-)

@schiessle
Copy link
Member

schiessle commented Apr 12, 2017

I fixed the footer for the plain text mails.

The plain text mail looks now better, also with the changes by @nickvergessen :

mail

Although I think the "Click the button below to open it." still sounds a bit strange for a plain-text mail. I think the best would be to create two separate templates. This way we can make sure that we have a useful text in both cases and that we have reasonable line breaks, which is not a issue here but maybe with other mails (something <= 80 chars).

@nickvergessen
Copy link
Member

@schiessle addBodyText has two parameters, just make use of it.

@Espina2
Copy link
Contributor

Espina2 commented Apr 12, 2017

I think you can more information like that. @MorrisJobke

shared folder

@enoch85
Copy link
Member

enoch85 commented Apr 12, 2017

@Espina2 Just plain perfection.

@schiessle
Copy link
Member

schiessle commented Apr 12, 2017

@nickvergessen I saw it. The "problem" is that we then have to split it up in two "addBodyText()" calls which will result in a line break, right? ... but I will try.

@schiessle
Copy link
Member

As expected it looks rather ugly on the html mail if it put in in two "addBodyText()" calls because of the line breaks:

mail

@nickvergessen
Copy link
Member

Not sure what you did, but I made two little commits and it looks good

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
MorrisJobke and others added 6 commits April 12, 2017 17:16
Signed-off-by: Morris Jobke <hey@morrisjobke.de>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
…--'. It also adds a line break in front so that there is some spacing between the mail body and the footer

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@codecov-io
Copy link

Codecov Report

Merging #4307 into master will decrease coverage by 16.34%.
The diff coverage is 0%.

@@              Coverage Diff              @@
##             master    #4307       +/-   ##
=============================================
- Coverage     54.03%   37.68%   -16.35%     
+ Complexity    21417    21407       -10     
=============================================
  Files          1321     1317        -4     
  Lines         81696    81630       -66     
  Branches       1305     1305               
=============================================
- Hits          44147    30765    -13382     
- Misses        37549    50865    +13316
Impacted Files Coverage Δ Complexity Δ
lib/private/Mail/EMailTemplate.php 0% <0%> (-76.06%) 30 <8> (+5)
apps/sharebymail/lib/ShareByMailProvider.php 60.94% <0%> (+0.47%) 62 <0> (-3) ⬇️
core/Command/TwoFactorAuth/Enable.php 0% <0%> (-100%) 4% <0%> (ø)
...factor_backupcodes/lib/Activity/GenericSetting.php 0% <0%> (-100%) 8% <0%> (ø)
apps/files/lib/Activity/Settings/FileChanged.php 0% <0%> (-100%) 8% <0%> (ø)
apps/dav/lib/CalDAV/Plugin.php 0% <0%> (-100%) 2% <0%> (ø)
lib/private/Files/SimpleFS/SimpleFolder.php 0% <0%> (-100%) 11% <0%> (ø)
lib/private/Files/SimpleFS/SimpleFile.php 0% <0%> (-100%) 9% <0%> (ø)
apps/dav/lib/CalDAV/Activity/Setting/Todo.php 0% <0%> (-100%) 8% <0%> (ø)
...ofactor_backupcodes/lib/Activity/GenericFilter.php 0% <0%> (-100%) 7% <0%> (ø)
... and 449 more

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 dccb892...983210d. Read the comment docs.

@MorrisJobke
Copy link
Member Author

Tested and works nice 👍

@MorrisJobke
Copy link
Member Author

MorrisJobke commented Apr 12, 2017

@Espina2 Looks nice, but something for a followup. First of all we should get rid of the super ugly way. ;) See #4334

@MorrisJobke MorrisJobke added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 12, 2017
@@ -461,7 +461,7 @@ public function addFooter($text = '') {

$this->htmlBody .= vsprintf($this->footer, [$text]);
$this->htmlBody .= $this->tail;
$this->plainBody .= '--' . PHP_EOL;
$this->plainBody .= PHP_EOL . '-- ' . PHP_EOL;
Copy link
Member Author

Choose a reason for hiding this comment

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

When you update stuff like this, then you should also update the unit tests. Let me do this.

Signed-off-by: Morris Jobke <hey@morrisjobke.de>
@MorrisJobke
Copy link
Member Author

I fixed the unit tests. Looks good now from my side 👍

@rullzer rullzer merged commit b3b2417 into master Apr 12, 2017
@rullzer rullzer deleted the sharing-emails branch April 12, 2017 19:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants