|
Description
Jonathan Druart
2019-05-03 21:33:09 UTC
Created attachment 89335 [details] [review] Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank. I have trouble finding the root of the issue, but I will propose a patch to fix both problem. Certainly a mismatch between bug 18887 and bug 18925. Test plan: 0. Do not apply the patch 1. Notice the issue on the circulation rules screen 2. Apply the patch 3. Confirm that the value you set are correctly displayed (and not the propagated ones!) Created attachment 89373 [details] [review] Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank. I have trouble finding the root of the issue, but I will propose a patch to fix both problem. Certainly a mismatch between bug 18887 and bug 18925. Test plan: 0. Do not apply the patch 1. Notice the issue on the circulation rules screen 2. Apply the patch 3. Confirm that the value you set are correctly displayed (and not the propagated ones!) Signed-off-by: Liz Rea <wizzyrea@gmail.com> Created attachment 89401 [details] [review] Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank. I have trouble finding the root of the issue, but I will propose a patch to fix both problem. Certainly a mismatch between bug 18887 and bug 18925. Test plan: 0. Do not apply the patch 1. Notice the issue on the circulation rules screen 2. Apply the patch 3. Confirm that the value you set are correctly displayed (and not the propagated ones!) Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Created attachment 89439 [details] [review] Bug 22847: Correctly displayed circ rule values for max_holds and maxissue* There are several things going wrong on the circulation rules screen. The global rule is displayed for specific libraries (even if the rule does not exist for the library), and max_holds is always blank. I have trouble finding the root of the issue, but I will propose a patch to fix both problem. Certainly a mismatch between bug 18887 and bug 18925. Test plan: 0. Do not apply the patch 1. Notice the issue on the circulation rules screen 2. Apply the patch 3. Confirm that the value you set are correctly displayed (and not the propagated ones!) Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Agustin Moyano <agustinmoyano@theke.io> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Awesome work all! Pushed to master for 19.05 Created attachment 89863 [details] [review] Bug 22847: [18.11] Correctly displayed circ rule values for max_holds Here a version for 18.11, it needs a signoff!
1. Fill the 3 fields of Default checkout, hold and return policy
2. Same for a particular library with different values
3. Unset the policy for the particular library
4. Then you will see that the field "Maximum total holds allowed (count)" ends up with the value for all libraries policy
5. Change the value for all libraries policy, then return to the particular library
6. You should see that the value was updated as well
7. Which is wrooooooong
8. Apply the patch
9. You should see that the particular library policy is empty
10. You should see in the all lib policy that the "Maximum total holds allowed (count)" field is not empty
- Is that expected???
11. Refill all the fields
12. Then you should see that for both policies, "Maximum total holds allowed (count)" stays empty
13. Wait this is definitely not supposed to happen!
14. Nooooooooooo! T_T
Looks like this isn't ready for 18.11.x yet.. setting to needs-more-work I'm investigating why does the UNIMARC dump fails since this was merged in master. (MARC21 upgrades fine) http://git.koha-community.org/gitweb/?p=contrib/global.git;a=history;f=sandbox/sql/sandbox2.sql.gz;hb=HEAD logs: Upgrade to 18.12.00.019 done (Bug 21728 - Add 'Reserve Fee' to the account_offset_types table if missing) DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`circulation_rules`, CONSTRAINT `circ_rules_ibfk_3` FOREIGN KEY (`itemtype`) REFERENCES `itemtypes` (`itemtype`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT IF(categorycode='*', NULL, categorycode), IF(branchcode='*', NULL, branchcode), IF(itemtype='*', NULL, itemtype), 'maxissueqty', COALESCE( maxissueqty, '' ) FROM issuingrules "] at /home/koha/src/installer/data/mysql/updatedatabase.pl line 17537. DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha`.`circulation_rules`, CONSTRAINT `circ_rules_ibfk_3` FOREIGN KEY (`itemtype`) REFERENCES `itemtypes` (`itemtype`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement " INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value ) SELECT IF(categorycode='*', NULL, categorycode), IF(branchcode='*', NULL, branchcode), IF(itemtype='*', NULL, itemtype), 'maxonsiteissueqty', COALESCE( maxonsiteissueqty, '' ) FROM issuingrules "] at /home/koha/src/installer/data/mysql/updatedatabase.pl line 17546. Upgrade to 18.12.00.020 done (Bug 18925 - Move maxissueqty and maxonsiteissueqty to circulation_rules) My bad, wrong bugzilla. (In reply to Victor Grousset/tuxayo from comment #7) I do not understand what you is trying to show. > 10. You should see in the all lib policy that the "Maximum total holds > allowed (count)" field is not empty > - Is that expected??? Yes of course, what do you expect? I contain the value you filled in before. > 11. Refill all the fields Which ones? For "all" they are already filled. > 12. Then you should see that for both policies, "Maximum total holds allowed > (count)" stays empty Nope, I see what I just entered. All good for me, please retest. (In reply to Victor Grousset/tuxayo from comment #7) I do not understand what you are trying to show. > 10. You should see in the all lib policy that the "Maximum total holds > allowed (count)" field is not empty > - Is that expected??? Yes of course, what do you expect? I contain the value you filled in before. > 11. Refill all the fields Which ones? For "all" they are already filled. > 12. Then you should see that for both policies, "Maximum total holds allowed > (count)" stays empty Nope, I see what I just entered. All good for me, please retest. I'm going to go for it here.. I can't replicate any issues once Jonathans patch is applied.. Thanks for your efforts in reworking it for 18.11.x series. Pushed to 18.11.x for 18.11.06 (In reply to Jonathan Druart from comment #12) > (In reply to Victor Grousset/tuxayo from comment #7) > > 10. You should see in the all lib policy that the "Maximum total holds > > allowed (count)" field is not empty > > - Is that expected??? > > Yes of course, what do you expect? I contain the value you filled in before. Okay so the patch manages to display the value that in the previous version was still saved but not displayed. I didn't understood this, so it seemed that the patch was changing the config. > > 11. Refill all the fields > > Which ones? For "all" they are already filled. "Default checkout, hold and return policy" It was to test if changing the values still worked. > > 12. Then you should see that for both policies, "Maximum total holds allowed > > (count)" stays empty > > Nope, I see what I just entered. > > All good for me, please retest. It might have an issue with my setup or misunderstood something functionally when trying to guess some details of the test plan. Thanks Joubu for the patch. Thanks Martin for retesting. |