pkp/pkp-lib#5885 Review remainder update #9612

touhidurabir · 2024-01-11T13:52:37Z

Vitaliy-1

Thanks @touhidurabir! I left a couple of minor comments but what I'm thinking about is the possibility for journals to set the number of reminders they want rather than predefined before... and after... fields. Have you considered this option?

Vitaliy-1 · 2024-01-18T16:54:36Z

classes/components/forms/context/PKPReviewSetupForm.php

-                'size' => 'small',
-            ]))
-                ->addField(new FieldText('numDaysBeforeSubmitReminder', [
+            // $this->addField(new FieldText('numDaysBeforeInviteReminder', [


Just keeping a reminder that these commented lines should be removed before merging

Vitaliy-1 · 2024-01-18T16:57:27Z

classes/components/forms/context/PKPReviewSetupForm.php

+            //     ]));
+
+            $this
+                ->addField(new FieldHTML('reviewRequestResponseRemainder', [


reviewRequestResponseRemainder => submissionReviewResponseReminder

Vitaliy-1 · 2024-01-18T20:09:15Z

classes/migration/upgrade/v3_5_0/I5885_RenameReviewRemainderSettingsName.php

+
+namespace PKP\migration\upgrade\v3_5_0;
+
+use Illuminate\Database\Schema\Blueprint;


Can you check which imports are not necessary here?

Vitaliy-1 · 2024-01-18T20:21:28Z

classes/task/ReviewReminder.php

+                        $currentDate->gte($dateResponseDue) && 
+                        $currentDate->diffInDays($dateResponseDue) >= $numDaysAfterReviewResponseReminderDue) {
+
+                        // ACTION:-> we need to sent a AFTER REVIEW REQUEST RESPONSE remainder


...to send the AFTER REVIEW REQUEST RESPONSE reminder

Can you check this in other comments too?

Vitaliy-1 · 2024-01-18T20:22:08Z

classes/task/ReviewReminder.php

+
+                if ($reviewAssignment->getDateConfirmed() === null) {
+                    // review request has not been responded
+                    // previous remainder was a BEFORE REVIEW REQUEST RESPONSE remainder


Vitaliy-1 · 2024-01-18T21:13:45Z

classes/validation/ValidatorDateConparison.php

+use Exception;
+use PKP\validation\ValidatorFactory;
+
+class ValidatorDateConparison extends Validator


ValidatorDateConparison => ValidatorDateComparison

Vitaliy-1 · 2024-01-18T21:49:58Z

classes/form/validation/FormValidatorDateCompare.php

+     * @param string                    $type           the type of check, either "required" or "optional"
+     * @param string                    $message        the error message for validation failures (i18n key)
+     */
+    public function __construct(&$form, $field, $comparingDate, $comparingRule,  $type = 'optional', $message = 'email.invalid')


The default value for $message looks wrong

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

Vitaliy-1 · 2024-01-18T22:13:51Z

jobs/email/ReviewRemainder.php

+
+        $submission = Repo::submission()->get($this->submissionId);
+
+        $contextService = Services::get("context");


"context" => 'context'

Vitaliy-1 · 2024-01-18T22:16:38Z

jobs/email/ReviewRemainder.php

+use PKP\jobs\BaseJob;
+
+
+class ReviewRemainder extends BaseJob


ReviewReminder

Vitaliy-1 · 2024-01-20T15:07:23Z

@touhidurabir, thinking about this further, one way to achieve this is adding a context setting that depicts a number of email reminders to show. Something like:

$this->addField(new FieldText('numReviewRequestResponseReminders', [
    'label' => __('manager.setup.reviewOptions.numReviewRequestResponseReminders'),
     'description' => __('manager.setup.reviewOptions.numReviewRequestResponseReminders.description'),
     'value' => $context->getData('numReviewRequestResponseReminders'),
     'size' => 'small',
]));

And a setting to hold values for all custom fields. Each reminder option should accept negative numbers if implementing this way

foreach ($context->getData('reviewRequestResponseReminders') as $reviewRequestResponseReminder) {
    $this->addField(new FieldText('requestResponseReminder', [
    ...
    ]));
}

This also requires Vue form modification to support new fields added/subtracted dynamically - by listening to the change event, which is emitted when a field prop changes.

Another way could be adding a new form field component that dynamically supports custom number of inputs depending on a value of another field...

touhidurabir · 2024-01-30T13:03:10Z

Hi @Vitaliy-1 , can you explain how the negative number works in bit more details , I am still bit confused how it will work . I have followed the following implementation approach

set reminder day before and after the review request response due date if invited reviewer has not yet responded to the review invitation request.
set reminder day before and after the review submission due date if the reviewer has accepted the invitation but has not submitted the review yet .

The implementation that I have designed is so that it will only sent one such reminder for each case that is

sent only one review request response due reminder before the response due date if has not yet responded to the request and only if that day value is set .
sent only one review request response due reminder after the response due date if has not yet responded to the request and only if that day value is set .
sent only one review submission due reminder before the review submission due date if reviewer has not yet submitted the review and only if that day value is set .
sent only one review submission due reminder after the review submission due date if reviewer has not yet submitted the review and only if that day value is set .

so for each above 4 cases, only one reminder for each case and no more than that .

asmecher · 2024-01-30T16:02:38Z

Hey @touhidurabir -- please hold off on this one until @Devika008 has had a chance to review things.

asmecher

This is promising, @touhidurabir; I've added some comments, and it also needs some work per Devika's design work to make it friendlier.

asmecher · 2024-02-01T20:23:12Z

classes/form/validation/FormValidatorDateCompare.php

+}
+
+if (!PKP_STRICT_MODE) {
+    class_alias('\PKP\form\validation\FormValidatorDateCompare', '\FormValidatorDateCompare');


These should only be necessary for old code; for new code, just make sure to use FQCNs and use statements where they're needed.

asmecher · 2024-02-01T20:24:00Z

classes/jats/exceptions/UnableToCreateJATSContentException.php

    {
-        parent::__construct(__('publication.jats.defaultContentCreationError'), null, $innerException);
+        parent::__construct(


Should these changes be in a different PR?

Should these changes be in a different PR?

that is true but because of this I was having some issue with some of workflow page loading to run some tests

It's already been resolved in main in 8ded2f3.

Then it will be removed at next rebase

This is still showing up in the PR.

I kept it because we are passing second param as null at here which is not allowed as that should be a int as specified here

classes/migration/upgrade/v3_5_0/I5885_RenameReviewReminderSettingsName.php

classes/task/ReviewReminder.php

jobs/email/ReviewReminder.php

touhidurabir · 2024-03-14T09:39:06Z

@asmecher take a look at the PR again .

asmecher

Thanks, @touhidurabir; here are some minor changes to suggest. When this is reviewed, could you follow up by posting some screenshots showing the change on the issue, and tag Devika for a look?

asmecher · 2024-04-19T22:16:36Z

classes/components/forms/context/PKPReviewSetupForm.php

+        $this
+            ->addDefaultFields($context)
+            ->addReminderFields($context)
+            ->addReminderDisbaleNoticeField($context);


asmecher · 2024-04-19T22:18:11Z

classes/form/validation/FormValidatorDateCompare.php

+/**
+ * @file classes/form/validation/FormValidatorDateCompare.php
+ *
+ * Copyright (c) 2014-2021 Simon Fraser University


2024 for new code.

asmecher · 2024-04-19T22:19:33Z

classes/jats/exceptions/UnableToCreateJATSContentException.php

    {
-        parent::__construct(__('publication.jats.defaultContentCreationError'), null, $innerException);
+        parent::__construct(


This is still showing up in the PR.

asmecher · 2024-04-19T22:35:48Z

classes/validation/ValidatorDateComparison.php

+
+    public function __construct(DateTimeInterface|Carbon $comparingDate, string $comparingRule)
+    {
+        if (!in_array($comparingRule, static::getComparingRules())) {


What do you think about using an Enum for this, instead of runtime validation? We've applied this to a few other places in the code, and so far I like it.

done refactoring to enum . I was not sure if we are using enums but as I see we are now, better to do that from now on rather than using constants .

asmecher · 2024-04-19T22:36:22Z

classes/validation/ValidatorDateComparison.php

+}
+
+if (!PKP_STRICT_MODE) {
+    class_alias('\PKP\validation\ValidatorDateComparison', '\ValidatorDateComparison');


We don't need class aliases for new code -- only for backwards compatibility with old code. (As above)

asmecher · 2024-04-19T22:37:10Z

jobs/email/ReviewReminder.php

+    {
+        parent::__construct();
+
+        $this->reviewAssignmentId = $reviewAssignmentId;


Since you're using promotion, there's no need to assign these in the constructor.

asmecher · 2024-04-19T22:38:36Z

jobs/email/ReviewReminder.php

+{
+    public function __construct(
+        public int $reviewAssignmentId,
+        public int $submissionId,


The $submissionId and $contextId can be derived from the review assignment; would it be better not to serialize them then have to check for discrepancies? (For example, later on we look at the $submissionId to fetch the submission from the database -- but there's no check to ensure it matches the submission ID in the review assignment. It would be better just to use the reviewAssignmentId, then get the submission ID etc. from that.)

I think thats better as we intend to keep the job serialized data as small as possible .

However I think we need to set the standard to passing contextId whenever it's possible as our Services class bit too hard to mock.

asmecher · 2024-04-19T22:40:01Z

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 euqal to response due date."


touhidurabir · 2024-04-24T11:56:57Z

tests/PKPTestCase.php

@@ -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);


a leftover from #9566. this and above in the function signature .

touhidurabir · 2024-04-25T06:21:29Z

classes/invitation/invitations/ReviewerAccessInvite.php

@@ -23,7 +23,7 @@
 use PKP\invitation\invitations\enums\InvitationStatus;
 use PKP\mail\variables\ReviewAssignmentEmailVariable;
 use PKP\security\Validation;
-use ReviewAssignment;
+use PKP\submission\reviewAssignment\ReviewAssignment;


This proper namespace update is required for mocking

touhidurabir · 2024-04-25T11:32:01Z

@asmecher can you take another look specifically at the commit test added for queue job . I have added some tests and at the same time I had to update in the core PKPTestCase .

touhidurabir · 2024-06-14T09:00:41Z

@asmecher rebased and all tests are passing . please take a final look .

asmecher · 2024-07-12T19:16:02Z

locale/en/manager.po

 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 "


Missing space before (

asmecher · 2024-07-12T19:17:18Z

locale/en/manager.po

-msgid "manager.setup.reviewOptions.reminders.submit"
-msgstr "Review Reminder"
+msgid "manager.setup.reviewOptions.reminders.response.before"
+msgstr "Response Request Response - Before Due Date"


Should be "Review Request Response", not "Response Request Response"

asmecher · 2024-07-12T19:17:52Z

locale/en/manager.po

-"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 "Response Request Response - After Due Date"


Also here: should be "Review Request Response", not "Response Request Response"

…ponent enhancement

… path before resolving from request

…plementation

…lass

touhidurabir mentioned this pull request Jan 11, 2024

[OJS/OMP] Ability to send review reminders prior to due date #5885

Closed

touhidurabir force-pushed the i5885_main branch 2 times, most recently from a530c90 to 0eca63d Compare January 12, 2024 10:24

Vitaliy-1 requested changes Jan 18, 2024

View reviewed changes

touhidurabir force-pushed the i5885_main branch from cc2d253 to 4a450bb Compare January 30, 2024 12:53

asmecher requested changes Feb 1, 2024

View reviewed changes

touhidurabir force-pushed the i5885_main branch 2 times, most recently from 60dae69 to 8cf585d Compare February 19, 2024 11:27

touhidurabir force-pushed the i5885_main branch from 8c4b2b0 to c2a1432 Compare March 14, 2024 09:34

touhidurabir force-pushed the i5885_main branch 2 times, most recently from 00e1ad2 to f545ee4 Compare April 19, 2024 11:27

asmecher requested changes Apr 19, 2024

View reviewed changes

touhidurabir force-pushed the i5885_main branch 2 times, most recently from 2936c39 to ccbbc26 Compare April 24, 2024 11:49

touhidurabir commented Apr 24, 2024

View reviewed changes

touhidurabir commented Apr 25, 2024

View reviewed changes

touhidurabir force-pushed the i5885_main branch 3 times, most recently from cd08a01 to e4761a8 Compare May 3, 2024 13:47

touhidurabir force-pushed the i5885_main branch from e4761a8 to 8f8d5c8 Compare May 9, 2024 13:39

touhidurabir force-pushed the i5885_main branch from 8f8d5c8 to 6de029f Compare May 24, 2024 09:37

touhidurabir force-pushed the i5885_main branch from fe7018e to 9033829 Compare June 13, 2024 12:47

touhidurabir force-pushed the i5885_main branch 2 times, most recently from b4b5fda to d4ad06b Compare July 1, 2024 07:17

asmecher reviewed Jul 12, 2024

View reviewed changes

touhidurabir force-pushed the i5885_main branch from b4023f2 to e68f392 Compare July 25, 2024 12:44

touhidurabir force-pushed the i5885_main branch from e68f392 to f7b56a5 Compare August 14, 2024 06:42

touhidurabir added 22 commits August 30, 2024 14:16

pkp#5885 Review remainder update

6553659

pkp#5885 Review remainder update issues fixed

1c6b007

pkp#5885 typo fixed

4634d89

pkp#5885 update remainder calculation

763efa1

pkp#5885 removed unnecessary select from migration

b5f216f

pkp#5885 updated and added new from component to handle reminder

9534d0b

pkp#5885 removed unnecessary fields and controls with field range com…

10c5684

…ponent enhancement

pkp#5885 updated using new slider component

1c252e1

pkp#5885 updated job class properties to public

9eabe38

pkp#5885 updated date validation rules to enum

88250e1

pkp#5885 fixed displaying error on date comparison validation failure

7644c86

pkp#5885 test added for queue job

3c6e156

pkp#5885 added mocked context service and context class for job testing

25881f3

pkp#5885 added context checking first if available to resolve context…

c87ff62

… path before resolving from request

pkp#5885 job test update

2f3f023

pkp#5885 job test update

a10b50a

pkp#5885 removed mistakenly pulled BaseInvitation during rebase

8b0ff12

pkp#5885 updated job and test based on new inviation functionality im…

182d711

…plementation

pkp#5885 updated task

2391787

pkp#5885 translation update

0c1cc5d

pkp#5885 tests updated and removed reference of deprecated Services c…

f77d1e6

…lass

pkp#5885 test update

98a5db2

touhidurabir force-pushed the i5885_main branch from 45e4be1 to 98a5db2 Compare August 30, 2024 08:16

pkp#5885 test update

e967484

asmecher merged commit 41b38eb into pkp:main Aug 30, 2024
1 check passed


		namespace PKP\migration\upgrade\v3_5_0;

		use Illuminate\Database\Schema\Blueprint;


		$submission = Repo::submission()->get($this->submissionId);

		$contextService = Services::get("context");

pkp/pkp-lib#5885 Review remainder update #9612

pkp/pkp-lib#5885 Review remainder update #9612

Conversation

touhidurabir commented Jan 11, 2024

Vitaliy-1 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

Vitaliy-1 commented Jan 20, 2024

touhidurabir commented Jan 30, 2024

asmecher commented Jan 30, 2024

asmecher 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

touhidurabir commented Mar 14, 2024

asmecher 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

touhidurabir Apr 22, 2024 • edited Loading

Choose a reason for hiding this comment

asmecher Apr 19, 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

touhidurabir commented Apr 25, 2024

touhidurabir commented Jun 14, 2024

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 Apr 22, 2024 •

edited

Loading

asmecher Apr 19, 2024 •

edited

Loading