From 41b38eb24052a5d7a51e658b3460d42da76e20c5 Mon Sep 17 00:00:00 2001 From: Touhidur Rahman Date: Fri, 30 Aug 2024 22:51:12 +0600 Subject: [PATCH] pkp/pkp-lib#5885 Review remainder update (#9612) * pkp/pkp-lib#5885 Review remainder update * pkp/pkp-lib#5885 Review remainder update issues fixed * pkp/pkp-lib#5885 typo fixed * pkp/pkp-lib#5885 update remainder calculation * pkp/pkp-lib#5885 removed unnecessary select from migration * pkp/pkp-lib#5885 updated and added new from component to handle reminder * pkp/pkp-lib#5885 removed unnecessary fields and controls with field range component enhancement * pkp/pkp-lib#5885 updated using new slider component * pkp/pkp-lib#5885 updated job class properties to public * pkp/pkp-lib#5885 updated date validation rules to enum * pkp/pkp-lib#5885 fixed displaying error on date comparison validation failure * pkp/pkp-lib#5885 test added for queue job * pkp/pkp-lib#5885 added mocked context service and context class for job testing * pkp/pkp-lib#5885 added context checking first if available to resolve context path before resolving from request * pkp/pkp-lib#5885 job test update * pkp/pkp-lib#5885 job test update * pkp/pkp-lib#5885 removed mistakenly pulled BaseInvitation during rebase * pkp/pkp-lib#5885 updated job and test based on new inviation functionality implementation * pkp/pkp-lib#5885 updated task * pkp/pkp-lib#5885 translation update * pkp/pkp-lib#5885 tests updated and removed reference of deprecated Services class * pkp/pkp-lib#5885 test update * pkp/pkp-lib#5885 test update --- classes/components/forms/FieldSlider.php | 15 +- .../forms/context/PKPReviewSetupForm.php | 125 +++++++++--- .../users/reviewer/PKPReviewerGridHandler.php | 7 +- .../validation/FormValidatorDateCompare.php | 44 +++++ .../UnableToCreateJATSContentException.php | 6 +- ...I5885_RenameReviewReminderSettingsName.php | 59 ++++++ .../submission/reviewAssignment/Collector.php | 51 ++++- classes/task/ReviewReminder.php | 166 ++++++++-------- .../validation/ValidatorDateComparison.php | 59 ++++++ .../validation/enums/DateComparisonRule.php | 26 +++ .../users/reviewer/form/EditReviewForm.php | 12 ++ .../grid/users/reviewer/form/ReviewerForm.php | 11 ++ jobs/email/ReviewReminder.php | 112 +++++++++++ locale/en/common.po | 3 + locale/en/editor.po | 6 + locale/en/manager.po | 38 ++-- schemas/context.json | 18 +- .../users/reviewer/form/editReviewForm.tpl | 2 +- .../reviewer/form/reviewerFormFooter.tpl | 2 +- tests/PKPTestCase.php | 5 +- tests/jobs/email/ReviewReminderTest.php | 187 ++++++++++++++++++ 21 files changed, 804 insertions(+), 150 deletions(-) create mode 100644 classes/form/validation/FormValidatorDateCompare.php create mode 100644 classes/migration/upgrade/v3_5_0/I5885_RenameReviewReminderSettingsName.php create mode 100644 classes/validation/ValidatorDateComparison.php create mode 100644 classes/validation/enums/DateComparisonRule.php create mode 100644 jobs/email/ReviewReminder.php create mode 100644 tests/jobs/email/ReviewReminderTest.php diff --git a/classes/components/forms/FieldSlider.php b/classes/components/forms/FieldSlider.php index c8299c5bb7e..a30489a757c 100644 --- a/classes/components/forms/FieldSlider.php +++ b/classes/components/forms/FieldSlider.php @@ -20,27 +20,20 @@ class FieldSlider extends Field /** @copydoc Field::$component */ public $component = 'field-slider'; - /** * Range min value - * - * @var int|float */ - public $min; + public int|float $min; /** * Range max value - * - * @var int|float */ - public $max; + public int|float $max; /** * Range step value - * - * @var int|float */ - public $step = 1; + public int|float $step = 1; /** * Label for min value, it displays actual value when not present @@ -67,8 +60,6 @@ class FieldSlider extends Field */ public ?string $valueLabelMax = null; - - /** * @copydoc Field::getConfig() */ diff --git a/classes/components/forms/context/PKPReviewSetupForm.php b/classes/components/forms/context/PKPReviewSetupForm.php index 673f89e2931..ab3b70019f0 100644 --- a/classes/components/forms/context/PKPReviewSetupForm.php +++ b/classes/components/forms/context/PKPReviewSetupForm.php @@ -2,8 +2,8 @@ /** * @file classes/components/form/context/PKPReviewSetupForm.php * - * Copyright (c) 2014-2021 Simon Fraser University - * Copyright (c) 2000-2021 John Willinsky + * Copyright (c) 2014-2024 Simon Fraser University + * Copyright (c) 2000-2024 John Willinsky * Distributed under the GNU GPL v3. For full terms see the file docs/COPYING. * * @class PKPReviewSetupForm @@ -17,10 +17,11 @@ namespace PKP\components\forms\context; use PKP\components\forms\FieldHTML; +use PKP\context\Context; use PKP\components\forms\FieldOptions; +use PKP\components\forms\FieldSlider; use PKP\components\forms\FieldText; use PKP\components\forms\FormComponent; -use PKP\config\Config; use PKP\submission\reviewAssignment\ReviewAssignment; class PKPReviewSetupForm extends FormComponent @@ -29,6 +30,12 @@ class PKPReviewSetupForm extends FormComponent public $id = self::FORM_REVIEW_SETUP; public $method = 'PUT'; + protected const REVIEW_SETTINGS_GROUP = 'reviewSettingsGroup'; + protected const REVIEW_REMINDER_GROUP = 'reviewReminderGroup'; + + public const MIN_REMINDER_NOTIFICATION_SEND_IN_DAYS = 0; + public const MAX_REMINDER_NOTIFICATION_SEND_IN_DAYS = 14; + /** * Constructor * @@ -41,23 +48,39 @@ public function __construct($action, $locales, $context) $this->action = $action; $this->locales = $locales; - $this->addField(new FieldOptions('defaultReviewMode', [ - 'label' => __('manager.setup.reviewOptions.reviewMode'), - 'type' => 'radio', - 'value' => $context->getData('defaultReviewMode'), - 'options' => [ - ['value' => ReviewAssignment::SUBMISSION_REVIEW_METHOD_DOUBLEANONYMOUS, 'label' => __('editor.submissionReview.doubleAnonymous')], - ['value' => ReviewAssignment::SUBMISSION_REVIEW_METHOD_ANONYMOUS, 'label' => __('editor.submissionReview.anonymous')], - ['value' => ReviewAssignment::SUBMISSION_REVIEW_METHOD_OPEN, 'label' => __('editor.submissionReview.open')], - ], - ])) + $this + ->addDefaultFields($context) + ->addReminderFields($context); + } + + /** + * Add the default review control fields + */ + protected function addDefaultFields(Context $context): static + { + $this + ->addGroup([ + 'id' => self::REVIEW_SETTINGS_GROUP + ]) + ->addField(new FieldOptions('defaultReviewMode', [ + 'label' => __('manager.setup.reviewOptions.reviewMode'), + 'type' => 'radio', + 'value' => $context->getData('defaultReviewMode'), + 'options' => [ + ['value' => ReviewAssignment::SUBMISSION_REVIEW_METHOD_DOUBLEANONYMOUS, 'label' => __('editor.submissionReview.doubleAnonymous')], + ['value' => ReviewAssignment::SUBMISSION_REVIEW_METHOD_ANONYMOUS, 'label' => __('editor.submissionReview.anonymous')], + ['value' => ReviewAssignment::SUBMISSION_REVIEW_METHOD_OPEN, 'label' => __('editor.submissionReview.open')], + ], + 'groupId' => self::REVIEW_SETTINGS_GROUP, + ])) ->addField(new FieldOptions('restrictReviewerFileAccess', [ 'label' => __('manager.setup.reviewOptions.restrictReviewerFileAccess'), 'type' => 'checkbox', 'value' => $context->getData('restrictReviewerFileAccess'), 'options' => [ ['value' => true, 'label' => __('manager.setup.reviewOptions.restrictReviewerFileAccess.description')], - ] + ], + 'groupId' => self::REVIEW_SETTINGS_GROUP, ])) ->addField(new FieldOptions('reviewerAccessKeysEnabled', [ 'label' => __('manager.setup.reviewOptions.reviewerAccessKeysEnabled'), @@ -66,37 +89,89 @@ public function __construct($action, $locales, $context) 'value' => $context->getData('reviewerAccessKeysEnabled'), 'options' => [ ['value' => true, 'label' => __('manager.setup.reviewOptions.reviewerAccessKeysEnabled.label')], - ] + ], + 'groupId' => self::REVIEW_SETTINGS_GROUP, ])) ->addField(new FieldText('numWeeksPerResponse', [ 'label' => __('manager.setup.reviewOptions.defaultReviewResponseTime'), 'description' => __('manager.setup.reviewOptions.numWeeksPerResponse'), 'value' => $context->getData('numWeeksPerResponse'), 'size' => 'small', + 'groupId' => self::REVIEW_SETTINGS_GROUP, ])) ->addField(new FieldText('numWeeksPerReview', [ 'label' => __('manager.setup.reviewOptions.defaultReviewCompletionTime'), 'description' => __('manager.setup.reviewOptions.numWeeksPerReview'), 'value' => $context->getData('numWeeksPerReview'), 'size' => 'small', + 'groupId' => self::REVIEW_SETTINGS_GROUP, ])) ->addField(new FieldText('numReviewersPerSubmission', [ 'label' => __('manager.setup.reviewOptions.numReviewersPerSubmission'), 'description' => __('manager.setup.reviewOptions.numReviewersPerSubmission.description'), 'value' => $context->getData('numReviewersPerSubmission'), 'size' => 'small', + 'groupId' => self::REVIEW_SETTINGS_GROUP, + ])); + + return $this; + } + + /** + * Add the review reminder control fields + */ + protected function addReminderFields(Context $context): static + { + $this + ->addGroup([ + 'id' => self::REVIEW_REMINDER_GROUP, + ]) + ->addField(new FieldHTML('reminderForReview', [ + 'label' => __('manager.setup.reviewOptions.reminders'), + 'description' => __('manager.setup.reviewOptions.reminders.description'), + 'groupId' => self::REVIEW_REMINDER_GROUP, ])) - ->addField(new FieldText('numDaysBeforeInviteReminder', [ - 'label' => __('manager.setup.reviewOptions.reminders.response'), - 'description' => __('manager.setup.reviewOptions.reminders.response.description'), - 'value' => $context->getData('numDaysBeforeInviteReminder'), - 'size' => 'small', + ->addField(new FieldSlider('numDaysBeforeReviewResponseReminderDue', [ + 'label' => __('manager.setup.reviewOptions.reminders.response.before'), + 'value' => $context->getData('numDaysBeforeReviewResponseReminderDue'), + 'min' => static::MIN_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'max' => static::MAX_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'minLabel' => __('manager.setup.reviewOptions.reminders.min.label'), + 'valueLabel' => __('manager.setup.reviewOptions.reminders.label.before.days'), + 'valueLabelMin' => __('manager.setup.reviewOptions.reminders.disbale.label'), + 'groupId' => self::REVIEW_REMINDER_GROUP, ])) - ->addField(new FieldText('numDaysBeforeSubmitReminder', [ - 'label' => __('manager.setup.reviewOptions.reminders.submit'), - 'description' => __('manager.setup.reviewOptions.reminders.submit.description'), - 'value' => $context->getData('numDaysBeforeSubmitReminder'), - 'size' => 'small', + ->addField(new FieldSlider('numDaysAfterReviewResponseReminderDue', [ + 'label' => __('manager.setup.reviewOptions.reminders.response.after'), + 'value' => $context->getData('numDaysAfterReviewResponseReminderDue'), + 'min' => static::MIN_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'max' => static::MAX_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'minLabel' => __('manager.setup.reviewOptions.reminders.min.label'), + 'valueLabel' => __('manager.setup.reviewOptions.reminders.label.after.days'), + 'valueLabelMin' => __('manager.setup.reviewOptions.reminders.disbale.label'), + 'groupId' => self::REVIEW_REMINDER_GROUP, + ])) + ->addField(new FieldSlider('numDaysBeforeReviewSubmitReminderDue', [ + 'label' => __('manager.setup.reviewOptions.reminders.submit.before'), + 'value' => $context->getData('numDaysBeforeReviewSubmitReminderDue'), + 'min' => static::MIN_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'max' => static::MAX_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'minLabel' => __('manager.setup.reviewOptions.reminders.min.label'), + 'valueLabel' => __('manager.setup.reviewOptions.reminders.label.before.days'), + 'valueLabelMin' => __('manager.setup.reviewOptions.reminders.disbale.label'), + 'groupId' => self::REVIEW_REMINDER_GROUP, + ])) + ->addField(new FieldSlider('numDaysAfterReviewSubmitReminderDue', [ + 'label' => __('manager.setup.reviewOptions.reminders.submit.after'), + 'value' => $context->getData('numDaysAfterReviewSubmitReminderDue'), + 'min' => static::MIN_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'max' => static::MAX_REMINDER_NOTIFICATION_SEND_IN_DAYS, + 'minLabel' => __('manager.setup.reviewOptions.reminders.min.label'), + 'valueLabel' => __('manager.setup.reviewOptions.reminders.label.after.days'), + 'valueLabelMin' => __('manager.setup.reviewOptions.reminders.disbale.label'), + 'groupId' => self::REVIEW_REMINDER_GROUP, ])); + + return $this; } } diff --git a/classes/controllers/grid/users/reviewer/PKPReviewerGridHandler.php b/classes/controllers/grid/users/reviewer/PKPReviewerGridHandler.php index 4e4d2418b7f..5bffcaa653b 100644 --- a/classes/controllers/grid/users/reviewer/PKPReviewerGridHandler.php +++ b/classes/controllers/grid/users/reviewer/PKPReviewerGridHandler.php @@ -409,15 +409,16 @@ public function updateReviewer($args, $request) // Form handling $reviewerForm = new $formClassName($this->getSubmission(), $this->getReviewRound()); $reviewerForm->readInputData(); + if ($reviewerForm->validate()) { $reviewAssignment = $reviewerForm->execute(); $json = DAO::getDataChangedEvent($reviewAssignment->getId()); $json->setGlobalEvent('update:decisions'); return $json; - } else { - // There was an error, redisplay the form - return new JSONMessage(true, $reviewerForm->fetch($request)); } + + // There was an error, redisplay the form + return new JSONMessage(false); } /** diff --git a/classes/form/validation/FormValidatorDateCompare.php b/classes/form/validation/FormValidatorDateCompare.php new file mode 100644 index 00000000000..186bbb06f3e --- /dev/null +++ b/classes/form/validation/FormValidatorDateCompare.php @@ -0,0 +1,44 @@ +getCode() ?? 0, + $innerException + ); } } diff --git a/classes/migration/upgrade/v3_5_0/I5885_RenameReviewReminderSettingsName.php b/classes/migration/upgrade/v3_5_0/I5885_RenameReviewReminderSettingsName.php new file mode 100644 index 00000000000..9819ec1f977 --- /dev/null +++ b/classes/migration/upgrade/v3_5_0/I5885_RenameReviewReminderSettingsName.php @@ -0,0 +1,59 @@ +getContextSettingsTable()) + ->where('setting_name', 'numDaysBeforeInviteReminder') + ->update([ + 'setting_name' => 'numDaysAfterReviewResponseReminderDue' + ]); + + DB::table($this->getContextSettingsTable()) + ->where('setting_name', 'numDaysBeforeSubmitReminder') + ->update([ + 'setting_name' => 'numDaysAfterReviewSubmitReminderDue' + ]); + } + + /** + * Reverse the migration + */ + public function down(): void + { + DB::table($this->getContextSettingsTable()) + ->where('setting_name', 'numDaysAfterReviewResponseReminderDue') + ->update([ + 'setting_name' => 'numDaysBeforeInviteReminder' + ]); + + DB::table($this->getContextSettingsTable()) + ->where('setting_name', 'numDaysAfterReviewSubmitReminderDue') + ->update([ + 'setting_name' => 'numDaysBeforeSubmitReminder' + ]); + } +} diff --git a/classes/submission/reviewAssignment/Collector.php b/classes/submission/reviewAssignment/Collector.php index efca0813fa0..7dc62b755e2 100644 --- a/classes/submission/reviewAssignment/Collector.php +++ b/classes/submission/reviewAssignment/Collector.php @@ -27,6 +27,9 @@ */ class Collector implements CollectorInterface, ViewsCount { + public const ORDER_DIR_ASC = 'ASC'; + public const ORDER_DIR_DESC = 'DESC'; + public DAO $dao; public ?array $contextIds = null; public ?array $submissionIds = null; @@ -41,6 +44,10 @@ class Collector implements CollectorInterface, ViewsCount public ?array $reviewMethods = null; public ?int $stageId = null; public ?array $reviewFormIds = null; + public bool $orderByContextId = false; + public ?string $orderByContextIdDirection = null; + public bool $orderBySubmissionId = false; + public ?string $orderBySubmissionIdDirection = null; public function __construct(DAO $dao) { @@ -184,6 +191,26 @@ public function offset(?int $offset): static return $this; } + /** + * Order/Sort the review assignments by associated context id + */ + public function orderByContextId(string $direction = self::ORDER_DIR_ASC): static + { + $this->orderByContextId = true; + $this->orderByContextIdDirection = $direction; + return $this; + } + + /** + * Order/Sort the review assignments by associated submission id + */ + public function orderBySubmissionId(string $direction = self::ORDER_DIR_ASC): static + { + $this->orderBySubmissionId = true; + $this->orderBySubmissionIdDirection = $direction; + return $this; + } + /** * @copydoc CollectorInterface::getQueryBuilder() */ @@ -307,16 +334,36 @@ public function getQueryBuilder(): Builder $q->when( $this->count !== null, - fn () => + fn (Builder $q) => $q->limit($this->count) ); $q->when( $this->offset !== null, - fn () => + fn (Builder $q) => $q->offset($this->offset) ); + $q->when( + $this->orderByContextId, + fn (Builder $q) => + $q->orderBy( + DB::table('submissions') + ->select('context_id') + ->whereColumn('submission_id', 'ra.submission_id'), + $this->orderByContextIdDirection + ) + ); + + $q->when( + $this->orderBySubmissionId, + fn (Builder $q) => + $q->orderBy( + 'ra.submission_id', + $this->orderBySubmissionIdDirection + ) + ); + return $q; } diff --git a/classes/task/ReviewReminder.php b/classes/task/ReviewReminder.php index e6badd4cd36..dae480fbce4 100644 --- a/classes/task/ReviewReminder.php +++ b/classes/task/ReviewReminder.php @@ -18,17 +18,12 @@ use APP\core\Application; use APP\facades\Repo; -use Illuminate\Support\Facades\Mail; -use PKP\context\Context; -use PKP\core\Core; -use PKP\core\PKPApplication; -use PKP\invitation\invitations\ReviewerAccessInvite; -use PKP\log\event\PKPSubmissionEventLogEntry; +use Carbon\Carbon; use PKP\mail\mailables\ReviewRemindAuto; use PKP\mail\mailables\ReviewResponseRemindAuto; use PKP\scheduledTask\ScheduledTask; use PKP\submission\PKPSubmission; -use PKP\submission\reviewAssignment\ReviewAssignment; +use PKP\jobs\email\ReviewReminder as ReviewReminderJob; class ReviewReminder extends ScheduledTask { @@ -40,69 +35,6 @@ public function getName(): string return __('admin.scheduledTask.reviewReminder'); } - /** - * Send the automatic review reminder to the reviewer. - */ - public function sendReminder( - ReviewAssignment $reviewAssignment, - PKPSubmission $submission, - Context $context, - ReviewRemindAuto|ReviewResponseRemindAuto $mailable - ): void { - - $reviewer = Repo::user()->get($reviewAssignment->getReviewerId()); - if (!isset($reviewer)) { - return; - } - - $primaryLocale = $context->getPrimaryLocale(); - $emailTemplate = Repo::emailTemplate()->getByKey($context->getId(), $mailable::getEmailTemplateKey()); - $mailable->subject($emailTemplate->getLocalizedData('subject', $primaryLocale)) - ->body($emailTemplate->getLocalizedData('body', $primaryLocale)) - ->from($context->getData('contactEmail'), $context->getData('contactName')) - ->recipients([$reviewer]); - - $mailable->setData($primaryLocale); - - $reviewerAccessKeysEnabled = $context->getData('reviewerAccessKeysEnabled'); - if ($reviewerAccessKeysEnabled) { // Give one-click access if enabled - $reviewInvitation = new ReviewerAccessInvite(); - $reviewInvitation->initialize($reviewAssignment->getReviewerId(), $context->getId(), null); - - $reviewInvitation->reviewAssignmentId = $reviewAssignment->getId(); - $reviewInvitation->updatePayload(); - - $reviewInvitation->invite(); - $reviewInvitation->updateMailableWithUrl($mailable); - } - - // deprecated template variables OJS 2.x - $mailable->addData([ - 'messageToReviewer' => __('reviewer.step1.requestBoilerplate'), - 'abstractTermIfEnabled' => ($submission->getCurrentPublication()->getLocalizedData('abstract') == '' ? '' : __('common.abstract')), - ]); - - Mail::send($mailable); - - Repo::reviewAssignment()->edit($reviewAssignment, [ - 'dateReminded' => Core::getCurrentDate(), - 'reminderWasAutomatic' => 1 - ]); - - $eventLog = Repo::eventLog()->newDataObject([ - 'assocType' => PKPApplication::ASSOC_TYPE_SUBMISSION, - 'assocId' => $submission->getId(), - 'eventType' => PKPSubmissionEventLogEntry::SUBMISSION_LOG_REVIEW_REMIND_AUTO, - 'userId' => null, - 'message' => 'submission.event.reviewer.reviewerRemindedAuto', - 'isTranslated' => false, - 'dateLogged' => Core::getCurrentDate(), - 'recipientId' => $reviewer->getId(), - 'recipientName' => $reviewer->getFullName(), - ]); - Repo::eventLog()->add($eventLog); - } - /** * @copydoc ScheduledTask::executeActions() */ @@ -112,15 +44,15 @@ public function executeActions(): bool $context = null; $contextDao = Application::getContextDAO(); + $incompleteAssignments = Repo::reviewAssignment() + ->getCollector() + ->filterByIsIncomplete(true) + ->orderByContextId() + ->orderBySubmissionId() + ->getMany(); - $incompleteAssignments = Repo::reviewAssignment()->getCollector()->filterByIsIncomplete(true)->getMany(); - $inviteReminderDays = $submitReminderDays = null; foreach ($incompleteAssignments as $reviewAssignment) { - // Avoid review assignments that a reminder exists for. - if ($reviewAssignment->getDateReminded() !== null) { - continue; - } - + // Fetch the submission if ($submission == null || $submission->getId() != $reviewAssignment->getSubmissionId()) { unset($submission); @@ -140,26 +72,82 @@ public function executeActions(): bool unset($context); $context = $contextDao->getById($submission->getData('contextId')); - $inviteReminderDays = $context->getData('numDaysBeforeInviteReminder'); - $submitReminderDays = $context->getData('numDaysBeforeSubmitReminder'); + $numDaysBeforeReviewResponseReminderDue = (int) $context->getData('numDaysBeforeReviewResponseReminderDue'); + $numDaysAfterReviewResponseReminderDue = (int) $context->getData('numDaysAfterReviewResponseReminderDue'); + $numDaysBeforeReviewSubmitReminderDue = (int) $context->getData('numDaysBeforeReviewSubmitReminderDue'); + $numDaysAfterReviewSubmitReminderDue = (int) $context->getData('numDaysAfterReviewSubmitReminderDue'); } $mailable = null; - if ($submitReminderDays >= 1 && $reviewAssignment->getDateDue() != null) { - $checkDate = strtotime($reviewAssignment->getDateDue()); - if (time() - $checkDate > 60 * 60 * 24 * $submitReminderDays) { - $mailable = new ReviewRemindAuto($context, $submission, $reviewAssignment); + $currentDate = Carbon::today(); + + $dateResponseDue = Carbon::parse($reviewAssignment->getDateResponseDue()); + $dateDue = Carbon::parse($reviewAssignment->getDateDue()); + + // after a REVIEW REQUEST has been responded, the value of `dateReminded` and `reminderWasAutomatic` + // get reset, see \PKP\submission\reviewer\ReviewerAction::confirmReview. + if ($reviewAssignment->getDateConfirmed() === null) { + // REVIEW REQUEST has not been responded + // only need to concern with BEFORE/AFTER REVIEW REQUEST RESPONSE reminder + + if ($reviewAssignment->getDateReminded() === null) { + // There has not been any reminder sent yet + // need to check should we sent a BEFORE REVIEW REQUEST RESPONSE reminder + if ($numDaysBeforeReviewResponseReminderDue > 0 && + $dateResponseDue->gt($currentDate) && + $dateResponseDue->diffInDays($currentDate) <= $numDaysBeforeReviewResponseReminderDue) { + + // ACTION:-> we need to send BEFORE REVIEW REQUEST RESPONSE reminder + $mailable = ReviewResponseRemindAuto::class; + } + } else { + // There has been a reminder already sent + // need to check should we sent a AFTER REVIEW REQUEST RESPONSE reminder + + $dateReminded = Carbon::parse($reviewAssignment->getDateReminded()); + + if ($numDaysAfterReviewResponseReminderDue > 0 && + $currentDate->gt($dateResponseDue) && + $dateReminded->lt($dateResponseDue) && + $currentDate->diffInDays($dateResponseDue) >= $numDaysAfterReviewResponseReminderDue) { + + // ACTION:-> we need to send AFTER REVIEW REQUEST RESPONSE reminder + $mailable = ReviewResponseRemindAuto::class; + } } - } - if ($inviteReminderDays >= 1 && $reviewAssignment->getDateConfirmed() == null) { - $checkDate = strtotime($reviewAssignment->getDateResponseDue()); - if (time() - $checkDate > 60 * 60 * 24 * $inviteReminderDays) { - $mailable = new ReviewResponseRemindAuto($context, $submission, $reviewAssignment); + } else { + // REVIEW REQUEST has been responded + // only need to concern with BEFORE/AFTER REVIEW SUBMIT reminder + + if ($reviewAssignment->getDateReminded() === null) { + // There has not been any reminder sent after responding to REVIEW REQUEST + // no REVIEW SUBMIT reminder has been sent + if ($numDaysBeforeReviewSubmitReminderDue > 0 && + $currentDate->lt($dateDue) && + $dateDue->diffInDays($currentDate) <= $numDaysBeforeReviewSubmitReminderDue) { + + // ACTION:-> we need to sent a BEFORE REVIEW SUBMIT reminder + $mailable = ReviewRemindAuto::class; + } + } else { + // There has been already sent a reminder after responding to REVIEW REQUEST + // need to check should we sent a AFTER REVIEW SUBMIT reminder + + $dateReminded = Carbon::parse($reviewAssignment->getDateReminded()); + + if ($numDaysAfterReviewSubmitReminderDue > 0 && + $currentDate->gt($dateDue) && + $dateReminded->lt($dateDue) && + $currentDate->diffInDays($dateDue) >= $numDaysAfterReviewSubmitReminderDue) { + + // ACTION:-> we need to send AFTER REVIEW SUBMIT reminder + $mailable = ReviewRemindAuto::class; + } } } if ($mailable) { - $this->sendReminder($reviewAssignment, $submission, $context, $mailable); + ReviewReminderJob::dispatch($context->getId(), $reviewAssignment->getId(), $mailable); } } diff --git a/classes/validation/ValidatorDateComparison.php b/classes/validation/ValidatorDateComparison.php new file mode 100644 index 00000000000..c5e2c2f7bf0 --- /dev/null +++ b/classes/validation/ValidatorDateComparison.php @@ -0,0 +1,59 @@ +comparingDate = $comparingDate instanceof Carbon + ? $comparingDate + : Carbon::parse($comparingDate); + } + + /** + * @copydoc Validator::isValid() + */ + public function isValid($value) + { + $validator = ValidatorFactory::make( + ['value' => $value], + ['value' => [ + 'date', + $this->rule->value . ':' . $this->comparingDate->toDateString() + ]] + ); + + return $validator->passes(); + } +} diff --git a/classes/validation/enums/DateComparisonRule.php b/classes/validation/enums/DateComparisonRule.php new file mode 100644 index 00000000000..63bfd75684b --- /dev/null +++ b/classes/validation/enums/DateComparisonRule.php @@ -0,0 +1,26 @@ +addCheck(new \PKP\form\validation\FormValidator($this, 'responseDueDate', 'required', 'editor.review.errorAddingReviewer')); $this->addCheck(new \PKP\form\validation\FormValidator($this, 'reviewDueDate', 'required', 'editor.review.errorAddingReviewer')); + + $this->addCheck( + new \PKP\form\validation\FormValidatorDateCompare( + $this, + 'reviewDueDate', + \Carbon\Carbon::parse(Application::get()->getRequest()->getUserVar('responseDueDate')), + \PKP\validation\enums\DateComparisonRule::GREATER_OR_EQUAL, + 'optional', + 'editor.review.errorAddingReviewer.dateValidationFailed' + ) + ); + $this->addCheck(new \PKP\form\validation\FormValidatorPost($this)); $this->addCheck(new \PKP\form\validation\FormValidatorCSRF($this)); } diff --git a/controllers/grid/users/reviewer/form/ReviewerForm.php b/controllers/grid/users/reviewer/form/ReviewerForm.php index c96b937209c..031b0ccc525 100644 --- a/controllers/grid/users/reviewer/form/ReviewerForm.php +++ b/controllers/grid/users/reviewer/form/ReviewerForm.php @@ -73,6 +73,17 @@ public function __construct($submission, $reviewRound) $this->addCheck(new \PKP\form\validation\FormValidator($this, 'responseDueDate', 'required', 'editor.review.errorAddingReviewer')); $this->addCheck(new \PKP\form\validation\FormValidator($this, 'reviewDueDate', 'required', 'editor.review.errorAddingReviewer')); + $this->addCheck( + new \PKP\form\validation\FormValidatorDateCompare( + $this, + 'reviewDueDate', + \Carbon\Carbon::parse(Application::get()->getRequest()->getUserVar('responseDueDate')), + \PKP\validation\enums\DateComparisonRule::GREATER_OR_EQUAL, + 'required', + 'editor.review.errorAddingReviewer.dateValidationFailed' + ) + ); + $this->addCheck(new \PKP\form\validation\FormValidatorPost($this)); $this->addCheck(new \PKP\form\validation\FormValidatorCSRF($this)); } diff --git a/jobs/email/ReviewReminder.php b/jobs/email/ReviewReminder.php new file mode 100644 index 00000000000..b2c023a5eae --- /dev/null +++ b/jobs/email/ReviewReminder.php @@ -0,0 +1,112 @@ +get($this->reviewAssignmentId); + $reviewer = Repo::user()->get($reviewAssignment->getReviewerId()); + + if (!isset($reviewer)) { + return; + } + + $submission = Repo::submission()->get($reviewAssignment->getData('submissionId')); + + $contextService = app()->get('context'); + $context = $contextService->get($this->contextId); + + /** @var ReviewRemindAuto|ReviewResponseRemindAuto $mailable */ + $mailable = new $this->mailableClass($context, $submission, $reviewAssignment); + + $primaryLocale = $context->getPrimaryLocale(); + $emailTemplate = Repo::emailTemplate()->getByKey( + $context->getId(), + $mailable::getEmailTemplateKey() + ); + + $mailable->subject($emailTemplate->getLocalizedData('subject', $primaryLocale)) + ->body($emailTemplate->getLocalizedData('body', $primaryLocale)) + ->from($context->getData('contactEmail'), $context->getData('contactName')) + ->recipients([$reviewer]); + + $mailable->setData($primaryLocale); + + $reviewerAccessKeysEnabled = $context->getData('reviewerAccessKeysEnabled'); + + if ($reviewerAccessKeysEnabled) { // Give one-click access if enabled + $reviewInvitation = new ReviewerAccessInvite(); + $reviewInvitation->initialize($reviewAssignment->getReviewerId(), $context->getId(), null); + + $reviewInvitation->reviewAssignmentId = $reviewAssignment->getId(); + $reviewInvitation->updatePayload(); + + $reviewInvitation->invite(); + $reviewInvitation->updateMailableWithUrl($mailable); + } + + // deprecated template variables OJS 2.x + $mailable->addData([ + 'messageToReviewer' => __('reviewer.step1.requestBoilerplate'), + 'abstractTermIfEnabled' => ($submission->getCurrentPublication()->getLocalizedData('abstract') == '' ? '' : __('common.abstract')), + ]); + + Mail::send($mailable); + + Repo::reviewAssignment()->edit($reviewAssignment, [ + 'dateReminded' => Core::getCurrentDate(), + 'reminderWasAutomatic' => 1 + ]); + + $eventLog = Repo::eventLog()->newDataObject([ + 'assocType' => PKPApplication::ASSOC_TYPE_SUBMISSION, + 'assocId' => $submission->getId(), + 'eventType' => PKPSubmissionEventLogEntry::SUBMISSION_LOG_REVIEW_REMIND_AUTO, + 'userId' => null, + 'message' => 'submission.event.reviewer.reviewerRemindedAuto', + 'isTranslated' => false, + 'dateLogged' => Core::getCurrentDate(), + 'recipientId' => $reviewer->getId(), + 'recipientName' => $reviewer->getFullName(), + ]); + Repo::eventLog()->add($eventLog); + } +} diff --git a/locale/en/common.po b/locale/en/common.po index 3bd99b29238..63490c19b1c 100644 --- a/locale/en/common.po +++ b/locale/en/common.po @@ -2153,6 +2153,9 @@ msgstr "This is not a valid currency." msgid "validator.date" msgstr "This is not a valid date." +msgid "validator.date.comparison" +msgstr "Invalid dates provided for comparison." + msgid "validator.date_format" msgstr "This does not match the format {$format}." diff --git a/locale/en/editor.po b/locale/en/editor.po index 3fada52787d..bb71149fe06 100644 --- a/locale/en/editor.po +++ b/locale/en/editor.po @@ -205,6 +205,9 @@ msgstr "Email to be sent to reviewer" msgid "editor.review.importantDates" msgstr "Important Dates" +msgid "editor.review.importantDates.notice" +msgstr "Review due date must be greater or equal to response due date." + msgid "editor.review.uploadRevision" msgstr "Upload Revision" @@ -321,6 +324,9 @@ msgstr "You must select a reviewer" msgid "editor.review.errorAddingReviewer" msgstr "There was an error adding the reviewer. Please try again." +msgid "editor.review.errorAddingReviewer.dateValidationFailed" +msgstr "There was an error adding the reviewer as review due date must be equal or greater than responde due date." + msgid "editor.review.errorDeletingReviewer" msgstr "There was an error deleting the reviewer. Please try again." diff --git a/locale/en/manager.po b/locale/en/manager.po index 2f3e8a5b3c3..54e1e87cfb6 100644 --- a/locale/en/manager.po +++ b/locale/en/manager.po @@ -1238,21 +1238,37 @@ msgstr "Never Remind" msgid "manager.setup.reviewOptions.noteOnModification" msgstr "Defaults can be modified for each review during the editorial process." -msgid "manager.setup.reviewOptions.reminders.response" -msgstr "Response Reminder" +msgid "manager.setup.reviewOptions.reminders" +msgstr "Set Reminders for Review" -msgid "manager.setup.reviewOptions.reminders.response.description" +msgid "manager.setup.reviewOptions.reminders.description" msgstr "" -"Send an email reminder if a reviewer has not responded to a review request " -"this many days after the response due date." +"Send an email reminder before or after for review request response (if reviewer has not responded " +"to review request yet) or review submission (if reviewer has not submitted review yet)" -msgid "manager.setup.reviewOptions.reminders.submit" -msgstr "Review Reminder" +msgid "manager.setup.reviewOptions.reminders.response.before" +msgstr "Review Request Response - Before Due Date" -msgid "manager.setup.reviewOptions.reminders.submit.description" -msgstr "" -"Send an email reminder if a reviewer has not submitted a recommendation " -"within this many days after the review's due date." +msgid "manager.setup.reviewOptions.reminders.response.after" +msgstr "Review Request Response - After Due Date" + +msgid "manager.setup.reviewOptions.reminders.submit.before" +msgstr "Review Submission - Before Due Date" + +msgid "manager.setup.reviewOptions.reminders.submit.after" +msgstr "Review Submission - After Due Date" + +msgid "manager.setup.reviewOptions.reminders.label.before.days" +msgstr "{$value} days before due date" + +msgid "manager.setup.reviewOptions.reminders.label.after.days" +msgstr "{$value} days after due date" + +msgid "manager.setup.reviewOptions.reminders.min.label" +msgstr "None" + +msgid "manager.setup.reviewOptions.reminders.disbale.label" +msgstr "No reminder set" msgid "manager.setup.reviewOptions.reviewMode" msgstr "Default Review Mode" diff --git a/schemas/context.json b/schemas/context.json index a2445718f66..b21cf817216 100644 --- a/schemas/context.json +++ b/schemas/context.json @@ -520,14 +520,28 @@ "min:0" ] }, - "numDaysBeforeInviteReminder": { + "numDaysAfterReviewResponseReminderDue": { "type": "integer", "validation": [ "nullable", "min:0" ] }, - "numDaysBeforeSubmitReminder": { + "numDaysBeforeReviewResponseReminderDue": { + "type": "integer", + "validation": [ + "nullable", + "min:0" + ] + }, + "numDaysAfterReviewSubmitReminderDue": { + "type": "integer", + "validation": [ + "nullable", + "min:0" + ] + }, + "numDaysBeforeReviewSubmitReminderDue": { "type": "integer", "validation": [ "nullable", diff --git a/templates/controllers/grid/users/reviewer/form/editReviewForm.tpl b/templates/controllers/grid/users/reviewer/form/editReviewForm.tpl index c360cc68c3d..664ca0124cc 100644 --- a/templates/controllers/grid/users/reviewer/form/editReviewForm.tpl +++ b/templates/controllers/grid/users/reviewer/form/editReviewForm.tpl @@ -23,7 +23,7 @@ - {fbvFormSection title="editor.review.importantDates"} + {fbvFormSection title="editor.review.importantDates" description="editor.review.importantDates.notice"} {fbvElement type="text" id="responseDueDate" name="responseDueDate" label="submission.task.responseDueDate" value=$responseDueDate inline=true size=$fbvStyles.size.MEDIUM class="datepicker"} {fbvElement type="text" id="reviewDueDate" name="reviewDueDate" label="editor.review.reviewDueDate" value=$reviewDueDate inline=true size=$fbvStyles.size.MEDIUM class="datepicker"} {/fbvFormSection} diff --git a/templates/controllers/grid/users/reviewer/form/reviewerFormFooter.tpl b/templates/controllers/grid/users/reviewer/form/reviewerFormFooter.tpl index 426380a8efa..9056033ee67 100644 --- a/templates/controllers/grid/users/reviewer/form/reviewerFormFooter.tpl +++ b/templates/controllers/grid/users/reviewer/form/reviewerFormFooter.tpl @@ -29,7 +29,7 @@ {fbvElement type="checkbox" id="skipEmail" name="skipEmail" label="editor.review.skipEmail"} {/fbvFormSection} - {fbvFormSection title="editor.review.importantDates"} + {fbvFormSection title="editor.review.importantDates" description="editor.review.importantDates.notice"} {fbvElement type="text" id="responseDueDate" name="responseDueDate" label="submission.task.responseDueDate" value=$responseDueDate inline=true size=$fbvStyles.size.MEDIUM class="datepicker"} {fbvElement type="text" id="reviewDueDate" name="reviewDueDate" label="editor.review.reviewDueDate" value=$reviewDueDate inline=true size=$fbvStyles.size.MEDIUM class="datepicker"} {/fbvFormSection} diff --git a/tests/PKPTestCase.php b/tests/PKPTestCase.php index daa08d13cb1..f17b6adc4d3 100644 --- a/tests/PKPTestCase.php +++ b/tests/PKPTestCase.php @@ -181,7 +181,7 @@ protected function setTestConfiguration($config, $configPath = 'config') * * @return Request */ - protected function mockRequest($path = 'index/test-page/test-op', $userId = null) + protected function mockRequest(string $path = 'index/test-page/test-op', int $userId = 0) { // Back up the default request. if (!isset($this->registryBackup['request'])) { @@ -205,8 +205,7 @@ protected function mockRequest($path = 'index/test-page/test-op', $userId = null $request->setRouter($router); // Test user. - $session = $request->getSession(); - $session->setUserId($userId); + $request->getSessionGuard()->setUserId($userId); return $request; } diff --git a/tests/jobs/email/ReviewReminderTest.php b/tests/jobs/email/ReviewReminderTest.php new file mode 100644 index 00000000000..564a4e116dd --- /dev/null +++ b/tests/jobs/email/ReviewReminderTest.php @@ -0,0 +1,187 @@ +assertInstanceOf( + ReviewReminder::class, + unserialize($this->serializedJobData) + ); + } + + /** + * Ensure that a serialized job can be unserialized and executed + */ + public function testRunSerializedJob(): void + { + /** @var ReviewReminder $reviewReminderJob */ + $reviewReminderJob = unserialize($this->serializedJobData); + + // Fake the mail facade + Mail::fake(); + + // need to mock request so that a valid context information is set and can be retrived + $this->mockRequest(); + + $reviewAssignmentMock = Mockery::mock(ReviewAssignment::class) + ->shouldReceive([ + 'getReviewerId' => 0, + 'getData' => 0, + 'getSubmissionId' => 0, + 'getRound' => 0, + 'getReviewMethod' => '', + 'getRecommendation' => '', + 'getReviewerFullName' => '', + 'getId' => 0, + 'getDateResponseDue' => \Carbon\Carbon::now()->format('Y-m-d H:i:s'), + 'getDateAssigned' => \Carbon\Carbon::now()->format('Y-m-d H:i:s'), + 'getDateDue' => \Carbon\Carbon::now()->format('Y-m-d H:i:s'), + ]) + ->withAnyArgs() + ->getMock(); + + $reviewAssignmentRepoMock = Mockery::mock(app(ReviewAssignmentRepository::class)) + ->makePartial() + ->shouldReceive([ + 'get' => $reviewAssignmentMock, + 'edit' => null, + ]) + ->withAnyArgs() + ->getMock(); + + app()->instance(ReviewAssignmentRepository::class, $reviewAssignmentRepoMock); + + $userMock = Mockery::mock(\PKP\user\User::class) + ->makePartial() + ->shouldReceive([ + 'getId' => 0, + 'getFullName' => 'Test User', + ]) + ->withAnyArgs() + ->getMock(); + + $userRepoMock = Mockery::mock(app(UserRepository::class)) + ->makePartial() + ->shouldReceive('get') + ->withAnyArgs() + ->andReturn($userMock) + ->getMock(); + + app()->instance(UserRepository::class, $userRepoMock); + + $contextMock = Mockery::mock(\PKP\context\Context::class) + ->makePartial() + ->shouldReceive([ + 'getPath' => '', + 'getId' => 0, + ]) + ->withAnyArgs() + ->getMock(); + + $contextServiceMock = Mockery::mock(\APP\services\ContextService::class) + ->makePartial() + ->shouldReceive('get') + ->withAnyArgs() + ->andReturn($contextMock) + ->getMock(); + + app()->instance('context', $contextServiceMock); + + $publicationMock = Mockery::mock(\APP\publication\Publication::class) + ->makePartial() + ->shouldReceive('getData') + ->with('authors') + ->andReturn(\Illuminate\Support\LazyCollection::make([new \PKP\author\Author()])) + ->getMock(); + + $submissionMock = Mockery::mock(\APP\submission\Submission::class) + ->makePartial() + ->shouldReceive([ + 'getId' => 0, + 'getData' => 0, + 'getCurrentPublication' => $publicationMock + ]) + ->withAnyArgs() + ->getMock(); + + $submissionRepoMock = Mockery::mock(app(SubmissionRepository::class)) + ->makePartial() + ->shouldReceive('get') + ->withAnyArgs() + ->andReturn($submissionMock) + ->getMock(); + + app()->instance(SubmissionRepository::class, $submissionRepoMock); + + $emailTemplateMock = Mockery::mock(\PKP\emailTemplate\EmailTemplate::class) + ->makePartial() + ->shouldReceive([ + 'getLocalizedData' => '', + ]) + ->withAnyArgs() + ->getMock(); + + $emailTemplateRepoMock = Mockery::mock(app(EmailTemplateRepository::class)) + ->makePartial() + ->shouldReceive([ + 'getByKey' => $emailTemplateMock, + ]) + ->withAnyArgs() + ->getMock(); + + app()->instance(EmailTemplateRepository::class, $emailTemplateRepoMock); + + $eventRepoMock = Mockery::mock(app(EventRepository::class)) + ->makePartial() + ->shouldReceive([ + 'newDataObject' => new \PKP\log\event\EventLogEntry, + 'add' => 0, + ]) + ->withAnyArgs() + ->getMock(); + + app()->instance(EventRepository::class, $eventRepoMock); + + $reviewReminderJob->handle(); + + $this->expectNotToPerformAssertions(); + } +}