Description
Lisette Scheer
2023-11-08 16:11:17 UTC
Created attachment 175201 [details] [review] Bug 35292: Database changes Created attachment 175202 [details] [review] Bug 35292: Template changes Created attachment 175203 [details] [review] Bug 35292: Have UpdateNotForLoanStatusOnCheckOut and UpdateNotForLoanStatusOnCheckIn use the same function Both sysprefs use the same codebase. Hence the recent upgrades for UpdateNotForLoanStatusOnCheckOut were not applyied to checkout. In this patch, we refacto code in order to have them both use the same function. Test plan: Do not apply patch: 1 - Set UpdateNotForLoanStatusOnCheckin to 2: 0 2 - Set UpdateNotForLoanStatusOnCheckout to _ALL_: 0: 2 3 - Check out a book -> Its notforloan status is set to 2 4 - Check in the book -> Its notforloan status is set to 0, you get a notification 6 - APPLY PATCH 7 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Set UpdateNotForLoanStatusOnCheckOut to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Check out a book -> Its notforloan status is set to 3 9 - Check in the book -> Its notforloan status is set to 0, you get a 10 - Check out a continuing resource -> Its notforloan status is set to 4 11 - Check in a continuing resource -> Its notforloan status is set to 0 11 - Check out another type of resource -> Its notforloan status is set to 2 12 - Check in the document -> Its notforloan status is set to 0 notification Removing status needs signoff, need to update the test suite Created attachment 175316 [details] [review] Bug 35292: Database changes Created attachment 175317 [details] [review] Bug 35292: Template changes Created attachment 175318 [details] [review] Bug 35292: Have UpdateNotForLoanStatusOnCheckOut and UpdateNotForLoanStatusOnCheckIn use the same function Both sysprefs use the same codebase. Hence the recent upgrades for UpdateNotForLoanStatusOnCheckOut were not applyied to checkout. In this patch, we refacto code in order to have them both use the same function. Test plan: Do not apply patch: 1 - Set UpdateNotForLoanStatusOnCheckin to 2: 0 2 - Set UpdateNotForLoanStatusOnCheckout to _ALL_: 0: 2 3 - Check out a book -> Its notforloan status is set to 2 4 - Check in the book -> Its notforloan status is set to 0, you get a notification 6 - APPLY PATCH 7 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Set UpdateNotForLoanStatusOnCheckOut to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Check out a book -> Its notforloan status is set to 3 9 - Check in the book -> Its notforloan status is set to 0, you get a 10 - Check out a continuing resource -> Its notforloan status is set to 4 11 - Check in a continuing resource -> Its notforloan status is set to 0 11 - Check out another type of resource -> Its notforloan status is set to 2 12 - Check in the document -> Its notforloan status is set to 0 notification Created attachment 175319 [details] [review] Bug 35292: Move tests of UpdateNotforloan together Note: These patches have been split for sake of understandability for review but they should probably be squashed Created attachment 175320 [details] [review] Bug 35292: Update tests for UpdateNotForloanStatusOnCheckout Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 175321 [details] [review] Bug 35292: Add tests for _updateNotForLoanFromYaml Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 175322 [details] [review] Bug 35292: Move tests for UpdateNotForLoanStatusOnCheckin in a subtest Note: These patches have been split for sake of understandability for review but they should probably be squashe Hi, I just tried this test but it quickly failed (even before applying the patch) Steps 1 to 2: ok Step 3 : ok in UNIMARC (but I had to erase the space which stands just before the 2 in UpdateNotForLoanStatusOnCheckout) but not ok in MARC21, the status is not set to 2 Step 4 : in UNIMARC, the status is not set to 0 (I didn't try in MARC21 because it failed before) is there anything I missed ? Same problem as Emmanuel, open a ticket "Bug 39386" Hi, Thanks for your attention to this bug, There was an issue in the test plan, hence it can't work. You need to invert the sysprefs CORRECTED 1&2: 1 - Set UpdateNotForLoanStatusOnCheckout to 2: 0 2 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 0: 2 My precedent comment is still confunsing with both sysprefs, so I edit it :p Hi, Thanks for your attention to this bug, There was an issue in the test plan, hence it can't work. You need to invert the sysprefs CORRECTED 1&2: 1 - Set UpdateNotForLoanStatusOnCheckout to 0: 2 2 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 To explain: UpdateNotForLoanStatusOnCheckout use the old syntax, which we want to update (the purpose of this patch). It should set the notforloanstatus to anything UpdateNotForLoanStatusOnCheckin uses the new syntax, it should, for our tests, set the notforloanstatus to something which does not prevent Better to fix the test plan in the patch. When training people to test patches, the least overwhelming strategy I found for now to avoid drowning in tons of comments with many old versions of the test plan, is to go to "Attachments" here in the ticket. And click on the date to jump directly to the current patches. But it will miss messages like the above. Which might get buried in more comment so not only beginners risk not getting the issue. Created attachment 179818 [details] [review] Bug 35292: Database changes Created attachment 179819 [details] [review] Bug 35292: Template changes Created attachment 179820 [details] [review] Bug 35292: Have UpdateNotForLoanStatusOnCheckOut and UpdateNotForLoanStatusOnCheckIn use the same function Both sysprefs use the same codebase. Hence the recent upgrades for UpdateNotForLoanStatusOnCheckOut were not applyied to checkout. In this patch, we refacto code in order to have them both use the same function. Test plan: Do not apply patch: 1 - Set UpdateNotForLoanStatusOnCheckout to 0: 2 2 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 3 - Check out a book -> Its notforloan status is set to 2 4 - Check in the book -> Its notforloan status is set to 0, you get a notification 6 - APPLY PATCH 7 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Set UpdateNotForLoanStatusOnCheckOut to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Check out a book -> Its notforloan status is set to 3 9 - Check in the book -> Its notforloan status is set to 0, you get a 10 - Check out a continuing resource -> Its notforloan status is set to 4 11 - Check in a continuing resource -> Its notforloan status is set to 0 11 - Check out another type of resource -> Its notforloan status is set to 2 12 - Check in the document -> Its notforloan status is set to 0 notification Created attachment 179821 [details] [review] Bug 35292: Move tests of UpdateNotforloan together Note: These patches have been split for sake of understandability for review but they should probably be squashed Created attachment 179822 [details] [review] Bug 35292: Update tests for UpdateNotForloanStatusOnCheckout Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 179823 [details] [review] Bug 35292: Add tests for _updateNotForLoanFromYaml Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 179824 [details] [review] Bug 35292: Move tests for UpdateNotForLoanStatusOnCheckin in a subtest Note: These patches have been split for sake of understandability for review but they should probably be squashe A problem occured when I tried to apply the patch error: sha1 information is lacking or useless (installer/data/mysql/mandatory/sysprefs.sql). error: could not build fake ancestor Patch failed at 0001 Bug 35292: Database changes [33mhint: Use 'git am --show-current-patch=diff' to see the failed patch[m When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-35292-Database-changes-uulcmt0i.patch Created attachment 180101 [details] [review] Bug 35292: Database changes Created attachment 180102 [details] [review] Bug 35292: Template changes Created attachment 180103 [details] [review] Bug 35292: Have UpdateNotForLoanStatusOnCheckOut and UpdateNotForLoanStatusOnCheckIn use the same function Both sysprefs use the same codebase. Hence the recent upgrades for UpdateNotForLoanStatusOnCheckOut were not applyied to checkout. In this patch, we refacto code in order to have them both use the same function. Test plan: Do not apply patch: 1 - Set UpdateNotForLoanStatusOnCheckout to 0: 2 2 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 3 - Check out a book -> Its notforloan status is set to 2 4 - Check in the book -> Its notforloan status is set to 0, you get a notification 6 - APPLY PATCH 7 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Set UpdateNotForLoanStatusOnCheckOut to _ALL_: 0: 2 BK: 0: 3 CR: 0: 4 8 - Check out a book -> Its notforloan status is set to 3 9 - Check in the book -> Its notforloan status is set to 0, you get a 10 - Check out a continuing resource -> Its notforloan status is set to 4 11 - Check in a continuing resource -> Its notforloan status is set to 0 11 - Check out another type of resource -> Its notforloan status is set to 2 12 - Check in the document -> Its notforloan status is set to 0 notification Created attachment 180104 [details] [review] Bug 35292: Move tests of UpdateNotforloan together Note: These patches have been split for sake of understandability for review but they should probably be squashed Created attachment 180105 [details] [review] Bug 35292: Update tests for UpdateNotForloanStatusOnCheckout Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180106 [details] [review] Bug 35292: Add tests for _updateNotForLoanFromYaml Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180181 [details] [review] Bug 35292: Database changes bsoletes: 180101 - Bug 35292: Database changes Created attachment 180182 [details] [review] Bug 35292: Template changes Created attachment 180183 [details] [review] Bug 35292: Have UpdateNotForLoanStatusOnCheckOut and UpdateNotForLoanStatusOnCheckIn use the same function Both sysprefs use the same codebase. Hence the recent upgrades for UpdateNotForLoanStatusOnCheckOut were not applyied to checkout. In this patch, we refacto code in order to have them both use the same function. Test plan: Do not apply patch: 1 - Set UpdateNotForLoanStatusOnCheckout to 0: 2 2 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 3 - Check out a book -> Its notforloan status is set to 2 4 - Check in the book -> Its notforloan status is set to 0, you get a notification 6 - APPLY PATCH 7 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Set UpdateNotForLoanStatusOnCheckOut to _ALL_: 0: 2 BK: 0: 3 CR: 0: 4 8 - Check out a book -> Its notforloan status is set to 3 9 - Check in the book -> Its notforloan status is set to 0, you get a 10 - Check out a continuing resource -> Its notforloan status is set to 4 11 - Check in a continuing resource -> Its notforloan status is set to 0 11 - Check out another type of resource -> Its notforloan status is set to 2 12 - Check in the document -> Its notforloan status is set to 0 notification Created attachment 180184 [details] [review] Bug 35292: Move tests of UpdateNotforloan together Note: These patches have been split for sake of understandability for review but they should probably be squashed Created attachment 180185 [details] [review] Bug 35292: Update tests for UpdateNotForloanStatusOnCheckout Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180186 [details] [review] Bug 35292: Add tests for _updateNotForLoanFromYaml Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180210 [details] [review] Bug 35292: Move tests for UpdateNotForLoanStatusOnCheckin in a subtest Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180212 [details] [review] Bug 35292: Database changes Created attachment 180213 [details] [review] Bug 35292: Template changes Created attachment 180214 [details] [review] Bug 35292: Have UpdateNotForLoanStatusOnCheckOut and UpdateNotForLoanStatusOnCheckIn use the same function Both sysprefs use the same codebase. Hence the recent upgrades for UpdateNotForLoanStatusOnCheckOut were not applyied to checkout. In this patch, we refacto code in order to have them both use the same function. Test plan: Do not apply patch: 1 - Set UpdateNotForLoanStatusOnCheckout to 0: 2 2 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 3 - Check out a book -> Its notforloan status is set to 2 4 - Check in the book -> Its notforloan status is set to 0, you get a notification 6 - APPLY PATCH 7 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Set UpdateNotForLoanStatusOnCheckOut to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Check out a book -> Its notforloan status is set to 3 9 - Check in the book -> Its notforloan status is set to 0, you get a 10 - Check out a continuing resource -> Its notforloan status is set to 4 11 - Check in a continuing resource -> Its notforloan status is set to 0 11 - Check out another type of resource -> Its notforloan status is set to 2 12 - Check in the document -> Its notforloan status is set to 0 notification Created attachment 180215 [details] [review] Bug 35292: Move tests of UpdateNotforloan together Note: These patches have been split for sake of understandability for review but they should probably be squashed Created attachment 180216 [details] [review] Bug 35292: Update tests for UpdateNotForloanStatusOnCheckout Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180217 [details] [review] Bug 35292: Add tests for _updateNotForLoanFromYaml Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180218 [details] [review] Bug 35292: Move tests for UpdateNotForLoanStatusOnCheckin in a subtest Note: These patches have been split for sake of understandability for review but they should probably be squashe Created attachment 180632 [details] [review] Bug 35292: Database changes Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> Created attachment 180633 [details] [review] Bug 35292: Template changes Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> Created attachment 180634 [details] [review] Bug 35292: Have UpdateNotForLoanStatusOnCheckOut and UpdateNotForLoanStatusOnCheckIn use the same function Both sysprefs use the same codebase. Hence the recent upgrades for UpdateNotForLoanStatusOnCheckOut were not applyied to checkout. In this patch, we refacto code in order to have them both use the same function. 1 - Set UpdateNotForLoanStatusOnCheckin to 2: 0 2 - Set UpdateNotForLoanStatusOnCheckout to _ALL_: 0: 2 3 - Check out a book -> Its notforloan status is set to 2 3b - Check you have 2, 3 and 4 in notforloan, if not add those values with any description 4 - Check in the book -> Its notforloan status is set to 0, you get a notification 6 - APPLY PATCH and update database 7 - Set UpdateNotForLoanStatusOnCheckin to _ALL_: 2: 0 BK: 3: 0 CR: 4: 0 8 - Set UpdateNotForLoanStatusOnCheckOut to _ALL_: 0: 2 BK: 0: 3 CR: 0: 4 8 - Check out a book -> Its notforloan status is set to 3 9 - Check in the book -> Its notforloan status is set to 0, you get a notification 10 - Check out a continuing resource -> Its notforloan status is set to 4 11 - Check in a continuing resource -> Its notforloan status is set to 0 11 - Check out another type of resource -> Its notforloan status is set to 2 12 - Check in the document -> Its notforloan status is set to 0 Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> Created attachment 180635 [details] [review] Bug 35292: Move tests of UpdateNotforloan together Note: These patches have been split for sake of understandability for review but they should probably be squashed Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> Created attachment 180636 [details] [review] Bug 35292: Update tests for UpdateNotForloanStatusOnCheckout Note: These patches have been split for sake of understandability for review but they should probably be squashe Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> Created attachment 180637 [details] [review] Bug 35292: Add tests for _updateNotForLoanFromYaml Note: These patches have been split for sake of understandability for review but they should probably be squashe Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> Created attachment 180638 [details] [review] Bug 35292: Move tests for UpdateNotForLoanStatusOnCheckin in a subtest Note: These patches have been split for sake of understandability for review but they should probably be squashe Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> Comment on attachment 180632 [details] [review] Bug 35292: Database changes >From 318c2ddedf9bacb00bebf1e8465a8c1df40d84fe Mon Sep 17 00:00:00 2001 >From: Baptiste <baptiste.wojtkowski@biblibre.com> >Date: Wed, 4 Dec 2024 17:22:06 +0100 >Subject: [PATCH] Bug 35292: Database changes >MIME-Version: 1.0 >Content-Type: text/plain; charset=UTF-8 >Content-Transfer-Encoding: 8bit > >Signed-off-by: Emmanuel Bétemps <e.betemps@gmail.com> >--- > ...pdateNotForLoanStatusOnCheckOut_syspref.pl | 35 +++++++++++++++++++ > installer/data/mysql/mandatory/sysprefs.sql | 2 +- > 2 files changed, 36 insertions(+), 1 deletion(-) > create mode 100755 installer/data/mysql/atomicupdate/bug_35292-amend_UpdateNotForLoanStatusOnCheckOut_syspref.pl > >diff --git a/installer/data/mysql/atomicupdate/bug_35292-amend_UpdateNotForLoanStatusOnCheckOut_syspref.pl b/installer/data/mysql/atomicupdate/bug_35292-amend_UpdateNotForLoanStatusOnCheckOut_syspref.pl >new file mode 100755 >index 0000000000..ad866723d6 >--- /dev/null >+++ b/installer/data/mysql/atomicupdate/bug_35292-amend_UpdateNotForLoanStatusOnCheckOut_syspref.pl >@@ -0,0 +1,35 @@ >+use Modern::Perl; >+ >+return { >+ bug_number => "35292", >+ description => "Migrating existing UpdateNotForLoanStatusOnCheckout rules to new format", >+ up => sub { >+ my ($args) = @_; >+ my ( $dbh, $out ) = @$args{qw(dbh out)}; >+ >+ my ($UpdateNotForLoanStatusOnCheckout) = $dbh->selectrow_array( >+ q| >+ SELECT value FROM systempreferences WHERE variable='UpdateNotForLoanStatusOnCheckout' >+ | >+ ); >+ >+ my ( $new_rules, $updated_rules ); >+ if ( $UpdateNotForLoanStatusOnCheckout && $UpdateNotForLoanStatusOnCheckout !~ /[0-9a-zA-Z_]:\r/ ) { >+ >+ # Split and re-format the existing rules under a single _ALL_ special term to affect all itemtypes >+ my @rules = split /\r/, $UpdateNotForLoanStatusOnCheckout; >+ foreach my $rule (@rules) { >+ $rule =~ s/^\s+|\s+$|\r|\n//g; >+ $new_rules .= ' ' . $rule . "\r"; >+ } >+ $updated_rules .= "_ALL_:\r$new_rules\r"; >+ $dbh->do( >+ qq{ >+ UPDATE systempreferences >+ SET value = '$updated_rules', explanation = "This is a list of item types and value pairs.\nExamples:\n_ALL_:\n -1: 0\n\nCR:\n 1: 0\n\nWhen an item is checked out, if its item type matches CR then when the value on the left (1) matches the items not for loan value it will be updated to the value on the right.\n\nThe special term _ALL_ is used on the left side of the colon (:) to affect all item types. This does not override all other rules\n\nEach item type needs to be defined on a separate line on the left side of the colon (:).\nEach pair of not for loan values, for that item type, should be listed on separate lines below the item type, each indented by a leading space." >+ WHERE variable = 'UpdateNotForLoanStatusOnCheckout' >+ } >+ ); >+ } >+ }, >+}; >diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql >index 2bf9d0b648..a765ad786b 100644 >--- a/installer/data/mysql/mandatory/sysprefs.sql >+++ b/installer/data/mysql/mandatory/sysprefs.sql >@@ -829,7 +829,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` > ('UpdateItemLostStatusWhenWriteoff', '0', NULL, 'Allows the status of lost items to be automatically changed to lost and paid for when written off', 'Integer'), > ('UpdateItemWhenLostFromHoldList','',NULL,'This is a list of values to update an item when it is marked as lost from the holds to pull screen','Free'), > ('UpdateNotForLoanStatusOnCheckin', '', 'NULL', 'This is a list of item types and value pairs.\nExamples:\n_ALL_:\n -1: 0\n\nCR:\n 1: 0\n\nWhen an item is checked in, if its item type matches CR then when the value on the left (1) matches the items not for loan value it will be updated to the value on the right.\n\nThe special term _ALL_ is used on the left side of the colon (:) to affect all item types. This does not override all other rules\n\nEach item type needs to be defined on a separate line on the left side of the colon (:).\nEach pair of not for loan values, for that item type, should be listed on separate lines below the item type, each indented by a leading space.', 'Free'), >-('UpdateNotForLoanStatusOnCheckout', '', 'NULL', 'This is a list of value pairs. When an item is checked out, if the not for loan value on the left matches the items not for loan value it will be updated to the right-hand value. E.g. ''-1: 0'' will cause an item that was set to ''Ordered'' to now be available for loan. Each pair of values should be on a separate line.', 'Free'), >+('UpdateNotForLoanStatusOnCheckout', '', 'NULL', 'This is a list of item types and value pairs.\nExamples:\n_ALL_:\n -1: 0\n\nCR:\n 1: 0\n\nWhen an item is checked out, if its item type matches CR then when the value on the left (1) matches the items not for loan value it will be updated to the value on the right.\n\nThe special term _ALL_ is used on the left side of the colon (:) to affect all item types. This does not override all other rules\n\nEach item type needs to be defined on a separate line on the left side of the colon (:).\nEach pair of not for loan values, for that item type, should be listed on separate lines below the item type, each indented by a leading space.', 'Free'), > ('UpdateTotalIssuesOnCirc','0',NULL,'Whether to update the totalissues field in the biblio on each circ.','YesNo'), > ('UploadPurgeTemporaryFilesDays','',NULL,'If not empty, number of days used when automatically deleting temporary uploads','integer'), > ('uppercasesurnames','0',NULL,'If ON, surnames are converted to upper case in patron entry form','YesNo'), >-- >2.39.5 I tried to do a one character fix via the bugzilla web interface ("edit attachmement as comment") and it didn't work ^^" (1) matches the items' vs (1) matches the items In dbrev vs new db Created attachment 180646 [details] [review] Bug 35292: (QA follow-up) To squash with the main patches |