|
Description
Ed Veal
2018-06-13 17:14:45 UTC
*** Bug 23142 has been marked as a duplicate of this bug. *** 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 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. Created attachment 98629 [details] [review] Bug 20936: Add OPACHoldsHistory syspref 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. 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 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. Augustin - it's because it's ASSIGNED I think - not NSO or SO. 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 Hi Kyle, fixed an issue in bug 24561... now it should work. Please try again. Thanks (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. Created attachment 100115 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 100116 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> This is not ready at all, diff looks wrong, please double check the patch. And tests, pretty please. 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 (In reply to Jonathan Druart from comment #16) > And tests, pretty please. How can I refuse if you ask so nicely XD (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 Created attachment 100205 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> 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 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. Created attachment 100471 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 100472 [details] [review] Bug 20936: Add koha_object_class and koha_objects_class methods to Schema classes 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> 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 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! Currently blocked by bug 24455 - please reset once dependencies have cleared up! (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. 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. 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 Created attachment 101724 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 101725 [details] [review] Bug 20936: Add koha_object_class and koha_objects_class methods to Schema classes 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> 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 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. 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. (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 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 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. 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 (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. Totally makes sense.. your right. 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 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. Moved followup to bug 24980 With the new dependency added I believe we are unblocked again here.. resetting to signed off. Just spotted.. we need to add the `Sponsored-by: ` Line into at least one of the patches for the release notes ;) Created attachment 101915 [details] [review] Bug 20936: (follow-up) use modified by bug 24980, fix display of page info and fix search without enter 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 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 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 """ > (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? (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 (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 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...
(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. (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. 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 (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. (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? Created attachment 103948 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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). 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> 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 (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 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 › 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 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> 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 (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. (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! (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. (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! Created attachment 104466 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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. 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> 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 Created attachment 112667 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 112668 [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> Created attachment 112730 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112731 [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> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112732 [details] [review] Bug 20936: (QA follow-up) Remove OPACHoldsPrivacy reference Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 112733 [details] [review] Bug 20936: (QA follow-up) Use Branches template plugin Signed-off-by: Nick Clemens <nick@bywatersolutions.com> What happened between May (comment 74) and now? Did we fix all the things we needed? (In reply to Jonathan Druart from comment #85) > What happened between May (comment 74) and now? Did we fix all the things we > needed? Ok, got it, we are not using the REST API. The controller script is weird. We are retrieving all the holds and then limit the size of the array. We will face performance issues without the benefit (having the info on the UI). Erk, and you have to, because of the sort option. What about: 1. display all if <= X (50, 100?) 2. If >X then we remove the sort? That's ugly but.. I am lacking imagination right now. What about: 1. display all if <= X (50, 100?) 2. If >X then we only display current holds with an option (checkbox) to display old ones as well? (In reply to Jonathan Druart from comment #85) > What happened between May (comment 74) and now? Did we fix all the things we > needed? We dropped the development using the API, and moved back to doing the old .pl way. And there were no remaining issues, besides the failure with the QA scripts Katrin reported. Can I get an answer on my last 2 comments please? (In reply to Jonathan Druart from comment #86) > (In reply to Jonathan Druart from comment #85) > > What happened between May (comment 74) and now? Did we fix all the things we > > needed? > > Ok, got it, we are not using the REST API. > > The controller script is weird. We are retrieving all the holds and then > limit the size of the array. We will face performance issues without the > benefit (having the info on the UI). We are retrieving all holds from a single patron. The controller looks weird because it grabs current holds and old holds, join and sort them and then limit the result.. When I coded this I could not use client side pagination, because unlike staff interface, there are no css classes prepared for datatables controles and it looked pretty ugly. I do not believe we'll face performance issues.. even if a patron placed 500 holds, penalties would be marginal. Created attachment 113446 [details] [review] Bug 20936: Add OPACHoldsHistory syspref Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 113447 [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> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 113448 [details] [review] Bug 20936: (QA follow-up) Use Branches template plugin Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Created attachment 113449 [details] [review] Bug 20936: (follow-up) add biblio and item relation to old holds and set a limit on search holds Pushed to master for 20.11, thanks to everybody involved! enhancement will not be backported t0 20.05.x "your holds history" tab label doesn't appear on translate yet? + var table = $("table_holdshistory").dataTable($.extend(true, {}, dataTablesDefaults, {
The selector is wrong, must be $("#table_holdshistory")
|
