Resolve PR review conversations

apps/files_sharing/js/app.js

@@ -214,16 +214,13 @@ OCA.Sharing.App = {
 		if (state === OC.Share.STATE_REJECTED) {
 			method = 'DELETE';
 		}
-
 		var endPoint = isRemote === true ? 'remote_shares/pending/' : 'shares/pending/';
 		var xhr = $.ajax({
 			url: OC.linkToOCS('apps/files_sharing/api/v1') + endPoint + encodeURIComponent(fileId) + '?format=json',
 			contentType: 'application/json',
 			dataType: 'json',
 			type: method,
-			data: JSON.stringify({
-				...(!!shareType && { shareType }),
-			}),
+			data: JSON.stringify((Boolean(shareType) ? { shareType: shareType } : {})),
 		});
 		xhr.fail(function(response) {
 			var message = '';
@@ -239,7 +236,7 @@ OCA.Sharing.App = {
 	_shareStateActionHandler: function(context, newState) {
 		var targetFileData = context.fileList.elementToFile(context.$file);
 		var isRemote = targetFileData.shareLocationType === 'remote';
-		const { shareType } = targetFileData;
+		const shareType = targetFileData.shareType;
 		function responseCallback(response, status) {
 			if (status === 'success') {
 				var meta = response.ocs.meta;

apps/files_sharing/js/sharedfilelist.js

@@ -346,7 +346,6 @@
 			files = _.chain(files)
 				// convert share data to file data
 				.map(function(share) {
-					const {} = share
 					var file = {
 						shareOwner: share.owner + '@' + share.remote.replace(/.*?:\/\//g, ""),
 						shareState: !!parseInt(share.accepted, 10) ? OC.Share.STATE_ACCEPTED : OC.Share.STATE_PENDING,

apps/files_sharing/lib/AppInfo/Application.php

@@ -121,6 +121,7 @@ public function __construct(array $urlParams = []) {
 					$uid
 				),
 				$server->getConfig(),
+				$server->getLogger(),
 				$uid
 			);
 		});

apps/files_sharing/lib/Controller/RemoteOcsController.php

@@ -30,6 +30,7 @@
 use OCP\Files\StorageNotAvailableException;
 use OCP\Files\StorageInvalidException;
 use OCP\IConfig;
+use OCP\ILogger;
 
 class RemoteOcsController extends OCSController {
 	/** @var IRequest */
@@ -46,26 +47,34 @@ class RemoteOcsController extends OCSController {
 	 */
 	protected $config;
 
+	/**
+	 * @var ILogger
+	 */
+	protected $logger;
+
 	/**
 	 * RemoteOcsController constructor.
 	 *
 	 * @param string $appName
 	 * @param IRequest $request
 	 * @param Manager $externalManager
 	 * @param IConfig config
+	 * @param ILogger $loggar
 	 * @param string $uid
 	 */
 	public function __construct(
 		$appName,
 		IRequest $request,
 		Manager $externalManager,
 		IConfig $config,
+		ILogger $logger,
 		$uid
 	) {
 		parent::__construct($appName, $request);
 		$this->request = $request;
 		$this->externalManager = $externalManager;
 		$this->config = $config;
+		$this->logger = $logger;
 		$this->uid = $uid;
 	}
 
@@ -156,9 +165,9 @@ public function getShares($includingPending = false) {
 				try {
 					$shares[] = $this->extendShareInfo($shareInfo);
 				} catch (StorageNotAvailableException $e) {
-					//TODO: Log the exception here? There are several logs already below the stack
+					$this->logger->logException($e, ['app' => 'files_sharing']);
 				} catch (StorageInvalidException $e) {
-					//TODO: Log the exception here? There are several logs already below the stack
+					$this->logger->logException($e, ['app' => 'files_sharing']);
 				}
 			}
 		}
@@ -181,7 +190,7 @@ function ($share) {
 				},
 				\array_merge(
 					$this->externalManager->getOpenShares(),
-					$groupExternalManager == null ? [] : $groupExternalManager->getOpenShares()
+					$groupExternalManager === null ? [] : $groupExternalManager->getOpenShares()
 				)
 			);
 			$shares = \array_merge($shares, $openShares);

apps/files_sharing/lib/Controller/Share20OcsController.php

@@ -1215,6 +1215,7 @@ private function getShareById($id, $recipient = null) {
 
 				continue;
 			} catch (ProviderException $e) {
+				// We should iterate all provider to find proper provider for given share
 				continue;
 			}
 		}
@@ -1300,11 +1301,12 @@ private function getSupportedShareTypes() {
 		$shareTypes = [];
 
 		foreach ($providersCapabilities as $capabilities) {
-			$shareTypes = array_merge($shareTypes, array_keys($capabilities));
+			foreach ($capabilities as $key => $value) {
+				$shareTypes[] = $key;
+			}
 		}
-
-		$shareTypes = array_keys(array_intersect(Share::CONVERT_SHARE_TYPE_TO_STRING, $shareTypes));
-
+
+		$shareTypes = \array_unique($shareTypes);
 		return $shareTypes;
 	}
 }

apps/files_sharing/lib/Controllers/ExternalSharesController.php

@@ -54,6 +54,7 @@ class ExternalSharesController extends Controller {
 	/** @var IConfig $config */
 	private $config;
 
+	public const group_share_type = "group";
 	/**
 	 * ExternalSharesController constructor.
 	 *
@@ -77,11 +78,6 @@ public function __construct(
 		$this->clientService = $clientService;
 		$this->dispatcher = $eventDispatcher;
 		$this->config = $config;
-		// Allow other apps to add an external manager for user-to-group shares
-		$managerClass = $this->config->getSystemValue('sharing.groupExternalManager');
-		if ($managerClass !== '') {
-			$this->groupExternalManager = \OC::$server->query($managerClass);
-		}
 	}
 
 	/**
@@ -92,8 +88,9 @@ public function __construct(
 	 */
 	public function index() {
 		$federatedGroupResult = [];
-		if ($this->groupExternalManager !== null) {
-			$federatedGroupResult = $this->groupExternalManager->getOpenShares();
+		$groupExternalManager = $this->initGroupManager();
+		if ($groupExternalManager !== null) {
+			$federatedGroupResult = $groupExternalManager->getOpenShares();
 		}
 		$result = array_merge($federatedGroupResult, $this->externalManager->getOpenShares());
 		return new JSONResponse($result);
@@ -108,11 +105,7 @@ public function index() {
 	 * @return JSONResponse
 	 */
 	public function create($id, $share_type) {
-		if ($share_type === "group" && $this->groupExternalManager !== null) {
-			$manager = $this->groupExternalManager;
-		} else {
-			$manager = $this->externalManager;
-		}
+		$manager = $this->getManagerForShareType($share_type);
 		$shareInfo = $manager->getShare($id);
 
 		if ($shareInfo !== false) {
@@ -163,9 +156,19 @@ public function destroy($id, $share_type) {
 		return new JSONResponse();
 	}
 
+	private function initGroupManager() {
+		// Allow other apps to add an external manager for user-to-group shares
+		$managerClass = $this->config->getSystemValue('sharing.groupExternalManager');
+		if ($managerClass !== '') {
+			return \OC::$server->query($managerClass);
+		}
+		return null;
+	}
+
 	private function getManagerForShareType($share_type) {
-		if ($share_type === "group" && $this->groupExternalManager !== null) {
-			$manager = $this->groupExternalManager;
+		$groupExternalManager = $this->initGroupManager();
+		if ($share_type === self::group_share_type && $groupExternalManager !== null) {
+			$manager = $groupExternalManager;
 		} else {
 			$manager = $this->externalManager;
 		}

apps/files_sharing/tests/Controller/RemoteOcsControllerTest.php

@@ -25,6 +25,7 @@
 use OCA\Files_Sharing\Controller\RemoteOcsController;
 use OCA\Files_Sharing\External\Manager;
 use OCP\IConfig;
+use OCP\ILogger;
 use OCP\IRequest;
 use PHPUnit\Framework\MockObject\MockObject;
 use Test\TestCase;
@@ -42,19 +43,24 @@ class RemoteOcsControllerTest extends TestCase {
 	/** @var IConfig */
 	protected $config;
 
+	/** @var ILogger */
+	protected $logger;
+
 	/** @var RemoteOcsController | MockObject */
 	protected $controller;
 
 	protected function setUp(): void {
 		$this->request = $this->createMock(IRequest::class);
 		$this->externalManager = $this->createMock(Manager::class);
 		$this->config = $this->createMock(IConfig::class);
-
+		$this->logger = $this->createMock(ILogger::class);
+
 		$this->controller = new RemoteOcsController(
 			$this->appName,
 			$this->request,
 			$this->externalManager,
 			$this->config,
+			$this->logger,
 			'user'
 		);
 	}

lib/private/Share20/Manager.php

@@ -1538,19 +1538,24 @@ public function getShareByToken($token) {
 					"shared file not found by token: $token for federated user share, try to check federated group share.",
 					['app' => __CLASS__]
 				);
-			}
-		}
-
-		if ($share === null) {
-			try {
-				$provider = $this->factory->getProviderForType(\OCP\Share::SHARE_TYPE_REMOTE_GROUP);
-				if ($provider !== null) {
-					$share = $provider->getShareByToken($token);
+				try {
+					$provider = $this->factory->getProviderForType(\OCP\Share::SHARE_TYPE_REMOTE_GROUP);
+					if ($provider !== null) {
+						$share = $provider->getShareByToken($token);
+					}
+				} catch (ShareNotFound $ex) {
+					$this->logger->error(
+						"shared file not found by token: $token for federated group share",
+						['app' => __CLASS__]
+					);
+				} catch (ProviderException $ex) {
+					$this->logger->logException(
+						$ex,
+						['app' => __CLASS__]
+					);
 				}
-			} catch (ProviderException $ex) {
 			}
 		}
-
 		if (self::shareHasExpired($share)) {
 			$this->activityManager->setAgentAuthor(IEvent::AUTOMATION_AUTHOR);
 			try {