[PKP-LIB][main] #10630 Add support for add existing user to a context #10631

defstat · 2024-11-24T20:41:03Z

No description provided.

ewhanson

Looks good, @defstat. Just a few comments/questions. Thanks!

ewhanson · 2024-12-12T18:55:34Z

api/v1/invitations/InvitationController.php

+        try {
+            return $this->selectedHandler->cancel();
+        } catch (\Exception $e) {
+            return response()->json([], Response::HTTP_INTERNAL_SERVER_ERROR);


It would be helpful if the error text could be included as is done below (e.g. ['error' => $e->getMessage]).

Changed to

try { return $this->selectedHandler->cancel(); } catch (\Exception $e) { return response()->json([ 'error' => $e->getMessage() ], Response::HTTP_INTERNAL_SERVER_ERROR); }

ewhanson · 2024-12-12T19:00:57Z

...nvitation/invitations/userRoleAssignment/handlers/api/UserRoleAssignmentCreateController.php

+        $result = $this->invitation->updateStatus(InvitationStatus::CANCELLED);
+
+        if (!$result) {
+            return response()->json([], Response::HTTP_CONFLICT);


Could a more descriptive error also be included? It seems like the previous iteration included some text about why it couldn't be canceled. I think in context, a 409 response might not be descriptive enough.

I agree. I have changed that to

if (!$result) { return response()->json([ 'error' => __('invitation.api.error.operationFailed') ], Response::HTTP_UNPROCESSABLE_ENTITY); }

where

msgid "invitation.api.error.operationFailed" msgstr "This invitation operation failed"

ewhanson · 2024-12-12T19:01:57Z

...nvitation/invitations/userRoleAssignment/handlers/api/UserRoleAssignmentCreateController.php

+        $mailable = $this->invitation->getMailable();
+
+        if (!isset($mailable)) {
+            return response()->json([], Response::HTTP_NOT_FOUND);


Same here. Could a additional description of what was not found, e.g. invitation.api.error.invitationTypeNotHasMailable be returned as well?

Sure! Changed to

if (!isset($mailable)) { return response()->json([ 'error' => __('invitation.api.error.invitationTypeNotHasMailable') ], Response::HTTP_NOT_FOUND); }

ewhanson · 2024-12-12T19:08:00Z

classes/invitation/invitations/userRoleAssignment/UserRoleAssignmentInvite.php

@@ -35,6 +35,7 @@
 use PKP\invitation\invitations\userRoleAssignment\rules\UserMustExistRule;
 use PKP\mail\mailables\UserRoleAssignmentInvitationNotify;
 use PKP\security\Validation;
+use PKP\user\User;


This import isn't used anywhere.

ewhanson · 2024-12-12T19:19:19Z

...invitation/invitations/userRoleAssignment/resources/BaseUserRoleAssignmentInviteResource.php

+use Illuminate\Http\Request;
+use Illuminate\Http\Resources\Json\JsonResource;
+use PKP\invitation\invitations\userRoleAssignment\payload\UserRoleAssignmentInvitePayload;
+use PKP\user\User;


This User import isn't used.

Isn't it used at

protected function transformUser(?User $user): ?array

and

protected function createNewUserFromPayload(UserRoleAssignmentInvitePayload $payload): User

?

Sorry about that. You're right. I think that comment was meant for classes/invitation/invitations/userRoleAssignment/resources/UserRoleAssignmentInviteManagerDataResource.php instead.

ewhanson · 2024-12-12T19:30:12Z

...ation/invitations/userRoleAssignment/handlers/UserRoleAssignmentInviteRedirectController.php

getInvitation() below has a typehint of UserRoleAssignmentInvite but it's not clear that $this->invitation will be of that specific type rather than the broader Invitation type in the parent class.

This is a valid comment, and I had also considered this when the code was developed.

My goal was to maintain $invitation at the parent level to allow shared functionality, while also specifying the correct type in the child class.

To achieve this, I added:

public function getInvitation(): UserRoleAssignmentInvite { return $this->invitation; }

at the child class level to ensure type specificity.

Additionally, since Invitation is an abstract class, it cannot be instantiated directly, which ensures that $invitation will always be an instance of a concrete subclass.

However, I understand that this design might not make the intended behavior obvious. Do you have any suggestions for making the returned types clearer?

Hmm... that's a good question. I haven't tried it, but if you redefined $invitation at the top of the file with the more specific concrete class, that would probably solve the problem I had, but I'm not sure if that will cause different problems. What do you think?

Thanks, @ewhanson! Adding $invitation in the child class is possible, but I feel it doesn’t entirely address your initial comment. It might still leave some ambiguity about the type consistency across the parent and child classes.

What are your thoughts on using generics through PHPDoc annotations? While PHP doesn't natively support generics, these annotations can improve clarity and help with IDE comments and general code analysis. However, I understand that this won’t enforce the type at runtime, so it might not fully address the overall concern.

My intent with this design was to guide developers to use the getInvitation() function rather than accessing $this->invitation directly. This ensures type safety and keeps the interaction consistent.

I’m open to other ideas or suggestions - I find this topic quite interesting!

Thanks, @defstat! I agree this is a tricky one! I think the idea of using generics in a PHPDoc comment may be the best way to go and keep the actual type hint the underlying Invitation type is probably the best way to go. I also agree trying to get developers to use the getInvitation() is a good call!

@ewhanson I have pushed the change - Would you like to take a look?

@defstat, I'm not super familiar with doing this in PHP, but it looks good to me. If it causes issues in the future we can always update it. Thanks!

ewhanson · 2024-12-12T19:34:27Z

...ation/invitations/userRoleAssignment/handlers/UserRoleAssignmentInviteRedirectController.php

If preRedirectActions() at the bottom doesn't return anything, both it and the method in the abstract class should probably be typehinted as returning void.

ewhanson · 2024-12-12T19:36:33Z

...ses/invitation/invitations/userRoleAssignment/resources/UserRoleAssignmentInviteResource.php

Both the Repo and JsonResource imports are no longer used.

defstat · 2024-12-16T18:55:51Z

@ewhanson thanks for the review. I have added the changes for the review comments that I have no follow-up questions. Also I rebased the branch on current main branch.

ewhanson · 2024-12-16T18:57:12Z

Thanks @defstat, I can have another look.

ewhanson · 2024-12-17T18:03:35Z

Thanks @defstat, I left two responses to your questions, but other than that, it looks good to me. 👍

touhidurabir · 2024-12-29T08:54:55Z

api/v1/invitations/InvitationController.php

@@ -124,7 +126,7 @@ public function getGroupRoutes(): void
            ]),
        ])->group(function () {

-            Route::get('', $this->getMany(...))
+            Route::get('{type}', $this->getMany(...))


@defstat this change is causing all tests break with latest pull of pkp-lib main branch . With the add of route param type, which probably has not been considered in some other places(from where AJAX request are made) cause the following exception

[29-Dec-2024 08:20:56 UTC] Symfony\Component\HttpKernel\Exception\NotFoundHttpException: The route publicknowledge/api/v1/invitations could not be found. in /Users/abir/Sites/code/ojs-main/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php:44 Stack trace: #0 /Users/abir/Sites/code/ojs-main/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Routing/RouteCollection.php(163): Illuminate\Routing\AbstractRouteCollection->handleMatchedRoute(Object(Illuminate\Http\Request), NULL) #1 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/core/PKPBaseController.php(104): Illuminate\Routing\RouteCollection->match(Object(Illuminate\Http\Request)) #2 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/middleware/traits/HasRequiredMiddleware.php(53): PKP\core\PKPBaseController::getRequestedRoute(Object(Illuminate\Http\Request)) #3 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/middleware/DecodeApiTokenWithValidation.php(64): PKP\middleware\DecodeApiTokenWithValidation->hasRequiredMiddleware(Object(Illuminate\Http\Request)) #4 /Users/abir/Sites/code/ojs-main/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): PKP\middleware\DecodeApiTokenWithValidation->handle(Object(Illuminate\Http\Request), Object(Closure)) #5 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/middleware/SetupContextBasedOnRequestUrl.php(63): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request)) #6 /Users/abir/Sites/code/ojs-main/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): PKP\middleware\SetupContextBasedOnRequestUrl->handle(Object(Illuminate\Http\Request), Object(Closure)) #7 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/middleware/AllowCrossOrigin.php(34): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request)) #8 /Users/abir/Sites/code/ojs-main/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(183): PKP\middleware\AllowCrossOrigin->handle(Object(Illuminate\Http\Request), Object(Closure)) #9 /Users/abir/Sites/code/ojs-main/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Pipeline/Pipeline.php(119): Illuminate\Pipeline\Pipeline->Illuminate\Pipeline\{closure}(Object(Illuminate\Http\Request)) #10 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/handler/APIHandler.php(102): Illuminate\Pipeline\Pipeline->then(Object(Closure)) #11 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/core/APIRouter.php(116): PKP\handler\APIHandler->runRoutes() #12 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/core/Dispatcher.php(158): PKP\core\APIRouter->route(Object(APP\core\Request)) #13 /Users/abir/Sites/code/ojs-main/lib/pkp/classes/core/PKPApplication.php(395): PKP\core\Dispatcher->dispatch(Object(APP\core\Request)) #14 /Users/abir/Sites/code/ojs-main/index.php(30): PKP\core\PKPApplication->execute() #15 /Users/abir/.composer/vendor/laravel/valet/server.php(110): require('/Users/abir/Sit...') #16 {main}

As the route has changed with the addition of a new route param, route is not found by router .

@touhidurabir, sorry about that - I have made the change required and waiting for the tests to finish.

@touhidurabir I don't see the error mentioned by you after the PR: pkp/ui-library#472 but those tests are still failing with other errors I think (see pkp/ojs#4573). Is there another problem that you know?

@defstat in the error log of failed tests , I can see the following exception

[Mon Dec 30 12:03:05 2024] Symfony\Component\HttpKernel\Exception\NotFoundHttpException: The route publicknowledge/api/v1/invitations/userRoleAssignment could not be found. in /home/runner/ojs/lib/pkp/lib/vendor/laravel/framework/src/Illuminate/Routing/AbstractRouteCollection.php:44

but I can tests with latest pkp-lib main checkout, this works for me .

However the current pkp-lib submodule reference to main app is not the latest main but few commits behind where the route in InvitationController set as

Route::get('', $this->getMany(...)) ->name('invitation.getMany');

I think thats causing the issue . I think you need include the latest pkp-lib submodule update also in your PR .

Thanks @touhidurabir! again it seems that the tests are failing down the PHPUnit part of the tests this time.

Tests are failing because of this

Test Errored (functional\ArticleBodyTest::testCreate) array_reduce(): Argument #1 ($array) must be of type array, null given After Test Method Called (functional\ArticleBodyTest::tearDown)

it's a issue with jatsTemplate plugin's unit test and I have added a fix in the PR of #10292 at https://github.com/pkp/jatsTemplate/pull/54/files#diff-c87e6397a6dd5c2bf754d5b10da491be897899cf411d80e1dbcefedb20bd9960R114

OK @touhidurabir, thanks. Everything is merged for the first error.

defstat mentioned this pull request Nov 24, 2024

Invitation API - UserRoleAssignment Invitation - Add existing user to a context that the user has no roles #10630

Closed

ewhanson requested changes Dec 12, 2024

View reviewed changes

defstat added 4 commits December 16, 2024 20:51

pkp#10630 Rebase Conflicts

e341e67

pkp#10630 Several Fixes

27bcd7f

pkp#10630 Add Change invitation case to decline

e55af97

pkp#10630 Review Fixes

558521a

defstat force-pushed the i10630 branch from 0d6edd2 to 558521a Compare December 16, 2024 18:52

pkp#10630 Review Fixes

230f25a

defstat merged commit 4788455 into pkp:main Dec 24, 2024
1 check passed

touhidurabir reviewed Dec 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PKP-LIB][main] #10630 Add support for add existing user to a context #10631

[PKP-LIB][main] #10630 Add support for add existing user to a context #10631

defstat commented Nov 24, 2024

ewhanson left a comment

ewhanson Dec 12, 2024

defstat Dec 16, 2024 •

edited

Loading

ewhanson Dec 12, 2024

defstat Dec 16, 2024

ewhanson Dec 12, 2024

defstat Dec 16, 2024

ewhanson Dec 12, 2024

defstat Dec 16, 2024

ewhanson Dec 12, 2024

defstat Dec 16, 2024 •

edited

Loading

ewhanson Dec 17, 2024

ewhanson Dec 12, 2024

defstat Dec 16, 2024

ewhanson Dec 17, 2024

defstat Dec 18, 2024 •

edited

Loading

ewhanson Dec 18, 2024

defstat Dec 19, 2024

ewhanson Dec 19, 2024

ewhanson Dec 12, 2024

defstat Dec 16, 2024

ewhanson Dec 12, 2024

defstat Dec 16, 2024

defstat commented Dec 16, 2024

ewhanson commented Dec 16, 2024

ewhanson commented Dec 17, 2024

touhidurabir Dec 29, 2024

defstat Dec 30, 2024

defstat Dec 30, 2024 •

edited

Loading

touhidurabir Dec 30, 2024

defstat Dec 30, 2024

touhidurabir Dec 31, 2024

defstat Jan 2, 2025

[PKP-LIB][main] #10630 Add support for add existing user to a context #10631

[PKP-LIB][main] #10630 Add support for add existing user to a context #10631

Conversation

defstat commented Nov 24, 2024

ewhanson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defstat Dec 16, 2024 • 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

defstat Dec 16, 2024 • 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

defstat Dec 18, 2024 • 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

defstat commented Dec 16, 2024

ewhanson commented Dec 16, 2024

ewhanson commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

defstat Dec 30, 2024 • 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

defstat Dec 16, 2024 •

edited

Loading

defstat Dec 16, 2024 •

edited

Loading

defstat Dec 18, 2024 •

edited

Loading

defstat Dec 30, 2024 •

edited

Loading