Bug 10859 - Alert if a patron already has an issue for the same biblio
Summary: Alert if a patron already has an issue for the same biblio
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 10863
Blocks: 11201
  Show dependency treegraph
 
Reported: 2013-09-10 11:56 UTC by Jonathan Druart
Modified: 2015-06-04 23:23 UTC (History)
6 users (show)

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


Attachments
Bug 10859: Alert if a borrower already has an issue for the same biblio (11.38 KB, patch)
2013-09-10 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10859: Alert if a borrower already has an issue for the same biblio (11.36 KB, patch)
2014-02-18 09:07 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed-off] Bug 10859: Alert if a borrower already has an issue for the same biblio (11.54 KB, patch)
2014-02-18 10:29 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 10859: wording (1.79 KB, patch)
2014-03-13 12:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 10859: GetIssues.t - Execute unit tests in a transaction (3.08 KB, patch)
2014-03-13 12:36 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10859: Alert if a borrower already has an issue for the same biblio (11.93 KB, patch)
2014-03-13 13:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10859: wording (1.86 KB, patch)
2014-03-13 13:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10859: GetIssues.t - Execute unit tests in a transaction (3.15 KB, patch)
2014-03-13 13:06 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10859: QA follow up: Fixing order in sysprefs.sql (1.67 KB, patch)
2014-03-13 13:06 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2013-09-10 11:56:40 UTC
This feature will a an alert if a patron already has an issue for the same biblio. The librarian can force the checkout anyway. It doesn't alert the librarian if the biblio is a subscription.
Comment 1 Jonathan Druart 2013-09-10 13:13:22 UTC Comment hidden (obsolete)
Comment 2 I'm just a bot 2013-09-18 11:10:12 UTC
Applying: Bug 10859: Alert if a borrower already has an issue for the same biblio
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10859: Alert if a borrower already has an issue for the same biblio
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 3 Jonathan Druart 2013-09-18 12:48:24 UTC
(In reply to I'm just a bot from comment #2)
Depend on bug 10863
Comment 4 I'm just a bot 2013-09-29 06:10:03 UTC
This bug depends on 10863 which is in status Failed QA
Comment 5 I'm just a bot 2013-10-27 06:28:16 UTC
Applying: Bug 10859: Alert if a borrower already has an issue for the same biblio
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 10859: Alert if a borrower already has an issue for the same biblio
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 6 Jonathan Druart 2013-11-05 11:22:42 UTC
I rebased bug 10863, this one applies cleanly now.
Comment 7 Marc Véron 2014-02-17 17:16:57 UTC
Would like to sign off, but does not apply.
Comment 8 Jonathan Druart 2014-02-18 09:07:32 UTC Comment hidden (obsolete)
Comment 9 Marc Véron 2014-02-18 10:29:08 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2014-03-11 14:16:22 UTC
Comment on attachment 25366 [details] [review]
[Signed-off] Bug 10859: Alert if a borrower already has an issue for the same biblio

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

Hi Julian,

some notes from me - could you take a look please?

::: C4/Circulation.pm
@@ +1051,5 @@
> +        my $is_a_subscription = C4::Serials::CountSubscriptionFromBiblionumber($biblionumber);
> +        unless ($is_a_subscription) {
> +            my $issues = GetIssues( {
> +                borrowernumber => $borrower->{borrowernumber},
> +                biblionumber   => $biblionumber,

I like that it excludes items that belong to serials. Could we add this to the system preference description for documentation?

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
@@ +341,5 @@
> +            - pref: AllowMultipleIssuesOnABiblio
> +              choices:
> +                  yes: Allow
> +                  no: "Don't allow"
> +            - patrons to check out multiple items from the same biblio.

Not a native speaker, but maybe:

Allow/Don't allow patrons to check out multiple items from the same record, except for records with subscriptions.

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
@@ +271,5 @@
>  [% END %]
> +
> +[% IF BIBLIO_ALREADY_ISSUED %]
> +  <li>
> +    Borrower has already an issue on another item from this biblio.

I think we prefer 'patron', 'check out' and 'record' in the interface, so maybe we could say:

Patron has already checked out another item from this record.

::: t/db_dependent/Circulation/GetIssues.t
@@ +97,5 @@
> +        DelBranch($branchcode);
> +    }
> +};
> +
> +done_testing;

The tests are failing for me :( 

prove t/db_dependent/Circulation/GetIssues.t
t/db_dependent/Circulation/GetIssues.t .. Undefined subroutine &main::GetIssues called at t/db_dependent/Circulation/GetIssues.t line 61.
t/db_dependent/Circulation/GetIssues.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
No subtests run 

I also notice that you don't use the 'new rollback transactions trick'. I think if you added this to your tests, you could also avoid having to cleanup as the database in the ind. It will just go back to the state it was in before the tests were run. I think you can find an example in Reserves.t and also some other test files.
Comment 11 Katrin Fischer 2014-03-11 14:17:54 UTC
Ah, now I realize the bug is assigned to Julian, but the patch is from Jonathan... or I think it is. Changing the assignee for now, please change back if wrong.
Comment 12 Jonathan Druart 2014-03-13 12:35:56 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-03-13 12:36:03 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2014-03-13 13:05:50 UTC
Created attachment 26267 [details] [review]
[PASSED QA] Bug 10859: Alert if a borrower already has an issue for the same biblio

The librarian can force the checkout anyway.
It doesn't alert the librarian if the biblio is a subscription

Test plan:
1. Create a biblio with at least 2 items
2. Checkout the first item for a borrower
3. Set syspref AllowMultipleIssuesOnABiblio to OFF.
4. Try to checkout the second item with the same borrower. A message
should appear telling you that this borrower already borrowed an item
from this biblio.
If you have the permission 'force_checkout' You should also see two
buttons to confirm (or not) the checkout
5. Click on 'No'. The checkout is not done
6. Repeat step 4 and click 'Yes', the checkout is done.
7. Return the second item.
8. Set syspref AllowMultipleIssuesOnABiblio to ON
9. Try to checkout the second item with the same borrower. This time
the checkout is done without warnings.

Followed test plan. Works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
All tests and QA script pass, works well. Tested:

* Permission to override
  * check out a second item from a record with subscriptions works
  * check out a second item from a 'normal' record is warned about,
  but can be done

* No permission to override
  * subscription item: can be checked out
  * normal item: can't be checked out

* Feature turned off
  * Check out never warns/blocks
Comment 15 Katrin Fischer 2014-03-13 13:05:55 UTC
Created attachment 26268 [details] [review]
[PASSED QA] Bug 10859: wording

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 16 Katrin Fischer 2014-03-13 13:06:00 UTC
Created attachment 26269 [details] [review]
[PASSED QA] Bug 10859: GetIssues.t - Execute unit tests in a transaction

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 17 Katrin Fischer 2014-03-13 13:06:09 UTC
Created attachment 26270 [details] [review]
Bug 10859: QA follow up:  Fixing order in sysprefs.sql
Comment 18 Galen Charlton 2014-04-21 05:56:12 UTC
Pushed to master, along with a follow-up that clarifies the logic in the case where the item to be checked out is already on loan to the patron.

Thanks, Jonathan!
Comment 19 Fridolin Somers 2014-09-10 14:58:41 UTC
I choose to not apply to 3.14.x so I set as resolved.