From f3c9a0a255b73c4e6e3509a248f8e1578d493e62 Mon Sep 17 00:00:00 2001
From: Roan Kattouw <roan.kattouw@gmail.com>
Date: Fri, 12 Apr 2019 15:34:28 -0700
Subject: [PATCH] AttributeManager: Check notify type availability before
 notifying

It was sort of checked in an implicit and roundabout way, in the sense
that the preferences page checks this and forces the relevant
preferences to on or off. But this was difficult to discover, and it's
not clear if this works correctly when defaults change.

Instead, explicitly check whether the requested notify type is available
for the event's category. Also rewrite the entire method to use
array_filter().

Change-Id: I9502a42fc705b2e56ef5edab433f1804f2924359
---
 includes/AttributeManager.php          | 20 +++++-------
 tests/phpunit/AttributeManagerTest.php | 45 ++++++++++++++++++++++----
 2 files changed, 46 insertions(+), 19 deletions(-)

diff --git a/includes/AttributeManager.php b/includes/AttributeManager.php
index c5a9e0c38..7853db528 100644
--- a/includes/AttributeManager.php
+++ b/includes/AttributeManager.php
@@ -132,19 +132,15 @@ class EchoAttributeManager {
 	 * @return string[]
 	 */
 	public function getUserEnabledEvents( User $user, $notifyType ) {
-		$eventTypesToLoad = $this->notifications;
-		foreach ( $eventTypesToLoad as $eventType => $eventData ) {
-			$category = $this->getNotificationCategory( $eventType );
-			// Make sure the user is eligible to receive this type of notification
-			if ( !$this->getCategoryEligibility( $user, $category ) ) {
-				unset( $eventTypesToLoad[$eventType] );
+		return array_values( array_filter(
+			array_keys( $this->notifications ),
+			function ( $eventType ) use ( $user, $notifyType ) {
+				$category = $this->getNotificationCategory( $eventType );
+				return $this->isNotifyTypeAvailableForCategory( $category, $notifyType ) &&
+					$this->getCategoryEligibility( $user, $category ) &&
+					$user->getOption( "echo-subscriptions-$notifyType-$category" );
 			}
-			if ( !$user->getOption( 'echo-subscriptions-' . $notifyType . '-' . $category ) ) {
-				unset( $eventTypesToLoad[$eventType] );
-			}
-		}
-
-		return array_keys( $eventTypesToLoad );
+		) );
 	}
 
 	/**
diff --git a/tests/phpunit/AttributeManagerTest.php b/tests/phpunit/AttributeManagerTest.php
index 099c658e2..93871b9ea 100644
--- a/tests/phpunit/AttributeManagerTest.php
+++ b/tests/phpunit/AttributeManagerTest.php
@@ -240,6 +240,9 @@ class EchoAttributeManagerTest extends MediaWikiTestCase {
 			'event_three' => [
 				'category' => 'category_three'
 			],
+			'event_four' => [
+				'category' => 'category_four'
+			]
 		];
 		$category = [
 			'category_one' => [
@@ -257,10 +260,22 @@ class EchoAttributeManagerTest extends MediaWikiTestCase {
 			'category_three' => [
 				'priority' => 10,
 			],
+			'category_four' => [
+				'priority' => 10,
+			]
 		];
-		$manager = new EchoAttributeManager( $notif, $category, [], [] );
-		$this->assertEquals( $manager->getUserEnabledEvents( $this->mockUser(), 'web' ),
-			[ 'event_two', 'event_three' ] );
+		$defaultNotifyTypeAvailability = [
+			'web' => true,
+			'email' => true,
+		];
+		$notifyTypeAvailabilityByCategory = [
+			'category_three' => [
+				'web' => false,
+				'email' => true,
+			]
+		];
+		$manager = new EchoAttributeManager( $notif, $category, $defaultNotifyTypeAvailability, $notifyTypeAvailabilityByCategory );
+		$this->assertEquals( [ 'event_two', 'event_four' ], $manager->getUserEnabledEvents( $this->mockUser(), 'web' ) );
 	}
 
 	public function testGetUserEnabledEventsbySections() {
@@ -279,6 +294,9 @@ class EchoAttributeManagerTest extends MediaWikiTestCase {
 			'event_four' => [
 				'category' => 'category_three',
 			],
+			'event_five' => [
+				'category' => 'category_five'
+			]
 		];
 		$category = [
 			'category_one' => [
@@ -290,26 +308,39 @@ class EchoAttributeManagerTest extends MediaWikiTestCase {
 			'category_three' => [
 				'priority' => 10
 			],
+			'category_five' => [
+				'priority' => 10
+			]
 		];
-		$manager = new EchoAttributeManager( $notif, $category, [], [] );
+		$defaultNotifyTypeAvailability = [
+			'web' => true,
+			'email' => true,
+		];
+		$notifyTypeAvailabilityByCategory = [
+			'category_five' => [
+				'web' => false,
+				'email' => true,
+			]
+		];
+		$manager = new EchoAttributeManager( $notif, $category, $defaultNotifyTypeAvailability, $notifyTypeAvailabilityByCategory );
 		$expected = [ 'event_one', 'event_three', 'event_four' ];
 		$actual = $manager->getUserEnabledEventsBySections( $this->mockUser(), 'web', [ 'alert' ] );
 		sort( $expected );
 		sort( $actual );
-		$this->assertEquals( $actual, $expected );
+		$this->assertEquals( $expected, $actual );
 
 		$expected = [ 'event_two' ];
 		$actual = $manager->getUserEnabledEventsBySections( $this->mockUser(), 'web', [ 'message' ] );
 		sort( $expected );
 		sort( $actual );
-		$this->assertEquals( $actual, $expected );
+		$this->assertEquals( $expected, $actual );
 
 		$expected = [ 'event_one', 'event_two', 'event_three', 'event_four' ];
 		$actual = $manager->getUserEnabledEventsBySections( $this->mockUser(), 'web',
 			[ 'message', 'alert' ] );
 		sort( $expected );
 		sort( $actual );
-		$this->assertEquals( $actual, $expected );
+		$this->assertEquals( $expected, $actual );
 	}
 
 	public static function getEventsByCategoryProvider() {