Bug 34589 - Update on bug 20256 is not idempotent
Summary: Update on bug 20256 is not idempotent
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 20256
Blocks:
  Show dependency treegraph
 
Reported: 2023-08-22 14:59 UTC by Nick Clemens (kidclamp)
Modified: 2023-10-12 15:30 UTC (History)
6 users (show)

See Also:
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


Attachments
Bug 34589: Only update users if permission is added (1.85 KB, patch)
2023-08-22 15:09 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34589: Only update users if permission is added (2.52 KB, patch)
2023-09-06 12:34 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 34589: Only update users if permission is added (2.68 KB, patch)
2023-09-11 16:05 UTC, Emily-Rose Francoeur
Details | Diff | Splinter Review
Bug 34589: Only update users if permission is added (2.77 KB, patch)
2023-09-15 06:18 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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