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

Add expiration date to share email #17379

Closed
wants to merge 3 commits into from
Closed

Add expiration date to share email #17379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2176866
Add expiration date to share email
RageZBla Oct 2, 2019
ce5bc5b
Update apps/sharebymail/lib/ShareByMailProvider.php
RageZBla Oct 3, 2019
fff5511
Update apps/sharebymail/lib/ShareByMailProvider.php
RageZBla Oct 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions apps/sharebymail/lib/ShareByMailProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -405,6 +405,10 @@ protected function sendMailNotification($filename,
$emailTemplate->addHeader();
$emailTemplate->addHeading($this->l->t('%1$s shared »%2$s« with you', [$initiatorDisplayName, $filename]), false);
$text = $this->l->t('%1$s shared »%2$s« with you.', [$initiatorDisplayName, $filename]);
if ($expiration instanceof \DateTime) {
$relativeDate = $expiration->diff(new \DateTime())->format('%a');
$text .= ' ' . $this->l->t('It will expire in %1$s days.', [$relativeDate]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Don't know much about UX but I think we should make this easier to understand.

Example:
A) You are able to access this file until xyz.
B) For security the share is only valid for x days. It's not possible to access the data after: xzy.

Let's ask @jancborchardt and @jospoortvliet for help.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be good to make this expiration info a second paragraph below the other one so that it’s clearly separated. (Also the sentence "Click the button below" can really be cut …)

This share will expire in 5 days, on October 30 at 12:00 UTC.

Copy link
Member

Choose a reason for hiding this comment

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

Otherwise this pull request seems good! Yes we can improve a lot on the design and should do that – @RageZBla your call if you want to do more here. :) (E.g. the triplication of subject, title and first sentence of the content.)

Copy link
Member

Choose a reason for hiding this comment

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

For the record, my comment above still stands. :) Also, it would be nice to handle singular correctly cause it looks weird if "this share will expire in 1 days". ;)

}

$emailTemplate->addBodyText(
htmlspecialchars($text . ' ' . $this->l->t('Click the button below to open it.')),
Expand Down
90 changes: 90 additions & 0 deletions apps/sharebymail/tests/ShareByMailProviderTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1112,4 +1112,94 @@ public function testSendMailNotificationWithDifferentUserAndNoUserEmail() {
null,
]);
}

public function testSendMailNotificationWithExpiration() {
$provider = $this->getInstance();
$initiatorUser = $this->createMock(IUser::class);
$this->userManager
->expects($this->once())
->method('get')
->with('InitiatorUser')
->willReturn($initiatorUser);
$initiatorUser
->expects($this->once())
->method('getDisplayName')
->willReturn('Mr. Initiator User');
$message = $this->createMock(Message::class);
$this->mailer
->expects($this->once())
->method('createMessage')
->willReturn($message);
$template = $this->createMock(IEMailTemplate::class);
$this->mailer
->expects($this->once())
->method('createEMailTemplate')
->willReturn($template);
$template
->expects($this->once())
->method('addHeader');
$template
->expects($this->once())
->method('addHeading')
->with('Mr. Initiator User shared »file.txt« with you');
$template
->expects($this->once())
->method('addBodyText')
->with(
'Mr. Initiator User shared »file.txt« with you. It will expire in 7 days. Click the button below to open it.',
'Mr. Initiator User shared »file.txt« with you. It will expire in 7 days.'
);
$template
->expects($this->once())
->method('addBodyButton')
->with(
'Open »file.txt«',
'https://example.com/file.txt'
);
$message
->expects($this->once())
->method('setTo')
->with(['john@doe.com']);
$this->defaults
->expects($this->once())
->method('getName')
->willReturn('UnitTestCloud');
$message
->expects($this->once())
->method('setFrom')
->with([
\OCP\Util::getDefaultEmailAddress('UnitTestCloud') => 'Mr. Initiator User via UnitTestCloud'
]);
$message
->expects($this->never())
->method('setReplyTo');
$template
->expects($this->once())
->method('addFooter')
->with('');
$template
->expects($this->once())
->method('setSubject')
->with('Mr. Initiator User shared »file.txt« with you');
$message
->expects($this->once())
->method('useTemplate')
->with($template);
$this->mailer
->expects($this->once())
->method('send')
->with($message);

$inOneWeek = (new \DateTime('now'))->add(new \DateInterval('P7D'));
self::invokePrivate(
$provider,
'sendMailNotification',
[
'file.txt',
'https://example.com/file.txt',
'InitiatorUser',
'john@doe.com',
$inOneWeek,
]);
}
}