Add symfony events for delete and create share #30930

sharidas · 2018-03-26T16:59:32Z

Add symfony events for delete share.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

Adding symfony events for delete share. This was missing in the code base.

Related Issue

Motivation and Context

Adding symfony events for delete share. This was missing in the codebase.

How Has This Been Tested?

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.

codecov · 2018-03-26T18:32:20Z

Codecov Report

Merging #30930 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #30930      +/-   ##
============================================
+ Coverage     62.31%   62.31%   +<.01%     
- Complexity    18208    18371     +163     
============================================
  Files          1142     1142              
  Lines         68190    68194       +4     
  Branches       1232     1232              
============================================
+ Hits          42494    42498       +4     
  Misses        25335    25335              
  Partials        361      361

Flag	Coverage Δ	Complexity Δ
#javascript	`52.04% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`63.49% <100%> (ø)`	`18371 <0> (+163)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
apps/files_sharing/lib/API/Share20OCS.php	`91.52% <ø> (-0.13%)`	`152 <0> (ø)`
lib/private/Share20/Manager.php	`97.01% <100%> (+0.04%)`	`212 <0> (ø)`	⬇️
lib/private/Server.php	`85% <100%> (+0.01%)`	`254 <0> (ø)`	⬇️

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 6e2b8aa...294bc9e. Read the comment docs.

PVince81 · 2018-03-27T12:07:45Z

lib/private/Share20/Manager.php

@@ -846,6 +846,8 @@ public function deleteShare(\OCP\Share\IShare $share) {
 		// Emit pre-hook
 		\OC_Hook::emit('OCP\Share', 'pre_unshare', $hookParams);

+		$beforeEvent = new GenericEvent(null, ['share' => $hookParams, 'shareObject' => $share]);
+		\OC::$server->getEventDispatcher()->dispatch('share.beforedelete', $beforeEvent);


please inject the event dispatcher with DI

Added to DI

PVince81 · 2018-03-27T12:08:33Z

tests/lib/Share20/ManagerTest.php

@@ -231,7 +232,26 @@ public function testDelete($shareType, $sharedWith) {
 			->method('post')
 			->with($hookListnerExpectsPost);

+		$calledBeforeEvent = [];
+		\OC::$server->getEventDispatcher()->addListener('share.beforedelete',


let's use a new EventDispatcher from now on: $this->eventDispatcher = new EventDispatcher() because reusing the existing one will pollute the server with events and will eventually cause side effects

Used $this->eventDispatcher = new EventDispatcher() instead of \OC::$server->getEventDispatcher().

jvillafanez · 2018-03-27T15:14:55Z

Note that expected events are "share.afterDelete" and "share.afterCreate", note the case-sensitive. The "share.afterCreate" event is still missing.

sharidas · 2018-03-28T12:56:02Z

@jvillafanez I have added event share.afterCreate.

sharidas · 2018-03-29T09:02:10Z

Not really getting the cause for drone failure.
Not able to reproduce the same in my local instance : http://paste.opensuse.org/29265128.

sharidas · 2018-03-29T11:01:14Z

Got the issue #30930 (comment). The drone was failing because of duplicate event "share.beforeCreate" and "share.afterCreate", at :

Removed the share.beforeCreate and share.afterCreate in this PR.

sharidas · 2018-03-29T13:21:34Z

Updated this PR, by moving the share.afterCreate and share.beforeCreate events to share manager. Have also tried to grab the data from the Share20OCS to Share20/Manager.php.

jvillafanez · 2018-04-03T07:48:16Z

apps/files_sharing/tests/API/Share20OCSTest.php

@@ -108,7 +108,8 @@ protected function setUp() {
 				['core', 'shareapi_default_permissions', \OCP\Constants::PERMISSION_ALL, \OCP\Constants::PERMISSION_READ | \OCP\Constants::PERMISSION_CREATE]
 			]));

-		$this->eventDispatcher = \OC::$server->getEventDispatcher();
+		//$this->eventDispatcher = \OC::$server->getEventDispatcher();


unnedeed comment

jvillafanez · 2018-04-03T07:52:06Z

Taking into account that the events will be thrown by core, I don't know if we should create custom public events instead of using the GenericEvent one.

Other than that, the code looks fine

PVince81 · 2018-04-03T08:25:45Z

I don't know if we should create custom public events instead of using the GenericEvent one.

I'm also undecided here. In many places we already use GenericEvent so let's stick with it when it works.

Add symfony events for delete share. And move create share to share manager. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81 · 2018-04-06T10:05:11Z

lib/private/Share20/Manager.php

@@ -635,6 +640,9 @@ public function createShare(\OCP\Share\IShare $share) {
 		];
 		\OC_Hook::emit('OCP\Share', 'pre_shared', $preHookData);

+		$beforeEvent = new GenericEvent(null, ['shareData' => $preHookData]);
+		$this->eventDispatcher->dispatch('share.beforeCreate', $beforeEvent);


this would break any listener that was relying on the old formatter pre-hook data.

this is only ok if the beforeCreate change was never released before, else this is an API change

ok, I see that this event did not exist in 10.0.7.

this makes this change critical for 10.0.8 as we don't want to introduce the old event in 10.0.8 and change it again

PVince81

👍

PVince81 · 2018-04-06T10:07:00Z

@sharidas please backport, p1-urgent because the current stable10 code would introduce the old hook format and this PR adjusts it to the correct one.

sharidas · 2018-04-06T11:20:19Z

Backported PR: #31026

lock · 2019-07-31T13:02:02Z

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.

sharidas added the 3 - To Review label Mar 26, 2018

sharidas added this to the development milestone Mar 26, 2018

sharidas self-assigned this Mar 26, 2018

sharidas requested a review from PVince81 March 26, 2018 16:59

PVince81 reviewed Mar 27, 2018

View reviewed changes

PVince81 mentioned this pull request Mar 27, 2018

Accept user share notification #30106

Merged

19 tasks

sharidas force-pushed the add-symfony-events-for-deleteshare branch 2 times, most recently from 0bafe2b to 30b1178 Compare March 28, 2018 12:54

sharidas changed the title ~~Add symfony events for delete share~~ Add symfony events for delete and create share Mar 28, 2018

sharidas changed the title ~~Add symfony events for delete and create share~~ Add symfony events for delete and share Mar 28, 2018

sharidas force-pushed the add-symfony-events-for-deleteshare branch from 30b1178 to 7b19884 Compare March 29, 2018 06:08

sharidas force-pushed the add-symfony-events-for-deleteshare branch from 7b19884 to ff554ba Compare March 29, 2018 10:58

sharidas changed the title ~~Add symfony events for delete and share~~ Add symfony events for delete share Mar 29, 2018

sharidas force-pushed the add-symfony-events-for-deleteshare branch from ff554ba to 09df813 Compare March 29, 2018 13:19

sharidas force-pushed the add-symfony-events-for-deleteshare branch from 09df813 to 403a046 Compare March 29, 2018 14:28

sharidas changed the title ~~Add symfony events for delete share~~ Add symfony events for delete and create share Mar 29, 2018

sharidas force-pushed the add-symfony-events-for-deleteshare branch from 403a046 to 13f93ee Compare March 30, 2018 07:34

PVince81 requested a review from jvillafanez April 3, 2018 07:26

jvillafanez reviewed Apr 3, 2018

View reviewed changes

sharidas force-pushed the add-symfony-events-for-deleteshare branch from 13f93ee to 8fdcf74 Compare April 3, 2018 07:53

Add symfony events for delete and create share

294bc9e

Add symfony events for delete share. And move create share to share manager. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the add-symfony-events-for-deleteshare branch from 8fdcf74 to 294bc9e Compare April 3, 2018 18:01

PVince81 reviewed Apr 6, 2018

View reviewed changes

PVince81 approved these changes Apr 6, 2018

View reviewed changes

PVince81 merged commit bedfb50 into master Apr 6, 2018

PVince81 deleted the add-symfony-events-for-deleteshare branch April 6, 2018 10:06

sharidas mentioned this pull request Apr 6, 2018

[stable10] Add symfony events for delete and create share #31026

Merged

9 tasks

ownclouders mentioned this pull request Apr 25, 2018

Symfony events for user preference changes #31264

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

Add symfony events for delete and create share #30930

Add symfony events for delete and create share #30930

sharidas commented Mar 26, 2018

codecov bot commented Mar 26, 2018 •

edited

Loading

PVince81 Mar 27, 2018

sharidas Mar 28, 2018

PVince81 Mar 27, 2018

sharidas Mar 28, 2018

jvillafanez commented Mar 27, 2018

sharidas commented Mar 28, 2018

sharidas commented Mar 29, 2018

sharidas commented Mar 29, 2018 •

edited

Loading

sharidas commented Mar 29, 2018

jvillafanez Apr 3, 2018

jvillafanez commented Apr 3, 2018

PVince81 commented Apr 3, 2018

PVince81 Apr 6, 2018

PVince81 Apr 6, 2018

PVince81 left a comment

PVince81 commented Apr 6, 2018

sharidas commented Apr 6, 2018

lock bot commented Jul 31, 2019

Add symfony events for delete and create share #30930

Add symfony events for delete and create share #30930

Conversation

sharidas commented Mar 26, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Mar 26, 2018 • edited Loading

Codecov Report

PVince81 Mar 27, 2018

Choose a reason for hiding this comment

sharidas Mar 28, 2018

Choose a reason for hiding this comment

PVince81 Mar 27, 2018

Choose a reason for hiding this comment

sharidas Mar 28, 2018

Choose a reason for hiding this comment

jvillafanez commented Mar 27, 2018

sharidas commented Mar 28, 2018

sharidas commented Mar 29, 2018

sharidas commented Mar 29, 2018 • edited Loading

sharidas commented Mar 29, 2018

jvillafanez Apr 3, 2018

Choose a reason for hiding this comment

jvillafanez commented Apr 3, 2018

PVince81 commented Apr 3, 2018

PVince81 Apr 6, 2018

Choose a reason for hiding this comment

PVince81 Apr 6, 2018

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Apr 6, 2018

sharidas commented Apr 6, 2018

lock bot commented Jul 31, 2019

codecov bot commented Mar 26, 2018 •

edited

Loading

sharidas commented Mar 29, 2018 •

edited

Loading