Bug 14919 - Holds history for patrons
Summary: Holds history for patrons
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Josef Moravec
QA Contact: Kyle M Hall
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 19541
  Show dependency treegraph
 
Reported: 2015-09-29 10:23 UTC by Josef Moravec
Modified: 2017-11-28 14:32 UTC (History)
11 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:
This new feature adds new page called "Holds history". The page is accessible from left menu on every patron related page. It allows librarians to see the history of all holds of given patron, with the actual status. It could be useful especially when the hold is cancelled. Before this patch the hold just disappeared when cancelled (automatically or manually).


Attachments
Bug 14919: Add test for Koha::Patron->old_holds subroutine (1.60 KB, patch)
2017-10-06 15:52 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14919: Add Koha::Patron->old_holds subroutine (922 bytes, patch)
2017-10-06 15:52 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14919: Add reserves history for patron (11.25 KB, patch)
2017-10-06 15:52 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14919: Add holds history for patron (11.30 KB, patch)
2017-10-07 19:39 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14919: Add test for Koha::Patron->old_holds subroutine (1.66 KB, patch)
2017-10-07 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: Add Koha::Patron->old_holds subroutine (987 bytes, patch)
2017-10-07 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: Add holds history for patron (11.38 KB, patch)
2017-10-07 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: (follow-up) Fixing some typos and adding to pref description (3.03 KB, patch)
2017-10-07 20:38 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: (follow-up) Fixing some typos and adding to pref description (3.07 KB, patch)
2017-10-08 06:57 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14919: (followup) Show accurate status in holds history (2.06 KB, patch)
2017-10-12 07:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14919: Add test for Koha::Patron->old_holds subroutine (1.71 KB, patch)
2017-10-14 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: Add Koha::Patron->old_holds subroutine (1.01 KB, patch)
2017-10-14 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: Add holds history for patron (11.41 KB, patch)
2017-10-14 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: (follow-up) Fixing some typos and adding to pref description (3.12 KB, patch)
2017-10-14 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: (followup) Show accurate status in holds history (2.11 KB, patch)
2017-10-14 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919 (follow-up) Changed status from 'in queue' to 'pending' (909 bytes, patch)
2017-10-14 20:23 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14919: Add test for Koha::Patron->old_holds subroutine (1.77 KB, patch)
2017-10-27 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14919: Add Koha::Patron->old_holds subroutine (1.07 KB, patch)
2017-10-27 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14919: Add holds history for patron (13.15 KB, patch)
2017-10-27 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14919: (follow-up) Fixing some typos and adding to pref description (3.18 KB, patch)
2017-10-27 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14919: (followup) Show accurate status in holds history (2.17 KB, patch)
2017-10-27 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14919 (follow-up) Changed status from 'in queue' to 'pending' (964 bytes, patch)
2017-10-27 13:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14919 [QA Followup] - Make script executable, fix typo (858 bytes, patch)
2017-10-27 13:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 14919: (followup) Make notificationdate and reminderdate columns hidden by default (779 bytes, patch)
2017-10-27 15:50 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14919: Remove notificationdate and reminderdate (2.53 KB, patch)
2017-10-27 17:34 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 Josef Moravec 2015-09-29 10:23:57 UTC
On patron page, librarians have access to patron's circulation history and Notices history. One our library wants to see the holds history too.

We have some data in table old_reserves, so it should be posible to show it somehow.
Comment 1 Brenda Turnbull 2017-01-25 15:53:22 UTC
I wondered if a report would help your staff with this request. I ahve placed the report in teh SQL area.  We palced a link on the home page fo staff to access this report. https://wiki.koha-community.org/wiki/SQL_Reports_Holds#Patrons_Holds_History_List
Comment 2 Josef Moravec 2017-01-25 17:07:48 UTC
(In reply to Brenda Turnbull from comment #1)
> I wondered if a report would help your staff with this request. I ahve
> placed the report in teh SQL area.  We palced a link on the home page fo
> staff to access this report.
> https://wiki.koha-community.org/wiki/
> SQL_Reports_Holds#Patrons_Holds_History_List

Yes, that's the solution we have adopted... but I think it's still valid to have it integrated into patron account better, I am assigning it to myself and will see if I will be able to come with something ;)
Comment 3 Josef Moravec 2017-10-06 15:52:42 UTC
Created attachment 67728 [details] [review]
Bug 14919: Add test for Koha::Patron->old_holds subroutine
Comment 4 Josef Moravec 2017-10-06 15:52:46 UTC
Created attachment 67729 [details] [review]
Bug 14919: Add Koha::Patron->old_holds subroutine

Test plan:
Run t/db_dependent/Koha/Patrons.t
Comment 5 Josef Moravec 2017-10-06 15:52:49 UTC
Created attachment 67730 [details] [review]
Bug 14919: Add reserves history for patron

Test plan:
0) Have a patron with some current and old reserves
1) Go to patron circulation page
2) Notice, there is new item called "Reserves history" in the left
circulation menu
3) Go to this page and confirm the data on this page are OK, and that
ui does behave as expected
Comment 6 Katrin Fischer 2017-10-07 13:59:16 UTC
Hi Josef,

some small things:
- 'Reserves' should be 'holds' everywhere (see terminology page)
- Please be careful about capitalization (page title)
- Should this be tied to intranetreadinghistory or not better be a new separate pref? Or leave it with borrowers permission for now?
+    [% IF ( CAN_user_borrowers ) %]
+        [% IF ( intranetreadinghistory ) %]
Comment 7 Josef Moravec 2017-10-07 19:39:04 UTC
Created attachment 67807 [details] [review]
Bug 14919: Add holds history for patron

Test plan:
0) Have a patron with some current and old reserves
1) Go to patron circulation page
2) Notice, there is new item called "Holds history" in the left
circulation menu
3) Go to this page and confirm the data on this page are OK, and that
ui does behave as expected
4) Go to adminitration, columns setting, try to change the setting for
holdshistory table and confirm it is taken into account on holds history
page
Comment 8 Josef Moravec 2017-10-07 19:42:26 UTC
(In reply to Katrin Fischer from comment #6)
> Hi Josef,

Hi Katrin, thanks for testing!

> some small things:
> - 'Reserves' should be 'holds' everywhere (see terminology page)
should be fixed now
> - Please be careful about capitalization (page title)
Also fixed
> - Should this be tied to intranetreadinghistory or not better be a new
> separate pref? Or leave it with borrowers permission for now?
> +    [% IF ( CAN_user_borrowers ) %]
> +        [% IF ( intranetreadinghistory ) %]

I thought about that an I think history of holds is very similar as history of checkouts... it's kind of part of checkouts... so I tied id with existing preference.
Comment 9 Katrin Fischer 2017-10-07 20:17:30 UTC
If we tie it to intranetreadinghistory, could you add a little note to the pref description so it's clear please? Retesting now!
Comment 10 Katrin Fischer 2017-10-07 20:38:33 UTC
Created attachment 67808 [details] [review]
Bug 14919: Add test for Koha::Patron->old_holds subroutine

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Katrin Fischer 2017-10-07 20:38:45 UTC
Created attachment 67809 [details] [review]
Bug 14919: Add Koha::Patron->old_holds subroutine

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2017-10-07 20:38:49 UTC
Created attachment 67810 [details] [review]
Bug 14919: Add holds history for patron

Test plan:
0) Have a patron with some current and old reserves
1) Go to patron circulation page
2) Notice, there is new item called "Holds history" in the left
circulation menu
3) Go to this page and confirm the data on this page are OK, and that
ui does behave as expected
4) Go to adminitration, columns setting, try to change the setting for
holdshistory table and confirm it is taken into account on holds history
page

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2017-10-07 20:38:56 UTC
Created attachment 67811 [details] [review]
Bug 14919: (follow-up) Fixing some typos and adding to pref description

- Adds 'holds' to pref description of intranetreadinghistory
- Fixes typo in link to holds history from patron account
- Changes reserve date to hold date in table heading
Comment 14 Katrin Fischer 2017-10-07 20:39:22 UTC
Hi Joseph, can you sign off my follow-up and switch status to signed off if it's ok?
Comment 15 Josef Moravec 2017-10-08 06:57:14 UTC
Created attachment 67818 [details] [review]
Bug 14919: (follow-up) Fixing some typos and adding to pref description

- Adds 'holds' to pref description of intranetreadinghistory
- Fixes typo in link to holds history from patron account
- Changes reserve date to hold date in table heading

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Josef Moravec 2017-10-08 06:57:41 UTC
(In reply to Katrin Fischer from comment #14)
> Hi Joseph, can you sign off my follow-up and switch status to signed off if
> it's ok?

Done, thanks for the followup!
Comment 17 Owen Leonard 2017-10-11 17:09:59 UTC
I'm confused about one aspect of this: In the status column I see three different statuses: "In queue," "Waiting," and "Cancelled." Shouldn't there be a status which appears if the person has checked out the item and the hold has been filled?

In my test "In queue" showed for a title before it was checked out, while it was checked out, and after it had been returned.

Also: The <title> tag contains HTML. Looks like it's from patron-title.inc.
Comment 18 Josef Moravec 2017-10-12 07:55:24 UTC
Created attachment 67988 [details] [review]
Bug 14919: (followup) Show accurate status in holds history

This patch fixes the status showed in holds history table.

The determination of status is following:

1. "hold.found is 'F'" - Fullfilled - the hold was fullfilled by checking the
item to patron. If hold reaches this state, it can't changed status
anymore, or be cancelled, that's why this goes first

2. "hold.cancellationdate is filled" - When the hold is cancelled, its
eventually waiting or in transit status remains the same, only
cancellation dat is filled, so this has to go second

3. "hold.found is 'W'" - Hold is waiting

4. "hold.found is 'T'" - Hold is in transit to target library

Test plan:
0) Apply the patch
2) Read the status determination note in this commit message and confirm
it makes sense
1) Have patron/patrons with at least one hold in every status
2) Go to holds history page and confirm it show the right status
3) Try to break the status determination ;)
Comment 19 Josef Moravec 2017-10-12 07:57:12 UTC
(In reply to Owen Leonard from comment #17)
> I'm confused about one aspect of this: In the status column I see three
> different statuses: "In queue," "Waiting," and "Cancelled." Shouldn't there
> be a status which appears if the person has checked out the item and the
> hold has been filled?
> 
> In my test "In queue" showed for a title before it was checked out, while it
> was checked out, and after it had been returned.
> 

You are right, the status was badly determinated, I added a followup for this

> Also: The <title> tag contains HTML. Looks like it's from patron-title.inc.

Hm, I don't have html in title tag...
Comment 20 Owen Leonard 2017-10-12 12:16:24 UTC
(In reply to Josef Moravec from comment #19)
> Hm, I don't have html in title tag...

Ah... It's because I have a salutation defined for this patron.

<title>Holds history for <span class="patron-title">Mr</span> Owen  Leonard</title>
Comment 21 Josef Moravec 2017-10-13 05:56:57 UTC
(In reply to Owen Leonard from comment #20)
> (In reply to Josef Moravec from comment #19)
> > Hm, I don't have html in title tag...
> 
> Ah... It's because I have a salutation defined for this patron.
> 
> <title>Holds history for <span class="patron-title">Mr</span> Owen 
> Leonard</title>

You are right, but is problem on more pages toot - circulation.pl, readingrec.pl, and so on...

I think it's out of the scope of this enhancement... we need to fix it generally... I'll create new bug report
Comment 22 Katrin Fischer 2017-10-14 20:23:26 UTC
Created attachment 68133 [details] [review]
Bug 14919: Add test for Koha::Patron->old_holds subroutine

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 23 Katrin Fischer 2017-10-14 20:23:31 UTC
Created attachment 68134 [details] [review]
Bug 14919: Add Koha::Patron->old_holds subroutine

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 24 Katrin Fischer 2017-10-14 20:23:35 UTC
Created attachment 68135 [details] [review]
Bug 14919: Add holds history for patron

Test plan:
0) Have a patron with some current and old reserves
1) Go to patron circulation page
2) Notice, there is new item called "Holds history" in the left
circulation menu
3) Go to this page and confirm the data on this page are OK, and that
ui does behave as expected
4) Go to adminitration, columns setting, try to change the setting for
holdshistory table and confirm it is taken into account on holds history
page

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Katrin Fischer 2017-10-14 20:23:39 UTC
Created attachment 68136 [details] [review]
Bug 14919: (follow-up) Fixing some typos and adding to pref description

- Adds 'holds' to pref description of intranetreadinghistory
- Fixes typo in link to holds history from patron account
- Changes reserve date to hold date in table heading

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 26 Katrin Fischer 2017-10-14 20:23:43 UTC
Created attachment 68137 [details] [review]
Bug 14919: (followup) Show accurate status in holds history

This patch fixes the status showed in holds history table.

The determination of status is following:

1. "hold.found is 'F'" - Fullfilled - the hold was fullfilled by checking the
item to patron. If hold reaches this state, it can't changed status
anymore, or be cancelled, that's why this goes first

2. "hold.cancellationdate is filled" - When the hold is cancelled, its
eventually waiting or in transit status remains the same, only
cancellation dat is filled, so this has to go second

3. "hold.found is 'W'" - Hold is waiting

4. "hold.found is 'T'" - Hold is in transit to target library

Test plan:
0) Apply the patch
2) Read the status determination note in this commit message and confirm
it makes sense
1) Have patron/patrons with at least one hold in every status
2) Go to holds history page and confirm it show the right status
3) Try to break the status determination ;)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 27 Katrin Fischer 2017-10-14 20:23:47 UTC
Created attachment 68138 [details] [review]
Bug 14919 (follow-up)  Changed status from 'in queue' to 'pending'
Comment 28 Katrin Fischer 2017-10-14 20:25:17 UTC
Made a tiny follow-up - 'pending' seems more common in Koha and is what we show the patron in the OPAC.

Josef filed bug 19456 for the problem with the title tag.
Comment 29 Kyle M Hall 2017-10-27 13:25:38 UTC
Created attachment 68695 [details] [review]
Bug 14919: Add test for Koha::Patron->old_holds subroutine

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2017-10-27 13:25:44 UTC
Created attachment 68696 [details] [review]
Bug 14919: Add Koha::Patron->old_holds subroutine

Test plan:
Run t/db_dependent/Koha/Patrons.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2017-10-27 13:25:48 UTC
Created attachment 68697 [details] [review]
Bug 14919: Add holds history for patron

Test plan:
0) Have a patron with some current and old reserves
1) Go to patron circulation page
2) Notice, there is new item called "Holds history" in the left
circulation menu
3) Go to this page and confirm the data on this page are OK, and that
ui does behave as expected
4) Go to adminitration, columns setting, try to change the setting for
holdshistory table and confirm it is taken into account on holds history
page

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2017-10-27 13:25:51 UTC
Created attachment 68698 [details] [review]
Bug 14919: (follow-up) Fixing some typos and adding to pref description

- Adds 'holds' to pref description of intranetreadinghistory
- Fixes typo in link to holds history from patron account
- Changes reserve date to hold date in table heading

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2017-10-27 13:25:54 UTC
Created attachment 68699 [details] [review]
Bug 14919: (followup) Show accurate status in holds history

This patch fixes the status showed in holds history table.

The determination of status is following:

1. "hold.found is 'F'" - Fullfilled - the hold was fullfilled by checking the
item to patron. If hold reaches this state, it can't changed status
anymore, or be cancelled, that's why this goes first

2. "hold.cancellationdate is filled" - When the hold is cancelled, its
eventually waiting or in transit status remains the same, only
cancellation dat is filled, so this has to go second

3. "hold.found is 'W'" - Hold is waiting

4. "hold.found is 'T'" - Hold is in transit to target library

Test plan:
0) Apply the patch
2) Read the status determination note in this commit message and confirm
it makes sense
1) Have patron/patrons with at least one hold in every status
2) Go to holds history page and confirm it show the right status
3) Try to break the status determination ;)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Kyle M Hall 2017-10-27 13:25:57 UTC
Created attachment 68700 [details] [review]
Bug 14919 (follow-up) Changed status from 'in queue' to 'pending'

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Kyle M Hall 2017-10-27 13:26:00 UTC
Created attachment 68701 [details] [review]
Bug 14919 [QA Followup] - Make script executable, fix typo

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Jonathan Druart 2017-10-27 15:19:26 UTC
Great job Josef!

It seems that notificationdate and reminderdate are not used, why do you want to display them? :)
Comment 37 Josef Moravec 2017-10-27 15:50:06 UTC
Created attachment 68756 [details] [review]
Bug 14919: (followup) Make notificationdate and reminderdate columns hidden by default
Comment 38 Josef Moravec 2017-10-27 15:50:52 UTC
(In reply to Jonathan Druart from comment #36)
> Great job Josef!
> 
> It seems that notificationdate and reminderdate are not used, why do you
> want to display them? :)

I wanted to remove it completely on another report...

But for now I've just added followup to make these two column hidden by default.
Comment 39 Jonathan Druart 2017-10-27 17:34:29 UTC
Created attachment 68780 [details] [review]
Bug 14919: Remove notificationdate and reminderdate

These columns are not used, we do not need to display them
Comment 40 Jonathan Druart 2017-10-27 17:34:55 UTC
(In reply to Josef Moravec from comment #38)
> (In reply to Jonathan Druart from comment #36)
> > Great job Josef!
> > 
> > It seems that notificationdate and reminderdate are not used, why do you
> > want to display them? :)
> 
> I wanted to remove it completely on another report...
> 
> But for now I've just added followup to make these two column hidden by
> default.

No need to wait, done.
Comment 41 Jonathan Druart 2017-10-27 19:10:21 UTC
Pushed to master for 17.11, thanks to everybody involved!