pkp/pkp-lib#4789 Permit declined reviews to be reinitiated #8242

touhidurabir · 2022-09-08T12:23:42Z

This aims to provide solution to #4789

CLAassistant · 2022-09-08T12:23:47Z

All committers have signed the CLA.

NateWr

This looks great @touhidurabir. I've left a few comments throughout. I've left the text review for after we resolve this question that I just added to the issue:

#4789 (comment)

We'll need to get clarity about that before we finalize this. But from a technical perspective it's looking great.

One other thing that i noticed is that when a review has been reconsidered, in the status it says "Review due: [date]" but it should say "Response due: [date]":

However, I'd suggest you not work on resolving this until we get an answer to the question in the issue. How we "fix" this will depend on how we present this feature to the editors.

NateWr · 2022-09-12T15:34:50Z

classes/controllers/grid/users/reviewer/PKPReviewerGridHandler.php

+        $reconsiderReviewerForm = new ReconsiderRequestReviewerForm($reviewAssignment, $reviewRound, $submission);
+        $reconsiderReviewerForm->readInputData();
+
+        // Reinstate the reviewer and return status message


This comment looks maybe out of place...

NateWr · 2022-09-12T15:35:53Z

classes/controllers/grid/users/reviewer/PKPReviewerGridHandler.php

+        if ($reconsiderReviewerForm->execute() && !$request->getUserVar('skipEmail')) {
+            $reviewer = Repo::user()->get($reviewAssignment->getReviewerId());
+            $user = $request->getUser();
+            $context = PKPServices::get('context')->get($submission->getData('contextId'));


Because this happens within a grid handler that can only be accessed through a HTTP request, you can pull the context from the request: $context = $request->getContext();

NateWr · 2022-09-12T15:37:46Z

classes/mail/mailables/ReviewerReconsiderRequest.php

+ * @class ReviewerReconsiderRequest
+ * @ingroup mail_mailables
+ *
+ * @brief Email sent to a declined reviewer to reconsider reviewing


Email sent to a review who has declined their review assignment to ask them
to reconsider accepting the review assignment

NateWr · 2022-09-12T15:38:07Z

classes/mail/mailables/ReviewerReconsiderRequest.php

+ * @file classes/mail/mailables/ReviewerReconsiderRequest.php
+ *
+ * Copyright (c) 2014-2021 Simon Fraser University
+ * Copyright (c) 2000-2021 John Willinsky


Date should be -2022. Annoying. I miss this all the time. :)

NateWr · 2022-09-12T15:43:07Z

classes/submission/reviewAssignment/ReviewAssignment.php

+
+        if ( $this->getReconsiderRequested() ) {
+            return false;
+        }


Some extra spaces around the (, ) and ! in this method.

NateWr · 2022-09-12T15:44:24Z

classes/submission/reviewAssignment/ReviewAssignmentDAO.php

@@ -434,7 +436,8 @@ public function updateObject($reviewAssignment)
 					reminder_was_automatic = ?,
 					review_form_id = ?,
 					review_round_id = ?,
-					unconsidered = ?
+					unconsidered = ?,
+                    reconsider_requested = ?


Looks like some old tabs in here for some reason, so there's some whitespace discrepancies.

NateWr · 2022-09-12T15:46:30Z

controllers/grid/users/reviewer/form/ReconsiderRequestReviewerForm.php

+     * @param ReviewRound $reviewRound
+     * @param Submission $submission
+     */
+    public function __construct($reviewAssignment, $reviewRound, $submission)


Go ahead and put the @param types into the function signature. It's hard to do with the older code in some places, but constructors are ok.

NateWr · 2022-09-12T15:48:33Z

controllers/grid/users/reviewer/form/ReconsiderRequestReviewerForm.php

+        $reviewAssignment = $this->getReviewAssignment(); /** @var ReviewAssignment $reviewAssignment */
+        $reviewAssignmentDao = DAORegistry::getDAO('ReviewAssignmentDAO'); /** @var ReviewAssignmentDAO $reviewAssignmentDao */
+
+        if (isset($reviewAssignment) && $reviewAssignment->getSubmissionId() == $submission->getId() && !Hook::call('EditorAction::reconsiderReviewer', [&$submission, $reviewAssignment])) {


The Hook here can be removed. This is part of an older pattern that we keep around for existing third-party code. But we don't need to add a new hook.

NateWr · 2022-09-12T15:49:57Z

controllers/grid/users/reviewer/form/ReconsiderRequestReviewerForm.php

+            $reviewAssignmentDao->updateObject($reviewAssignment);
+
+            // Stamp the modification date
+            $submission->stampModified();


I think we want to use stampLastActivity() here instead of stampModified(). I think we only use stampModified() for changes to the submission itself, but stampLastActivity() is for whenever activity takes place related to the submission.

NateWr · 2022-09-12T15:51:19Z

locale/en_US/common.po

@@ -1727,6 +1727,9 @@ msgstr "Reviewer cancelled."
 msgid "notification.reinstatedReviewer"
 msgstr "Reviewer reinstated."

+msgid "notification.requestReconsiderReviewer"
+msgstr "Reviewer requested to reconsider."


Request to reconsider the review assignment was sent.

NateWr

Looks good! I found a couple of small things but it seems just about ready to go. 👍

NateWr · 2022-09-15T10:32:49Z

locale/en_US/editor.po

@@ -278,6 +281,9 @@ msgstr "Review Submitted"
 msgid "editor.review.reviewerThanked"
 msgstr "Reviewer Thanked"

+msgid "editor.review.ReviewerResendRequest"
+msgstr "Reviewer Requested to Reconsider"


Request Resent

NateWr · 2022-09-15T10:42:23Z

locale/en_US/emails.po

+"upload review files, and submit your review request.</p>"
+"<p>If you have any questions, please contact me.</p>"
+"<p>Kind regards,</p>"
+"{$signature}"


"Dear {$recipientName}," "Recently, you declined our request to review a submission, {$submission Title}, for {$contextName}. I'm writing to see if you are able to conduct the review after all." "We would be grateful if you're able to perform this review, but we understand if that is not possible at this time. Either way, please <a href=\"{$reviewAssignmentUrl}\">accept or decline the request</a> by {$responseDueDate}, so that we can find an alternate reviewer." "If you have any questions, please contact me." "Kind regards," "{$signature}"

NateWr · 2022-09-15T10:43:46Z

locale/en_US/emails.po

@@ -312,3 +312,17 @@ msgstr ""
 "<p>If you have any questions, please contact me from your <a href=\"{$authorSubmissionUrl}\">submission dashboard</a>.</p>"
 "<p>Kind regards,</p>"
 "<p>{$signature}</p>"
+
+msgid "emails.reviewResendRequest.subject"
+msgstr "Can you reconsider to review for {$contextName}?"


Requesting your review again for {$contextName}

NateWr · 2022-09-15T10:46:56Z

classes/mail/mailables/ReviewerResendRequest.php

+    protected static array $fromRoleIds = [Role::ROLE_ID_MANAGER];
+    protected static array $toRoleIds = [Role::ROLE_ID_REVIEWER];
+
+    public function __construct(Context $context, PKPSubmission $submission, ReviewAssignment $reviewAssignment)


For some reason, the variables for the review assignment are not coming through to the email template that is loaded in the UI. I added reviewAssignmentUrl and responseDueDate to my template but they aren't rendered:

They were rendered correctly in the email that was sent, so I think it's only in the email preview that the variables are missing.

Oh, this is because of this code:

https://github.com/pkp/pkp-lib/blob/main/controllers/grid/users/reviewer/form/ReviewerNotifyActionForm.php#L74-L87

Vitaliy is in the process of removing the last bits of the SubmissionMailTemplate and this will probably get fixed when that is done. Can you file an issue to solve this later?

@NateWr issue filed at #8262 .

NateWr · 2022-09-15T10:50:51Z

locale/en_US/submission.po

@@ -1854,6 +1857,9 @@ msgstr "This review has been completed."
 msgid "submission.review.status.thanked"
 msgstr "This review is complete and the reviewer has been thanked for their contribution."

+msgid "submission.review.status.reconsiderRequested"
+msgstr "Waiting for a response from the reviewer to reconsider previously declined review request."


This only shows up to a review on a submission they are assigned to. So I think we can reuse submission.review.status.awaitingResponse:

Waiting for a response from the reviewer.

From their perspective, it doesn't matter if it's a new request or a second request. Just that they need to respond.

NateWr · 2022-09-15T10:55:11Z

classes/submission/reviewAssignment/ReviewAssignment.php

@@ -54,6 +54,7 @@ class ReviewAssignment extends \PKP\core\DataObject
    public const REVIEW_ASSIGNMENT_STATUS_COMPLETE = 8; // review has been confirmed by an editor
    public const REVIEW_ASSIGNMENT_STATUS_THANKED = 9; // reviewer has been thanked
    public const REVIEW_ASSIGNMENT_STATUS_CANCELLED = 10; // reviewer cancelled review request
+    public const REVIEW_ASSIGNMENT_STATUS_REQUEST_RESEND = 11; // requested reviewer to reconsider declined invitation


// request resent to reviewer after they declined

NateWr · 2022-09-15T10:55:43Z

classes/submission/reviewAssignment/ReviewAssignment.php

+    public function setReconsiderRequested($reconsider)
+    {
+        $this->setData('reconsider_requested', $reconsider);
+    }


Maybe rename these getter/setter methods to xxxRequestResent()?

NateWr · 2022-09-15T10:57:26Z

locale/en_US/manager.po

@@ -2860,6 +2863,11 @@ msgstr ""
 "This email is sent when a reviewer that was unassigned is reinstated by an "
 "editor."

+msgid "mailable.reviewerResendRequest.description"
+msgstr ""
+"This email is sent when a reviewer who has declined review invitation previously "


This email is sent by an editor to a reviewer who has declined a review request, when the editor wishes to resend the request.

NateWr · 2022-09-15T10:58:17Z

locale/en_US/submission.po

@@ -1824,6 +1824,9 @@ msgstr "{$editorName} has marked the round {$round} review for submission {$subm
 msgid "log.review.reviewReinstated"
 msgstr "The round {$round} review by {$reviewerName} for submission {$submissionId} has been reinitiated."

+msgid "log.review.reviewerResendRequest"
+msgstr "The round {$round} review by {$reviewerName} for submission {$submissionId} has been requested to reconsider."


Resent the request to review in round {$round} to {$reviewerName} for submission {$submissionid}.

NateWr · 2022-09-15T10:59:47Z

controllers/grid/users/reviewer/form/ResendRequestReviewerForm.php

+                    'stageId'               => $reviewAssignment->getStageId(), 
+                    'round'                 => $reviewAssignment->getRound(),
+                ]
+            );


When I viewed the activity log after resending a request, I got the following error. I don't know if this is related to this log entry, but can you double-check the event is showing up alright in the activity log?

PHP Fatal error: Uncaught Error: Call to a member function getFullName() on null in ojs/lib/pkp/classes/log/EmailLogEntry.php:138 Stack trace: #0 ojs/lib/pkp/controllers/grid/eventLog/EventLogGridCellProvider.php(103): PKP\log\EmailLogEntry->getSenderFullName() #1 ojs/lib/pkp/classes/controllers/grid/GridCellProvider.php(57): PKP\controllers\grid\eventLog\EventLogGridCellProvider->getTemplateVarsFromRowColumn(Object(PKP\controllers\grid\eventLog\EventLogGridRow), Object(PKP\controllers\grid\GridColumn)) #2 ojs/lib/pkp/classes/controllers/grid/GridHandler.php(1254): PKP\controllers\grid\GridCellProvider->render(Object(APP\core\Request), Object(PKP\controllers\grid\eventLog\EventLogGridRow), Object(PKP\controllers\grid\GridColumn)) #3 ojs/lib/pkp/classes/controllers/grid/GridHandler.php(1155): PKP\controllers\grid\GridHandler->_renderCellInternally(Object(APP\core\Request), Object(PKP\controllers\grid\eventLog\EventLogGridRow), Object(PKP\controllers\grid\GridColumn)) #4 ojs/lib/pkp/classes/controllers/grid/GridHandler.php(1130): PKP\controllers\grid\GridHandler->renderRowInternally(Object(APP\core\Request), Object(PKP\controllers\grid\eventLog\EventLogGridRow)) #5 ojs/lib/pkp/classes/controllers/grid/GridHandler.php(1180): PKP\controllers\grid\GridHandler->renderRowsInternally(Object(APP\core\Request), Array) #6 ojs/lib/pkp/classes/controllers/grid/GridHandler.php(1055): PKP\controllers\grid\GridHandler->renderGridBodyPartsInternally(Object(APP\core\Request)) #7 ojs/lib/pkp/classes/controllers/grid/GridHandler.php(726): PKP\controllers\grid\GridHandler->doSpecificFetchGridActions(Array, Object(APP\core\Request), Object(APP\template\TemplateManager)) #8 [internal function]: PKP\controllers\grid\GridHandler->fetchGrid(Array, Object(APP\core\Request)) #9 ojs/lib/pkp/classes/core/PKPRouter.php(465): call_user_func(Array, Array, Object(APP\core\Request)) #10 ojs/lib/pkp/classes/core/PKPComponentRouter.php(291): PKP\core\PKPRouter->_authorizeInitializeAndCallRequest(Array, Object(APP\core\Request), Array) #11 ojs/lib/pkp/classes/core/Dispatcher.php(163): PKP\core\PKPComponentRouter->route(Object(APP\core\Request)) #12 ojs/lib/pkp/classes/core/PKPApplication.php(379): PKP\core\Dispatcher->dispatch(Object(APP\core\Request)) #13 ojs/index.php(21): PKP\core\PKPApplication->execute() #14 {main} thrown in ojs/lib/pkp/classes/log/EmailLogEntry.php on line 138

@NateWr there seems to be some sort of problem but it seems unrelated to this PR, I may be mistaken but it seems that way after digging deeper into it . The problem arise when the sender_id column in the email_log table set to 0 . In my testing, it set to 0 when an author submit a new submission and a mail set to the author for which this log generated with sender_id set to 0 . for now I set a fix which at least not cause the activity log to break and load it via ?-> . I think this need to be addressed via a separate issue . I will file a new one and looked into it .

Ok thanks for l ooking into it. Did you get that issue filed?

filed at #8273 .

NateWr · 2022-09-19T11:16:14Z

locale/en_US/emails.po

+msgid "emails.reviewResendRequest.body"
+msgstr ""
+"<p>Dear {$recipientName},</p>"
+"<p>Recently, you declined our request to review a submission, {$submission Title}, for {$contextName}. I'm writing to see if you are able to conduct the review after all.</p>"


The typo was in my suggestion, but there's an extra space in {$submission Title}.

touhidurabir mentioned this pull request Sep 8, 2022

Permit declined reviews to be reinitiated #4789

Closed

NateWr suggested changes Sep 12, 2022

View reviewed changes

touhidurabir force-pushed the i4789_main branch from 27ac1ee to 201813b Compare September 15, 2022 08:22

NateWr suggested changes Sep 15, 2022

View reviewed changes

touhidurabir force-pushed the i4789_main branch from eccee7b to 1f9b311 Compare September 16, 2022 10:10

NateWr reviewed Sep 19, 2022

View reviewed changes

pkp#4789 Permit declined reviews to be reinitiated

79daa42

touhidurabir force-pushed the i4789_main branch from a89033f to 79daa42 Compare September 19, 2022 15:18

touhidurabir mentioned this pull request Sep 19, 2022

Workflow Activity Log not loading #8273

Closed

NateWr merged commit aecccf0 into pkp:main Sep 20, 2022

pkp/pkp-lib#4789 Permit declined reviews to be reinitiated #8242

pkp/pkp-lib#4789 Permit declined reviews to be reinitiated #8242

Conversation

touhidurabir commented Sep 8, 2022

CLAassistant commented Sep 8, 2022 • edited Loading

NateWr left a 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

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

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

NateWr left a 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

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

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

touhidurabir Sep 16, 2022 • 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

CLAassistant commented Sep 8, 2022 •

edited

Loading

touhidurabir Sep 16, 2022 •

edited

Loading