pkp#10131 cleanup for Orcid implementation cleanup

taslangraham · Sep 5, 2024 · 43e51b3 · 43e51b3
1 parent 41b38eb
commit 43e51b3
Show file tree

Hide file tree

Showing 5 changed files with 79 additions and 74 deletions.
diff --git a/api/v1/orcid/OrcidController.php b/api/v1/orcid/OrcidController.php
@@ -17,6 +17,7 @@
 
 namespace PKP\API\v1\orcid;
 
+use APP\author\Author;
 use APP\facades\Repo;
 use Illuminate\Http\JsonResponse;
 use Illuminate\Http\Request;
@@ -72,45 +73,15 @@ public function getGroupRoutes(): void
     public function requestAuthorVerification(Request $illuminateRequest): JsonResponse
     {
         $context = $this->getRequest()->getContext();
-        if (!OrcidManager::isEnabled($context)) {
-            return response()->json([
-                'error' => __('api.orcid.403.orcidNotEnabled'),
-            ], Response::HTTP_FORBIDDEN);
-        }
-
-        $authorId = (int) $illuminateRequest->route('authorId');
-        $author = Repo::author()->get($authorId);
+        $validate = $this->validateRequest($illuminateRequest);
 
-        if (empty($author)) {
+        if ($validate['error']) {
             return response()->json([
-                'error' => __('api.orcid.404.authorNotFound'),
-            ], Response::HTTP_NOT_FOUND);
-        }
-
-        $user = $this->getRequest()->getUser();
-        $currentRoles = array_map(
-            function (Role $role) {
-                return $role->getId();
-            },
-            $user->getRoles($context->getId())
-        );
-
-        if (!array_intersect([Role::ROLE_ID_SITE_ADMIN, Role::ROLE_ID_MANAGER], $currentRoles)) {
-            $publicationId = $author->getData('publicationId');
-            $submissionId = Repo::publication()->get($publicationId)->getData('submissionId');
-
-            $editorAssignment = StageAssignment::withSubmissionIds([$submissionId])
-                ->withRoleIds([Role::ROLE_ID_SUB_EDITOR])
-                ->withUserId($user->getId())
-                ->first();
-
-            if ($editorAssignment === null) {
-                return response()->json([
-                    'error' => __('api.orcid.403.editWithoutPermission'),
-                ], Response::HTTP_FORBIDDEN);
-            }
+                'error' => $validate['error'],
+            ], $validate['status']);
         }
 
+        $author = $validate['author']; /** @var Author $author */
         try {
             dispatch(new SendAuthorMail($author, $context, true));
         } catch (\Exception $exception) {
@@ -128,51 +99,89 @@ function (Role $role) {
      */
     public function deleteForAuthor(Request $illuminateRequest): JsonResponse
     {
-        $context = $this->getRequest()->getContext();
-        if (!OrcidManager::isEnabled($context)) {
-            return response()->json([
-                'error' => __('api.orcid.403.orcidNotEnabled'),
-            ], Response::HTTP_FORBIDDEN);
+        $validate = $this->validateRequest($illuminateRequest);
+
+        if ($validate['error']) {
+            return response()->json(['error' => $validate['error'],], $validate['status']);
         }
 
-        $authorId = (int) $illuminateRequest->route('authorId');
-        $author = Repo::author()->get($authorId);
+        $author = $validate['author']; /** @var Author $author */
 
-        if (empty($author)) {
-            return response()->json([
-                'error' => __('api.orcid.404.authorNotFound'),
-            ], Response::HTTP_NOT_FOUND);
-        }
+        $author->setOrcid(null);
+        $author->setOrcidVerified(false);
+        OrcidManager::removeOrcidAccessToken($author);
+        Repo::author()->edit($author, []);
 
+        return response()->json([], Response::HTTP_OK);
+    }
+
+    /**
+     * Check if ORCID is enabled in the current context
+     */
+    private function isOrcidEnabled(): bool
+    {
+        $context = $this->getRequest()->getContext();
+        return OrcidManager::isEnabled($context);
+    }
+
+    /**
+     * Check if the current user has editor permissions
+     */
+    private function hasEditorPermissions(int $publicationId): bool
+    {
         $user = $this->getRequest()->getUser();
+        $context = $this->getRequest()->getContext();
         $currentRoles = array_map(
             function (Role $role) {
                 return $role->getId();
             },
             $user->getRoles($context->getId())
         );
 
-        if (!array_intersect([Role::ROLE_ID_SITE_ADMIN, Role::ROLE_ID_MANAGER], $currentRoles)) {
-            $publicationId = $author->getData('publicationId');
-            $submissionId = Repo::publication()->get($publicationId)->getData('submissionId');
+        if (array_intersect([Role::ROLE_ID_SITE_ADMIN, Role::ROLE_ID_MANAGER], $currentRoles)) {
+            return true;
+        }
+
+        $submissionId = Repo::publication()->get($publicationId)->getData('submissionId');
+        $editorAssignment = StageAssignment::withSubmissionIds([$submissionId])
+            ->withRoleIds([Role::ROLE_ID_SUB_EDITOR])
+            ->withUserId($user->getId())
+            ->first();
+
+        return $editorAssignment !== null;
+    }
+
+    /**
+     * Performs common validation logic for controller endpoints, return validated data or error information.
+     *
+     * @return array Returns an associative array containing either the validated data or error information
+     */
+    private function validateRequest(Request $illuminateRequest): array
+    {
+        if (!$this->isOrcidEnabled()) {
+            return [
+                'error' => __('api.orcid.403.orcidNotEnabled'),
+                'status' => Response::HTTP_FORBIDDEN
+            ];
+        }
+
+        $authorId = (int)$illuminateRequest->route('authorId');
+        $author = Repo::author()->get($authorId);
 
-            $editorAssignment = StageAssignment::withSubmissionIds([$submissionId])
-                ->withRoleIds([Role::ROLE_ID_SUB_EDITOR])
-                ->withUserId($user->getId())
-                ->first();
+        if (empty($author)) {
+            ['error' => __('api.orcid.404.authorNotFound'),
+                'status' => Response::HTTP_NOT_FOUND
+            ];
+        }
 
-            if ($editorAssignment === null) {
-                return response()->json([
-                    'error' => __('api.orcid.403.editWithoutPermission'),
-                ], Response::HTTP_FORBIDDEN);
-            }
+        if (!$this->hasEditorPermissions($author->getData('publicationId'))) {
+            return [
+                'error' => __('api.orcid.403.editWithoutPermission'),
+                'status' => Response::HTTP_FORBIDDEN
+            ];
         }
 
-        $author->setOrcid(null);
-        $author->setOrcidVerified(false);
-        OrcidManager::removeOrcidAccessToken($author);
-        Repo::author()->edit($author, []);
+        return ['author' => $author];
 
-        return response()->json([], Response::HTTP_OK);
     }
 }
diff --git a/classes/orcid/OrcidManager.php b/classes/orcid/OrcidManager.php
@@ -101,7 +101,7 @@ public static function isEnabled(?Context $context = null): bool
      */
     public static function getOrcidUrl(?Context $context = null): string
     {
-        $apiType = self::getApiType();
+        $apiType = self::getApiType($context);
         return in_array($apiType, [self::API_PUBLIC_PRODUCTION, self::API_MEMBER_PRODUCTION]) ? self::ORCID_URL : self::ORCID_URL_SANDBOX;
     }
 
@@ -112,7 +112,7 @@ public static function getOrcidUrl(?Context $context = null): string
      */
     public static function getApiPath(?Context $context = null): string
     {
-        $apiType = self::getApiType();
+        $apiType = self::getApiType($context);
 
         return match ($apiType) {
             self::API_PUBLIC_SANDBOX => self::ORCID_API_URL_PUBLIC_SANDBOX,
@@ -129,7 +129,7 @@ public static function getApiPath(?Context $context = null): string
      */
     public static function isSandbox(?Context $context = null): bool
     {
-        $apiType = self::getApiType();
+        $apiType = self::getApiType($context);
         return in_array($apiType, [self::API_PUBLIC_SANDBOX, self::API_MEMBER_SANDBOX]);
     }
 
@@ -208,7 +208,7 @@ public static function getCountry(?Context $context = null): string
      */
     public static function isMemberApiEnabled(?Context $context = null): bool
     {
-        $apiType = self::getApiType();
+        $apiType = self::getApiType($context);
         return in_array($apiType, [self::API_MEMBER_PRODUCTION, self::API_MEMBER_SANDBOX]);
     }
 

diff --git a/jobs/orcid/DepositOrcidSubmission.php b/jobs/orcid/DepositOrcidSubmission.php
@@ -39,7 +39,7 @@ public function __construct(
     /**
      * @inheritDoc
      */
-    public function handle()
+    public function handle(): void
     {
         // If the application is set to sandbox mode, it will not reach out to external services
         if (Config::getVar('general', 'sandbox', false)) {

diff --git a/jobs/orcid/RevokeOrcidToken.php b/jobs/orcid/RevokeOrcidToken.php
@@ -36,7 +36,7 @@ public function __construct(
     /**
      * @inheritDoc
      */
-    public function handle()
+    public function handle(): void
     {
         $token = $this->identity->getData('orcidAccessToken');
         $httpClient = Application::get()->getHttpClient();

diff --git a/jobs/orcid/SendAuthorMail.php b/jobs/orcid/SendAuthorMail.php
@@ -17,11 +17,8 @@
 namespace PKP\jobs\orcid;
 
 use APP\author\Author;
-use APP\core\Application;
 use APP\facades\Repo;
-use GuzzleHttp\Exception\ClientException;
 use Illuminate\Support\Facades\Mail;
-use PKP\config\Config;
 use PKP\context\Context;
 use PKP\jobs\BaseJob;
 use PKP\mail\mailables\OrcidCollectAuthorId;
@@ -30,7 +27,6 @@
 
 class SendAuthorMail extends BaseJob
 {
-
     public function __construct(
         private Author $author,
         private Context $context,