Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Emit postSetEnabled for user #26984

Merged
merged 4 commits into from
Feb 2, 2017
Merged

Emit postSetEnabled for user #26984

merged 4 commits into from
Feb 2, 2017

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Jan 19, 2017

Description

Implements user enabled/disabled event

Related Issue

#23970

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.

@VicDeo VicDeo added this to the 10.0 milestone Jan 19, 2017
@VicDeo
Copy link
Member Author

VicDeo commented Jan 20, 2017

@PVince81 is it good enough?

@PVince81
Copy link
Contributor

@VicDeo yeah. A unit test would be nice if possible.

@VicDeo
Copy link
Member Author

VicDeo commented Jan 20, 2017

@PVince81 possible. Added.

@PVince81
Copy link
Contributor

👍

@DeepDiver1975
Copy link
Member

This will not help. The emitter is on the user object. In order to consume the event a listener needs to be attached to each and every user. Pointless.

This is why the whole emitter concept is 💩

Please use the EventDispatcher - THX

@VicDeo
Copy link
Member Author

VicDeo commented Jan 23, 2017

@DeepDiver1975

This will not help.

Untrue. Tested both auto and manually, and works. ;)

Please use the EventDispatcher - THX

OK

@DeepDiver1975
Copy link
Member

Untrue. Tested both auto and manually, and works. ;)

show me the code which will listen to enabling of all users - good luck 😉

@VicDeo
Copy link
Member Author

VicDeo commented Jan 23, 2017

@DeepDiver1975 enabling / disabling is NOT a backend method anyway:

$this->config->setUserValue($this->uid, 'core', 'enabled', $enabled);

It can be done on per user level only. Because backend knows nothing about this operation.

With or without EventDispatcher IDK how to do it for all users ATM.

@@ -337,6 +337,9 @@ public function setEnabled($enabled) {
$this->enabled = $enabled;
$enabled = ($enabled) ? 'true' : 'false';
$this->config->setUserValue($this->uid, 'core', 'enabled', $enabled);
if ($this->emitter) {
$this->emitter->emit('\OC\User', 'postSetEnabled', [$this, $enabled]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace this with something like:

	$eventDispatcher = \OC::$server->getEventDispatcher();
	$eventDispatcher->dispatch('OC\User::postSetEnabled', $this');

then to listen, an app just does this:

$eventDispatcher = \OC::$server->getEventDispatcher();
$eventDispatcher->addListener(
	'OC\User::postSetEnabled',
	function(IUser $user) {
             // ...
        });

(the param of the callback might be something else, I don't remember exactly)

@PVince81
Copy link
Contributor

or without EventDispatcher IDK how to do it for all users ATM.

@VicDeo see https://github.com/owncloud/core/pull/26984/files#r97399288

@VicDeo
Copy link
Member Author

VicDeo commented Jan 23, 2017

@PVince81 Ok, let it be EventDispatcher.

$test->assertEquals('foo', $event->getSubject()->getUID());
};

$eventDispatcher = \OC::$server->getEventDispatcher();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please don't use the event listener from the container - this can cause side effects.
It should be enough to use a mock and assert on the dispatch function

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VicDeo please take care so that we can merge this one - THX

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to do so I need to inject EventDispatcher next to \OC\Hooks\Emitter 😭

*/
public function __construct($uid, $backend, $emitter = null, IConfig $config = null, $urlGenerator = null) {
public function __construct($uid, $backend, $emitter = null, IConfig $config = null,
$urlGenerator = null, EventDispatcher $eventDispatcher = null
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is $eventDispatcher ever passed into this ctor?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@VicDeo ^^^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DeepDiver1975 passed now.

@PVince81
Copy link
Contributor

PVince81 commented Feb 2, 2017

👍

@PVince81 PVince81 merged commit 660abad into master Feb 2, 2017
@PVince81 PVince81 deleted the dispatch-endisable branch February 2, 2017 15:53
@lock
Copy link

lock bot commented Aug 4, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants