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

handle mail send error gracefully #12636

Merged
merged 4 commits into from
Jan 30, 2019
Merged

handle mail send error gracefully #12636

merged 4 commits into from
Jan 30, 2019

Conversation

schiessle
Copy link
Member

@schiessle schiessle commented Nov 23, 2018 •
edited
Loading

log the error in case a notification mail of a new share couldn't
be send to the recipient and finish the share operation successfully

I would suggest to backport it at least to stable15 and stable14

fix #12597

@schiessle schiessle added this to the Nextcloud 15 milestone Nov 23, 2018
@rullzer
Copy link
Member

rullzer commented Nov 26, 2018

/backport to stable15

@rullzer
Copy link
Member

rullzer commented Nov 26, 2018

/backport to stable14

@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 Nov 28, 2018
@ChristophWurst ChristophWurst added 3. to review Waiting for reviews and removed 4. to release Ready to be released and/or waiting for tests to finish labels Nov 29, 2018
ChristophWurst
ChristophWurst requested changes Nov 29, 2018
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Typo 😉

Is the returned bool used?

lib/private/Share20/Manager.php Outdated Show resolved Hide resolved
ChristophWurst
ChristophWurst requested changes Nov 29, 2018
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Result is never checked.

Ref

server/lib/private/Share20/Manager.php

Lines 687 to 694 in 0f8de6e

$this->sendMailNotification(
$l,
$share->getNode()->getName(),
$this->urlGenerator->linkToRouteAbsolute('files.viewcontroller.showFile', ['fileid' => $share->getNode()->getId()]),
$share->getSharedBy(),
$emailAddress,
$share->getExpirationDate()
);

@ChristophWurst

This comment has been minimized.

Sign in to view
@ChristophWurst

This comment has been minimized.

Sign in to view
@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Nov 30, 2018
@skjnldsv skjnldsv mentioned this pull request Dec 4, 2018
Closed
@schiessle schiessle added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Dec 4, 2018
@schiessle
Copy link
Member Author

typo fixed

@MorrisJobke MorrisJobke mentioned this pull request Jan 4, 2019
Merged
3 tasks
@MorrisJobke MorrisJobke mentioned this pull request Jan 30, 2019
Merged
@schiessle @ChristophWurst
handle mail send error gracefully
a943580 
log the error in case a notification mail of a new share couldn't
be send to the recipient and finish the share operation successfully

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@schiessle @ChristophWurst
log full exception
ab01402 
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
@schiessle @ChristophWurst
fix typo
8ed004d 
Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>
ChristophWurst
ChristophWurst approved these changes Jan 30, 2019
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

I'm fine with @schiessle's changes as this method is solely used in this class 👍

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 30, 2019
@ChristophWurst
Fix typos and unused return values
7e3e3be 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
MorrisJobke
MorrisJobke approved these changes Jan 30, 2019
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Code looks good 👍

@MorrisJobke MorrisJobke mentioned this pull request Jan 30, 2019
Merged
@rullzer rullzer merged commit 85d6d7b into master Jan 30, 2019
@rullzer rullzer deleted the fix-12597 branch January 30, 2019 19:05
@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 30, 2019
Merged
@backportbot-nextcloud
Copy link

backport to stable15 in #13930

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 30, 2019
Merged
@backportbot-nextcloud
Copy link

backport to stable14 in #13931

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

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@blizzz blizzz blizzz left review comments

@kesselb kesselb kesselb left review comments

@rullzer rullzer rullzer approved these changes

@MorrisJobke MorrisJobke MorrisJobke approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: sharing
Projects
None yet
Milestone
Nextcloud 16
Development

Successfully merging this pull request may close these issues.

Error when sharing to user with email
6 participants
@schiessle @rullzer @ChristophWurst @MorrisJobke @blizzz @kesselb