Bug 14045 - Add specific quotas to on-site checkout
Summary: Add specific quotas to on-site checkout
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 14194 14195
Blocks: 16569 18357
  Show dependency treegraph
 
Reported: 2015-04-22 13:49 UTC by Nicolas Legrand
Modified: 2017-11-02 16:27 UTC (History)
7 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Large patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14045: Add specific quotas to on-site checkouts (21.78 KB, patch)
2015-05-13 16:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Change prototype of TooMany to raise a better warning (14.88 KB, patch)
2015-05-13 16:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Introduce the maxonsiteissueqty field as a copy of maxissueqty (19.08 KB, patch)
2015-05-13 16:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Make GetBranchBorrowerCircRule return maxonsiteissueqty (10.53 KB, patch)
2015-05-13 16:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: DB Changes (7.77 KB, patch)
2015-05-13 16:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: DBIC Changes (6.49 KB, patch)
2015-05-13 16:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Add specific quotas to on-site checkouts (21.84 KB, patch)
2015-05-27 10:35 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: Add specific quotas to on-site checkouts (21.84 KB, patch)
2015-05-27 10:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: Add specific quotas to on-site checkouts (21.84 KB, patch)
2015-05-27 10:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: Change prototype of TooMany to raise a better warning (14.94 KB, patch)
2015-05-27 10:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: Introduce the maxonsiteissueqty field as a copy of maxissueqty (19.14 KB, patch)
2015-05-27 10:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: Make GetBranchBorrowerCircRule return maxonsiteissueqty (10.60 KB, patch)
2015-05-27 10:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: DB Changes (7.84 KB, patch)
2015-05-27 10:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: DBIC Changes (6.56 KB, patch)
2015-05-27 10:37 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 14045: Add specific quotas to on-site checkouts (21.84 KB, patch)
2015-09-03 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Change prototype of TooMany to raise a better warning (14.94 KB, patch)
2015-09-03 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Introduce the maxonsiteissueqty field as a copy of maxissueqty (19.13 KB, patch)
2015-09-03 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Make GetBranchBorrowerCircRule return maxonsiteissueqty (10.35 KB, patch)
2015-09-03 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: DB Changes (7.83 KB, patch)
2015-09-03 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: DBIC Changes (6.55 KB, patch)
2015-09-03 13:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Fix typo in template (1.35 KB, patch)
2015-10-06 17:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 14045: Add specific quotas to on-site checkouts (21.91 KB, patch)
2015-10-12 05:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14045: Change prototype of TooMany to raise a better warning (15.01 KB, patch)
2015-10-12 05:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14045: Introduce the maxonsiteissueqty field as a copy of maxissueqty (19.20 KB, patch)
2015-10-12 05:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14045: Make GetBranchBorrowerCircRule return maxonsiteissueqty (10.42 KB, patch)
2015-10-12 05:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14045: DB Changes (7.90 KB, patch)
2015-10-12 05:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14045: DBIC Changes (6.62 KB, patch)
2015-10-12 05:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 14045: Fix typo in template (1.42 KB, patch)
2015-10-12 05:55 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14045: Add missing th in tfoot (1.13 KB, patch)
2015-10-15 10:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14045: Fix failing tests if issuing rules exist (942 bytes, patch)
2015-11-06 13:25 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicolas Legrand 2015-04-22 13:49:56 UTC
the BULAC (http://www.bulac.fr/) would like to have independent quotas for on-site checkouts with a column “Current on-site checkouts allow” added to the circulation matrix.

As long as Change sponsored? is set on “Seeking developer”, anyone wanting to quote us for the development of such a feature or just ask for more information is welcome to write to us: <koha@bulac.fr>.
Comment 1 Jonathan Druart 2015-05-13 16:03:56 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-05-13 16:04:01 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-05-13 16:04:05 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-05-13 16:04:09 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-05-13 16:04:13 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-05-13 16:04:17 UTC Comment hidden (obsolete)
Comment 7 Indranil Das Gupta 2015-05-13 16:22:31 UTC
Comment on attachment 39151 [details] [review]
Bug 14045: DB Changes

Review of attachment 39151 [details] [review]:
-----------------------------------------------------------------

Hi Jonathan,

I've a newbie question. Why are you putting in this code in the updatedatabase.pl, instead of in the atomicupdates? Or did I get it wrong?

http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39151&action=diff#a/installer/data/mysql/updatedatabase.pl_sec1
Comment 8 Jonathan Druart 2015-05-18 08:44:46 UTC
(In reply to Indranil Das Gupta from comment #7)
> I've a newbie question. Why are you putting in this code in the
> updatedatabase.pl, instead of in the atomicupdates? Or did I get it wrong?

We cannot add more than 1 query in 1 atomic update file. So I should have created ~7 files, which I considered as more work than rebase 1 patch.
Comment 9 Nicolas Legrand 2015-05-18 14:04:34 UTC
Thanks a lot for the patches.

Test II.4 doesn't pass, it says "Please confirm checkout, Too many checked out. 2 checked out, only 2 are allowed."
Comment 10 Jonathan Druart 2015-05-20 10:21:14 UTC
(In reply to Nicolas Legrand from comment #9)
> Thanks a lot for the patches.
> 
> Test II.4 doesn't pass, it says "Please confirm checkout, Too many checked
> out. 2 checked out, only 2 are allowed."

At this point, the patron should have only 1 on-site checkout in his/her checkout list.
The 3 steps (I, II and III) are independent.
I have just retested and it works as expected.
Comment 11 Nicolas Legrand 2015-05-20 11:06:56 UTC
Mhmm, tested, again, indeed, it works :), sorry.
Comment 12 Nicolas Legrand 2015-05-27 10:35:38 UTC Comment hidden (obsolete)
Comment 13 Nicolas Legrand 2015-05-27 10:37:00 UTC Comment hidden (obsolete)
Comment 14 Nicolas Legrand 2015-05-27 10:37:31 UTC Comment hidden (obsolete)
Comment 15 Nicolas Legrand 2015-05-27 10:37:34 UTC Comment hidden (obsolete)
Comment 16 Nicolas Legrand 2015-05-27 10:37:38 UTC Comment hidden (obsolete)
Comment 17 Nicolas Legrand 2015-05-27 10:37:42 UTC Comment hidden (obsolete)
Comment 18 Nicolas Legrand 2015-05-27 10:37:46 UTC Comment hidden (obsolete)
Comment 19 Nicolas Legrand 2015-05-27 10:37:50 UTC Comment hidden (obsolete)
Comment 20 Nicolas Legrand 2015-05-27 10:45:36 UTC
Works perfectly.

We just add a problem with the new syspref:  ConsiderOnSiteCheckoutsAsNormalCheckouts. The explanation puzzled a tester: "If enabled, the number of checkouts allowed will be normal checkouts + on-site checkouts. If disabled, both values will be checked separately."

As the syspref is not enabled but set on “consider” or “don't consider”, I propose a wording less ambiguous: "If set on “consider”, the number of checkouts allowed will be normal checkouts + on-site checkouts. Otherwise, both values will be checked separately." I didn't patch it because maybe a native english speaker will find a better wording or find the first one was best :).
Comment 21 Marcel de Rooy 2015-09-03 13:11:21 UTC
 Merge conflict in t/db_dependent/Circulation_Branch.t
Comment 22 Jonathan Druart 2015-09-03 13:24:24 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-09-03 13:24:35 UTC Comment hidden (obsolete)
Comment 24 Jonathan Druart 2015-09-03 13:24:39 UTC Comment hidden (obsolete)
Comment 25 Jonathan Druart 2015-09-03 13:24:42 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-09-03 13:24:49 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2015-09-03 13:24:52 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2015-10-01 09:29:22 UTC
Everything is signed off here.
Comment 29 Katrin Fischer 2015-10-04 20:45:18 UTC
Working on this now.
Comment 30 Katrin Fischer 2015-10-04 21:21:51 UTC
Started with testing the circulation rules GUI and found a problem:

Default checkout, hold and return policy  (second part of the page)- the onsite-checkouts number doesn't show in the field after saving, but it looks like it IS actually saved to the table. Can you check?
Comment 31 Jonathan Druart 2015-10-06 17:22:55 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2015-10-06 17:24:08 UTC
(In reply to Katrin Fischer from comment #30)
> Started with testing the circulation rules GUI and found a problem:
> 
> Default checkout, hold and return policy  (second part of the page)- the
> onsite-checkouts number doesn't show in the field after saving, but it looks
> like it IS actually saved to the table. Can you check?

Came from a typo in the template:
  default_onsitemaxissueqty vs default_maxonsiteissueqty
Should be fixed now.
Comment 33 Katrin Fischer 2015-10-12 05:52:01 UTC
Hm, I think I might have uncovered a small bug in master: If you set the circulation rule max to 2, but have a default total of checkouts allowed of 1 - the circulation rule seems to be looked at first, and you can check out 2 books total instead of only 1. But this appears to happen with and without the patches... so just a testing note for now.
Comment 34 Katrin Fischer 2015-10-12 05:54:58 UTC
Created attachment 43326 [details] [review]
[PASSED QA] Bug 14045: Add specific quotas to on-site checkouts

This patch set adds the ability to defined independent quotas for on-site
checkouts.
This will be done using the circulation rules matrix where a new column
“Current on-site checkouts allow” will be added.

This feature is going to use the same method as the existing fields maxissueqty
("Current checkouts allowed"), the new fields will be added to the
different tables (see the "DB changes" patch) and will be named
maxonsiteissueqty (for consistency).

In order to keep the existing behavior and to let more flexibility,
a new system preference is added (ConsiderOnSiteCheckoutsAsNormalCheckouts).
This syspref will let the liberty to the library to decide if an on-site
checkout should be considered as a "normal" checkout or not.

To keep the existing behavior, the syspref will be disabled (i.e. an on-site
checkout is considered as a normal checkout) and the number of on-site
checkouts will be the same as the number of checkout (maxissueqty ==
maxonsiteissueqty).

Technically:
There are only very few tests for the Circulation module, and the 2
subroutines impacted by this patch set were not tested at all.
It is necessary to introduce non-regression tests for this area.
The 2 subroutines are: C4::Circulation::GetBranchBorrowerCircRule
and C4::Circulation::TooMany (only called by
C4::Circulation::CanBookBeIssued, so we will take the liberty to change
the prototype to raise a better warning to the end user).

Test plan:
I. Confirm there is no regression and the existing behavior is kept
 0/ Let the syspref disabled
 1/ Set a rule to limit to 2 the number of checkouts allowed
 2/ Do a normal checkout
 3/ Do an on-site checkout
 4/ Try to checkout (on-site or normal) an item again.
 You should not be allowed.

II. Test the new feature - pref disabled
 0/ Let the syspref disabled
 1/ Set a rule to limit to 2 the number of checkouts allowed and to 1
 the number of on-site checkouts allowed.
 2/ Do an on-site checkout
 3/ Try to do another one, you should not be allowed to do it.
 4/ A normal checkout should pass successfully
Note that it does not make sense to have the number of on-site checkouts
alowed > number of checkouts allowed.

III. Test the new feature - pref enabled
 0/ Enable the syspref
Now an on-site checkout is *not* counted as a normal checkout.
This means you can have the number of on-site checkouts > number of
checkouts allowed.
 1/ Set the values you want for the 2 types of checkouts (normal vs
on-site).
 2/ Even if a patron has reached the maximum of checkouts allowed, he
 will be allowed to do a on-site checkout (vice versa).

IV. Stress the developper
 Using the different configurations available in the circulation matrix,
 try to find one where the checkout is allowed and not should be.

Sponsored-by: BULAC - http://www.bulac.fr/
Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 35 Katrin Fischer 2015-10-12 05:55:02 UTC
Created attachment 43327 [details] [review]
[PASSED QA] Bug 14045: Change prototype of TooMany to raise a better warning

With this patch, the user will know why the checkout is refused.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 36 Katrin Fischer 2015-10-12 05:55:06 UTC
Created attachment 43328 [details] [review]
[PASSED QA] Bug 14045: Introduce the maxonsiteissueqty field as a copy of maxissueqty

This patch adds the new fields maxonsiteissueqty as a copy of the
existing maxissueqty field.

There is no brainy code here, it only adds the ability to fill the new
field from the admin interface (admin/smart-rules.pl).

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 37 Katrin Fischer 2015-10-12 05:55:10 UTC
Created attachment 43329 [details] [review]
[PASSED QA] Bug 14045: Make GetBranchBorrowerCircRule return maxonsiteissueqty

GetBranchBorrowerCircRule should return the value for maxissueqty and
maxonsiteissueqty. It's what this patch does.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 38 Katrin Fischer 2015-10-12 05:55:13 UTC
Created attachment 43330 [details] [review]
[PASSED QA] Bug 14045: DB Changes

This patch is the DB changes for the feature.
It adds 5 new columns named 'maxonsiteissueqty' to the following tables:
- branch_borrower_circ_rules
- default_borrower_circ_rules
- default_branch_circ_rules
- default_circ_rules
- issuingrules

It also adds the pref ConsiderOnSiteCheckoutsAsNormalCheckouts.

See main patch for more details.

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 39 Katrin Fischer 2015-10-12 05:55:16 UTC
Created attachment 43331 [details] [review]
[PASSED QA] Bug 14045: DBIC Changes

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 40 Katrin Fischer 2015-10-12 05:55:20 UTC
Created attachment 43332 [details] [review]
[PASSED QA] Bug 14045: Fix typo in template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 41 Tomás Cohen Arazi 2015-10-13 15:15:43 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 42 Nicolas Legrand 2015-10-13 15:33:10 UTC
\o/ thanks !
Comment 43 Jonathan Druart 2015-10-15 10:28:35 UTC
Created attachment 43423 [details] [review]
Bug 14045: Add missing th in tfoot

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2015-10-15 10:29:37 UTC
Tomas, there is a small bug caused by a conflict with bug 14290 (add a tfoot to the table).
The last patch is really trivial and could be pushed bypassing all so/qa steps I think.
Comment 45 Jonathan Druart 2015-11-06 13:25:48 UTC
Created attachment 44557 [details] [review]
Bug 14045: Fix failing tests if issuing rules exist

If there is issuing rules defined, one test does not pass as it assumes
there is none defined.
Comment 46 Tomás Cohen Arazi 2015-11-06 15:11:49 UTC
Followup pushed to master.

Thanks Jonathan!