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

(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 ;)

Created attachment 67728 [details] [review]
Bug 14919: Add test for Koha::Patron->old_holds subroutine

Created attachment 67729 [details] [review]
Bug 14919: Add Koha::Patron->old_holds subroutine

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

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

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 ) %]

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

(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.

If we tie it to intranetreadinghistory, could you add a little note to the pref description so it's clear please? Retesting now!

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>

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>

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>

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

Hi Joseph, can you sign off my follow-up and switch status to signed off if it's ok?

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>

(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!

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.

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 ;)

(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...

(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>

(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

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>

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>

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>

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>

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>

Created attachment 68138 [details] [review]
Bug 14919 (follow-up)  Changed status from 'in queue' to 'pending'

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.

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>

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>

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>

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>

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>

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>

Created attachment 68701 [details] [review]
Bug 14919 [QA Followup] - Make script executable, fix typo

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Great job Josef!

It seems that notificationdate and reminderdate are not used, why do you want to display them? :)

Created attachment 68756 [details] [review]
Bug 14919: (followup) Make notificationdate and reminderdate columns hidden by default

(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.

Created attachment 68780 [details] [review]
Bug 14919: Remove notificationdate and reminderdate

These columns are not used, we do not need to display them

(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.

Pushed to master for 17.11, thanks to everybody involved!