[master] Add emitters for user preferences #31307

sharidas · 2018-04-30T06:40:32Z

Description

Add events for user preference changes

Related Issue

Fixes #31264

Motivation and Context

How Has This Been Tested?

add unit tests

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-04-30T07:41:19Z

Codecov Report

Merging #31307 into master will increase coverage by 0.01%.
The diff coverage is 90.9%.

@@             Coverage Diff              @@
##             master   #31307      +/-   ##
============================================
+ Coverage     62.85%   62.86%   +0.01%     
+ Complexity    18375    18373       -2     
============================================
  Files          1151     1151              
  Lines         69067    69091      +24     
  Branches       1260     1260              
============================================
+ Hits          43412    43434      +22     
- Misses        25286    25288       +2     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`52.39% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`64.06% <90.9%> (+0.01%)`	`18373 <1> (-2)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
lib/base.php	`4.19% <0%> (-0.02%)`	`152 <0> (ø)`
lib/private/AllConfig.php	`93.15% <100%> (+1.15%)`	`43 <1> (-2)`	⬇️
lib/private/Server.php	`85.1% <100%> (+0.01%)`	`250 <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 9e85841...3476362. Read the comment docs.

sharidas · 2018-04-30T08:36:37Z

Backport PR: #31266

Add emitters for user preferences and unit tests Co-authored-by: Sujith H <sharidasan@owncloud.com> Signed-off-by: Vincent Petry <pvince81@owncloud.com> Signed-off-by: Sujith H <sharidasan@owncloud.com>

DeepDiver1975 · 2018-06-05T07:34:46Z

lib/private/AllConfig.php

 	 */
 	public function deleteAllUserValues($userId) {
 		// TODO - FIXME
 		$this->fixDIInit();

+		$arguments = ['uid' => $userId];
+		$this->eventDispatcher->dispatch('userpreferences.beforeDeleteUser', new GenericEvent(null, $arguments));
+


one day we need to use query builder everywhere .....

DeepDiver1975 · 2018-06-05T07:35:23Z

tests/lib/AllConfigTest.php

 	protected function getConfig($systemConfig = null, $connection = null) {
+		$this->eventDispatcher = new  EventDispatcher();


I would have a used a mock here .... no need to run all the event dispatcher logic
and there is one space too much ;-)

it's easier to use the real object instead of mocking all its methods (I tried to mock this one in another PR and gave up). To note is that we use a standalone dispatcher and not the real \OC::$server->getEventDispatcher() because that would register and trigger events for the whole system

only one method needs to me mocked and the assertion code is much less - see below

DeepDiver1975 · 2018-06-05T07:37:36Z

tests/lib/AllConfigTest.php

@@ -60,8 +67,45 @@ public function testSetUserValue() {
 		$selectAllSQL = 'SELECT `userid`, `appid`, `configkey`, `configvalue` FROM `*PREFIX*preferences` WHERE `userid` = ?';
 		$config = $this->getConfig();

+		$calledBeforeUserPrefSetVal = [];
+		$this->eventDispatcher->addListener('userpreferences.beforeSetValue',


This code should read much easier using a mock

$this->eventDispatcher->expects($this->once())->method('dispatch')->with('userpreferences.beforeDeleteApp', new GenericEvent(null, [....]))

PVince81 · 2018-06-05T08:54:21Z

@sharidas when backporting or forward porting please make sure it is clear. When reading this PR it looks like this is the original PR but it isn't. The original PR is the stable10 one and this one here is a forward port. And now you're getting review change requests for something that is just a port.

PVince81 · 2018-06-05T09:06:38Z

@DeepDiver1975 considering that this is a master port (wasn't clear from the first post), I'd rather keep the code as is. If you think this code must absolutely be changed then we shall do so in a separate PR that we will backport again to stable10.

sharidas · 2018-06-05T09:21:39Z

Sorry for the confusion created. This PR is a forward port from #31266 ( which is merged ).

phil-davis · 2018-06-05T09:43:50Z

The challenge with forward-ports is that extra CI in master (like codecov) might point out some issue or deficiency that is already in the code merged to stable10
In most cases, code should first be done in master. If the code concept needs to also be demonstrated for stable10 (e.g. related to behavior of old versions of dependencies or...) then it would be best to develop 2 PRs, 1 for each branch, to see that it can work for both. Then at the end make a "proper" backport from master ...

PVince81 · 2018-06-05T09:49:54Z

@phil-davis ok that makes sense. The original stable10 PR was created on said branch to save some time back then in case it was needed quickly. You pointed out good reasons to continue starting with master first.

DeepDiver1975 · 2018-06-05T09:53:10Z

@DeepDiver1975 considering that this is a master port (wasn't clear from the first post), I'd rather keep the code as is.

yes - agreed

If you think this code must absolutely be changed then we shall do so in a separate PR that we will backport again to stable10.

I'd really like this to be changed @sharidas please have a look if time permits and @PVince81 agrees as well

DeepDiver1975 · 2018-06-05T09:54:05Z

In most cases, code should first be done in master.

this is the prime rule for many years - whenever we broke this rule - code broke as well.

PVince81 · 2018-06-05T09:57:37Z

@sharidas has the requested changes ready, please push to a separate master PR which we will backport after review. Thanks

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

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.

ownclouders added the enhancement label Apr 30, 2018

ownclouders assigned sharidas Apr 30, 2018

sharidas added the 2 - Developing label Apr 30, 2018

sharidas force-pushed the master-symfony-user-preferences branch 2 times, most recently from 0bd8bad to c85685c Compare April 30, 2018 07:19

sharidas removed the 2 - Developing label Apr 30, 2018

sharidas mentioned this pull request Apr 30, 2018

[stable10] Add events for user preference changes #31266

Merged

10 tasks

sharidas changed the title ~~Master symfony user preferences~~ Add events for user preference changes and unit tests Apr 30, 2018

ownclouders added the status/STALE label May 8, 2018

sharidas force-pushed the master-symfony-user-preferences branch from c85685c to 0e34e6c Compare June 4, 2018 17:12

ownclouders added status/STALE and removed status/STALE labels Jun 4, 2018

sharidas changed the title ~~Add events for user preference changes and unit tests~~ Add emitters for user preferences Jun 4, 2018

ownclouders removed the status/STALE label Jun 4, 2018

Add emitters for user preferences

3476362

Add emitters for user preferences and unit tests Co-authored-by: Sujith H <sharidasan@owncloud.com> Signed-off-by: Vincent Petry <pvince81@owncloud.com> Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the master-symfony-user-preferences branch from 0e34e6c to 3476362 Compare June 5, 2018 06:08

ownclouders added status/STALE and removed status/STALE labels Jun 5, 2018

sharidas requested review from PVince81 and phil-davis June 5, 2018 06:49

sharidas added this to the development milestone Jun 5, 2018

DeepDiver1975 requested changes Jun 5, 2018

View reviewed changes

PVince81 changed the title ~~Add emitters for user preferences~~ [master] Add emitters for user preferences Jun 5, 2018

PVince81 merged commit a308c24 into master Jun 5, 2018

PVince81 deleted the master-symfony-user-preferences branch June 5, 2018 09:57

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

This was referenced Mar 19, 2019

Sync code for 'add events for user preference changes' 31266 and 31307 #34819

Merged

[stable10] Sync code for 'add events for user preference changes' 31266 and 31307 #34820

Merged

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

[master] Add emitters for user preferences #31307

[master] Add emitters for user preferences #31307

sharidas commented Apr 30, 2018

codecov bot commented Apr 30, 2018 •

edited

Loading

sharidas commented Apr 30, 2018

DeepDiver1975 Jun 5, 2018

DeepDiver1975 Jun 5, 2018

PVince81 Jun 5, 2018

DeepDiver1975 Jun 5, 2018

DeepDiver1975 Jun 5, 2018

PVince81 commented Jun 5, 2018

PVince81 commented Jun 5, 2018

sharidas commented Jun 5, 2018

phil-davis commented Jun 5, 2018 •

edited

Loading

PVince81 commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

PVince81 commented Jun 5, 2018

lock bot commented Jul 31, 2019

		protected function getConfig($systemConfig = null, $connection = null) {
		$this->eventDispatcher = new EventDispatcher();

[master] Add emitters for user preferences #31307

[master] Add emitters for user preferences #31307

Conversation

sharidas commented Apr 30, 2018

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

codecov bot commented Apr 30, 2018 • edited Loading

Codecov Report

sharidas commented Apr 30, 2018

DeepDiver1975 Jun 5, 2018

Choose a reason for hiding this comment

DeepDiver1975 Jun 5, 2018

Choose a reason for hiding this comment

PVince81 Jun 5, 2018

Choose a reason for hiding this comment

DeepDiver1975 Jun 5, 2018

Choose a reason for hiding this comment

DeepDiver1975 Jun 5, 2018

Choose a reason for hiding this comment

PVince81 commented Jun 5, 2018

PVince81 commented Jun 5, 2018

sharidas commented Jun 5, 2018

phil-davis commented Jun 5, 2018 • edited Loading

PVince81 commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

DeepDiver1975 commented Jun 5, 2018

PVince81 commented Jun 5, 2018

lock bot commented Jul 31, 2019

codecov bot commented Apr 30, 2018 •

edited

Loading

phil-davis commented Jun 5, 2018 •

edited

Loading