search

pkp / pkp-lib

The library used by PKP's applications OJS, OMP and OPS, open source software for scholarly publishing.
https://pkp.sfu.ca
GNU General Public License v3.0
297 stars 442 forks source link

Data loss at I7191_EditorAssignments migration #9851

Open jonasraoni opened 3 months ago

jonasraoni commented 3 months ago

Describe the bug The migration I7191_EditorAssignments is causing data loss at the editor assignments.

Problems:

  1. The setUserGroup() is supposed to set the user_group_id for all existing assignments, but it's just taking a single context_id, see: https://github.com/pkp/pkp-lib/blob/1dae820fa8b07868f1efa999c25cc3745f19f30a/classes/migration/upgrade/v3_4_0/I7191_EditorAssignments.php#L78-L79
  2. The migration is clearing the assignments if there's no section editor role, but a pre-flight check to ask the user to create the role sounds like a better choice.
  3. The addAutomatedAssignments() seems to be assigning unexpected users (a user that had a designer/ROLE_ID_REVIEWER role was assigned as section editor)

To Reproduce

  1. Ensure you have at least 2 journals on your installation
  2. Setup editor assignments for both at the Journal > Section
  3. Migrate to OJS 3.4
  4. Watch the assignments

What application are you using? OJS 3.4

jonasraoni commented 3 months ago

@asmecher Please, take a look at the PRs:

asmecher commented 2 months ago

Looks good, @jonasraoni. Can you port to OMP and OPS as well, and forward to main?

asmecher commented 1 month ago

@jonasraoni, I'm getting an error on an attempted upgrade from 3.2.1-x to 3.4.0 that didn't happen prior to these changes:

ERROR: Upgrade failed: DB: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '32-156-530-125' for key 'section_editors_unique' (SQL: insert into subeditor_submission_group (assoc_id, assoc_type, context_id, user_group_id, user_id) values (6, 530, 5, 71, 16), (52, 530, 5, 71, 16), (53, 530, 5, 71, 16), (54, 530, 5, 71, 16), (55, 530, 5, 71, 16), (56, 530, 5, 71, 16), (57, 530, 5, 71, 16), (58, 530, 5, 71, 16), (59, 530, 5, 71, 16), (9, 530, 7, 111, 97), (130, 530, 7, 111, 97), (133, 530, 7, 111, 97), (134, 530, 7, 111, 97), (135, 530, 7, 111, 97), (137, 530, 7, 111, 97), (138, 530, 7, 111, 97), (140, 530, 7, 111, 97), (98, 530, 19, 310, 118), (99, 530, 19, 310, 118), (100, 530, 19, 310, 118), (101, 530, 19, 310, 118), (102, 530, 19, 310, 118), (103, 530, 19, 310, 118), (104, 530, 19, 310, 118), (105, 530, 19, 310, 118), (106, 530, 19, 310, 118), (107, 530, 19, 310, 118), (108, 530, 19, 310, 118), (109, 530, 19, 310, 118), (110, 530, 19, 310, 118), (111, 530, 19, 310, 118), (112, 530, 19, 310, 118), (148, 530, 27, 446, 110), (155, 530, 31, 514, 10), (156, 530, 32, 532, 125), (156, 530, 32, 538, 125), (168, 530, 39, 651, 168), (169, 530, 40, 668, 166))

I can provide a copy of the DB if you like.

jonasraoni commented 1 month ago

@asmecher Thanks! I was planning to run an upgrade against a large installation to test this one better, together with another PR. Extra backups are always welcome.

jonasraoni commented 1 month ago

I finally checked this issue and looks it's related to https://github.com/pkp/pkp-lib/issues/8737, I'll remove the migration that Touhidur created and move it to the I7191_EditorAssignments, which is the place where the new field was introduced and should probably address the error above.

jonasraoni commented 1 month ago

@asmecher If you still have the database in hands, you might try to re-test with the PRs below in place.

asmecher commented 1 month ago

I've passed those on to SciELO for a test, thanks!

jonasraoni commented 1 month ago

Great, once we get a confirmation, I'll merge and cherry pick. The large installation that I tested didn't have this issue.

nils-stefan-weiher commented 5 days ago

Dear @jonasraoni ,

If found an issue with one migration belonging to this issue while doing upgrade test with one of our 3.2.1 OJS installations.

It seems the current stable_3_4_0 version of the migration: https://github.com/pkp/pkp-lib/blob/stable_3_4_0/classes/migration/upgrade/v3_4_0/I7191_EditorAssignments.php leads to the following SQL error during upgrade:

2024-07-02 18:28:19 [migration: APP\migration\upgrade\v3_4_0\InstallEmailTemplates]
2024-07-02 18:28:27 [migration: PKP\migration\upgrade\v3_4_0\I7874_NotificationMetadataModifiedRemove]
2024-07-02 18:28:27 [migration: APP\migration\upgrade\v3_4_0\I7191_EditorAssignments]
2024-07-02 18:28:28 [revert migration: PKP\migration\upgrade\v3_4_0\I7874_NotificationMetadataModifiedRemove]
2024-07-02 18:28:28 [revert migration: APP\migration\upgrade\v3_4_0\InstallEmailTemplates]
2024-07-02 18:28:28 [revert migration: APP\migration\upgrade\v3_4_0\I5716_EmailTemplateAssignments]
2024-07-02 18:28:28 [revert migration: PKP\migration\upgrade\v3_4_0\I7287_RemoveEmailTemplatesDefault]
2024-07-02 18:29:00 [revert migration: APP\migration\upgrade\v3_4_0\I7796_UpdateCrossrefSchema]
2024-07-02 18:29:00 [downgrade for "APP\migration\upgrade\v3_4_0\I7796_UpdateCrossrefSchema" unsupported: Downgrade not supported]
ERROR: Upgrade failed: DB: SQLSTATE[HY093]: Invalid parameter number (SQL: update `subeditor_submission_group` as `ssg` set `user_group_id` = (select `ug`.`user_group_id` from `user_groups` as `ug` where `ug`.`context_id` = `ssg`.`context_id` and `ug`.`role_id` = ? order by `ug`.`is_default` desc, `ug`.`permit_metadata_edit` desc limit 1) where `user_group_id` is null)

This was updated recently with the following commit: https://github.com/pkp/pkp-lib/commit/5d644008ff1c67a8c7d4defccf02a3853ab6c9d2

I am now testing if reverting this change back to whereRaw fixes the issue.

nils-stefan-weiher commented 5 days ago

Running the migration with the line of code before the commit https://github.com/pkp/pkp-lib/commit/5d644008ff1c67a8c7d4defccf02a3853ab6c9d2 works (using whereRaw instead of where).

I think it probably has to do with the fact, that its later used embedded as a raw SQL query inside the update query executed in line 90 to 92:

DB::table('subeditor_submission_group', 'ssg')
            ->whereNull('user_group_id')
            ->update(['user_group_id' => DB::raw("({$bestUserGroupIdQuery->toSql()})")]);
jonasraoni commented 5 days ago

Thanks @nils-stefan-weiher! When I was updating, I suspected there was something smelling on this place (I'm the person who added this whereRaw 😁), but looks like I forgot to investigate further.

And FYI, this issue has extra pending PRs due to another problem that I found: