Bug 34589

Summary: Update on bug 20256 is not idempotent
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: aspencatteam, dcook, fridolin.somers, kyle, m.de.rooy, matt.blenkinsop
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04
Circulation function:
Bug Depends on: 20256    
Bug Blocks:    
Attachments: Bug 34589: Only update users if permission is added
Bug 34589: Only update users if permission is added
Bug 34589: Only update users if permission is added
Bug 34589: Only update users if permission is added

Description Nick Clemens (kidclamp) 2023-08-22 14:59:52 UTC
The last commit attempted to make the update idempotent, however, it missed a case.

If the update is run, then a patron has the permission removed, they will get it again on a subsequent update.

We should check if the permission exists in the permissions table, and only perform the first two updates if it does not
Comment 1 Nick Clemens (kidclamp) 2023-08-22 15:09:52 UTC
Created attachment 154702 [details] [review]
Bug 34589: Only update users if permission is added
Comment 2 David Cook 2023-09-06 00:31:34 UTC
Can we get a test plan for this one?
Comment 3 Nick Clemens (kidclamp) 2023-09-06 12:34:53 UTC
Created attachment 155271 [details] [review]
Bug 34589: Only update users if permission is added

To test:
1 - Assign a user 'edit_items' permission
2 - Manually run the update
    perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/221200003.pl"));'
3 - Edit patron permissions
4 - Note they now have edit_any_item permission
5 - Remove that permission
6 - Run the update again
7 - Edit patron permissions
8 - They have edit_any_item again - remove it
9 - Apply patch
10 - Run the update
11 - Edit patron permissions
12 - No new permissions added!
13 - sudo koha-mysql kohadev
     DELETE FROM permissions WHERE code = 'edit_any_item'
14 - Run the update again
15 - Edit patron permissions
16 - They do have edit_any_item
Comment 4 Emily-Rose Francoeur 2023-09-11 16:05:12 UTC
Created attachment 155501 [details] [review]
Bug 34589: Only update users if permission is added

To test:
1 - Assign a user 'edit_items' permission
2 - Manually run the update
    perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/221200003.pl"));'
3 - Edit patron permissions
4 - Note they now have edit_any_item permission
5 - Remove that permission
6 - Run the update again
7 - Edit patron permissions
8 - They have edit_any_item again - remove it
9 - Apply patch
10 - Run the update
11 - Edit patron permissions
12 - No new permissions added!
13 - sudo koha-mysql kohadev
     DELETE FROM permissions WHERE code = 'edit_any_item'
14 - Run the update again
15 - Edit patron permissions
16 - They do have edit_any_item

Signed-off-by: Émily-Rose Francoeur <emily-rose.francoeur@inLibro.com>
Comment 5 Marcel de Rooy 2023-09-15 06:16:51 UTC
I wont object to this patch, but I think it is arguable.
Insert in permissions, insert in userpermissions. Repeat does it again. Feels quite idempotent imo.
Comment 6 Marcel de Rooy 2023-09-15 06:18:02 UTC
And another note: DBI::do does not promise a good return: Returns the number of rows affected or undef on error. A return value of -1 means the number of rows is not known, not applicable, or not available.
Comment 7 Marcel de Rooy 2023-09-15 06:18:43 UTC
Created attachment 155634 [details] [review]
Bug 34589: Only update users if permission is added

To test:
1 - Assign a user 'edit_items' permission
2 - Manually run the update
    perl -e 'use C4::Installer; warn Data::Dumper::Dumper(C4::Installer::run_db_rev("installer/data/mysql/db_revs/221200003.pl"));'
3 - Edit patron permissions
4 - Note they now have edit_any_item permission
5 - Remove that permission
6 - Run the update again
7 - Edit patron permissions
8 - They have edit_any_item again - remove it
9 - Apply patch
10 - Run the update
11 - Edit patron permissions
12 - No new permissions added!
13 - sudo koha-mysql kohadev
     DELETE FROM permissions WHERE code = 'edit_any_item'
14 - Run the update again
15 - Edit patron permissions
16 - They do have edit_any_item

Signed-off-by: Émily-Rose Francoeur <emily-rose.francoeur@inLibro.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Tomás Cohen Arazi (tcohen) 2023-09-15 18:50:24 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 9 Fridolin Somers 2023-09-18 08:46:12 UTC
Pushed to 23.05.x for 23.05.04
Comment 10 Matt Blenkinsop 2023-10-12 15:30:57 UTC
Bug 202356 is not in 22.11.x, not backporting