Clear user notifications from table after they are deleted #221

sharidas · 2018-08-07T06:51:22Z

Clear user notifications from table after they are deleted.

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

codecov-io · 2018-08-07T07:53:30Z

Codecov Report

Merging #221 into master will increase coverage by 0.22%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #221      +/-   ##
============================================
+ Coverage     89.55%   89.78%   +0.22%     
- Complexity      134      137       +3     
============================================
  Files            18       18              
  Lines           536      548      +12     
============================================
+ Hits            480      492      +12     
  Misses           56       56

Flag	Coverage Δ	Complexity Δ
#phpunit	`89.78% <100%> (+0.22%)`	`137 <3> (+3)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/Handler.php	`99.3% <100%> (+0.02%)`	`33 <1> (+1)`	⬆️
lib/AppInfo/Application.php	`91.42% <100%> (+1.77%)`	`8 <2> (+2)`	⬆️
appinfo/app.php	`100% <100%> (ø)`	`0 <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 4452637...b258fea. Read the comment docs.

sharidas · 2018-08-07T08:41:14Z

Requires -> ~~Add method to clear notification when user is deletd core#32260~~

jvillafanez · 2018-08-08T08:58:25Z

lib/App.php

+	 * @param string $uid uid of the user
+	 * @since 10.0.10
+	 */
+	public function removeUserNotifications($uid) {


While I don't have a strong opinion against this, I'd prefer to keep the implementation as close as possible to the interface. If the interface doesn't have this public method, I'd prefer to remove it.

jvillafanez · 2018-08-08T08:59:58Z

tests/Unit/AppInfo/ApplicationTest.php

+
+	public function testUserDeleteEvent() {
+		\OC::$server->getEventDispatcher()->dispatch('user.afterdelete', new GenericEvent(null, ['uid' => 'foo']));
+		$this->assertInstanceOf(App::class, $this->container->query('OCA\Notifications\App'));


Not sure what you're testing here....

Was trying to please the code coverage. I will remove this test, and see if the coverage looks ok.

jvillafanez · 2018-08-08T10:45:38Z

tests/Unit/HandlerTest.php

@@ -23,6 +23,7 @@
 namespace OCA\Notifications\Tests\Unit;


+use OC\Notification\Notification;


Prefer public over private (OCP\Notification\INotification)

jvillafanez · 2018-08-08T10:46:16Z

tests/Unit/AppInfo/ApplicationTest.php

@@ -22,8 +22,10 @@

 namespace OCA\Notifications\Tests\Unit\AppInfo;

+use OCA\Notifications\App;


It doesn't seem to be used here. The GenericEvent neither.

Removed and updated.

jvillafanez · 2018-08-08T10:46:38Z

lib/AppInfo/Application.php

@@ -60,6 +64,11 @@ public function __construct (array $urlParams = array()) {
 		});
 		$container->registerCapability('Capabilities');

+		$container->getServer()->getEventDispatcher()->addListener('user.afterdelete', function (GenericEvent $event) use ($container) {
+			$handler = $container->query('OCA\Notifications\Handler');


Use Handler::class

Updated to Handler::class

jvillafanez · 2018-08-08T10:50:01Z

lib/AppInfo/Application.php

@@ -60,6 +64,11 @@ public function __construct (array $urlParams = array()) {
 		});
 		$container->registerCapability('Capabilities');

+		$container->getServer()->getEventDispatcher()->addListener('user.afterdelete', function (GenericEvent $event) use ($container) {


It might be worthy to move this block to another function, similarly to what we have for the setupConsumerAndNotifier function. Note that you'll need to call the function in the app.php file.

Moved this block to a new method in the new version of the pr.

jvillafanez · 2018-08-08T10:51:22Z

lib/AppInfo/Application.php

 use OCA\Notifications\Controller\EndpointController;
 use OCA\Notifications\Handler;
 use OCA\Notifications\App as NotificationApp;
+use OCA\Notifications\Mailer\NotificationMailer;


Check what of these new classes are being used here.

Removed the unused classes added due to this PR.

jvillafanez · 2018-08-08T10:53:05Z

Just some minor changes to keep the code as clean as possible.

PVince81 · 2018-08-08T13:36:38Z

@sharidas did you try adding tests for the new method in the Application class ?

sharidas · 2018-08-08T14:11:10Z

did you try adding tests for the new method in the Application class ?

Will update test for Application for new method.

sharidas · 2018-08-08T16:58:29Z

Added a test to increase coverage.

jvillafanez · 2018-08-08T17:13:33Z

tests/Unit/AppInfo/ApplicationTest.php

@@ -64,4 +69,53 @@ public function dataContainerQuery() {
 	public function testContainerQuery($service, $expected) {
 		$this->assertTrue($this->container->query($service) instanceof $expected);
 	}
+
+	public function testSetupSymfonyEventListeners() {
+		$app = $this->getMockBuilder(Application::class)


Why are you mocking the class you should be testing? 😕

I took this approach, because internally its calling getContainer.

Since this can't be unittested properly, just test the whole stack: add some notifications in the DB, and trigger the event. Make sure you clean up everything after the test (the listener and the information added in the DB table).

The other option is not to test this method, at least with unittests. Acceptance test should be able to cover the scenario.

Updated the test.. Tried to add a user, then added an entry into the notification table. Deleted the user.. Verified that entry is not there in the table.

Clear user notifications from table after they are deleted. Signed-off-by: Sujith H <sharidasan@owncloud.com>

PVince81

👍

PVince81 · 2018-08-20T08:22:31Z

@sharidas please backport to stable10

sharidas · 2018-08-20T10:05:57Z

Backport to stable10 #223

sharidas self-assigned this Aug 7, 2018

sharidas added the 2 - Developing label Aug 7, 2018

sharidas added this to the development milestone Aug 7, 2018

sharidas mentioned this pull request Aug 7, 2018

Add method to clear notification when user is deletd owncloud/core#32260

Closed

11 tasks

sharidas force-pushed the clear-notifications-after-user-deleted branch from f10f715 to 4d543ec Compare August 7, 2018 07:53

sharidas added 3 - To Review and removed 2 - Developing labels Aug 7, 2018

sharidas requested review from PVince81 and jvillafanez August 7, 2018 08:32

sharidas force-pushed the clear-notifications-after-user-deleted branch 3 times, most recently from 644c4dc to 437d372 Compare August 8, 2018 07:56

jvillafanez reviewed Aug 8, 2018

View reviewed changes

sharidas force-pushed the clear-notifications-after-user-deleted branch 2 times, most recently from a07dd21 to 2bf7c7a Compare August 8, 2018 10:38

jvillafanez reviewed Aug 8, 2018

View reviewed changes

sharidas force-pushed the clear-notifications-after-user-deleted branch from 2bf7c7a to a369ac6 Compare August 8, 2018 11:18

jvillafanez approved these changes Aug 8, 2018

View reviewed changes

sharidas force-pushed the clear-notifications-after-user-deleted branch from a369ac6 to abb1cd3 Compare August 8, 2018 16:46

jvillafanez reviewed Aug 8, 2018

View reviewed changes

Clear user notifications from table after they are deleted

b258fea

Clear user notifications from table after they are deleted. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the clear-notifications-after-user-deleted branch from abb1cd3 to b258fea Compare August 9, 2018 07:30

PVince81 approved these changes Aug 9, 2018

View reviewed changes

PVince81 merged commit db956fc into master Aug 20, 2018

PVince81 deleted the clear-notifications-after-user-deleted branch August 20, 2018 08:22

PVince81 added the backport-request label Aug 20, 2018

PVince81 mentioned this pull request Aug 20, 2018

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

Closed

sharidas mentioned this pull request Aug 20, 2018

[stable10] Backport of Clear user notifications from table after they… #223

Merged

phil-davis mentioned this pull request Sep 28, 2018

Test Results for 2.0.1 release owncloud/password_policy#139

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear user notifications from table after they are deleted #221

Clear user notifications from table after they are deleted #221

sharidas commented Aug 7, 2018

codecov-io commented Aug 7, 2018 •

edited

Loading

sharidas commented Aug 7, 2018 •

edited

Loading

jvillafanez Aug 8, 2018

jvillafanez Aug 8, 2018

sharidas Aug 8, 2018

jvillafanez Aug 8, 2018

sharidas Aug 8, 2018

jvillafanez Aug 8, 2018

sharidas Aug 8, 2018

jvillafanez Aug 8, 2018

sharidas Aug 8, 2018

jvillafanez Aug 8, 2018

sharidas Aug 8, 2018

jvillafanez Aug 8, 2018

sharidas Aug 8, 2018

jvillafanez commented Aug 8, 2018

PVince81 commented Aug 8, 2018

sharidas commented Aug 8, 2018

sharidas commented Aug 8, 2018

jvillafanez Aug 8, 2018

sharidas Aug 8, 2018

jvillafanez Aug 9, 2018

sharidas Aug 9, 2018

PVince81 left a comment

PVince81 commented Aug 20, 2018

sharidas commented Aug 20, 2018

		@@ -23,6 +23,7 @@
		namespace OCA\Notifications\Tests\Unit;


		use OC\Notification\Notification;

		@@ -22,8 +22,10 @@

		namespace OCA\Notifications\Tests\Unit\AppInfo;

		use OCA\Notifications\App;

Clear user notifications from table after they are deleted #221

Clear user notifications from table after they are deleted #221

Conversation

sharidas commented Aug 7, 2018

codecov-io commented Aug 7, 2018 • edited Loading

Codecov Report

sharidas commented Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jvillafanez commented Aug 8, 2018

PVince81 commented Aug 8, 2018

sharidas commented Aug 8, 2018

sharidas commented Aug 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

PVince81 commented Aug 20, 2018

sharidas commented Aug 20, 2018

codecov-io commented Aug 7, 2018 •

edited

Loading

sharidas commented Aug 7, 2018 •

edited

Loading