pkp/pkp-lib#7725 Decision constants sync up

[touhidurabir]

PR related to pkp/pkp-lib#7725

[CLAassistant]

All committers have signed the CLA.

[NateWr]

Will this cause problems with one value effecting another? For example, I found the following:

// \PKP\decision\Decision::ACCEPT
[
    'stage_id' => [WORKFLOW_STAGE_ID_EDITING],
    'current_value' => 1,
    'updated_value' => 2,
],

...

// \PKP\decision\Decision::PENDING_REVISIONS
[
    'stage_id' => [WORKFLOW_STAGE_ID_EXTERNAL_REVIEW],
    'current_value' => 2,
    'updated_value' => 4,
],

Will Decision::ACCEPT get updated to 2 before Decision::PENDING_REVISIONS is changed to 4? And if so, will that cause Decision::ACCEPT to end up as 4? Or is this what transactions are meant to avoid?

[touhidurabir]

is this what transactions are meant to avoid?

No , that is not the purpose of transaction here . Basically we want to have all updated or if failed , no changes to be committed to DB so that we don't end up with unsynced values .

@NateWr you have raised a very important point . The example you have provided , for that case , it will not cause that issue as we are using here combination of current decision value and stage_id. As those are different , it will not be an issue .

But for the following one case

// \PKP\decision\Decision::PENDING_REVISIONS
[
    'stage_id' => [WORKFLOW_STAGE_ID_EXTERNAL_REVIEW],
    'current_value' => 2,
    'updated_value' => 4,
],

// \PKP\decision\Decision::RESUBMIT
[
    'stage_id' => [WORKFLOW_STAGE_ID_EXTERNAL_REVIEW],
    'current_value' => 3,
    'updated_value' => 5,
],

// \PKP\decision\Decision::DECLINE
[
    'stage_id' => [WORKFLOW_STAGE_ID_EXTERNAL_REVIEW],
    'current_value' => 4,
    'updated_value' => 6,
],

here state_id is same that. So in first loop it will update value 2 to 4 . but in the third loop it will update that from 4 to 6 which is undesirable and we end up with unsynced value . One way I can think of updating it is to have a updated_at column and then only update those that has not updated in last 300 seconds . what you think ?

[NateWr]

Yes that could work. Another option is to do a 2-step process, with an intermediate value. Like: 2 -> 9994 -> 4. But maybe there are better ways to handle such DB changes. Maybe ask in #pkp-dev to see if anyone has good ideas?

[touhidurabir]

fixed it .

[NateWr]

Looks good. I had a look at pkp-lib. Do you need to add the updated_at column to the SubmissionsMigration for fresh installs?

[touhidurabir]

Do you need to add the updated_at column to the SubmissionsMigration for fresh installs?

no need for that . right now we do not have any timestamp functionality which used by laravel models . So it will be attached, then updated and then removed . no point in keeping something in DB that we will not use right now.

[NateWr]

Ah, right! I didn't see that it was removed during the upgrade. Good thinking!

[NateWr]

Up to you if you want to leave them in, but these shouldn't exist in any version before 3.4.

[touhidurabir]

yes it should not exist but I put those just to make sure that if any of us devs wont get in trouble for the update with current submission as our own installation . For example , I already have several submission in my own installation with those decisions and once this get merged , if those updates not pushed , it will have unsynced decision value . But if you think it's better to remove, I will do it.

[NateWr]

Yeah makes sense. I'm ok to leave it in.