Skip to content

pkp/pkp-lib#11241 Added missing decisions migration#4823

Merged
asmecher merged 1 commit intopkp:stable-3_5_0from
touhidurabir:i11241_stable_3_5_0
Apr 17, 2025
Merged

pkp/pkp-lib#11241 Added missing decisions migration#4823
asmecher merged 1 commit intopkp:stable-3_5_0from
touhidurabir:i11241_stable_3_5_0

Conversation

@touhidurabir
Copy link
Copy Markdown
Member

Copy link
Copy Markdown
Member

@asmecher asmecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about the complicated case you've documented after our conversation -- I think I'd like to try to use the versions table to determine what submissions will be at risk for the incorrect data. I would suggest:

  • Update the 3.3 to 3.4 migrations to properly handle the missing cases -- that way we won't have bad data in the first place, and users who migrate right from 3.3 LTS to 3.5 will have a smooth pathway. (I would also recommend that we do this for the stable-3_4_0 branch -- but that will introduce more risk and complications than I would consider worthwhile.)
  • This migration should be moved into the <upgrade minversion="3.4.0.0" maxversion="3.4.9.9"> clause in dbscripts/xml/upgrade.xml, so that it won't be run for installations that didn't go through 3.4.0 first. This will reduce the risk for the 3.3 to 3.5 pathway, since the migrations for that will be corrected above.
  • This migration should be adapted to check the versions table so that it only modifies editorial decisions that were recorded before the upgrade to 3.4.0.

I think that'll cover it!

@touhidurabir
Copy link
Copy Markdown
Member Author

touhidurabir commented Apr 11, 2025

Hi @asmecher can you please take a look at PR as I have tried to accommodate your suggestion at #4823 (review) into the update .

This need to port to main and will do once finalise for 3.5.0 .

Copy link
Copy Markdown
Member

@asmecher asmecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @touhidurabir! Just two trivial comments.

Comment thread dbscripts/xml/upgrade.xml Outdated

<upgrade minversion="3.4.0.0" maxversion="3.4.9.9">
<migration class="APP\migration\upgrade\v3_4_0\I9813_QuickSubmitSubmissionProgressType"/>
<migration class="APP\migration\upgrade\v3_4_0\I11241_MissingDecisionConstantsUpdate"/>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a 3.4.0 to 3.5.0 upgrade script, so it should be in the v3_5_0 directory.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I had previously . As this migration can also run when upgrading from 3.4.0-* like what we have for APP\migration\upgrade\v3_4_0\I9813_QuickSubmitSubmissionProgressType which added at pkp/pkp-lib#9813 , so I moved it to v3_4_0 directory . As this can run also any new release of 3.4.0 and run on such upgrade , or I am misunderstanding the process ?

->orderBy('date_installed')
->first();

if ($firstInstalledVersion->major === 3 && $firstInstalledVersion->minor === 4) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To guard against more cases of pkp/pkp-lib#10034, I'd suggest relaxing the strict === checks on numeric values to == instead (and !== to !=1 below).

@touhidurabir touhidurabir force-pushed the i11241_stable_3_5_0 branch 2 times, most recently from 74aa4bf to c2b2a9d Compare April 17, 2025 18:16
pkp/pkp-lib#11241 moved the migration in 3.4.0.0-3.4.9.9 block and fixed older migration for 7725

pkp/pkp-lib#11241 relax the strict comparison

pkp/pkp-lib#11241 moved the migration back to v3_5_0 directory
@asmecher asmecher merged commit bd7691b into pkp:stable-3_5_0 Apr 17, 2025
16 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants