Accept user share notification #30106

PVince81 · 2018-01-11T18:49:03Z

Description

New notification to accept shares and pending shares list

Reusing the "oc_share.accepted" field to store share states.
Added checkbox in settings page to enable automatically accepting shares, this defaults to enabled currently. Unchecking this option will prompt users to accept or reject shares through notifications
Added pending and rejected shares in the "Shared with you" view
The "unshare from self" action now changes the state of shares to "Rejected" instead of deleting directly

Related Issue

#19153

Motivation and Context

How Has This Been Tested?

TODO - for testcases see #30106 (comment)

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

TODOs:

PVince81 · 2018-01-11T18:57:38Z

To test you need to untick the new auto accept option in the admin settings for sharing

jvillafanez · 2018-01-12T07:54:42Z

apps/files_sharing/lib/Notifier.php

+		switch ($notification->getSubject()) {
+			case 'local_share':
+				$params = $notification->getSubjectParameters();
+				if ($params[0] !== $params[1] && $params[1] !== null) {


Theorically, only share notifications should reach this point, and we expect that all the share notifications to have 2 parameters. Taking this into account, I'm not sure if we should add additional checks here.

jvillafanez · 2018-01-12T08:02:21Z

lib/private/Share20/Share.php

@@ -76,6 +79,7 @@ class Share implements \OCP\Share\IShare {
 	public function __construct(IRootFolder $rootFolder, IUserManager $userManager) {
 		$this->rootFolder = $rootFolder;
 		$this->userManager = $userManager;
+		$this->state = Constants::STATE_ACCEPTED;


I'm not sure if setting "accepted" as default value for the state is a good idea. Maybe setting null as default and enforcing that the state has a valid value just before saving it in the DB is a better idea.

jvillafanez · 2018-01-12T08:22:21Z

apps/files_sharing/lib/Notifier.php

+		// TODO: discard if target user is not in target group any more
+
+		switch ($notification->getSubject()) {
+			case 'local_share':


We should move this block to a private function. If it's implemented as "case" I expect more cases (public shares, federated shares, etc) that will eventually be implemented. Those cases will follow what is implemented here, so there will be more big blocks increasing the lines of this function making it more difficult to read.

this code is copied from the Notifier in the federatedfileshare app which has "remote_share" here, so not sure. We could provide a more generic share notifier with these type by refactoring.

My point isn't refactor, it's just move the code block to a private function in order to make this swicth block as compact as possible.

jvillafanez · 2018-01-12T08:23:50Z

apps/files_sharing/lib/Notifier.php

+					);
+				} else {
+					$notification->setParsedSubject(
+						(string)$l->t('User %1$s shared "%3$s" with you', $params)


Missing space after the typecasting (for consistency with the rest of the code)

PVince81 · 2018-01-16T10:51:17Z

if autoaccept disabled, do send a notification to the receiver that a new share was added (note: we could even do this as a separate PR as this can be standalone)
if autoaccept enabled, send notification to share sender about acceptation/rejection => we decided to not send any notification for now

codecov · 2018-01-16T11:34:49Z

Codecov Report

Merging #30106 into master will increase coverage by 0.09%.
The diff coverage is 82.13%.

@@             Coverage Diff              @@
##             master   #30106      +/-   ##
============================================
+ Coverage     62.72%   62.82%   +0.09%     
- Complexity    18292    18369      +77     
============================================
  Files          1148     1151       +3     
  Lines         68669    69051     +382     
  Branches       1234     1260      +26     
============================================
+ Hits          43076    43381     +305     
- Misses        25232    25301      +69     
- Partials        361      369       +8

Flag	Coverage Δ	Complexity Δ
#javascript	`52.39% <71.91%> (+0.34%)`	`0 <0> (ø)`	⬇️
#phpunit	`64.02% <86.51%> (+0.08%)`	`18369 <113> (+77)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
core/js/share.js	`32.56% <ø> (ø)`	`0 <0> (ø)`	⬇️
apps/files_sharing/list.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
settings/templates/panels/admin/filesharing.php	`17.5% <0%> (-0.93%)`	`0 <0> (ø)`
apps/files_sharing/templates/list.php	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
apps/files_sharing/appinfo/app.php	`40% <0%> (-1.87%)`	`0 <0> (ø)`
core/Migrations/Version20180302155233.php	`0% <0%> (ø)`	`1 <1> (?)`
...ederatedfilesharing/lib/FederatedShareProvider.php	`63.42% <100%> (+0.18%)`	`78 <1> (+1)`	⬆️
settings/Panels/Admin/FileSharing.php	`100% <100%> (ø)`	`6 <0> (ø)`	⬇️
apps/files_sharing/appinfo/routes.php	`98.5% <100%> (+0.2%)`	`0 <0> (ø)`	⬇️
lib/private/Share20/Manager.php	`97.21% <100%> (-0.03%)`	`227 <1> (-4)`
... and 20 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 0376174...10197f6. Read the comment docs.

PVince81 · 2018-01-16T11:35:54Z

Added notification when autoaccept is disabled. It includes a private link for the accepted share.

Also added notification icon.

PVince81 · 2018-01-19T15:44:47Z

TODO: migrate permissions=0 to accepted=1 and make sure old permissions zero code still works in case of old data

PVince81 · 2018-01-19T15:56:09Z

needs extensive testing with reshares...

PVince81 · 2018-01-19T16:00:05Z

TODO: feedback after rejecting/accepted button from notification:
- refresh file view
- display yellow notification

PVince81 · 2018-01-19T16:06:26Z

I've pushed some changes.

It is already possible to accept and reject shares from the notification buttons.
Unshare from self now implicitly triggers a rejection of a share. This means that user or group share are not getting deleted but just marked as rejected.

This is still very rough and I'm afraid we might have side effects. I'm not happy by the fact that we have a single endpoint "/shares" for both outgoing and received shares and also common code for formatShare() which is supposed to cover both cases. This is all still very messy and might need another round of refactoring, but probably another time as we don't want to break more.

I've added some todos above, we need to make sure this works with reshare / share initiator. It will anyway require a full retest of the sharing feature for user/group and also the "shared with me" and "shared with you" file lists, which seem to be affected by the share state somehow.

The endpoint "/shares?shared_with_me=true" now has an additional arguments "state":

if omitted, it defaults to 0 (accepted). This is to prevent breaking any clients which expect only receiving a list of already accepted shares
if set to empty string state= or state=all, it will retrieve shares with all possible states
if set to a specific value, it will return only the shares with the given state. (0 = accepted, 1 = pending, 2 = rejected)

From here on we'll be able to adjust the "shared with you" list to also visually display pending and rejected shares.

jvillafanez · 2018-01-22T09:01:54Z

if set to empty string state= or state=all, it will retrieve shares with all possible states

I don't think that state= and state=all should have the same behaviour. To have consistency across all the API calls, state= should either cause an error due to invalid arguments, or use the default value.

It might be nice if the state can accept a list, something like state=0,1 or state=0,2, but taking into account that I don't think there will be more states, it's probably too much work for a little gain. I don't have a use case where this could be useful at the moment.

PVince81 · 2018-01-22T09:29:53Z

I don't think that state= and state=all should have the same behaviour.

Thanks for your comment. I wasn't sure either.

"state=" should use the default value. Keep "all" for all.

I don't want to add muultiple state values as input as it would introduce a lot more logic for not much gain as you said.

PVince81 · 2018-02-01T15:46:22Z

Now struggling with the fileactions part: the "Shared with you" list already displays the pending shares now. We need to adjust the file actions menu to display "accept" and "reject" actions or others, but not the default actions.

Currently, the FileActions object was designed based on its legacy: to define file actions based on mime type + permissions. Ideally it should be refactored to provide custom actions with custom filtering behavior instead of mime/permissions. This way we could have the custom handler filter out actions based on the current file row that was clicked. Whatever change we do, we need to keep it compatible with the old way to avoid breaking apps that register their actions.

PVince81 · 2018-02-13T16:48:59Z

I've made a new PR to look into making file actions more flexible: #30478

PVince81 · 2018-02-26T15:25:42Z

Pending shares can now be accepted in "shared with you", but with many bugs:

prevent sidebar to open for pending/rejected shares
implement custom sort by state
prevent "unshare from self" from removing element: redisplay it in rejected mode instead
~~do we really need three states ?~~ => to be discussed after usability testing
missing star placeholder / alignment when pending share is the only entry

PVince81 · 2018-02-26T15:53:51Z

the favorite star is missing so the first column is not wide enough for pending shares:

I wonder if we could reuse that space to display the icon as a state ? @felixheidecke

jvillafanez · 2018-05-30T12:04:34Z

Rebased to fix conflict in the ManagerTest. Note that 7ded301 has been added due to a bad conflict resolution (probably) since that piece wasn't there before the rebase.

PVince81 · 2018-05-30T12:11:08Z

@jvillafanez seems you discarded @individual-it's commit, likely because you guys updated at the same time. @individual-it can you push it again ?

PVince81 · 2018-05-30T12:11:19Z

commit was 66584e1

- Reusing the "oc_share.accepted" field to store share states. - Added checkbox in settings page to enable automatically accepting shares, this defaults to enabled currently. Unchecking this option will prompt users to accept or reject shares through notifications - Added pending and rejected shares in the "Shared with you" view - The "unshare from self" action now changes the state of shares to "Rejected" instead of deleting directly Introduce updateShareForRecipient methods - New method updateShareForRecipient on share manager and default share provider to update both "files_target" and "accepted" fields. - Rewired moveShare and updateShareForRecipient to use updateShareForRecipient. - Moved most of the validation code to the share provider Migrate oc_share.permissions zero to accepted field Added migration that sets oc_share.accepted to 2 (rejected) for all sub-share entries that had oc_share.permissions=0. The zero permissions is left there to avoid expensive migration and is now gracefully ignored by the current code.

Improve visual share status (icons)

Shared with you can now be sorted by share state In that order: accepted, pending, rejected and then by file name. Better error handling when switching share state Pending share support for multiple received shares Whenever multiple shares for the same node are received through different, the accept/reject endpoint now properly sets the state for all related shares. Make pending shares unclickable Exclude share owner and initiatior from notifications Hack to make share file lists reload properly Fixes issue with Firefox soft reload due to hidden field keeping its value. Deduplicate share target directly when accepting share Solves issue where the order in which the deduplication is done in the mount provider doesn't always match the accepting order Resolve private link for shared with you Sharing's notification publisher now rely on symfony event Notification is sent on share.afterCreate and discarded on share.afterDelete Share object is now passed through share creation event so no need to get it. Remove notification on auto accept

remove class .disable-click; .share-state-rejected handles the job Default cursor for folder and sharer in pending state

better xpath for finding file on Trashbin use better behat steps in tests API test to reaccept shares enable apps via APPS_TO_ENABLE in api test script UI tests to check notifications API tests to check notifications check notifications with regular expressions enable auto_accept_share between tests api tests for Discard notification if target user is not member of the group anymore test notification link redirection

Remove the notification once an action has been taken Remote shares will keep the old behaviour

run sharing notifications tests enable notifications app

Fix retarget problem after accept / decline the share Fix deduplication logic and add tests for it Fix icon url for non-root ownCloud servers Code style fixes and adjust acceptance test Remove summary when the filelist is destroyed

only source file if its availiable notifications do show users display name, fix expectations in tests

phil-davis · 2018-05-30T14:15:49Z

I made the same changes in a new commit, rebased and pushed again. All is good with acceptance tests.

PVince81 · 2018-05-30T21:10:30Z

🎉 fantastic work, guys ! 🎉

@jvillafanez please backport to stable10

phil-davis · 2018-06-08T14:04:28Z

Backport stable10 #31613

lock · 2019-07-31T04:02:40Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

PVince81 added the 2 - Developing label Jan 11, 2018

PVince81 added this to the development milestone Jan 11, 2018

PVince81 self-assigned this Jan 11, 2018

PVince81 mentioned this pull request Jan 11, 2018

User Can Accept Shares from Other Users #19153

Closed

PVince81 force-pushed the accept-user-share branch from 0933bcf to 2837e77 Compare January 11, 2018 19:55

jvillafanez reviewed Jan 12, 2018

View reviewed changes

PVince81 modified the milestones: development, planned Jan 12, 2018

butonic added the blue-ticket label Jan 16, 2018

ownclouders added the status/STALE label Feb 9, 2018

PVince81 mentioned this pull request Feb 13, 2018

[WIP] More flexible file actions API in web UI #30478

Closed

9 tasks

ownclouders removed the status/STALE label Feb 13, 2018

PVince81 mentioned this pull request Feb 15, 2018

Add pending shares OCS endpoint #24439

Closed

ownclouders added the status/STALE label Feb 21, 2018

PVince81 force-pushed the accept-user-share branch from 5367bb4 to 7f12790 Compare February 26, 2018 11:19

ownclouders removed the status/STALE label Feb 26, 2018

Vincent Petry and others added 14 commits May 30, 2018 19:54

Remove icons, add gray filter

9e04938

Improve visual share status (icons)

Reformat CSS, remove -ms-filter (deprecated)

66677b3

remove class .disable-click; .share-state-rejected handles the job Default cursor for folder and sharer in pending state

Discard notification if target user is not member of the group anymore

9220344

Make sure events are only thrown by core

d607b66

Remove the notification once an action has been taken Remote shares will keep the old behaviour

fix fed-tests to use correct button to unshare from shared-with-you-page

44bdd4d

run sharing notifications tests enable notifications app

use the displayname in the notifications

ccfaf30

Fix retarget problem after accept / decline the share Fix deduplication logic and add tests for it Fix icon url for non-root ownCloud servers Code style fixes and adjust acceptance test Remove summary when the filelist is destroyed

source enviroment from file set by other steps

6d8632b

only source file if its availiable notifications do show users display name, fix expectations in tests

code style fixes

96189dc

Acceptance test review minor changes

18ab715

Remove test that was revived from a conflict resolution

021b9ae

fix wrong acceptance tests

10197f6

phil-davis force-pushed the accept-user-share branch from 7ded301 to 10197f6 Compare May 30, 2018 14:14

PVince81 merged commit b9bfa6c into master May 30, 2018

PVince81 deleted the accept-user-share branch May 30, 2018 21:10

jvillafanez mentioned this pull request Jun 1, 2018

[stable10] Accept user share stable10 #31613

Merged

9 tasks

phil-davis removed the backport-request label Jun 8, 2018

PVince81 modified the milestones: development, QA Jun 13, 2018

ownclouders mentioned this pull request Aug 6, 2018

NotifiactionManager should be able to clear notifications of a user #32243

Closed

lock bot locked as resolved and limited conversation to collaborators Jul 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accept user share notification #30106

Accept user share notification #30106

PVince81 commented Jan 11, 2018 •

edited by jvillafanez

Loading

PVince81 commented Jan 11, 2018

jvillafanez Jan 12, 2018

jvillafanez Jan 12, 2018

jvillafanez Jan 12, 2018

PVince81 Jan 12, 2018

jvillafanez Jan 12, 2018

PVince81 Jan 16, 2018

jvillafanez Jan 12, 2018

PVince81 commented Jan 16, 2018 •

edited

Loading

codecov bot commented Jan 16, 2018 •

edited

Loading

PVince81 commented Jan 16, 2018

PVince81 commented Jan 19, 2018 •

edited

Loading

PVince81 commented Jan 19, 2018

PVince81 commented Jan 19, 2018 •

edited

Loading

PVince81 commented Jan 19, 2018

jvillafanez commented Jan 22, 2018

PVince81 commented Jan 22, 2018 •

edited

Loading

PVince81 commented Feb 1, 2018

PVince81 commented Feb 13, 2018

PVince81 commented Feb 26, 2018 •

edited

Loading

PVince81 commented Feb 26, 2018

jvillafanez commented May 30, 2018

PVince81 commented May 30, 2018

PVince81 commented May 30, 2018

phil-davis commented May 30, 2018

PVince81 commented May 30, 2018

phil-davis commented Jun 8, 2018

lock bot commented Jul 31, 2019

Accept user share notification #30106

Accept user share notification #30106

Conversation

PVince81 commented Jan 11, 2018 • edited by jvillafanez Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

TODOs:

PVince81 commented Jan 11, 2018

jvillafanez Jan 12, 2018

Choose a reason for hiding this comment

jvillafanez Jan 12, 2018

Choose a reason for hiding this comment

jvillafanez Jan 12, 2018

Choose a reason for hiding this comment

PVince81 Jan 12, 2018

Choose a reason for hiding this comment

jvillafanez Jan 12, 2018

Choose a reason for hiding this comment

PVince81 Jan 16, 2018

Choose a reason for hiding this comment

jvillafanez Jan 12, 2018

Choose a reason for hiding this comment

PVince81 commented Jan 16, 2018 • edited Loading

codecov bot commented Jan 16, 2018 • edited Loading

Codecov Report

PVince81 commented Jan 16, 2018

PVince81 commented Jan 19, 2018 • edited Loading

PVince81 commented Jan 19, 2018

PVince81 commented Jan 19, 2018 • edited Loading

PVince81 commented Jan 19, 2018

jvillafanez commented Jan 22, 2018

PVince81 commented Jan 22, 2018 • edited Loading

PVince81 commented Feb 1, 2018

PVince81 commented Feb 13, 2018

PVince81 commented Feb 26, 2018 • edited Loading

PVince81 commented Feb 26, 2018

jvillafanez commented May 30, 2018

PVince81 commented May 30, 2018

PVince81 commented May 30, 2018

phil-davis commented May 30, 2018

PVince81 commented May 30, 2018

phil-davis commented Jun 8, 2018

lock bot commented Jul 31, 2019

PVince81 commented Jan 11, 2018 •

edited by jvillafanez

Loading

PVince81 commented Jan 16, 2018 •

edited

Loading

codecov bot commented Jan 16, 2018 •

edited

Loading

PVince81 commented Jan 19, 2018 •

edited

Loading

PVince81 commented Jan 19, 2018 •

edited

Loading

PVince81 commented Jan 22, 2018 •

edited

Loading

PVince81 commented Feb 26, 2018 •

edited

Loading