Please don't forget to assign your bugs to yourself! (Is it Kyle or Jesse here?)

No test plan?

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