Bug 14368 - Add discharges history
Summary: Add discharges history
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Matthias Meusburger
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 8007
Blocks:
  Show dependency treegraph
 
Reported: 2015-06-09 12:34 UTC by Matthias Meusburger
Modified: 2016-06-21 21:40 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14368: Add discharges history. (8.13 KB, patch)
2015-06-10 15:06 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 14368: Add discharges history. (8.19 KB, patch)
2015-06-16 10:17 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 14368: Add discharges history. (8.27 KB, patch)
2015-06-23 07:04 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 14368: Add discharges history. (8.26 KB, patch)
2015-06-23 09:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14368 [Follow-up]: Fix tests. (1.60 KB, patch)
2015-09-16 12:55 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 14368 [Follow-up]: Add filter on borrower when displaying discharges history on borrower's page. (786 bytes, patch)
2015-09-16 14:14 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 14368: Add some missing tests (7.77 KB, patch)
2015-09-17 09:29 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14368: Add discharges history. (8.33 KB, patch)
2015-09-17 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14368 [Follow-up]: Fix tests. (1.67 KB, patch)
2015-09-17 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14368 [Follow-up]: Add filter on borrower when displaying discharges history on borrower's page. (853 bytes, patch)
2015-09-17 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14368: Add some missing tests (7.85 KB, patch)
2015-09-17 14:03 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14368 [Follow-up]: Fix workflow. (2.25 KB, patch)
2015-09-21 12:53 UTC, Matthias Meusburger
Details | Diff | Splinter Review
Bug 14368 [Follow-up]: Fix workflow. (2.32 KB, patch)
2015-09-21 14:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14368: (RM followup) DBIx update (1.69 KB, patch)
2015-09-28 15:10 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Matthias Meusburger 2015-06-09 12:34:31 UTC
Currently there is no way to see when a user was discharged, or to have an history if the user was discharged several time.
Comment 1 Matthias Meusburger 2015-06-10 15:06:13 UTC Comment hidden (obsolete)
Comment 2 Indranil Das Gupta 2015-06-14 06:58:35 UTC
I get the part about having discharge history. But why would an user be 'discharged' several times? Is that not counter intuitive? Could you elaborate with an use-case where there could be multiple "discharge"?
Comment 3 Indranil Das Gupta 2015-06-14 07:01:45 UTC
Academic libraries in India particularly require discharge management feature. A multiple discharge of the same patron will create confusion and major patron management headaches (fees, fines, access card issuance etc).

Setting this to IN DISCUSSION
Comment 4 vinod 2015-06-14 07:49:50 UTC
Please mention some case where this kind of feature is needed and it could not be manage by temporarily block feature?
Comment 5 Indranil Das Gupta 2015-06-16 10:17:16 UTC Comment hidden (obsolete)
Comment 6 Indranil Das Gupta 2015-06-16 10:19:29 UTC
Signed off based on discussions referenced at http://irc.koha-community.org/koha/2015-06-16#i_1688844
Comment 7 Jonathan Druart 2015-06-18 09:22:58 UTC
The indentation should be 4 spaces, please resubmit.
Comment 8 Matthias Meusburger 2015-06-23 07:04:33 UTC Comment hidden (obsolete)
Comment 9 Matthias Meusburger 2015-06-23 07:05:12 UTC
Submitted with 4 spaces identation.
Comment 10 Jonathan Druart 2015-06-23 09:01:52 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2015-07-07 18:56:09 UTC
It makes sense to have more than one discharge in a patron's lifetime (requested when ending studies, and then if he leaves the institution as a professor...).

I tested the feature and it showed more information about the discharge, but I doesn't seem to be working as expected as only the first discharge is being saved...
Comment 12 Tomás Cohen Arazi 2015-07-07 19:51:46 UTC
Matts, a few notes on this patch.

- I like the idea, I think we should've had this for the 3.20 release already.
- There's a bug on the unit test you introduce for lifting the discharge, because it should look like:

    ok( ! Koha::Borrower::Discharge::is_discharged({ borrowernumber => $borrowernumber }),
        'The patron is not discharged after the restriction has been lifted' );

And you will notice it fails :/

- There's a problem with Koha::Borrower::Discharge::discharge (not introduced by your patch, but affects the beaviour): it overwrites the already defined discharges... so no history! Heh.
- For fixing the previous, you will need to add a column to address the fact that the discharge is active/non active. So you can have more than one discharges for a borrowernumber, and always check only one is active at a time.

I'm attaching a followup with those changes on the tests, It'd be great if you could take a look and maybe talk tomorrow on IRC about this to have this ready ASAP.
Comment 13 Matthias Meusburger 2015-09-16 12:53:46 UTC
(In reply to Tomás Cohen Arazi from comment #12)

> - There's a bug on the unit test you introduce for lifting the discharge,
> because it should look like:
> 
>     ok( ! Koha::Borrower::Discharge::is_discharged({ borrowernumber =>
> $borrowernumber }),
>         'The patron is not discharged after the restriction has been lifted'
> );
> 
> And you will notice it fails :/

Indeed. Misuse of DelDebarment. Fixed in the follow-up.

> 
> - There's a problem with Koha::Borrower::Discharge::discharge (not
> introduced by your patch, but affects the beaviour): it overwrites the
> already defined discharges... so no history! Heh.

Hmm, the sql patch introducing discharge_id in the discharges table and the following part of the patch should prevent this behavior, shouldn't it ?


-    my $discharge = $rs->search({ borrower => $borrowernumber });
+    my $discharge = $rs->search({ borrower => $borrowernumber }, { order_by => { -desc => 'needed' }, rows => 1 });
Comment 14 Matthias Meusburger 2015-09-16 12:55:27 UTC Comment hidden (obsolete)
Comment 15 Matthias Meusburger 2015-09-16 14:14:20 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2015-09-17 09:29:57 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2015-09-17 09:30:56 UTC
It does not work anymore.
When I try to allow the discharge (/members/discharges.pl?op=allow&borrowernumber=XXXX), I get
DBIx::Class::ResultSet::_rs_update_delete(): Unable to perform complex resultset update() without an identifying set of columns on source 'Discharge' at /home/koha/src/Koha/Borrower/Discharge.pm line 96
Comment 18 Matthias Meusburger 2015-09-17 13:36:54 UTC
Jonathan, did you run misc/devel/update_dbix_class_files.pl , as stated in the commit message, to update the DBIx Discharge schema ?
Comment 19 Jonathan Druart 2015-09-17 13:52:10 UTC
(In reply to Matthias Meusburger from comment #18)
> Jonathan, did you run misc/devel/update_dbix_class_files.pl , as stated in
> the commit message, to update the DBIx Discharge schema ?

Oops, no!
Comment 20 Jonathan Druart 2015-09-17 14:03:18 UTC
Created attachment 42670 [details] [review]
Bug 14368: Add discharges history.

Currently, once a user is discharged, lifting the discharge debarment does
not allow the user to ask for a discharge again.

This patch adds the ability for a user to be discharged several times and to
keep an history of the discharges.

Test plan:

 - As a user, ask for a discharge.
 - As a librarian, allow the discharge.
 - Check that the requested and validated dates appear on the discharge panel of the borrower in the admin interface.
 - Lift the discharge debarment of the user.
 - Check that you can repeat these steps and that the new discharges are added in the discharge panel of the borrower in the admin interface.

Please note that you will need to run misc/devel/update_dbix_class_files.pl after the SQL update.
Adding a primary key to the discharges table was mandatory for DBIX::Class to be able to query with order_by.

Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 21 Jonathan Druart 2015-09-17 14:03:22 UTC
Created attachment 42671 [details] [review]
Bug 14368 [Follow-up]: Fix tests.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 22 Jonathan Druart 2015-09-17 14:03:25 UTC
Created attachment 42672 [details] [review]
Bug 14368 [Follow-up]: Add filter on borrower when displaying discharges history on borrower's page.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 23 Jonathan Druart 2015-09-17 14:03:30 UTC
Created attachment 42673 [details] [review]
Bug 14368: Add some missing tests

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2015-09-17 14:14:57 UTC
Hum, something does not work correctly IMO.
Let say the given workflow:
- A patron asks for a discharge at the OPAC
- The patron comes at the librarian's office.
- On the patron page, there is no track of this demand, so the librarian goes on the discharge page (members/discharge.pl) and generates the discharge
- The discharge does not appear in the discharge list but still exist in the pending discharge requests.

There is something which is not consistent here, it should be fixed.
Failing QA (sorry!)
Comment 25 Matthias Meusburger 2015-09-21 12:53:28 UTC Comment hidden (obsolete)
Comment 26 Matthias Meusburger 2015-09-21 12:54:09 UTC
The last patch fixes the bug you described, Jonathan.
Comment 27 Jonathan Druart 2015-09-21 14:05:09 UTC
Created attachment 42723 [details] [review]
Bug 14368 [Follow-up]: Fix workflow.

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 28 Jonathan Druart 2015-09-21 14:05:41 UTC
(In reply to Matthias Meusburger from comment #26)
> The last patch fixes the bug you described, Jonathan.

Thanks, everything sounds good now!
Comment 29 Tomás Cohen Arazi 2015-09-28 15:10:50 UTC
Created attachment 42930 [details] [review]
Bug 14368: (RM followup) DBIx update

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 30 Tomás Cohen Arazi 2015-09-28 15:15:56 UTC
Patches pushed to master.

Good job, Matthias!