Bug 20936 - Holds history for patrons in OPAC
Summary: Holds history for patrons in OPAC
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Agustín Moyano
QA Contact: Testopia
URL:
Keywords:
: 23142 (view as bug list)
Depends on: 14919 20806
Blocks:
  Show dependency treegraph
 
Reported: 2018-06-13 17:14 UTC by Ed Veal
Modified: 2020-07-05 01:19 UTC (History)
14 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Northeast Kansas Library System (NEKLS)
Version(s) released in:


Attachments
Bug 20936: Add OPACHoldsHistory syspref (3.29 KB, patch)
2020-02-10 00:06 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (34.84 KB, patch)
2020-02-10 00:06 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add OPACHoldsHistory syspref (3.37 KB, patch)
2020-03-04 13:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20936: Add OPACHoldsHistory syspref (3.37 KB, patch)
2020-03-04 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (34.91 KB, patch)
2020-03-04 13:42 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20936: Add OPACHoldsHistory syspref (3.35 KB, patch)
2020-03-05 22:35 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (28.35 KB, patch)
2020-03-05 22:36 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add test (3.15 KB, patch)
2020-03-05 22:36 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add OPACHoldsHistory syspref (3.35 KB, patch)
2020-03-10 18:32 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add koha_object_class and koha_objects_class methods to Schema classes (3.48 KB, patch)
2020-03-10 18:32 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (25.14 KB, patch)
2020-03-10 18:32 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add test (3.15 KB, patch)
2020-03-10 18:32 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add OPACHoldsHistory syspref (3.35 KB, patch)
2020-03-25 15:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add koha_object_class and koha_objects_class methods to Schema classes (3.48 KB, patch)
2020-03-25 15:49 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (24.96 KB, patch)
2020-03-25 15:50 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add test (3.15 KB, patch)
2020-03-25 15:50 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: (QA follow-up) Only add timezone if date-time (1.29 KB, patch)
2020-03-25 20:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 20936: (follow-up) use modified by bug 24980, fix display of page info and fix search without enter (2.32 KB, patch)
2020-03-26 21:50 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: (follow-up) change opac-holdsrecord to opac-holdshistory, set translatable strings and set rows per page to 20 (4.08 KB, patch)
2020-03-31 22:55 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add OPACHoldsHistory syspref (3.06 KB, patch)
2020-04-29 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20936: Add required relations for the OldReserve schema (2.28 KB, patch)
2020-04-29 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (24.87 KB, patch)
2020-04-29 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20936: Add API tests (3.15 KB, patch)
2020-04-29 14:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (24.45 KB, patch)
2020-04-30 14:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20936: Add API tests (3.15 KB, patch)
2020-04-30 14:29 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20936: Add OPACHoldsHistory syspref (3.06 KB, patch)
2020-05-06 23:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (15.55 KB, patch)
2020-05-06 23:09 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 20936: Add patron's hold history menu in OPAC (15.61 KB, patch)
2020-05-20 00:15 UTC, ByWater Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ed Veal 2018-06-13 17:14:45 UTC
I think it would be very nice to add the "Hold History" for the patrons to access in the OPAC. I don't know if the bug to add it to the staff site would be useful or not but here it is.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14919

Ed
Comment 1 Lisette Scheer 2019-06-17 21:58:49 UTC
*** Bug 23142 has been marked as a duplicate of this bug. ***
Comment 2 Christofer Zorn 2019-06-18 12:11:50 UTC
Good morning,

I've been asked several times by our staff and patrons to include a Holds History in the public facing OPAC. Our patrons would find this helpful, especially for those holds that have lapsed. This would allow someone to put a hold back on that item that they missed.

Thanks for considering.

Christofer
Comment 3 Joy Nelson 2019-09-25 21:41:38 UTC
Create the ability for patrons to view their holds history in the OPAC once they login.

Provide a way for the patron to be able to delete holds history in the OPAC if they are allowed to (similar to circ history).

Adjust Patron Privacy preferences to include holds history into privacy considerations.   
If patron privacy setting is set to never, never retain holds history (anonymize it).  
If privacy is set to keep, keep holds history for the patron forever. 
If privacy is set to default, keep it until library chooses to anonymize hold history.
Comment 4 Agustín Moyano 2020-02-10 00:06:50 UTC
Created attachment 98629 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref
Comment 5 Agustín Moyano 2020-02-10 00:06:54 UTC
Created attachment 98630 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC. Right now, it only shows records from old_reserves table, but I'll wait till bug 20271 is pushed to show full history (old and new holds)

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds canceled or fulfilled
6. Filter, order and change page
SUCCESS => All controls work as expected
7. Sign off.

Table content is fetched from the api. If you see data, and you can order and filter then please sign off bug 24561.

Date columns use $date function to transform dates strings from api (for example '2020-02-20') to 'dateformat' prefernce format ('02/20/20202'). If you change dateformat prefernce and see the changes reflected in date columns, please sign off bug 24455.
Comment 6 Hugo Agud 2020-02-14 08:44:35 UTC
98624 - Bug 24561: Datatables api wrapper also filter and order embedded columns

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 24561: Datatables api wrapper also filter and order embedded columns
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/js/datatables.js).
error: could not build fake ancestor
Patch failed at 0001 Bug 24561: Datatables api wrapper also filter and order embedded columns
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-24561-Datatables-api-wrapper-also-filter-and-o-Tegmir.patch
Comment 7 Agustín Moyano 2020-02-19 15:03:48 UTC
For some reason, automatic dependency resolution of git bz doesn't show bug 20212 (that ist's a dependency of 24561) as an option to apply.

To successfuly apply this patch, first apply 20212 manually.
Comment 8 Katrin Fischer 2020-02-19 15:25:41 UTC
Augustin - it's because it's ASSIGNED I think - not NSO or SO.
Comment 9 Kyle M Hall 2020-03-02 17:38:28 UTC
When testing I get the following error in the console in FireFox on the OPAC on the holds history page:

TypeError: $(...).api is not a function opac-holdsrecord.pl:793:36
Comment 10 Agustín Moyano 2020-03-03 15:23:47 UTC
Hi Kyle, fixed an issue in bug 24561... now it should work.

Please try again.

Thanks
Comment 11 Agustín Moyano 2020-03-03 15:25:30 UTC
(In reply to Agustín Moyano from comment #7)
> For some reason, automatic dependency resolution of git bz doesn't show bug
> 20212 (that ist's a dependency of 24561) as an option to apply.
> 
> To successfuly apply this patch, first apply 20212 manually.

I almost forgot.. bug 24561 no longer depends on 20212.

Now 20212 depends on 24561, so there's no need to apply that bug manually.
Comment 12 Kyle M Hall 2020-03-04 13:41:44 UTC
Created attachment 100115 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 13 Kyle M Hall 2020-03-04 13:42:08 UTC
Created attachment 100116 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Kyle M Hall 2020-03-04 13:42:16 UTC
Created attachment 100117 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC. Right now, it only shows records from old_reserves table, but I'll wait till bug 20271 is pushed to show full history (old and new holds)

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds canceled or fulfilled
6. Filter, order and change page
SUCCESS => All controls work as expected
7. Sign off.

Table content is fetched from the api. If you see data, and you can order and filter then please sign off bug 24561.

Date columns use $date function to transform dates strings from api (for example '2020-02-20') to 'dateformat' prefernce format ('02/20/20202'). If you change dateformat prefernce and see the changes reflected in date columns, please sign off bug 24455.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Jonathan Druart 2020-03-04 14:05:06 UTC
This is not ready at all, diff looks wrong, please double check the patch.
Comment 16 Jonathan Druart 2020-03-04 14:06:27 UTC
And tests, pretty please.
Comment 17 Tomás Cohen Arazi 2020-03-04 15:54:42 UTC
A few remarks:
- Some POD are wrong (koha_object_class for effective_itemtype?)
- You copied and pasted from other API controllers, but kept deprecated things like _to_model that are not even used
- There's some debugging leftovers
- There are no tests for the controller

Please solve them and re-submit
Comment 18 Agustín Moyano 2020-03-05 17:06:06 UTC
(In reply to Jonathan Druart from comment #16)
> And tests, pretty please.

How can I refuse if you ask so nicely XD
Comment 19 Agustín Moyano 2020-03-05 17:06:50 UTC
(In reply to Tomás Cohen Arazi from comment #17)
> A few remarks:
> - Some POD are wrong (koha_object_class for effective_itemtype?)
> - You copied and pasted from other API controllers, but kept deprecated
> things like _to_model that are not even used
> - There's some debugging leftovers
> - There are no tests for the controller
> 
> Please solve them and re-submit

I was too eager to get this going.. I'll attend them ASAP
Comment 20 Agustín Moyano 2020-03-05 22:35:58 UTC
Created attachment 100205 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Agustín Moyano 2020-03-05 22:36:03 UTC
Created attachment 100206 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC. Right now, it only shows records from old_reserves table, but I'll wait till bug 20271 is pushed to show full history (old and new holds)

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds canceled or fulfilled
6. Filter, order and change page
SUCCESS => All controls work as expected
7. Sign off.

Table content is fetched from the api. If you see data, and you can order and filter then please sign off bug 24561.

Date columns use $date function to transform dates strings from api (for example '2020-02-20') to 'dateformat' prefernce format ('02/20/20202'). If you change dateformat prefernce and see the changes reflected in date columns, please sign off bug 24455.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Agustín Moyano 2020-03-05 22:36:08 UTC
Created attachment 100207 [details] [review]
Bug 20936: Add test

Add test to /api/v1/public/patrons/:patron_id/holds endpoint

To test:
1. prove t/db_dependent/api/v1/patrons_holds.t
Comment 23 Jonathan Druart 2020-03-06 13:22:51 UTC
In case someone else comes for QA before me, see bug 24608 comment 26 and see if the problem exists in this feature as well.


Agustin, a couple of things I spot:
 * authnotrequired needs to be removed (there is a new check in the QA script)
 * Schema files should be in a separate patch.
Comment 24 Agustín Moyano 2020-03-10 18:32:41 UTC
Created attachment 100471 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Agustín Moyano 2020-03-10 18:32:46 UTC
Created attachment 100472 [details] [review]
Bug 20936: Add koha_object_class and koha_objects_class methods to Schema classes
Comment 26 Agustín Moyano 2020-03-10 18:32:50 UTC
Created attachment 100473 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC. Right now, it only shows records from old_reserves table, but I'll wait till bug 20271 is pushed to show full history (old and new holds)

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds canceled or fulfilled
6. Filter, order and change page
SUCCESS => All controls work as expected
7. Sign off.

Table content is fetched from the api. If you see data, and you can order and filter then please sign off bug 24561.

Date columns use $date function to transform dates strings from api (for example '2020-02-20') to 'dateformat' prefernce format ('02/20/20202'). If you change dateformat prefernce and see the changes reflected in date columns, please sign off bug 24455.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Agustín Moyano 2020-03-10 18:32:55 UTC
Created attachment 100474 [details] [review]
Bug 20936: Add test

Add test to /api/v1/public/patrons/:patron_id/holds endpoint

To test:
1. prove t/db_dependent/api/v1/patrons_holds.t
Comment 28 Agustín Moyano 2020-03-10 18:37:08 UTC
Hi Jonathan

(In reply to Jonathan Druart from comment #23)
> In case someone else comes for QA before me, see bug 24608 comment 26 and
> see if the problem exists in this feature as well.

I don't believe you'll have the same problem.. in this operation there is no save method, only retrieves holds history.

> 
> Agustin, a couple of things I spot:
>  * authnotrequired needs to be removed (there is a new check in the QA
> script)

Weird that when I did qa, the script didn't mention anything about authnotrequired, but I removed it anyway.

>  * Schema files should be in a separate patch.

Done.

Cheers!
Comment 29 Katrin Fischer 2020-03-22 16:04:24 UTC
Currently blocked by bug 24455 - please reset once dependencies have cleared up!
Comment 30 Tomás Cohen Arazi 2020-03-22 17:54:43 UTC
(In reply to Katrin Fischer from comment #29)
> Currently blocked by bug 24455 - please reset once dependencies have cleared
> up!

You can QA it anyways. I think Martin and Agustín has a conversation about this but the status wasn't updated.
Comment 31 Jonathan Druart 2020-03-25 15:09:30 UTC
https://snipboard.io/qrCmGP.jpg

MariaDB [koha_kohadev]> select * from old_reserves\G
*************************** 1. row ***************************
      reserve_id: 1
  borrowernumber: 5
     reservedate: 2020-03-25
    biblionumber: 1
      branchcode: CPL
notificationdate: NULL
    reminderdate: NULL
cancellationdate: NULL
    reservenotes:
        priority: 0
           found: F
       timestamp: 2020-03-25 15:07:18
      itemnumber: 1
     waitingdate: 2020-03-25
  expirationdate: 2020-03-26
  lowestPriority: 0
         suspend: 0
   suspend_until: NULL
        itemtype: NULL
 item_level_hold: 0


Compare date values.
Comment 32 Jonathan Druart 2020-03-25 15:13:34 UTC
So, 2020-03-25 in DB is displayed 24/03

server time is Wed Mar 25 15:12:57 UTC 2020
Client time is Wed 25 Mar 16:12:54 CET 2020
Comment 33 Agustín Moyano 2020-03-25 15:49:51 UTC
Created attachment 101724 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Agustín Moyano 2020-03-25 15:49:56 UTC
Created attachment 101725 [details] [review]
Bug 20936: Add koha_object_class and koha_objects_class methods to Schema classes
Comment 35 Agustín Moyano 2020-03-25 15:50:01 UTC
Created attachment 101726 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC. Right now, it only shows records from old_reserves table, but I'll wait till bug 20271 is pushed to show full history (old and new holds)

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds canceled or fulfilled
6. Filter, order and change page
SUCCESS => All controls work as expected
7. Sign off.

Table content is fetched from the api. If you see data, and you can order and filter then please sign off bug 24561.

Date columns use $date function to transform dates strings from api (for example '2020-02-20') to 'dateformat' prefernce format ('02/20/20202'). If you change dateformat prefernce and see the changes reflected in date columns, please sign off bug 24455.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 36 Agustín Moyano 2020-03-25 15:50:05 UTC
Created attachment 101727 [details] [review]
Bug 20936: Add test

Add test to /api/v1/public/patrons/:patron_id/holds endpoint

To test:
1. prove t/db_dependent/api/v1/patrons_holds.t
Comment 37 Jonathan Druart 2020-03-25 15:58:34 UTC
Agustin, please do not amend patches that have been signed off yet, submit follow-ups instead. They can be squashed later if needed.

Still not correct: https://snipboard.io/0QNHgX.jpg
There is one non formatted string, I guess that is what you modified.
Comment 38 Jonathan Druart 2020-03-25 16:07:35 UTC
Also found:
Search "foo", nothing is displayed, info is "showing page 1 of 0"
https://snipboard.io/stcEuY.jpg

Then remove "foo", you need to hit enter to see the unfiltered result, not sure it's the expected behavior as filtering works without hitting enter.
Comment 39 Agustín Moyano 2020-03-25 16:40:16 UTC
(In reply to Jonathan Druart from comment #37)
> Agustin, please do not amend patches that have been signed off yet, submit
> follow-ups instead. They can be squashed later if needed.

Ok, will rememeber that
Comment 40 Martin Renvoize 2020-03-25 17:05:32 UTC
Comment on attachment 101726 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

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

::: api/v1/swagger/paths/public_patrons.json
@@ +301,5 @@
> +                  {
> +                    "name": "timestamp",
> +                    "in": "query",
> +                    "description": "Time of latest update",
> +                    "type": "string"

This feels wrong.. surely this needs a `format: date-time` entry

@@ +313,5 @@
> +                  {
> +                    "name": "waiting_date",
> +                    "in": "query",
> +                    "description": "Date the item was marked as waiting for the patron",
> +                    "type": "string"

This feels wrong.. surely this needs a `format: date-time` entry

@@ +319,5 @@
> +                  {
> +                    "name": "expiration_date",
> +                    "in": "query",
> +                    "description": "Date the hold expires",
> +                    "type": "string"

This feels wrong.. surely this needs a `format: date-time` entry

@@ +337,5 @@
> +                  {
> +                    "name": "suspended_until",
> +                    "in": "query",
> +                    "description": "Suspended until",
> +                    "type": "string"

This feels wrong.. surely this needs a `format: date-time` entry
Comment 41 Martin Renvoize 2020-03-25 17:13:06 UTC
Indeed, dropping the use of the call to `date` for formatting breaks it entirely.. i.e we don't format using the sysprefs at all.
Comment 42 Martin Renvoize 2020-03-25 17:31:32 UTC
OK.. this issue here is that the js code in bug 24455 does not distinguish between the perfectly valid 'full-date' and 'date-time' resolutions of RFC3339.

Both are valid formats, but one includes a time offset whilst the other does not.  The 'full-date' option does not include time resolution and as such also does not include an offset.  When moment parses this, it 'upgrades' the internal representation from full-date (2020-03-25) to date-time (2020-03-25T00:00:00+00:00) internally.. When you then apply a timezone to that (say +0300 for Argentina I believe), we end up with an internal date of 2020-03-24T21:00:00-03:00.. spot the day change.. formating this back down to just 'date' will loose the day.

Hope that makes sense.. so.. what we need to do is detect if we're parsing a date-time vs a full-date and only apply timezone maths to the former
Comment 43 Agustín Moyano 2020-03-25 19:12:14 UTC
(In reply to Martin Renvoize from comment #40)
> Comment on attachment 101726 [details] [review] [review]
> Bug 20936: Add patron's hold history menu in OPAC
> 
> Review of attachment 101726 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: api/v1/swagger/paths/public_patrons.json
> @@ +301,5 @@
> > +                  {
> > +                    "name": "timestamp",
> > +                    "in": "query",
> > +                    "description": "Time of latest update",
> > +                    "type": "string"
> 
> This feels wrong.. surely this needs a `format: date-time` entry
> 
> @@ +313,5 @@
> > +                  {
> > +                    "name": "waiting_date",
> > +                    "in": "query",
> > +                    "description": "Date the item was marked as waiting for the patron",
> > +                    "type": "string"
> 
> This feels wrong.. surely this needs a `format: date-time` entry
> 
> @@ +319,5 @@
> > +                  {
> > +                    "name": "expiration_date",
> > +                    "in": "query",
> > +                    "description": "Date the hold expires",
> > +                    "type": "string"
> 
> This feels wrong.. surely this needs a `format: date-time` entry
> 
> @@ +337,5 @@
> > +                  {
> > +                    "name": "suspended_until",
> > +                    "in": "query",
> > +                    "description": "Suspended until",
> > +                    "type": "string"
> 
> This feels wrong.. surely this needs a `format: date-time` entry

These are input parameters.. these are for search, including partial search.. so you cannot put 'format: date-time' if you plan to search by all dates from year 2020, for example.
Comment 44 Martin Renvoize 2020-03-25 20:45:49 UTC
Totally makes sense.. your right.
Comment 45 Martin Renvoize 2020-03-25 20:46:13 UTC
Created attachment 101772 [details] [review]
Bug 20936: (QA follow-up) Only add timezone if date-time

This patch prevents a timezone from being assigned to the js moment if
the input string is only at the full-date and not date-time resolution
Comment 46 Martin Renvoize 2020-03-25 20:47:12 UTC
Agustin, could you take a look at my followup and confirm that it makes sense?

I think I should probably push it as an RM follow-up to bug 24455 rather than here.. but would like a sanity check from you first.
Comment 47 Martin Renvoize 2020-03-26 09:58:42 UTC
Moved followup to bug 24980
Comment 48 Martin Renvoize 2020-03-26 20:50:20 UTC
With the new dependency added I believe we are unblocked again here.. resetting to signed off.
Comment 49 Martin Renvoize 2020-03-26 20:51:22 UTC
Just spotted.. we need to add the `Sponsored-by: ` Line into at least one of the patches for the release notes ;)
Comment 50 Agustín Moyano 2020-03-26 21:50:19 UTC
Created attachment 101915 [details] [review]
Bug 20936: (follow-up) use  modified by bug 24980, fix display of page info and fix search without enter
Comment 51 Agustín Moyano 2020-03-31 16:12:26 UTC
Failed QA because of comment 11 of bug 24980, that are really for this bug

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24980#c11
Comment 52 Agustín Moyano 2020-03-31 22:55:42 UTC
Created attachment 102166 [details] [review]
Bug 20936: (follow-up) change opac-holdsrecord to opac-holdshistory, set translatable strings and set rows per page to 20
Comment 53 Jonathan Druart 2020-04-01 08:14:47 UTC
Copy/pasting a comment from bug 24980 (I sent it to the wrong bug number):

"""
Hi Jonathan,

(In reply to Jonathan Druart from comment #11)
> 0. Dates look great now! :)
> 
> 1. Why not having current holds in the table (behavior intranet-side,
> members/holdshistory.pl)

Controller uses object.search, and places order by in dbic.. to replicate current behaviour in intranet I should one object.search for both current and old holds, merge and sort both arrays depending on search column... I decided to wait till requests and old_requests tables were merged to have current intranet behaviour 

> 
> 2. Is there a plan to make existing intranet-side view using the REST API?
> ie. only 1 code to maintain.
> 

Once we can replicate current intranet behaviour, we can think on changing intranet-side

> 3. Why only 10 results displayed by default? Usually 20
> 

ok, will change that

> 4. Why no way to modify the number of results displayed? Basically, why
> DataTable does not display as other table? (like no page number, search box,
> etc.)
> Is that the OPAC style?
> 

OPAC has no style for DataTables.. only intranet has. What you see in other tables in OPAC was coded outside DataTables. I thought it best to have a search box and page number, but I'm open to suggestions.

> 5. Not translatable strings:
> 187                         if(row.cancelation_date) return 'Canceled';
> 188                         if (data == 'F') return 'Fulfilled';
> 189                         if (data == 'W') return 'Waiting';
> 190                         if (data == 'T') return 'In Transit';
> 191                         return 'Pending';
> 

Good catch.. will fix that

> 6. opac-holdsrecord.pl vs members/holdshistory.pl
> Should not we use the same script names?

copied the name from opac-readingrecord, but it can be changed



Comment 14 Agustín Moyano(reporter)(assignee) 2020-03-30 17:53:20 CEST

(In reply to Jonathan Druart from comment #12)
> 7. Should not deal with that in js-date-format.inc?
>   (data&&$date(data))||'';

ok, it can be done
"""
Comment 54 Jonathan Druart 2020-04-01 08:16:45 UTC
> (In reply to Jonathan Druart from comment #11)
> > 0. Dates look great now! :)
> > 
> > 1. Why not having current holds in the table (behavior intranet-side,
> > members/holdshistory.pl)
> 
> Controller uses object.search, and places order by in dbic.. to replicate
> current behaviour in intranet I should one object.search for both current
> and old holds, merge and sort both arrays depending on search column... I
> decided to wait till requests and old_requests tables were merged to have
> current intranet behaviour 

This is not going to happen any time soon.

> > 2. Is there a plan to make existing intranet-side view using the REST API?
> > ie. only 1 code to maintain.
> > 
> 
> Once we can replicate current intranet behaviour, we can think on changing
> intranet-side

So waiting for tables to be merged?
Comment 55 Tomás Cohen Arazi 2020-04-01 11:48:09 UTC
(In reply to Jonathan Druart from comment #54)
> > (In reply to Jonathan Druart from comment #11)
> > > 0. Dates look great now! :)
> > > 
> > > 1. Why not having current holds in the table (behavior intranet-side,
> > > members/holdshistory.pl)
> > 
> > Controller uses object.search, and places order by in dbic.. to replicate
> > current behaviour in intranet I should one object.search for both current
> > and old holds, merge and sort both arrays depending on search column... I
> > decided to wait till requests and old_requests tables were merged to have
> > current intranet behaviour 
> 
> This is not going to happen any time soon.
> 
> > > 2. Is there a plan to make existing intranet-side view using the REST API?
> > > ie. only 1 code to maintain.
> > > 
> > 
> > Once we can replicate current intranet behaviour, we can think on changing
> > intranet-side
> 
> So waiting for tables to be merged?

We don't need to solve intranet-side on this bug :-D
Comment 56 Agustín Moyano 2020-04-01 19:20:49 UTC
(In reply to Jonathan Druart from comment #54)
> > (In reply to Jonathan Druart from comment #11)
> > > 0. Dates look great now! :)
> > > 
> > > 1. Why not having current holds in the table (behavior intranet-side,
> > > members/holdshistory.pl)
> > 
> > Controller uses object.search, and places order by in dbic.. to replicate
> > current behaviour in intranet I should one object.search for both current
> > and old holds, merge and sort both arrays depending on search column... I
> > decided to wait till requests and old_requests tables were merged to have
> > current intranet behaviour 
> 
> This is not going to happen any time soon.
> 
> > > 2. Is there a plan to make existing intranet-side view using the REST API?
> > > ie. only 1 code to maintain.
> > > 
> > 
> > Once we can replicate current intranet behaviour, we can think on changing
> > intranet-side
> 
> So waiting for tables to be merged?

Basically yes.. seeing that bug 20271 was moving, I suspected that it was going to happen in a reasonable time.

Is it far from happening?, what would you suggest?

Thanks
Comment 57 Jonathan Druart 2020-04-02 11:22:25 UTC
Some more QA comments:

1. Enabled by default?
At least it should not in the upgrade.

2. Users can still access their holds history using the REST API even if the pref is off, is that expected?

3. in controller script:

 55 my $borr = Koha::Patrons->find( $borrowernumber )->unblessed;
 56 
 57 $template->param(%{$borr});

Sure we need that?

4. Too many use statements in controller script

5. return (data&&$date(data))||'';
Should be removed (see one of the follow-ups on bug 24980).

6.
koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc:                [% IF ( OPACPrivacy || Koha.Preference('OPACHoldsPrivacy') == 1 ) %]

OPACHoldsPrivacy is coming from nowhere?

7. why do you need koha_objects_class for items and biblios in the schema? You should not.

8. DBIC schema adds FK on old_reserves, that's wrong.

9. Even if Signed off already, please squash some patches when needed (rename of the script for instance).

(In reply to Agustín Moyano from comment #56)
> Basically yes.. seeing that bug 20271 was moving, I suspected that it was
> going to happen in a reasonable time.
> 
> Is it far from happening?, what would you suggest?

Bug 20271 is far from happening, cannot imagine reserves/old_reserves...
Comment 58 Tomás Cohen Arazi 2020-04-02 11:47:18 UTC
(In reply to Jonathan Druart from comment #57)
> 
> 7. why do you need koha_objects_class for items and biblios in the schema?
> You should not.

Take a look at Koha::Object->prefetch_whitelist, it is used to recursively embed objects.
Comment 59 Jonathan Druart 2020-04-02 11:54:59 UTC
(In reply to Tomás Cohen Arazi from comment #58)
> (In reply to Jonathan Druart from comment #57)
> > 
> > 7. why do you need koha_objects_class for items and biblios in the schema?
> > You should not.
> 
> Take a look at Koha::Object->prefetch_whitelist, it is used to recursively
> embed objects.

Koha::Object->_get_object_class should be called instead.
Comment 60 Jonathan Druart 2020-04-02 13:48:17 UTC
Use of union could be a solution, but I don't think it could be implemented easily, see bug 25043 for an example using DBIx::Class::ResultSet

https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::SetOperations
Comment 61 Jonathan Druart 2020-04-02 14:05:28 UTC
(In reply to Jonathan Druart from comment #57)
> 7. why do you need koha_objects_class for items and biblios in the schema?
> You should not.

See bug 25044.
Comment 62 Jonathan Druart 2020-04-02 14:47:45 UTC
(In reply to Jonathan Druart from comment #60)
> Use of union could be a solution, but I don't think it could be implemented
> easily, see bug 25043 for an example using DBIx::Class::ResultSet
> 
> https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::SetOperations

If that does not work (or hard to implement), did you try to create a view?
Comment 63 Tomás Cohen Arazi 2020-04-29 14:18:15 UTC
Created attachment 103948 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 64 Tomás Cohen Arazi 2020-04-29 14:18:30 UTC
Created attachment 103949 [details] [review]
Bug 20936: Add required relations for the OldReserve schema

In order to make it work as the Reserve schema, we need those relations
defined so we can fetch related object (e.g items).
Comment 65 Tomás Cohen Arazi 2020-04-29 14:18:40 UTC
Created attachment 103950 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC. Right now, it only shows records from old_reserves table, but I'll wait till bug 20271 is pushed to show full history (old and new holds)

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds canceled or fulfilled
6. Filter, order and change page
SUCCESS => All controls work as expected
7. Sign off.

Table content is fetched from the api. If you see data, and you can order and filter then please sign off bug 24561.

Date columns use $date function to transform dates strings from api (for example '2020-02-20') to 'dateformat' prefernce format ('02/20/20202'). If you change dateformat prefernce and see the changes reflected in date columns, please sign off bug 24455.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 66 Tomás Cohen Arazi 2020-04-29 14:18:45 UTC
Created attachment 103951 [details] [review]
Bug 20936: Add API tests

Add test to /api/v1/public/patrons/:patron_id/holds endpoint

To test:
1. prove t/db_dependent/api/v1/patrons_holds.t
Comment 67 Tomás Cohen Arazi 2020-04-29 14:34:18 UTC
(In reply to Jonathan Druart from comment #57)
> Some more QA comments:
> 
> 1. Enabled by default?
> At least it should not in the upgrade.

Solved.

> 2. Users can still access their holds history using the REST API even if the
> pref is off, is that expected?

I think it is, the syspref is about visibility.

> 5. return (data&&$date(data))||'';
> Should be removed (see one of the follow-ups on bug 24980).

What is the status of that bug and why is it still In discussion?

> 7. why do you need koha_objects_class for items and biblios in the schema?
> You should not.

This is fixed.

> 8. DBIC schema adds FK on old_reserves, that's wrong.

It adds relationships. There's nothing wrong about it, They are used in Koha::Objects->to_api to embed related objects.

> 9. Even if Signed off already, please squash some patches when needed
> (rename of the script for instance).

Done
Comment 68 Josef Moravec 2020-04-30 11:59:00 UTC
Comment on attachment 103950 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

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

::: Koha/REST/V1/Patrons/Holds.pm
@@ +61,5 @@
> +        my $holds     = $c->objects->search( $holds_set );
> +        return $c->render( status => 200, openapi => $holds );
> +    }
> +    catch {
> +        $c->unhandlede_exception($_);

typo, should be unhandled_excecption

::: koha-tmpl/opac-tmpl/bootstrap/en/includes/usermenu.inc
@@ +58,4 @@
>                  <a href="/cgi-bin/koha/opac-search-history.pl">your search history</a></li>
>              [% END %]
>  
> +            [% IF opacreadinghistory || Koha.Preference('OPACHoldsHistory') == 1 %]

You don't need this combined condition

@@ +77,2 @@
>                  [% END %]
> +                [% IF ( OPACPrivacy || Koha.Preference('OPACHoldsPrivacy') == 1 ) %]

the OPACHoldsPrivacy does not exist, this change should be reverted

::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-holdshistory.tt
@@ +3,5 @@
> +[% USE KohaDates %]
> +[% INCLUDE 'doc-head-open.inc' %]
> +<title>[% IF ( LibraryNameTitle ) %][% LibraryNameTitle | html %][% ELSE %]Koha online[% END %] catalog &rsaquo; Your holds history</title>
> +[% INCLUDE 'doc-head-close.inc' %]
> +[% BLOCK cssinclude %]

CSS should be in own file and added by Asset plugin

@@ +118,5 @@
> +[% INCLUDE 'datatables.inc' %]
> +[% INCLUDE 'js-date-format.inc' %]
> +<script>
> +    $(document).ready(function(){
> +        [% IF ( GoogleJackets ) %]KOHA.Google.GetCoverFromIsbn();[% END %]

Why Google Jackets and not other services?

::: opac/opac-holdshistory.pl
@@ +33,5 @@
> +use Koha::Patrons;
> +
> +use Koha::ItemTypes;
> +use Koha::Ratings;
> +

You don't need most of these imports
Comment 69 Tomás Cohen Arazi 2020-04-30 14:29:41 UTC
Created attachment 104040 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC. Right now, it only shows records from old_reserves table, but I'll wait till bug 20271 is pushed to show full history (old and new holds)

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds canceled or fulfilled
6. Filter, order and change page
SUCCESS => All controls work as expected
7. Sign off.

Table content is fetched from the api. If you see data, and you can order and filter then please sign off bug 24561.

Date columns use $date function to transform dates strings from api (for example '2020-02-20') to 'dateformat' prefernce format ('02/20/20202'). If you change dateformat prefernce and see the changes reflected in date columns, please sign off bug 24455.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 70 Tomás Cohen Arazi 2020-04-30 14:29:49 UTC
Created attachment 104041 [details] [review]
Bug 20936: Add API tests

Add test to /api/v1/public/patrons/:patron_id/holds endpoint

To test:
1. prove t/db_dependent/api/v1/patrons_holds.t
Comment 71 Tomás Cohen Arazi 2020-04-30 14:33:22 UTC
(In reply to Josef Moravec from comment #68)
> Comment on attachment 103950 [details] [review] [review]
> Bug 20936: Add patron's hold history menu in OPAC
> 
> Review of attachment 103950 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Patrons/Holds.pm
> @@ +61,5 @@
> > +        my $holds     = $c->objects->search( $holds_set );
> > +        return $c->render( status => 200, openapi => $holds );
> > +    }
> > +    catch {
> > +        $c->unhandlede_exception($_);
> 
> typo, should be unhandled_excecption

Done!

> @@ +118,5 @@
> > +[% INCLUDE 'datatables.inc' %]
> > +[% INCLUDE 'js-date-format.inc' %]
> > +<script>
> > +    $(document).ready(function(){
> > +        [% IF ( GoogleJackets ) %]KOHA.Google.GetCoverFromIsbn();[% END %]
> 
> Why Google Jackets and not other services?

That didn't make any sense, I cleaned it.

> ::: opac/opac-holdshistory.pl
> @@ +33,5 @@
> > +use Koha::Patrons;
> > +
> > +use Koha::ItemTypes;
> > +use Koha::Ratings;
> > +
> 
> You don't need most of these imports

Cleaned the .pl on behalf of Agustin.
Comment 72 Agustín Moyano 2020-04-30 15:08:43 UTC
(In reply to Tomás Cohen Arazi from comment #71)
> (In reply to Josef Moravec from comment #68)
> > Comment on attachment 103950 [details] [review] [review] [review]
> > Bug 20936: Add patron's hold history menu in OPAC
> > 
> > Review of attachment 103950 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: Koha/REST/V1/Patrons/Holds.pm
> > @@ +61,5 @@
> > > +        my $holds     = $c->objects->search( $holds_set );
> > > +        return $c->render( status => 200, openapi => $holds );
> > > +    }
> > > +    catch {
> > > +        $c->unhandlede_exception($_);
> > 
> > typo, should be unhandled_excecption
> 
> Done!
> 
> > @@ +118,5 @@
> > > +[% INCLUDE 'datatables.inc' %]
> > > +[% INCLUDE 'js-date-format.inc' %]
> > > +<script>
> > > +    $(document).ready(function(){
> > > +        [% IF ( GoogleJackets ) %]KOHA.Google.GetCoverFromIsbn();[% END %]
> > 
> > Why Google Jackets and not other services?
> 
> That didn't make any sense, I cleaned it.
> 
> > ::: opac/opac-holdshistory.pl
> > @@ +33,5 @@
> > > +use Koha::Patrons;
> > > +
> > > +use Koha::ItemTypes;
> > > +use Koha::Ratings;
> > > +
> > 
> > You don't need most of these imports
> 
> Cleaned the .pl on behalf of Agustin.

Thanks!
Comment 73 Agustín Moyano 2020-05-05 16:08:18 UTC
(In reply to Jonathan Druart from comment #60)
> Use of union could be a solution, but I don't think it could be implemented
> easily, see bug 25043 for an example using DBIx::Class::ResultSet
> 
> https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::SetOperations

Throwing the towel here, at least untill bug 25260 gets pushed..

union cannot be used here, because it doesn't accept prefetch of tables, and we need to filter and order by prefetched tables, and making a view with the union of both tables won't work either, because it hasn't got relationships with other tables.

I'll have to put the logic in the pl file and wait till reserves/old_reserves get merged properly.
Comment 74 Tomás Cohen Arazi 2020-05-05 17:09:26 UTC
(In reply to Agustín Moyano from comment #73)
> (In reply to Jonathan Druart from comment #60)
> > Use of union could be a solution, but I don't think it could be implemented
> > easily, see bug 25043 for an example using DBIx::Class::ResultSet
> > 
> > https://metacpan.org/pod/DBIx::Class::Helper::ResultSet::SetOperations
> 
> Throwing the towel here, at least untill bug 25260 gets pushed..
> 
> union cannot be used here, because it doesn't accept prefetch of tables, and
> we need to filter and order by prefetched tables, and making a view with the
> union of both tables won't work either, because it hasn't got relationships
> with other tables.
> 
> I'll have to put the logic in the pl file and wait till
> reserves/old_reserves get merged properly.

You did a great job anyways, Agustín. Don't let this put you down!
We'll finish this and more with the API really soon!
Comment 75 Agustín Moyano 2020-05-06 23:09:26 UTC
Created attachment 104466 [details] [review]
Bug 20936: Add OPACHoldsHistory syspref

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 76 Agustín Moyano 2020-05-06 23:09:33 UTC
Created attachment 104467 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC.

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds
6. Change order, and list limit
SUCCESS => All controls work as expected
7. Sign off.
Comment 77 ByWater Sandboxes 2020-05-20 00:15:54 UTC
Created attachment 105131 [details] [review]
Bug 20936: Add patron's hold history menu in OPAC

This patch adds patron's hold history in OPAC.

To test:
1. apply this patch
2. Find a patron, place several holds and cancel or fulfill them
3. Go to patron's opac
CHECK => There is no 'your holds history' option in menu
4. In admin preferences enable OPACHoldsHistory
5. Go back to patron's opac
SUCCESS => There is a 'your holds history' menu option
        => Holds history displays all holds
6. Change order, and list limit
SUCCESS => All controls work as expected
7. Sign off.

Signed-off-by: Todd <tgoatley@gmail.com>
Comment 78 Katrin Fischer 2020-07-05 01:19:15 UTC
Can you please check the QA tools failure?

 FAIL	koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-holdshistory.tt
   FAIL	  valid_template
		ColumnsSettings: plugin not found