Trigger eventdispatcher for federated share by sharidas · Pull Request #29511 · owncloud/core

sharidas · 2017-11-08T17:28:27Z

Trigger event dispatcher for federated share
receiving.

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

Description

Change to trigger event dispatcher event for federated share receive.

Related Issue

Motivation and Context

This will help users to log events for fed share receive.

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 · 2017-11-08T21:18:24Z

Codecov Report

Merging #29511 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #29511      +/-   ##
============================================
+ Coverage     61.63%   61.69%   +0.06%     
  Complexity    17477    17477              
============================================
  Files          1044     1044              
  Lines         57693    57701       +8     
============================================
+ Hits          35557    35599      +42     
+ Misses        22136    22102      -34

Impacted Files	Coverage Δ	Complexity Δ
ocs/routes.php	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
apps/files_sharing/lib/External/Manager.php	`77.84% <100%> (+22.07%)`	`30 <0> (ø)`	⬇️
apps/federatedfilesharing/lib/RequestHandler.php	`38.88% <100%> (+1.23%)`	`80 <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 517df62...54de0bf. Read the comment docs.

PVince81 · 2017-11-09T09:05:09Z

why no injection?

Updated with injection.

PVince81 · 2017-11-09T09:06:48Z

I don't think the event name should match the class + method names.

As an event consumer, I'd rather expect the event name to be understandable and meaningful: what is this event about on a high-level ? The namespace also doesn't need to be one of a class. I think if we keep it to OCA\FederatedFileSharing::eventName it would be fine.

Updated the event names:

\OCA\FederatedFileSharing::local_shareReceived

\OCA\FederatedFileSharing::remote_shareReceived

sharidas · 2017-11-09T10:52:38Z

@PVince81 There is one issue which is bugging me. The Notifier is the place where I found the local fed shares are informed to the recipient. While doing manual testing, I found that lets say user delays to accept the fed share, then the logger keeps sending the message about arrival of share until the recipient accepts. I am pretty sure the admins wont be happy with this behaviour.

PVince81 · 2017-11-09T11:00:36Z

@sharidas hmm, can you dig into the root cause of this ? Maybe there is some pinging/polling happening. Somehow I don't remember such mechanism to exist so let's find out whether it's some kind of bug.

sharidas · 2017-11-09T15:16:27Z

There was an app, notification which was polling in my env. When I removed the app, and then refreshed the browser, I get the logs for received local fed share. No more repeated messages.

PVince81 · 2017-11-09T15:56:42Z

@sharidas we need to find a way to log the message only once even when notifications app is enabled. I still don't get why the notification app would trigger such event. Maybe the event trigger needs to be moved to a different more accurate location ?

sharidas · 2017-11-10T12:29:56Z

Verified this change as below:

fed shares within the local instance, users, works
fed shares with remote instance, users works
Verified that only one log data is there for the receive for a share. No more duplication of the log.

PVince81

👍 looks good

I've restarted the Drone build

PVince81 · 2017-11-10T12:40:12Z

I guess we don't have more detailed info for the hook ?

Updated with more details. I guess this should be enough.

PVince81 · 2017-11-10T16:14:49Z

+				$event = new GenericEvent(null, ['name' => $name, 'targetuser' => $sharedByFederatedId,
+								'owner' => $owner, 'sharewith' => $shareWith,
+								'sharedby' => $sharedBy, 'remoteid' => $remoteId]);
+				\OC::$server->getEventDispatcher()->dispatch('\OCA\FederatedFileSharing::remote_shareReceived', $event);


inject the event dispatcher ?

Trigger event dispatcher for federated share receiving. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81 · 2018-04-10T09:44:04Z

@sharidas please backport asap

sharidas · 2018-04-10T09:47:45Z

The backport PR is already merged to stable 10.
Backport PR: #29566

lock · 2019-07-31T11:02:29Z

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 self-assigned this Nov 8, 2017

sharidas added this to the development milestone Nov 8, 2017

sharidas added the 2 - Developing label Nov 8, 2017

sharidas force-pushed the fedsharelog-beforeacceptorreject branch from 877576b to 15fb709 Compare November 9, 2017 06:16

sharidas changed the title ~~[WIP] Trigger eventdispatcher for federated share~~ Trigger eventdispatcher for federated share Nov 9, 2017

sharidas added 3 - To Review and removed 2 - Developing labels Nov 9, 2017

sharidas requested a review from PVince81 November 9, 2017 07:00

PVince81 reviewed Nov 9, 2017

View reviewed changes

sharidas force-pushed the fedsharelog-beforeacceptorreject branch 2 times, most recently from 80ef9ff to 47b780d Compare November 9, 2017 10:46

sharidas force-pushed the fedsharelog-beforeacceptorreject branch from 47b780d to d135fca Compare November 9, 2017 15:13

sharidas force-pushed the fedsharelog-beforeacceptorreject branch from d135fca to 33b7ea0 Compare November 10, 2017 12:13

PVince81 approved these changes Nov 10, 2017

View reviewed changes

sharidas force-pushed the fedsharelog-beforeacceptorreject branch from 33b7ea0 to 52e0978 Compare November 10, 2017 15:32

PVince81 reviewed Nov 10, 2017

View reviewed changes

sharidas force-pushed the fedsharelog-beforeacceptorreject branch 2 times, most recently from fdc8506 to ff0c834 Compare November 13, 2017 09:09

Trigger eventdispatcher for federated share

54de0bf

Trigger event dispatcher for federated share receiving. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the fedsharelog-beforeacceptorreject branch from ff0c834 to 54de0bf Compare November 13, 2017 10:17

PVince81 merged commit 26f1baa into master Nov 14, 2017

PVince81 deleted the fedsharelog-beforeacceptorreject branch November 14, 2017 15:52

ownclouders added the status/STALE label Dec 20, 2017

PVince81 added the backport-request label Apr 10, 2018

ownclouders removed the status/STALE label Apr 10, 2018

sharidas mentioned this pull request Apr 10, 2018

[stable10] Trigger eventdispatcher for federated share #29566

Merged

9 tasks

PVince81 removed the backport-request label Apr 10, 2018

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

Uh oh!

Conversation

sharidas commented Nov 8, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

codecov Bot commented Nov 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PVince81 Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

sharidas Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

sharidas Nov 9, 2017

Choose a reason for hiding this comment

Uh oh!

sharidas commented Nov 9, 2017

Uh oh!

PVince81 commented Nov 9, 2017

Uh oh!

sharidas commented Nov 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PVince81 commented Nov 9, 2017

Uh oh!

sharidas commented Nov 10, 2017

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

sharidas Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 Nov 10, 2017

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Apr 10, 2018

Uh oh!

sharidas commented Apr 10, 2018

Uh oh!

lock Bot commented Jul 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Nov 8, 2017 •

edited

Loading

sharidas commented Nov 9, 2017 •

edited

Loading