allow to add a personal note to a share #10218

schiessle · 2018-07-12T13:22:59Z

Allow users to add a personal note to a share which will then be send to the recipient (in case of a user, group or mail share) or displayed at the public link page (in case of a link or mail share)

back-end to set/get a note to a share
sending notes by mail implemented for user, group and mail shares
UI to add a note to a share (user, group, mail)
UI to add note to public links
display note on public link page (maybe it could look nice, but it works 😉 )
fix/update unit tests

For the UI to add a note to a share I would suggest to add another menu option to the "..." menu for user, group and mail shares called "Add note" if the user clicks on it we show a input field to add the note. For public links I would add a similar option under the check-boxes.

After the user set a note it is send to the server over the already existing OCS API, same as we already use to update permission, expire date, etc. Example call:

curl -X PUT http://admin:admin@localhost/server/ocs/v2.php/apps/files_sharing/api/v1/shares/1 -d note="hello world2!" -H OCS-APIRequest:true

schiessle · 2018-07-12T13:24:09Z

@skjnldsv maybe you have some time to look at the UI part?

nickvergessen · 2018-07-12T13:32:34Z

apps/sharebymail/lib/ShareByMailProvider.php

+		$initiatorDisplayName = ($initiatorUser instanceof IUser) ? $initiatorUser->getDisplayName() : $initiator;
+		$initiatorEmailAddress = ($initiatorUser instanceof IUser) ? $initiatorUser->getEMailAddress() : null;
+
+		$plainBodyPart = $this->l->t("%s shared »%s« with you and want to add:\n", [$initiatorDisplayName, $filename]);


Single quotes

Please use counted placeholders: %$1s and %$2s, otherwise Right-to-Left languages have problems.

schiessle · 2018-07-12T14:26:58Z

The current implementation has one drawback: Notes for internal user/group shares might never reach the user if no email address is configured. Could be nice to display them somehow in the file listing, people might still miss it if they use desktop/mobile clients only but at least it is visible "somehow". But don't know how hard it would be to add it... @skjnldsv (would also put it at the end of the todo list)

nickvergessen · 2018-07-12T14:55:27Z

If okay, I would only allow this for mail/public shares.
Internal shares should use comments?

nickvergessen · 2018-07-12T14:56:45Z

apps/sharebymail/lib/ShareByMailProvider.php

+		$initiatorDisplayName = ($initiatorUser instanceof IUser) ? $initiatorUser->getDisplayName() : $initiator;
+		$initiatorEmailAddress = ($initiatorUser instanceof IUser) ? $initiatorUser->getEMailAddress() : null;
+
+		$plainHeading = $this->l->t('%1s shared »%2s« with you and want to add:', [$initiatorDisplayName, $filename]);


Missing the $ between the number and s: %1$s and %2$s

nickvergessen · 2018-07-12T14:57:11Z

lib/private/Share20/DefaultShareProvider.php

+			// The "From" contains the sharers name
+			$instanceName = $this->defaults->getName();
+			$senderName = $this->l->t(
+				'%s via %s',


nickvergessen · 2018-07-12T14:57:20Z

lib/private/Share20/DefaultShareProvider.php

+			$initiatorDisplayName = ($initiatorUser instanceof IUser) ? $initiatorUser->getDisplayName() : $initiator;
+			$initiatorEmailAddress = ($initiatorUser instanceof IUser) ? $initiatorUser->getEMailAddress() : null;
+
+			$plainHeading = $this->l->t('%1s shared »%2s« with you and want to add:', [$initiatorDisplayName, $filename]);


schiessle · 2018-07-12T15:01:27Z

If okay, I would only allow this for mail/public shares.
Internal shares should use comments?

No, one of the important use case is to avoid the typical work flow: share a document with a colleague, go to the mail client and write a mail "hey, I just have shared document X with you. Can you please review it so that we can do XYZ". So it is important that it also works for internal shares.

Btw, this reminds me that I should add a link to the internal share to the mail. Is there a easy way to generate it?

nickvergessen · 2018-07-13T09:12:17Z

core/Migrations/Version14000Date20180712153140.php

+
+		$table = $schema->getTable('share');
+		$table->addColumn('note', 'text', [
+			'notnull' => true,


any reason for this?
Just received the error when testing:

An exception occurred while executing 'INSERT INTO `oc_share` (`share_type`, `item_type`, `item_source`, `file_source`, `share_with`, `uid_owner`, `uid_initiator`, `permissions`, `token`, `password`, `stime`, `file_target`) VALUES(?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' with params [4, "file", 3, 3, "test@****.com", "admin", "admin", 1, "dLzxBkWpFcjZnFN", null, 1531473009, ""]: SQLSTATE[HY000]: General error: 1364 Field 'note' doesn't have a default value

nickvergessen · 2018-07-13T09:49:41Z

apps/files_sharing/templates/public.php

@@ -31,6 +31,11 @@
 <input type="hidden" name="maxSizeAnimateGif" value="<?php p($_['maxSizeAnimateGif']); ?>" id="maxSizeAnimateGif">
 <?php if (!isset($_['hideFileList']) || (isset($_['hideFileList']) && $_['hideFileList'] === false)) { ?>
 	<div id="files-public-content">
+		<?php if (isset($_['note']) && $_['note'] !== '') : ?>
+		<div id="note">
+			<?php p($l->t('Note: ')); p($_['note']); ?>


space should not be part of the translation, or you insert the note with a placeholder

nickvergessen · 2018-07-13T09:50:32Z

apps/sharebymail/lib/ShareByMailProvider.php

+		$initiatorEmailAddress = ($initiatorUser instanceof IUser) ? $initiatorUser->getEMailAddress() : null;
+
+		$plainHeading = $this->l->t('%1$s shared »%2$s« with you and want to add:', [$initiatorDisplayName, $filename]);
+		$htmlHeading = $this->l->t('%1$s shared »%2$s« with you and want to add:', [$initiatorDisplayName, $filename]);


*wants (3rd person singular - he/she/it, das s muss mit :) )

I think in the HTML email the trailing : looks strange

nickvergessen · 2018-07-13T09:57:22Z

Currently it always sends a new email when the note is updated, is that intentional?
Could otherwise also check if the note was already set and only send the email when it was empty before

schiessle · 2018-07-13T10:06:06Z

Currently it always sends a new email when the note is updated, is that intentional?

Well, good question. I thought that it might make sense to send the note again if the user changes it. If you replace the note with another one you would expect that the user receives it, right?

If we send it only once we would also need to block editing once a note was set, but this could confuse people if they can't fix a typo, extend the note, etc.

skjnldsv · 2018-07-14T10:04:02Z

schiessle · 2018-07-14T19:10:19Z

Would it be possible to grow the open "..." menu and put the input field in the menu? Putting it below the share link looks confusing. Especially if there are more then one share and maybe even a public link.

When I try to save a note I get "An error occurred. Unable to save the note". It looks like you send a PUT request to " http://localhost/server/index.php/apps/files/", the right URL would be http://localhost/server/ocs/v2.php/apps/files_sharing/api/v1/shares/<shareid>, see curl example at the top.

I also couldn't find a way to set a not for a public link share.

skjnldsv · 2018-07-14T19:33:39Z

@schiessle I used the design examples provided by the other issue :)
The request works fine here, I'm guessing this is an issue with the OC.generateURL, I will change that, I need to use the linktoOCS method.

The public share isn't available indeed, I realized that later and did not found the proper design for this yet, I'll think about it.

schiessle · 2018-07-16T08:33:22Z

@skjnldsv afaik the design was from a stage of discussion where we considered haveing only one comment per file (for all shares). But as we now want to have one comment per share I think it makes more sense to display the input filed more next to the share like we do it with the password field for mail shares:

For the link shares I would just add another checkbox and behave also the same as the "Password protect" checkbox for link shares behave.

schiessle · 2018-07-16T09:09:58Z

Does anyone know why this tests fail: https://drone.nextcloud.com/nextcloud/server/8944/107

When I saw it the first time I fixed it here: a0ac9b3

Now all tests pass locally but it seems like CI ignores this commit 😕

skjnldsv · 2018-07-16T10:24:59Z

@schiessle sure :)
I'll move it to the popover then!

Still unsure on how to properly implement a nice ui for the public shares then. I'll check if @jancborchardt have an idea ;)

jospoortvliet · 2018-07-16T12:19:21Z

If okay, I would only allow this for mail/public shares.
Internal shares should use comments?

No, one of the important use case is to avoid the typical work flow: share a document with a colleague, go to the mail client and write a mail "hey, I just have shared document X with you. Can you please review it so that we can do XYZ". So it is important that it also works for internal shares.

Wouldn't it make sense to 'simply' make this the first comment on the file? Sorry, just thinking out loud...

schiessle · 2018-07-17T09:29:09Z

Wouldn't it make sense to 'simply' make this the first comment on the file? Sorry, just thinking out loud...

No, because it should be personal 1:1 comment, only seen by the recipient not "randomly" the first comment seen by everyone.

MorrisJobke · 2018-07-18T09:46:10Z

The current implementation has one drawback: Notes for internal user/group shares might never reach the user if no email address is configured. Could be nice to display them somehow in the file listing, people might still miss it if they use desktop/mobile clients only but at least it is visible "somehow". But don't know how hard it would be to add it... @skjnldsv (would also put it at the end of the todo list)

It's kind of weird. Shouldn't it be just a comment?

No, one of the important use case is to avoid the typical work flow: share a document with a colleague, go to the mail client and write a mail "hey, I just have shared document X with you. Can you please review it so that we can do XYZ". So it is important that it also works for internal shares.

This should be a comment as well and you should receive this as email.

Another thing that came to my mind: A recipient is getting two emails then? That's also not optimal, as it then is again a missing context.

jancborchardt · 2018-07-18T09:51:15Z

I mean, we had a discussion about this, no? And we discussed the proper way to do this is:

Have a sidebar in shares to display comments
Be able to toggle certain file comments to be publicly visible

Adding yet another different way to add text to a file (remember we will already have the Comments vs Talk chat issue) will just be a mess.

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Signed-off-by: Julius Härtl <jus@bitgrid.net>

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

The share link UI no longer uses its own layout below the other shares; now it is shown as a share row with a menu for the actions (except enabling it, which is shown in the row itself), just like the other shares. The share link is no longer shown, either; now the link is got by clicking on a "Copy URL" menu item, which copies the link to the clipboard. As the clipboard is not accessible from the acceptance tests the URL is now extracted from the attributes of that menu item (although the menu item is clicked anyway to mimic the user behaviour). Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

danxuliu · 2018-07-21T15:47:11Z

Acceptance tests fixed; the test Viewing a favorite file in its folder does not prevent opening the details view in "All files" section fails not due to changes in this pull request, but due to #9982

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv · 2018-07-24T07:10:53Z

Failure unrelated for files : sh: 1: kill: No such process

I'll handle the user acceptance failure in another pr since it's on master!

Closes #876 I didn't test on NC < 14, but it should be fine since it only puts back old sidebar behaviour before nextcloud/server#10218 Signed-off-by: Thomas Citharel <tcit@tcit.fr>

schiessle added enhancement feature: sharing labels Jul 12, 2018

schiessle added this to the Nextcloud 14 milestone Jul 12, 2018

nickvergessen reviewed Jul 12, 2018

View reviewed changes

nickvergessen self-requested a review July 12, 2018 13:33

nickvergessen reviewed Jul 12, 2018

View reviewed changes

schiessle force-pushed the share-comments branch from c3482fc to 7cbd4aa Compare July 13, 2018 08:33

nickvergessen reviewed Jul 13, 2018

View reviewed changes

skjnldsv force-pushed the share-comments branch from 5d07502 to a0ac9b3 Compare July 14, 2018 10:01

skjnldsv force-pushed the share-comments branch 2 times, most recently from 83f66dc to a20cabb Compare July 18, 2018 15:46

schiessle and others added 7 commits July 21, 2018 15:02

fix unit tests

e951ad5

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

Public share

c71d3dc

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Public fixes

3a11c26

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

move the note up, so that it is also visible in the file drop view

977c462

Signed-off-by: Bjoern Schiessle <bjoern@schiessle.org>

No remote note and various fixes

d4de77a

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Don't use shorthand syntax for functions

e302cad

Signed-off-by: Julius Härtl <jus@bitgrid.net>

Tests fixes and design bump

58debbe

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv force-pushed the share-comments branch from e4e2444 to 14a5bed Compare July 21, 2018 13:02

skjnldsv and others added 2 commits July 21, 2018 16:09

Design structure fixes

da0bdd1

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

danxuliu force-pushed the share-comments branch from 14a5bed to 70262fb Compare July 21, 2018 15:41

skjnldsv mentioned this pull request Jul 21, 2018

Cleanup structure #9982

Merged

skjnldsv added 2 commits July 21, 2018 18:31

Design fixes

e6ded2a

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

Comply @SInCE requirements

fee62fd

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>

skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jul 24, 2018

rullzer merged commit b41d0d3 into master Jul 24, 2018

rullzer deleted the share-comments branch July 24, 2018 07:12

skjnldsv mentioned this pull request Jul 24, 2018

Public share note needs a better ui #10361

Closed

This was referenced Jul 24, 2018

The loading icon is not at the same position as the checkbox it replaces #7724

Closed

[WIP] Move actions of sharing link to popover menu #7010

Closed

tcitworld mentioned this pull request Aug 6, 2018

Quickfix for sidebar on NC14 nextcloud/calendar#885

Merged

juliusknorr mentioned this pull request Aug 10, 2018

Link sharing broken with enforced password protection #10644

Closed

MorrisJobke mentioned this pull request Aug 24, 2018

Feature Request: Add optional message to shared files and folders via email #10559

Closed

nextcloud-bot mentioned this pull request Sep 11, 2018

Allow multiple share links #11169

Closed

5 tasks

nextcloud-bot mentioned this pull request Oct 9, 2018

Dav endpoint: add share note #11702

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow to add a personal note to a share #10218

allow to add a personal note to a share #10218

schiessle commented Jul 12, 2018 •

edited

Loading

schiessle commented Jul 12, 2018

nickvergessen Jul 12, 2018

schiessle Jul 12, 2018

schiessle commented Jul 12, 2018

nickvergessen commented Jul 12, 2018

nickvergessen Jul 12, 2018

nickvergessen Jul 12, 2018

nickvergessen Jul 12, 2018

schiessle commented Jul 12, 2018

nickvergessen Jul 13, 2018

nickvergessen Jul 13, 2018

nickvergessen Jul 13, 2018

nickvergessen Jul 13, 2018

nickvergessen commented Jul 13, 2018

schiessle commented Jul 13, 2018 •

edited

Loading

skjnldsv commented Jul 14, 2018

schiessle commented Jul 14, 2018 •

edited

Loading

skjnldsv commented Jul 14, 2018

schiessle commented Jul 16, 2018 •

edited

Loading

schiessle commented Jul 16, 2018 •

edited

Loading

skjnldsv commented Jul 16, 2018

jospoortvliet commented Jul 16, 2018

schiessle commented Jul 17, 2018

MorrisJobke commented Jul 18, 2018 •

edited

Loading

jancborchardt commented Jul 18, 2018

danxuliu commented Jul 21, 2018

skjnldsv commented Jul 24, 2018

allow to add a personal note to a share #10218

allow to add a personal note to a share #10218

Conversation

schiessle commented Jul 12, 2018 • edited Loading

schiessle commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiessle commented Jul 12, 2018

nickvergessen commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiessle commented Jul 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Jul 13, 2018

schiessle commented Jul 13, 2018 • edited Loading

skjnldsv commented Jul 14, 2018

schiessle commented Jul 14, 2018 • edited Loading

skjnldsv commented Jul 14, 2018

schiessle commented Jul 16, 2018 • edited Loading

schiessle commented Jul 16, 2018 • edited Loading

skjnldsv commented Jul 16, 2018

jospoortvliet commented Jul 16, 2018

schiessle commented Jul 17, 2018

MorrisJobke commented Jul 18, 2018 • edited Loading

jancborchardt commented Jul 18, 2018

danxuliu commented Jul 21, 2018

skjnldsv commented Jul 24, 2018

schiessle commented Jul 12, 2018 •

edited

Loading

schiessle commented Jul 13, 2018 •

edited

Loading

schiessle commented Jul 14, 2018 •

edited

Loading

schiessle commented Jul 16, 2018 •

edited

Loading

schiessle commented Jul 16, 2018 •

edited

Loading

MorrisJobke commented Jul 18, 2018 •

edited

Loading