Description
Kyle M Hall (khall)
2017-07-13 11:17:44 UTC
Created attachment 65247 [details] [review] Bug 18936 - Convert issuingrules fields to circulation_rules Created attachment 65248 [details] [review] Bug 18936: (followup) Fix upgrade step Created attachment 65249 [details] [review] Bug 18936: Fix api/v1/holds.t Created attachment 67629 [details] [review] Bug 18936: (follow-up) Add foreign key and scope enhancement to circ rules This necessitates moving the circ rules from using '*' to using undef/NULL. Created attachment 67758 [details] [review] Bug 18936: (follow-up) Update REST definition for /.../kinds Please don't forget to assign your bugs to yourself! (Is it Kyle or Jesse here?) Created attachment 68236 [details] [review] Bug 18936: (follow-up) fix rental odiscount lookup No test plan? Created attachment 71034 [details] [review] Bug 18936 - Convert issuingrules fields to circulation_rules Created attachment 71035 [details] [review] Bug 18936: (followup) Fix upgrade step Created attachment 71036 [details] [review] Bug 18936: Fix api/v1/holds.t Created attachment 71037 [details] [review] Bug 18936: (follow-up) Add foreign key and scope enhancement to circ rules This necessitates moving the circ rules from using '*' to using undef/NULL. Created attachment 71038 [details] [review] Bug 18936: (follow-up) Update REST definition for /.../kinds Created attachment 71039 [details] [review] Bug 18936: (follow-up) fix rental odiscount lookup Created attachment 71040 [details] [review] Bug 18936: (follow-up) Remove issuingrules from kohastructure.sql Created attachment 71041 [details] [review] Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules Created attachment 72230 [details] [review] Bug 18936 - Convert issuingrules fields to circulation_rules Created attachment 72231 [details] [review] Bug 18936: (followup) Fix upgrade step Created attachment 72232 [details] [review] Bug 18936: Fix api/v1/holds.t Created attachment 72233 [details] [review] Bug 18936: (follow-up) Add foreign key and scope enhancement to circ rules This necessitates moving the circ rules from using '*' to using undef/NULL. Created attachment 72234 [details] [review] Bug 18936: (follow-up) Update REST definition for /.../kinds Created attachment 72235 [details] [review] Bug 18936: (follow-up) fix rental odiscount lookup Created attachment 72236 [details] [review] Bug 18936: (follow-up) Remove issuingrules from kohastructure.sql Created attachment 72237 [details] [review] Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules Created attachment 72238 [details] [review] Bug 18936: (follow-up) Fix tests, replace old get_onshelfholds_policy method Hi Jesse, how could this one be tested ? Kyle, assigning this to you, ok? I have started a rebase but it's getting very hard here. I will wait a bit for other patches to get attention. Just for reference: https://gitlab.com/joubu/Koha/commits/bug_18936 It is not supposed to be tested, I am not expecting it to work correctly. Remove branch rebased and updated. TODO: "notes for rule" is certainly broken (bug 12365) Kyle, I have now 1 line per category and item type on the circ rule admin page, is that expected? (In reply to Jonathan Druart from comment #30) > Remove branch rebased and updated. > > TODO: "notes for rule" is certainly broken (bug 12365) > > Kyle, I have now 1 line per category and item type on the circ rule admin > page, is that expected? If I understand what you are saying, the answer is yes. From the user perspective, the circ rules should look pretty much the same as they always have. No it did not, that's why I asked :) I pushed another commit that fixes the problem I got. Thanks for the answer! Is this ready for testing? When I run updatedatabase I get some errors: DEV atomic update: bug_18925.perl DBD::mysql::db selectrow_array failed: Table 'koha_kohadev.branch_borrower_circ_rules' doesn't exist [for Statement " SHOW COLUMNS FROM branch_borrower_circ_rules WHERE Field = ? "] at /home/vagrant/kohaclone/C4/Installer.pm line 531. ...etc. (In reply to Owen Leonard from comment #33) > Is this ready for testing? When I run updatedatabase I get some errors: > > DEV atomic update: bug_18925.perl > DBD::mysql::db selectrow_array failed: Table > 'koha_kohadev.branch_borrower_circ_rules' doesn't exist [for Statement " > SHOW COLUMNS FROM branch_borrower_circ_rules > WHERE Field = ? > "] at /home/vagrant/kohaclone/C4/Installer.pm line 531. > > ...etc. Hi Owen, I have rebased the whole dependency tree and updated the remote branch. The upgrade process should pass if the DB structure comes from master. However I have seen those errors if you run it twice. It is a bug in column_exists that does not test if the table still exist. I am not confident with the patches from this bug report (lot of rebases), maybe it would be better to validate that the ones from the deps are ok. Remote branch rebased. *** Bug 8858 has been marked as a duplicate of this bug. *** Should this bug be in "assigned" instead of "Needs Signoff"? updatedabase fails: logs: https://framabin.org/p/?ee2bc83c735fb710#wvTUZlLVsrUIoShkp6kyGxpjY9Ov3osvFLltCqpwFNU= dump: latest MARC21 http://git.koha-community.org/gitweb/?p=contrib/global.git;a=history;f=sandbox/sql/sandbox1.sql.gz;h=19268bccb43b2a33d5644b7d86cbb1abb323016b;hb=HEAD code:7ca6504be2d9ce96ef929c90b3b83a84a935244e https://gitlab.com/joubu/Koha/commits/bug_18936 I think these errors will put this on a failed QA state (Sorry Jonathan...) Liz (In reply to Jonathan Druart from comment #29) > Just for reference: https://gitlab.com/joubu/Koha/commits/bug_18936 > > It is not supposed to be tested, I am not expecting it to work correctly. Remote branch rebased. Previous comment still applies, please focus on dependencies first. Created attachment 91953 [details] [review] Bug 18936: Convert issuingrules fields to circulation_rules Created attachment 91954 [details] [review] Bug 18936: (followup) Fix upgrade step Created attachment 91955 [details] [review] Bug 18936: Fix api/v1/holds.t Created attachment 91956 [details] [review] Bug 18936: (follow-up) Add foreign key and scope enhancement to circ rules This necessitates moving the circ rules from using '*' to using undef/NULL. Created attachment 91957 [details] [review] Bug 18936: (follow-up) Update REST definition for /.../kinds Created attachment 91958 [details] [review] Bug 18936: (follow-up) fix rental odiscount lookup Created attachment 91959 [details] [review] Bug 18936: (follow-up) Remove issuingrules from kohastructure.sql Created attachment 91960 [details] [review] Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules Created attachment 91961 [details] [review] Bug 18936: (follow-up) Fix tests, replace old get_onshelfholds_policy method Created attachment 91962 [details] [review] Bug 18936: move get_opacitemholds_policy method Created attachment 91963 [details] [review] Bug 18936: Move guess_article_requestable_itemtypes method Created attachment 91964 [details] [review] Bug 18936: Remove last bit of Koha::IssuingRule Created attachment 91965 [details] [review] Bug 18936: Fix syntax issue TODO: Replace croak with exceptions Created attachment 91966 [details] [review] Bug 18936: Fix several issues Created attachment 91967 [details] [review] Bug 18936: chargename removed by bug 21753 Created attachment 91968 [details] [review] Bug 18936: new suspension_chargeperiod Created attachment 91969 [details] [review] Bug 18936: new holds_per_day Created attachment 91970 [details] [review] Bug 18936: Replace an other insert into issuingrules Created attachment 91971 [details] [review] Bug 18936: Fix Circulation.t Created attachment 91972 [details] [review] Bug 18936: More fixes Created attachment 91973 [details] [review] Bug 18936: Add missing filters Created attachment 91974 [details] [review] Bug 18936: Fix Update DB entry Created attachment 91975 [details] [review] Bug 18936: Fix Update DB entry This is now ready for testing! The remote branch has been rebased and adjusted. Patches attached to this bug report as well. Test plan: - checkout master and create several circulation rules - apply these patches and run the updatedatabase.pl script - confirm that the screen displays the correct value - add/edit/delete rules and confirm that there is no regression - focus on empty strings vs 0 vs undef values - pick some rules you know well and confirm the restrictions applies accordingly to the values you have defined (renewalsallowed for instance) Created attachment 94179 [details] [review] Bug 18936: Convert issuingrules fields to circulation_rules Created attachment 94180 [details] [review] Bug 18936: (followup) Fix upgrade step Created attachment 94181 [details] [review] Bug 18936: Fix api/v1/holds.t Created attachment 94182 [details] [review] Bug 18936: (follow-up) Add foreign key and scope enhancement to circ rules This necessitates moving the circ rules from using '*' to using undef/NULL. Created attachment 94183 [details] [review] Bug 18936: (follow-up) Update REST definition for /.../kinds Created attachment 94184 [details] [review] Bug 18936: (follow-up) fix rental odiscount lookup Created attachment 94185 [details] [review] Bug 18936: (follow-up) Remove issuingrules from kohastructure.sql Created attachment 94186 [details] [review] Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules Created attachment 94187 [details] [review] Bug 18936: (follow-up) Fix tests, replace old get_onshelfholds_policy method Created attachment 94188 [details] [review] Bug 18936: move get_opacitemholds_policy method Created attachment 94189 [details] [review] Bug 18936: Move guess_article_requestable_itemtypes method Created attachment 94190 [details] [review] Bug 18936: Remove last bit of Koha::IssuingRule Created attachment 94191 [details] [review] Bug 18936: Fix syntax issue TODO: Replace croak with exceptions Created attachment 94192 [details] [review] Bug 18936: Fix several issues Created attachment 94193 [details] [review] Bug 18936: chargename removed by bug 21753 Created attachment 94194 [details] [review] Bug 18936: new suspension_chargeperiod Created attachment 94195 [details] [review] Bug 18936: new holds_per_day Created attachment 94196 [details] [review] Bug 18936: Replace an other insert into issuingrules Created attachment 94197 [details] [review] Bug 18936: Fix Circulation.t Created attachment 94198 [details] [review] Bug 18936: More fixes Created attachment 94199 [details] [review] Bug 18936: Add missing filters Created attachment 94200 [details] [review] Bug 18936: Fix Update DB entry sorry, tried to apply, but got: Applying: Bug 18936: More fixes error: sha1 information is lacking or useless (C4/Reserves.pm). I think this bug probably warrants the 'Just a git branch' approach and should have the patches removed from here.. those capable of testing this should be capable of adding a remote and checking out the branch that Jonathan is maintaining :) I tested using git branch and found two issues: - holds per day are not transferred in database update - in note column, I can see link "view notes" even if there is no note - if the rule is specific for library and patron category, or for library and itemtype, the rules for current checkouts allowed and current on-site checkouts allowed are not transferred - No automatic renewal after (hard limit) does not respect dateformat and is shown in iso format - Overdue fines cap (amount) is not possible to edit in GUI (In reply to Josef Moravec from comment #88) Hi Josef, thanks for testing this! > I tested using git branch and found two issues: > > - holds per day are not transferred in database update Fixed! > - in note column, I can see link "view notes" even if there is no note Fixed! > - if the rule is specific for library and patron category, or for library > and itemtype, the rules for current checkouts allowed and current on-site > checkouts allowed are not transferred I do not recreate that. checkout master create specific rule for CPL and a given patron's category checkout bug_18936 update => I do see the value for current checkouts allowed > - No automatic renewal after (hard limit) does not respect dateformat and is > shown in iso format Fixed! > - Overdue fines cap (amount) is not possible to edit in GUI I do not recreate: Edit the line Modify the value for 'Overdue fines cap (amount)' Save => The value is correctly displayed in the table This all scans well, if you get a chance it would be great to have a final SO/QA by January as it's one I'm targetting for an early push. Hi Jonathan, failing the patches "Bug 18936: More fixes", "Bug 18936: Fix Circulation.t", "Bug 18936: Fix several issues" and "Bug 18936: Fix Update DB entry" since they miss description of the changes so there is no way to know what is the intended change here therefore testing will be super difficult. Also with "Bug 18936: Fix several issues" it would be more logical to have then several commits for those. So I will take a look again once the descriptions of changes are added, ok? (In reply to Joonas Kylmälä from comment #91) > Hi Jonathan, > > failing the patches "Bug 18936: More fixes", "Bug 18936: Fix Circulation.t", > "Bug 18936: Fix several issues" and "Bug 18936: Fix Update DB entry" since > they miss description of the changes so there is no way to know what is the > intended change here therefore testing will be super difficult. Also with > "Bug 18936: Fix several issues" it would be more logical to have then > several commits for those. > > So I will take a look again once the descriptions of changes are added, ok? Hi Joonas, Those patches are follow-up sI submitted when I retested the feature after a feature. I can understand they do not make sense in the git log, but they do for me (as I am rebasing this for 2 years now). I would suggest to squash the patches when reviewing them, and you will see that the changes make sense. Fix Circulation.t: Make the tests Circulation.t pass (maxissueqty was missing when the rule was created) Fix several issues: branchcode, categorycode and itemtype are no longer mandatory to create a new rule Fix Update DB entry: Some rules were missing (or added when they should not). Also branchcode, categorycode and itemtype must not be '*' but NULL, otherwise the FK constraint will not be satisfied. We are at Signoff step, that should not block testers to test this feature. Patches rebased. There is one test that is failing I do not manage to fix (t/db_dependent/Koha/Charges/Fees.t). Testers should consider using the branch bug_18936 from my koha-misc4dev repo to fix reset_all: https://gitlab.com/joubu/koha-misc4dev/commits/bug_18936 Will be cleaned and merged into master when these patches will be pushed. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Tested following: - Added several rules for different combinations of patron category and item type - Modified rules - Removed rules - Added new rules - Tested functionalities against rules: Hard due date, Loan period, Loans allowed (total), Renewals allowed (count), No renewal before, Holds allowed (total) Problems I noticed after doing database upgrade and comparing the web page before db upgrade and after db upgrade: - The "Clone these rules to: " feature is missing after the patch set - "Holds allowed (daily)" was previously empty now with the patch set it says "Unlimited" - "Fine amount" is now with 6 decimals instead of previous 2 (In reply to Joonas Kylmälä from comment #96) > - "Holds allowed (daily)" was previously empty now with the patch set it > says "Unlimited" This is fine, the empty value previously also meant "Unlimited", here it is just made explicit. Even though it should be allowed, I get this error message when making a hold for Student patron for Music item: "Daily hold limit reached for patron". I have rules: category = Student, itemtype = Music, Holds allowed (daily) = Unlimited and category = All, itemtype = ALL, Holds allowed (daily) = 0 If for the latter "All rule" I change it to 1 then things work. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Tested following: - Added several rules for different combinations of patron category and item type - Modified rules - Removed rules - Added new rules - Tested functionalities against rules: Hard due date, Loan period, Loans allowed (total), Renewals allowed (count), No renewal before, Holds allowed (total) - Tested especially empty values on rules (In reply to Minna Kivinen from comment #99) > Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> > Tested following: > - Added several rules for different combinations of patron category and item > type > - Modified rules > - Removed rules > - Added new rules > - Tested functionalities against rules: Hard due date, Loan period, Loans > allowed (total), Renewals allowed (count), No renewal before, Holds allowed > (total) > - Tested especially empty values on rules Thank you! I have added your signed-off-by lines to the commit messages. The qa-koha.pl tool detected an issue with constant being redefined:
> FAIL Koha/CirculationRules.pm
> FAIL valid
> Constant subroutine >Koha::CirculationRules::GUESSED_ITEMTYPES_KEY redefined
This appears to be from circular dependency:
Koha/CirculationRules.pm -> Koha/CirculationRule.pm -> Koha/ItemTypes.pm -> Koha/ItemType.pm -> Koha/CirculationRules.pm
if someone has the time to remove the circularity here would be great!
Minor: Koha::CirculationRule ->item_type, ->library and ->patron_category must use the DBIC RS (In reply to Joonas Kylmälä from comment #101) > The qa-koha.pl tool detected an issue with constant being redefined: > > > FAIL Koha/CirculationRules.pm > > FAIL valid > > Constant subroutine >Koha::CirculationRules::GUESSED_ITEMTYPES_KEY redefined > > This appears to be from circular dependency: > > Koha/CirculationRules.pm -> Koha/CirculationRule.pm -> Koha/ItemTypes.pm -> > Koha/ItemType.pm -> Koha/CirculationRules.pm > > if someone has the time to remove the circularity here would be great! I would not care much about that. Note that the QA script is ignoring the "subroutine ... redefined" warning. Here it displays "Constant subroutine ... redefined" and that is why it's not ignored. The circular dependency already existed: % git checkout master % perl -wc Koha/CirculationRules.pm Subroutine get_effective_rule redefined at Koha/CirculationRules.pm line 42. Subroutine set_rule redefined at Koha/CirculationRules.pm line 82. Subroutine set_rules redefined at Koha/CirculationRules.pm line 140. Subroutine delete redefined at Koha/CirculationRules.pm line 171. Subroutine _type redefined at Koha/CirculationRules.pm line 184. Subroutine object_class redefined at Koha/CirculationRules.pm line 192. Koha/CirculationRules.pm syntax OK I've given this a look over myself now and am pretty happy.. as soon as we have your stamp of approval Joonas we'll go for it! :) Well done guys! I would still like to wait Joubu to update https://gitlab.com/joubu/koha-misc4dev/commits/bug_18936 so that using that branch all the tests pass again. The defaults currently differ from the original values and therefore the tests don't pass. (In reply to Joonas Kylmälä from comment #105) > I would still like to wait Joubu to update > https://gitlab.com/joubu/koha-misc4dev/commits/bug_18936 so that using that > branch all the tests pass again. The defaults currently differ from the > original values and therefore the tests don't pass. I can actually just do upgradedatabase from master and run the tests so this is not necessary. Sorry for the noise. I followed the test plan, and as requested there to test some specific rules I know, I choose to test hold related rules and renewal rules. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Big thanks for all involved on this 2+ years old bug! Created attachment 98299 [details] [review] Bug 18936: Convert issuingrules fields to circulation_rules Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98300 [details] [review] Bug 18936: (followup) Fix upgrade step Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98301 [details] [review] Bug 18936: Fix api/v1/holds.t Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98302 [details] [review] Bug 18936: (follow-up) Add foreign key and scope enhancement to circ rules This necessitates moving the circ rules from using '*' to using undef/NULL. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98303 [details] [review] Bug 18936: (follow-up) Update REST definition for /.../kinds Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98304 [details] [review] Bug 18936: (follow-up) fix rental odiscount lookup Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98305 [details] [review] Bug 18936: (follow-up) Remove issuingrules from kohastructure.sql Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98306 [details] [review] Bug 18936: (follow-up) fix tests, null vs. empty behavior for limiting rules Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98307 [details] [review] Bug 18936: (follow-up) Fix tests, replace old get_onshelfholds_policy method Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98308 [details] [review] Bug 18936: move get_opacitemholds_policy method Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98309 [details] [review] Bug 18936: Move guess_article_requestable_itemtypes method Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98310 [details] [review] Bug 18936: Remove last bit of Koha::IssuingRule Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98311 [details] [review] Bug 18936: Fix syntax issue TODO: Replace croak with exceptions Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98312 [details] [review] Bug 18936: Fix several issues Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98313 [details] [review] Bug 18936: chargename removed by bug 21753 Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98314 [details] [review] Bug 18936: new suspension_chargeperiod Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98315 [details] [review] Bug 18936: new holds_per_day Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98316 [details] [review] Bug 18936: Replace an other insert into issuingrules Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98317 [details] [review] Bug 18936: Fix Circulation.t Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98318 [details] [review] Bug 18936: More fixes Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98319 [details] [review] Bug 18936: Add missing filters Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98320 [details] [review] Bug 18936: Fix Update DB entry Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98321 [details] [review] Bug 18936: fixes from comment #88 - holds per day are not transferred in database update (actually not displayed) - in note column, I can see link "view notes" even if there is no note - No automatic renewal after (hard limit) does not respect dateformat and is shown in iso format Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98322 [details] [review] Bug 18936: Fix warning about page about wrong lengthunit Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98323 [details] [review] Bug 18936: (follow-up) Don't retrieve unused issuing rule The chargename rule is not used anywhere in the code Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98324 [details] [review] Bug 18936: Fix some more tests * CanItemBeReserved Prior to "Bug 18936: Convert issuingrules fields to circulation_rules", GetHoldRule returned holds_per_record even if no reservesallowed was defined. This change restores this behavior. FIXME Note: In GetHoldRule we return itemtype only if reservesallowed is set, not sure it is correct. * t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t When setting returnbranch, holdallowed and hold_fulfillment_policy, we should not provide categorycode. * t/db_dependent/Holds.t Prefer to keep the existing rules instead of removing them. It got quite hard to understand what was going on here because of the mixup with the rule reservesallowed that was in issuingrules, and the other rules we used for the tests. Also, categorycode should not be passed to set those 3 rules (holdallowed, hold_fulfillment_policy and returnbranch) * t/db_dependent/Circulation.t Setting lengthunit to 'hours', no need to make sure the rule has been correctly be saved * t/db_dependent/Circulation/CalcDateDue.t It uses hardcoded data that is not in the sample data (categorycode=C). Let use K that exists and postpone a refactore of the whole script (to make it create the data it needs). * t/db_dependent/Circulation/ReturnClaims.t * t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t Simple replace Koha::IssuingRule with Koha::CirculationRules * t/db_dependent/Koha/Charges/Fees.t => FIXME Still failing, stuck here, need help Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98325 [details] [review] Bug 18936: (follow-up) Add missing circulation rule to fix tests The tests passed before by a chance because the default kohadevbox installation script added a issuing rule that had lengthunit = days. If you use a Koha version before the commits introduced by Bug 18936 you can reproduce the Fees.t tests failing by deleting all the issuing rules from database with > delete issuingrules; and then running $ prove t/db_dependent/Koha/Charges/Fees.t Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98326 [details] [review] Bug 18936: Make sure we default to '*' if no rule exist Making sure the code is doing what we expect. Also this patch fix CirculationRules.t Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98327 [details] [review] Bug 18936: Insert default rules during the onboarding tool Those default values have been picked from bug 15522 (Koha::CirculationRules) Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98328 [details] [review] Bug 18936: Replace '*' with undef No test was provided initially so considering out of the scope for this one as well, but that's lacking terribly! Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98329 [details] [review] Bug 18936: Fix the delete rule buttons The itemtype and categorycode were always set to '*' Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98330 [details] [review] Bug 18936: (follow-up 2) Add foreign key and scope enhancement to circ rules The previous patch modifies: - categorycode => undef, - itemtype => undef, - branchcode => $branch, + branchcode => undef, But in that condition $branchcode is not '*' and so we should not set it to undef Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98331 [details] [review] Bug 18936: (follow-up) Fix typo in documentation Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98332 [details] [review] Bug 18936: (follow-up) Add missing import of Koha::CirculationRules Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98333 [details] [review] Bug 18936: Fix inconsistencies in IssuingRules.t * get_effective_rule should not pass rule_value * indentation fix * add comment to list the rules we have when the test _is_row_match is executed * compare ->rule_name and ->rule_value returns Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98334 [details] [review] Bug 18936: (follow-up2) Fix tests, replace old get_onshelfholds_policy method Previous change to this file was different from the one that existed prior to this patchset. Restoring value for onshelfholds, note that it did not change anything in the test result. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98335 [details] [review] Bug 18936: (follow-up) Add cloning of circulation rules back to Koha The "Clone these rules" feature in admin/smart-rules.pl was dropped. This re-implements the cloning using Koha objects. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98336 [details] [review] Bug 18936: (follow-up) Make possible to have unlimited value for holds_per_day Without this it is not possible to make for a specific patron category or itemtype a more specific unlimited holds_per_day rule if you have a catch all rule (categorycode = *, itemtype = *) with limited holds_per_day value. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98337 [details] [review] Bug 18936: (follow-up2) Make possible to have unlimited value for holds_per_day Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98338 [details] [review] Bug 18936: Fix IssuingRules/guess_article_requestable_itemtypes.t Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98339 [details] [review] Bug 18936: (follow-up) Adjust circ rule related tests so that they pass Bunch of tests broke after moving from issuingrules to circulation_rules. This accomodates the tests for this refactoring and DB change. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98340 [details] [review] Bug 18936: (follow-up2) Adjust circ rule related tests so that they pass The itemtype did not exist in the DB. We create it at the beginning of the tests. Ideally we would like to create it filling the form. Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98341 [details] [review] Bug 18936: (follow-up) Fix checkouts endpoint and its tests - Fixes a bug caused by the refactoring in Koha/REST/V1/Checkouts.pm (run the tests without this patch and you will see the tests fails because 'rule_name' parameter was forgotten). - Changes the tests to use the new Koha::CirculationRule objects instead of directly using issuingrules DB table Signed-off-by: Minna Kivinen <minna.kivinen@hamk.fi> Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98342 [details] [review] Bug 18936: (QA follow-up) Add missing POD Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98343 [details] [review] Bug 18936: (follow-up) More fixes Previous "More fixes" patch added a debugging statement, not needed. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98344 [details] [review] Bug 18936: (QA follow-up) Convert remaining tests to use Koha::CirculationRules These two uses of Koha::IssuingRule were forgotten so this converts them to use the new Koha::CirculationRules object. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98345 [details] [review] Bug 18936: Remove 2 new occurrences of Koha::IssuingRules Fix conflict with bug 13121 Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98346 [details] [review] Bug 18936: (QA follow-up) Fix failing test Using categorycode as a scope is not possible for any of these rules so lets remove it. Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Created attachment 98347 [details] [review] Bug 18936: Fix failing test in api/v1/holds.t Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Nice work everyone! Pushed to master for 20.05 Guys, this has been a real stirling effort! Thanks to all involved in pushing it over the finish line, amazing work! Created attachment 98407 [details] [review] Bug 18936: Display 'Unlimited' for Holds allowed (total) holds_per_day and reservesallowed are set to unlimited in the rules inserted by misc4dev Created attachment 98408 [details] [review] Bug 18936: Fix selenium/administration_tasks.t Nice work everyone! Pushed to master for 20.05 Not backporting to 19.11.x |