Merge pull request #505 from owncloud/remove-unsettable-settings-so-default-is-used

"list self action" should only affect file create, change, delete and restore

appinfo/application.php

@@ -64,9 +64,13 @@ class Application extends App {
 
 
 		$container->registerService('Consumer', function(IContainer $c) {
+			/** @var \OC\Server $server */
+			$server = $c->query('ServerContainer');
+
 			return new Consumer(
 				$c->query('ActivityData'),
-				$c->query('UserSettings')
+				$c->query('UserSettings'),
+				$server->getL10NFactory()
 			);
 		});
 
@@ -145,7 +149,6 @@ class Application extends App {
 				$c->query('ActivityL10N'),
 				$server->getActivityManager(),
 				$server->getURLGenerator(),
-				$c->query('UserSettings'),
 				$c->query('CurrentUID'),
 				$rssToken
 			);

appinfo/info.xml

@@ -15,7 +15,7 @@
 	<licence>AGPL</licence>
 	<author>Frank Karlitschek, Joas Schilling</author>
 	<shipped>true</shipped>
-	<version>2.3.1</version>
+	<version>2.3.2</version>
 	<default_enable/>
 	<types>
 		<filesystem/>

appinfo/update.php

@@ -18,6 +18,9 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>
  *
  */
+
+use OCP\DB\QueryBuilder\IQueryBuilder;
+
 $installedVersion = \OC::$server->getConfig()->getAppValue('activity', 'installed_version');
 $connection = \OC::$server->getDatabaseConnection();
 
@@ -47,3 +50,32 @@ if (version_compare($installedVersion, '1.2.2', '<')) {
 // Cron job for sending emails and pruning the activity list
 \OC::$server->getJobList()->add('OCA\Activity\BackgroundJob\EmailNotification');
 \OC::$server->getJobList()->add('OCA\Activity\BackgroundJob\ExpireActivities');
+
+// Delete notification settings that can not be changed, so we correctly fall
+// back to the default value.
+$deleteNotificationTypes = [];
+/**
+ * For now we need to do it manually because the other apps might not be loaded
+ * on the update.
+$notificationTypes = \OC::$server->getActivityManager()->getNotificationTypes('en');
+foreach ($notificationTypes as $type => $data) {
+	if (is_array($data) && isset($data['methods'])) {
+		if (!in_array(\OCP\Activity\IExtension::METHOD_MAIL, $data['methods'])) {
+			$deleteNotificationTypes[] = 'notify_email_' . $type;
+		}
+		if (!in_array(\OCP\Activity\IExtension::METHOD_STREAM, $data['methods'])) {
+			$deleteNotificationTypes[] = 'notify_stream_' . $type;
+		}
+	}
+}
+ */
+$deleteNotificationTypes[] = 'notify_stream_comments';
+$deleteNotificationTypes[] = 'notify_email_files_favorites';
+
+if (!empty($deleteNotificationTypes)) {
+	$query = $connection->getQueryBuilder();
+	$query->delete('preferences')
+		->where($query->expr()->eq('appid', $query->createNamedParameter('activity')))
+		->andWhere($query->expr()->in('configkey', $query->createNamedParameter($deleteNotificationTypes, IQueryBuilder::PARAM_STR_ARRAY)));
+	$query->execute();
+}

controller/settings.php

@@ -102,18 +102,22 @@ class Settings extends Controller {
 			$notify_setting_selfemail = false) {
 		$types = $this->data->getNotificationTypes($this->l10n);
 
-		foreach ($types as $type => $desc) {
-			$this->config->setUserValue(
-				$this->user, 'activity',
-				'notify_email_' . $type,
-				(int) $this->request->getParam($type . '_email', false)
-			);
+		foreach ($types as $type => $data) {
+			if (!is_array($data) || (isset($data['methods']) && in_array(IExtension::METHOD_MAIL, $data['methods']))) {
+				$this->config->setUserValue(
+					$this->user, 'activity',
+					'notify_email_' . $type,
+					(int) $this->request->getParam($type . '_email', false)
+				);
+			}
 
-			$this->config->setUserValue(
-				$this->user, 'activity',
-				'notify_stream_' . $type,
-				(int) $this->request->getParam($type . '_stream', false)
-			);
+			if (!is_array($data) || (isset($data['methods']) && in_array(IExtension::METHOD_STREAM, $data['methods']))) {
+				$this->config->setUserValue(
+					$this->user, 'activity',
+					'notify_stream_' . $type,
+					(int) $this->request->getParam($type . '_stream', false)
+				);
+			}
 		}
 
 		$email_batch_time = 3600;

lib/consumer.php

@@ -24,8 +24,10 @@ namespace OCA\Activity;
 
 use OCP\Activity\IConsumer;
 use OCP\Activity\IEvent;
+use OCP\Activity\IExtension;
 use OCP\Activity\IManager;
 use OCP\AppFramework\IAppContainer;
+use OCP\L10N\IFactory;
 
 class Consumer implements IConsumer {
 	/**
@@ -35,7 +37,7 @@ class Consumer implements IConsumer {
 	 * @param IAppContainer $container
 	 */
 	public static function register(IManager $am, IAppContainer $container) {
-		$am->registerConsumer(function() use ($am, $container) {
+		$am->registerConsumer(function() use ($container) {
 			return $container->query('Consumer');
 		});
 	}
@@ -46,15 +48,20 @@ class Consumer implements IConsumer {
 	/** @var UserSettings */
 	protected $userSettings;
 
+	/** @var IFactory */
+	protected $l10nFactory;
+
 	/**
 	 * Constructor
 	 *
 	 * @param Data $data
 	 * @param UserSettings $userSettings
+	 * @param IFactory $l10nFactory
 	 */
-	public function __construct(Data $data, UserSettings $userSettings) {
+	public function __construct(Data $data, UserSettings $userSettings, IFactory $l10nFactory) {
 		$this->data = $data;
 		$this->userSettings = $userSettings;
+		$this->l10nFactory = $l10nFactory;
 	}
 
 	/**
@@ -69,13 +76,26 @@ class Consumer implements IConsumer {
 		$emailSetting = $this->userSettings->getUserSetting($event->getAffectedUser(), 'email', $event->getType());
 		$emailSetting = ($emailSetting) ? $this->userSettings->getUserSetting($event->getAffectedUser(), 'setting', 'batchtime') : false;
 
+		$types = $this->data->getNotificationTypes($this->l10nFactory->get('activity'));
+		$typeData = $types[$event->getType()];
+
+		// User is not the author or wants to see their own actions
+		$createStream = !$selfAction || $this->userSettings->getUserSetting($event->getAffectedUser(), 'setting', 'self');
+		// User can not control the setting
+		$createStream = $createStream || is_array($typeData) && isset($typeData['methods']) && !in_array(IExtension::METHOD_STREAM, $typeData['methods']);
+
 		// Add activity to stream
-		if ($streamSetting && (!$selfAction || $this->userSettings->getUserSetting($event->getAffectedUser(), 'setting', 'self'))) {
+		if ($streamSetting && $createStream) {
 			$this->data->send($event);
 		}
 
+		// User is not the author or wants to see their own actions
+		$createEmail = !$selfAction || $this->userSettings->getUserSetting($event->getAffectedUser(), 'setting', 'selfemail');
+		// User can not control the setting
+		$createEmail = $createEmail || is_array($typeData) && isset($typeData['methods']) && !in_array(IExtension::METHOD_MAIL, $typeData['methods']);
+
 		// Add activity to mail queue
-		if ($emailSetting && (!$selfAction || $this->userSettings->getUserSetting($event->getAffectedUser(), 'setting', 'selfemail'))) {
+		if ($emailSetting && $createEmail) {
 			$latestSend = $event->getTimestamp() + $emailSetting;
 			$this->data->storeMail($event, $latestSend);
 		}

lib/data.php

@@ -212,12 +212,31 @@ class Data {
 		if ($filter === 'self') {
 			$query->andWhere($query->expr()->eq('user', $query->createNamedParameter($user)));
 
-		} else if ($filter === 'by' || $filter === 'all' && !$userSettings->getUserSetting($user, 'setting', 'self')) {
+		} else if ($filter === 'by') {
 			$query->andWhere($query->expr()->neq('user', $query->createNamedParameter($user)));
 
+		} else if ($filter === 'all' && !$userSettings->getUserSetting($user, 'setting', 'self')) {
+			$query->andWhere($query->expr()->orX(
+				$query->expr()->neq('user', $query->createNamedParameter($user)),
+				$query->expr()->notIn('type', $query->createNamedParameter([
+					'file_created',
+					'file_changed',
+					'file_deleted',
+					'file_restored',
+				], IQueryBuilder::PARAM_STR_ARRAY))
+			));
+
 		} else if ($filter === 'filter') {
 			if (!$userSettings->getUserSetting($user, 'setting', 'self')) {
-				$query->andWhere($query->expr()->neq('user', $query->createNamedParameter($user)));
+				$query->andWhere($query->expr()->orX(
+					$query->expr()->neq('user', $query->createNamedParameter($user)),
+					$query->expr()->notIn('type', $query->createNamedParameter([
+						'file_created',
+						'file_changed',
+						'file_deleted',
+						'file_restored',
+					], IQueryBuilder::PARAM_STR_ARRAY))
+				));
 			}
 
 			$query->andWhere($query->expr()->eq('object_type', $query->createNamedParameter($objectType)));

lib/navigation.php

@@ -43,9 +43,6 @@ class Navigation {
 	/** @var IURLGenerator */
 	protected $URLGenerator;
 
-	/** @var UserSettings */
-	protected $userSettings;
-
 	/** @var string */
 	protected $active;
 
@@ -61,7 +58,6 @@ class Navigation {
 	 * @param IL10N $l
 	 * @param IManager $manager
 	 * @param IURLGenerator $URLGenerator
-	 * @param UserSettings $userSettings
 	 * @param string $user
 	 * @param string $rssToken
 	 * @param null|string $active Navigation entry that should be marked as active
@@ -69,14 +65,12 @@ class Navigation {
 	public function __construct(IL10N $l,
 								IManager $manager,
 								IURLGenerator $URLGenerator,
-								UserSettings $userSettings,
 								$user,
 								$rssToken,
 								$active = 'all') {
 		$this->l = $l;
 		$this->activityManager = $manager;
 		$this->URLGenerator = $URLGenerator;
-		$this->userSettings = $userSettings;
 		$this->user = $user;
 		$this->active = $active;
 
@@ -126,18 +120,16 @@ class Navigation {
 			],
 		];
 
-		if ($this->user && $this->userSettings->getUserSetting($this->user, 'setting', 'self')) {
-			$topEntries[] = [
-				'id' => 'self',
-				'name' => (string) $this->l->t('Activities by you'),
-				'url' => $this->URLGenerator->linkToRoute('activity.Activities.showList', array('filter' => 'self')),
-			];
-			$topEntries[] = [
-				'id' => 'by',
-				'name' => (string) $this->l->t('Activities by others'),
-				'url' => $this->URLGenerator->linkToRoute('activity.Activities.showList', array('filter' => 'by')),
-			];
-		}
+		$topEntries[] = [
+			'id' => 'self',
+			'name' => (string) $this->l->t('Activities by you'),
+			'url' => $this->URLGenerator->linkToRoute('activity.Activities.showList', array('filter' => 'self')),
+		];
+		$topEntries[] = [
+			'id' => 'by',
+			'name' => (string) $this->l->t('Activities by others'),
+			'url' => $this->URLGenerator->linkToRoute('activity.Activities.showList', array('filter' => 'by')),
+		];
 
 		$additionalEntries = $this->activityManager->getNavigation();
 		$topEntries = array_merge($topEntries, $additionalEntries['top']);

templates/personal.php

@@ -47,7 +47,7 @@ style('activity', 'settings');
 	<br />
 	<input id="notify_setting_self" name="notify_setting_self" type="checkbox" class="checkbox"
 		value="1" <?php if ($_['notify_self']): ?> checked="checked"<?php endif; ?> />
-	<label for="notify_setting_self"><?php p($l->t('List your own actions in the stream')); ?></label>
+	<label for="notify_setting_self"><?php p($l->t('List your own file actions in the stream')); ?></label>
 	<br />
 	<input id="notify_setting_selfemail" name="notify_setting_selfemail" type="checkbox" class="checkbox"
 		value="1" <?php if ($_['notify_selfemail']): ?> checked="checked"<?php endif; ?> />

tests/backgroundjob/emailnotificationtest.php

@@ -80,12 +80,6 @@ class EmailNotificationTest extends TestCase {
 				'test1',
 				'test2',
 			]);
-		$mailQueueHandler->expects($this->any())
-			->method('getItemsForUsers')
-			->willReturn([
-				'test1' => [],
-				'test2' => [],
-			]);
 		$mailQueueHandler->expects($this->once())
 			->method('sendEmailToUser')
 			->with('test1', 'test1@localhost', 'de', date_default_timezone_get(), $this->anything());

tests/consumertest.php

@@ -23,7 +23,6 @@ namespace OCA\Activity\Tests;
 
 use OC\ActivityManager;
 use OCA\Activity\Consumer;
-use OCP\Activity\IConsumer;
 use OCP\DB;
 
 /**
@@ -39,6 +38,9 @@ class ConsumerTest extends TestCase {
 	/** @var \OCA\Activity\Data|\PHPUnit_Framework_MockObject_MockObject */
 	protected $data;
 
+	/** @var \OCP\L10N\IFactory|\PHPUnit_Framework_MockObject_MockObject */
+	protected $l10nFactory;
+
 	/** @var \OCA\Activity\UserSettings */
 	protected $userSettings;
 
@@ -55,6 +57,18 @@ class ConsumerTest extends TestCase {
 			->disableOriginalConstructor()
 			->getMock();
 
+		$l10n = $this->getMockBuilder('OCP\IL10N')
+			->disableOriginalConstructor()
+			->getMock();
+
+		$this->l10nFactory = $this->getMockBuilder('OCP\L10N\IFactory')
+			->disableOriginalConstructor()
+			->getMock();
+		$this->l10nFactory->expects($this->any())
+			->method('get')
+			->with('activity')
+			->willReturn($l10n);
+
 		$this->userSettings->expects($this->any())
 			->method('getUserSetting')
 			->with($this->stringContains('affectedUser'), $this->anything(), $this->anything())
@@ -111,7 +125,7 @@ class ConsumerTest extends TestCase {
 	 * @param array|false $expected
 	 */
 	public function testReceiveStream($type, $author, $affectedUser, $subject, $expected) {
-		$consumer = new Consumer($this->data, $this->userSettings);
+		$consumer = new Consumer($this->data, $this->userSettings, $this->l10nFactory);
 		$event = \OC::$server->getActivityManager()->generateEvent();
 		$event->setApp('test')
 			->setType($type)
@@ -146,7 +160,7 @@ class ConsumerTest extends TestCase {
 	 */
 	public function testReceiveEmail($type, $author, $affectedUser, $subject, $expected) {
 		$time = time();
-		$consumer = new Consumer($this->data, $this->userSettings);
+		$consumer = new Consumer($this->data, $this->userSettings, $this->l10nFactory);
 		$event = \OC::$server->getActivityManager()->generateEvent();
 		$event->setApp('test')
 			->setType($type)
@@ -176,7 +190,7 @@ class ConsumerTest extends TestCase {
 			$this->getMock('OCP\IUserSession'),
 			$this->getMock('OCP\IConfig')
 		);
-		$consumer = new Consumer($this->data, $this->userSettings);
+		$consumer = new Consumer($this->data, $this->userSettings, $this->l10nFactory);
 		$container = $this->getMock('\OCP\AppFramework\IAppContainer');
 		$container->expects($this->any())
 			->method('query')

tests/controller/settingstest.php

@@ -102,16 +102,28 @@ class SettingsTest extends TestCase {
 	public function testPersonalNonTypeSettings($expectedBatchTime, $notifyEmail, $notifyStream, $notifySettingBatchTime, $notifySettingSelf, $notifySettingSelfEmail) {
 		$this->data->expects($this->any())
 			->method('getNotificationTypes')
-			->willReturn(['NotificationTestTypeShared' => 'Share description']);
+			->willReturn([
+				'NotificationTestTypeShared' => 'Share description',
+				'NotificationTestNoStream' => [
+					'desc' => 'Email description',
+					'methods' => [IExtension::METHOD_MAIL],
+				],
+				'NotificationTestNoEmail' => [
+					'desc' => 'Stream description',
+					'methods' => [IExtension::METHOD_STREAM],
+				],
+			]);
 
 		$this->request->expects($this->any())
 			->method('getParam')
 			->willReturnMap([
 				['NotificationTestTypeShared_email', false, $notifyEmail],
 				['NotificationTestTypeShared_stream', false, $notifyStream],
+				['NotificationTestNoStream_email', false, $notifyEmail],
+				['NotificationTestNoEmail_stream', false, $notifyStream],
 			]);
 
-		$this->config->expects($this->exactly(5))
+		$this->config->expects($this->exactly(7))
 			->method('setUserValue');
 		$this->config->expects($this->at(0))
 			->method('setUserValue')
@@ -130,6 +142,22 @@ class SettingsTest extends TestCase {
 				$notifyStream
 			);
 		$this->config->expects($this->at(2))
+			->method('setUserValue')
+			->with(
+				'test',
+				'activity',
+				'notify_email_NotificationTestNoStream',
+				$notifyEmail
+			);
+		$this->config->expects($this->at(3))
+			->method('setUserValue')
+			->with(
+				'test',
+				'activity',
+				'notify_stream_NotificationTestNoEmail',
+				$notifyStream
+			);
+		$this->config->expects($this->at(4))
 			->method('setUserValue')
 			->with(
 				'test',
@@ -137,7 +165,7 @@ class SettingsTest extends TestCase {
 				'notify_setting_batchtime',
 				$expectedBatchTime
 			);
-		$this->config->expects($this->at(3))
+		$this->config->expects($this->at(5))
 			->method('setUserValue')
 			->with(
 				'test',
@@ -145,7 +173,7 @@ class SettingsTest extends TestCase {
 				'notify_setting_self',
 				$notifySettingSelf
 			);
-		$this->config->expects($this->at(4))
+		$this->config->expects($this->at(6))
 			->method('setUserValue')
 			->with(
 				'test',

tests/datatest.php

@@ -223,7 +223,7 @@ class DataTest extends TestCase {
 			['desc', 'self', 2],
 			['asc', 'by', 3],
 			['desc', 'by', 4],
-			['asc', 'filter', 3, 'object1', 23],
+			['asc', 'filter', 1, 'object1', 23],
 			['desc', 'filter', 4, 'object2', 42],
 		];
 	}

tests/navigationtest.php

@@ -25,6 +25,12 @@ use OC\ActivityManager;
 use OCA\Activity\Navigation;
 use OCA\Activity\Tests\Mock\Extension;
 
+/**
+ * Class NavigationTest
+ *
+ * @package OCA\Activity\Tests
+ * @group DB
+ */
 class NavigationTest extends TestCase {
 	public function getTemplateData() {
 		return array(
@@ -48,18 +54,10 @@ class NavigationTest extends TestCase {
 		$activityManager->registerExtension(function() use ($activityLanguage) {
 			return new Extension($activityLanguage, $this->getMock('\OCP\IURLGenerator'));
 		});
-		$userSettings = $this->getMockBuilder('OCA\Activity\UserSettings')
-			->disableOriginalConstructor()
-			->getMock();
-		$userSettings->expects($this->exactly(2))
-			->method('getUserSetting')
-			->with('test', 'setting', 'self')
-			->willReturn(true);
 		$navigation = new Navigation(
 			$activityLanguage,
 			$activityManager,
 			\OC::$server->getURLGenerator(),
-			$userSettings,
 			'test',
 			$rssToken,
 			$constructorActive