add ocs api for public link share email notification#36063

karakayasemi · 2019-08-19T13:41:44Z

Description

This PR adds an OCS API for public link e-mail notifications. old ajax endpoints removed and oc10 adjusted to use the new API.

I aimed to keep the same functionality as oc10. In addition, the current code is simplified by removing redundant post parameters:

The old endpoint is taking many pieces of information related to the share of link as POST parameter like expiration date, filename, itemType, etc. It is also trying to validate them on the backend. However, we can already extract these parameters by reaching related share object on the backend. So, I considered them redundant and, removed.
I could not find any usage of cc and bcc fields in the codebase. So, I removed these options regards to YAGNI principle.
When toSelf parameter is posted, the backend side is only adding user's e-mail to the recipient list. I moved this logic to the client-side and removed toSelf from POST parameters.

UPDATE

Internal email notification mechanism also moved to the new controller.

Usage

URL
/ocs/v1.php/apps/files_sharing/api/v1/notification/notifypubliclinkwithemail
Method:
POST
POST Params
Required:
recipients=[string[]]
link=[string]
personalNote=[string]

* **Success Response:**
{"ocs":{"meta":{"status":"ok","statuscode":100,"message":"OK","totalitems":"","itemsperpage":""},"data":[]}}
 
* **Fail Response:**
{"ocs":{"meta":{"status":"error","statuscode":400,"message":"Couldn't send mail to following recipient(s): test@email.com","totalitems":"","itemsperpage":""},"data":[]}}

Related Issue

Part of Share Notifications API #35983

Motivation and Context

In Phoenix, shared links shall be sent via email. We need an API for that.

How Has This Been Tested?

Unit tests are added.
Manually by sending public link e-mails from the web interface.
Manually with API call.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

Code changes
Unit tests added
Acceptance tests added
Documentation ticket raised:

apps/files_sharing/appinfo/routes.php

apps/files_sharing/lib/Controller/NotificationController.php

DeepDiver1975 · 2019-08-19T13:49:02Z

apps/files_sharing/lib/Controller/NotificationController.php

+	public function notifyPublicLinkRecipientsWithEmail($link, $recipients, $personalNote) {
+		$message = null;
+		$code = 100;
+		$mailNotification = new MailNotifications(


maybe simplify this be injecting class MailNotifications?

If we refactor this class, it will be unit testable. Since Internal email notification part of the code is also using this class. I can move internal email notification to the new controller after refactoring.

MailNotifications class is now injectable. Internal sharing email notification mechanism also moved to the new controller. PR description updated to according to this.

apps/files_sharing/lib/Controller/NotificationController.php

DeepDiver1975 · 2019-08-19T13:51:56Z

apps/files_sharing/lib/Controller/NotificationController.php

+	/**
+	 * @NoAdminRequired
+	 *
+	 * @param string $link


~~This way this API could be used for sending any data to anybody. We might want to change the api to reference a share id and also check if the current use is allowed to access the share.~~

DeepDiver1975 · 2019-08-19T14:51:28Z

The email subject did change. Before this change: 'Admin shared foo.txt with you' After the change: 'Admin shares public link with you' - link name va file name
^ same with the email body
unrelated to this change but found while testing: the subject is html escaped - which is bad

codecov · 2019-08-19T15:00:38Z

Codecov Report

Merging #36063 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #36063   +/-   ##
=======================================
  Coverage   53.85%   53.85%           
=======================================
  Files          63       63           
  Lines        7377     7377           
  Branches     1301     1302    +1     
=======================================
  Hits         3973     3973           
  Misses       3019     3019           
  Partials      385      385

Flag	Coverage Δ
#javascript	`53.85% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
core/js/sharedialogmailview.js	`62.37% <100%> (ø)`	⬆️

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 5c15243...ab4138f. Read the comment docs.

DeepDiver1975 · 2019-08-20T06:11:14Z

acceptance tests need to be adjusted I guess - see https://drone.owncloud.com/owncloud/core/20273/76/14

karakayasemi · 2019-08-20T18:26:29Z

@DeepDiver1975 Reason behind the e-mail subject and, body change is related to my mistake. I mistakenly used $share->getName() as file name. Now, I corrected it.~~Html escape issue is also related to this mistake. Since it's no longer needed, I've reverted your last commit changes.~~

Acceptance test fail is also related to this mistake. I hope this time, it will pass. Please, review the PR one more time. Thanks.

DeepDiver1975 · 2019-08-20T20:30:14Z

Html escape issue is also related to this mistake. Since it's no longer needed, I've reverted your last commit changes.

If the file name holds e.g. äöü - the subject is not escaped? This issue also existed before this PR - please double check. THX

…o new controller

karakayasemi · 2019-08-22T08:54:06Z

@DeepDiver1975 You were right, I re-added your commit changes. Thank you for guidance.

There was conflicts due to the recent merges, now they are resolved. I believe this is ready to review again.

jvillafanez · 2019-08-23T11:41:57Z

apps/files_sharing/lib/Controller/NotificationController.php

+
+		$userFolder = $this->rootFolder->getUserFolder($sender->getUID());
+		$nodes = $userFolder->getById($itemSource, true);
+		$node = $nodes[0] ?? null;


$node = $nodes[0] ? I don't think we need more than that...
In addition, what happens with the rest of the nodes? Consider to add a comment explainig.

@jvillafanez in the beginning, this pr aimed only to add ocs api for public link email notification. However, I had to refactor MailNotifications class to make it injectable and testable.

The only other consumer of this class was this method. Since Share20OcsController already pretty big, I did not want to inject new dependency to it. To keep the change minimum, I just moved this method from Share20OcsController to the new NotificationController and, adapted the code to use injected MailNotifications class.

So, I did not interest with the logic behind this method. I guess this node will be used for checking both sender and recipient have it 😕.

we'll have to check it at some point. It's ok for now.

settermjd · 2019-10-04T12:46:59Z

@karakayasemi, I've not been able to use the API successfully, as yet. The only response I get is the following:

<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>403</statuscode>
  <message>Public link mail notification is not allowed</message>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data/>
</ocs>

That said, the following two configuration options are in effect:

'shareapi_allow_public_notification' => 'yes',
'dav.enable.tech_preview' => true,

Any suggestions?

karakayasemi · 2019-10-04T13:01:28Z

Please enable the option that I highlighted in the image from admin sharing settings.

settermjd · 2019-10-04T13:03:53Z

Thanks for the clarification. I had thought that setting the config value would be sufficient.

settermjd · 2019-10-04T13:14:26Z

It seems I don't have the same version as you do.

settermjd · 2019-10-04T13:15:33Z

I'm running 10.3.0 alpha2 (cloned from master).

karakayasemi · 2019-10-04T13:18:18Z

There is another option appearing after allowing users to share via link.

karakayasemi added the 2 - Developing label Aug 19, 2019

karakayasemi requested a review from DeepDiver1975 August 19, 2019 13:41

karakayasemi self-assigned this Aug 19, 2019

DeepDiver1975 requested changes Aug 19, 2019

View reviewed changes

karakayasemi force-pushed the notification-api branch from 2117391 to 6f27e25 Compare August 20, 2019 18:13

karakayasemi requested a review from DeepDiver1975 August 20, 2019 18:26

karakayasemi added 3 - To Review and removed 2 - Developing labels Aug 20, 2019

karakayasemi marked this pull request as ready for review August 20, 2019 18:33

karakayasemi force-pushed the notification-api branch from 6f27e25 to 522da89 Compare August 20, 2019 18:44

karakayasemi force-pushed the notification-api branch from 522da89 to 38744a8 Compare August 21, 2019 06:35

karakayasemi and others added 2 commits August 22, 2019 11:43

add ocs api for public link share email notification

c84d530

Don't escape the file name in the email subject

5d879e1

karakayasemi force-pushed the notification-api branch from 38744a8 to 04f339e Compare August 22, 2019 08:46

make MailNotifications injectable, move internal mail notifications t…

ab4138f

…o new controller

karakayasemi force-pushed the notification-api branch from 04f339e to ab4138f Compare August 22, 2019 08:48

PVince81 requested a review from jvillafanez August 23, 2019 09:49

jvillafanez reviewed Aug 23, 2019

View reviewed changes

jvillafanez approved these changes Aug 26, 2019

View reviewed changes

PVince81 merged commit 266dfc7 into master Aug 26, 2019

delete-merged-branch bot deleted the notification-api branch August 26, 2019 13:14

phil-davis added the release:10.3.0 label Aug 26, 2019

karakayasemi mentioned this pull request Aug 26, 2019

Add client api New Share Notifications API for public link email notifications owncloud/owncloud-sdk#334

Open

davitol mentioned this pull request Sep 3, 2019

10.3 - Changelog test Results #36142

Closed

13 tasks

individual-it mentioned this pull request Sep 4, 2019

api for public link share email notification should generate the URL itself #36155

Open

pmaier1 mentioned this pull request Sep 30, 2019

Document new public link endpoints owncloud/docs#1871

Closed

12 tasks

settermjd mentioned this pull request Oct 4, 2019

Add the first draft of the notify public link by email api documentation owncloud/docs#1880

Merged

Conversation

karakayasemi commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

UPDATE

Usage

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

Uh oh!

Uh oh!

DeepDiver1975 Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

karakayasemi Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

karakayasemi Aug 20, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DeepDiver1975 Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

DeepDiver1975 commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Aug 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DeepDiver1975 commented Aug 20, 2019

Uh oh!

karakayasemi commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DeepDiver1975 commented Aug 20, 2019

Uh oh!

karakayasemi commented Aug 22, 2019

Uh oh!

jvillafanez Aug 23, 2019

Choose a reason for hiding this comment

Uh oh!

karakayasemi Aug 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jvillafanez Aug 26, 2019

Choose a reason for hiding this comment

Uh oh!

settermjd commented Oct 4, 2019

Uh oh!

karakayasemi commented Oct 4, 2019

Uh oh!

settermjd commented Oct 4, 2019

Uh oh!

settermjd commented Oct 4, 2019

Uh oh!

settermjd commented Oct 4, 2019

Uh oh!

karakayasemi commented Oct 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

karakayasemi commented Aug 19, 2019 •

edited

Loading

DeepDiver1975 commented Aug 19, 2019 •

edited

Loading

codecov bot commented Aug 19, 2019 •

edited

Loading

karakayasemi commented Aug 20, 2019 •

edited

Loading

karakayasemi Aug 23, 2019 •

edited

Loading