Currently when a staff member is viewing a list of ILL requests, it is only possible to sort the results by column. This bug proposes providing the ability to filter the ILL request list via values displayed in the table columns.
The work done in this bug will need to be revisited once bug 20581 makes it into master as the filters will need to be adapted to allow for the status alias that requests can have.
Created attachment 74640 [details] [review] Bug 20600: API - Format dates according to syspref Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed" & "updated" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4
Created attachment 74641 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy.
There is a problem with the status_name variable. The error in the Chromium console is this: Uncaught ReferenceError: status_name is not defined at getStatusName (ill-requests.pl:674) at createStatus (ill-requests.pl:649) at jquery.dataTables.min.js:18 at Object.b.fnGetData (jquery.dataTables.min.js:12) at B (jquery.dataTables.min.js:16) at Ia (jquery.dataTables.min.js:14) at mb (jquery.dataTables.min.js:66) at T (jquery.dataTables.min.js:30) at ia (jquery.dataTables.min.js:48) at HTMLTableElement.<anonymous> (jquery.dataTables.min.js:95) status_name is defined on line 281, and used again on line 309: 277 // Render function for request status 278 var createStatus = function(data, type, row, meta) { 279 var origData = meta.settings.oInit.originalData; 280 if (origData.length > 0) { 281 var status_name = meta.settings.oInit.originalData[0].capabilities[ 282 row.status 283 ].name; 284 return getStatusName(status_name); 285 } else { 286 return ''; 287 } 288 }; 289 290 var getStatusName = function(origName) { 291 switch( origName ) { 292 case "New request": 293 return _("New request"); 294 case "Requested": 295 return _("Requested"); 296 case "Requested from partners": 297 return _("Requested from partners"); 298 case "Request reverted": 299 return _("Request reverted"); 300 case "Queued request": 301 return _("Queued request"); 302 case "Cancellation requested": 303 return _("Cancellation requested"); 304 case "Completed": 305 return _("Completed"); 306 case "Delete request": 307 return _("Delete request"); 308 default: 309 return status_name; 310 } 311 }; I think it should either be declared outside the function that starts on line 278, or status_name on line 309 should really be origName? Also, there is a mismatch between some of the names used in the "Filters" box and those used in the table: Date modified = Updated on Pickup branch = Library(?) Borrower card number = Patron barcode This will be a really good enhancement!
Good spot Magnuse, thanks for that. I'll submit a replacement patch to fix this.
Created attachment 75202 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4
Created attachment 75276 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh!
1. Do not use loop variable $_ + foreach(@req_list) { + foreach my $field(@format_dates) { + if (defined $_->{$field}) { + $_->{$field . "_formatted"} = format_sqldatetime( 2. Why do not you use the datepicker plugin as we use it elsewhere? :) 3. For dates in datatables we usually do not use it that way, search for "title-string" in the *.tt files I am not sure you need the 2 *_formatted variables. 4. Is it really useful to provide only a specific dates and not a date range?
Created attachment 75734 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop
Created attachment 75735 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required
Hi Jonathan Thanks for your feedback. 1. OK, addressed in https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75734 2. OK, addressed in https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=75735 3. Thanks for the pointer towards title-string. I have used the *_formatted fields to allow me to display the date fields in the datatable according to the date format set in the sysprefs. Since the data is coming in to the datatable from an API call, I'm not sure how else I can do it. $KohaDates obviously won't work, I couldn't get title-string to either, though, having done some research, it appears it is used to control sorting, rather than display formatting. 4. Good point, I'll create a follow-up patch and submit it
There is a date range filter example in serials/claims.tt
Created attachment 75753 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that.
Hi Jonathan I've now added a follow-up patch to add the date range filtering. I ended up not following the example you cited, it appears to take a very manual approach of hiding all table rows and then showing them depending on whether they pass the filter test. Doing some research, Datatables has an API that allows custom filters to be defined, it felt cleaner to do this the proper Datatables way. Anyway, thanks for your help on this! :)
Created attachment 75755 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that.
I had not time to test, but wondering if this will work with our various dateformat settings? Adding Owen to take a look :)
(In reply to Katrin Fischer from comment #16) > I had not time to test, but wondering if this will work with our various > dateformat settings? Filtering by date didn't work for me when the dateformat preference was set to dmydot or metric.
Created attachment 75844 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison.
Oooh, good spot Owen, thanks! I've just attached a follow-up patch that should address this.
Created attachment 75845 [details] [review] Bug 20600: (follow-up) Remove console.log!
Created attachment 75848 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 75849 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 75850 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 75851 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 75852 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 75853 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 75854 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected.
Created attachment 75871 [details] [review] Bug 20600: (sign-off) Move template JavaScript to the footer Works great, thanks very much for that Owen!
Well, yes it works, but I do not think it is the way to do. I still think we should use the standard title-string sort method. I am not going to consider it as blocker anyway.
Created attachment 77691 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77692 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77693 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77694 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77695 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77696 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 77697 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected. Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
I've rebased these patches and moved the changes in staff-global.css to staff-global.scss. Staff client CSS will have to be recompiled for testing.
Created attachment 78439 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78440 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78441 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78442 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78443 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78444 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78445 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78446 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected. Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 78463 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78464 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78465 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78466 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78467 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78468 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 78469 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected. Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 78470 [details] [review] Bug 20600: (follow-up) Fix bad rebase Rebasing didn't do the right thing, so this patch fixes it
Created attachment 79183 [details] [review] Bug 20600: (follow-up) Fix bad rebase Rebasing didn't do the right thing, so this patch fixes it Signed-off-by: Owen Leonard <oleonard@myacpl.org>
*** Bug 18590 has been marked as a duplicate of this bug. ***
Created attachment 80468 [details] [review] Bug 20600: (follow-up) Fix bad rebase
Comment on attachment 78463 [details] [review] Bug 20600: Add filtering of ILL requests in list Review of attachment 78463 [details] [review]: ----------------------------------------------------------------- ::: Koha/REST/V1/Illrequests.pm @@ +65,5 @@ > } > + > + # Create new "formatted" columns for each date column > + # that needs formatting > + foreach(@req_list) { I do not like this. The date should not be formatted on API level. It should be left to client/UI.
(In reply to Josef Moravec from comment #57) > Comment on attachment 78463 [details] [review] [review] > Bug 20600: Add filtering of ILL requests in list > > Review of attachment 78463 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/REST/V1/Illrequests.pm > @@ +65,5 @@ > > } > > + > > + # Create new "formatted" columns for each date column > > + # that needs formatting > > + foreach(@req_list) { > > I do not like this. The date should not be formatted on API level. It should > be left to client/UI. Thanks for this. I don't like it either. But, having spent a long time trying to figure out how, I came to the conclusion that there's no way of sanely using the date formatting syspref setting from Javascript. Hence me having to pre-format at the server end. There is a precedent for this, though I didn't know it when I implemented it in Illrequests.pm svc/checkouts does exactly the same thing in order to pre-format dates for the API response used in checkouts.js: https://github.com/PTFS-Europe/koha/blob/master/svc/checkouts#L207-L218
Created attachment 81992 [details] [review] Bug 20600: (follow-up) Rebase on current master Significant things have changed in master due to ILL enhancement patches being pushed. This patch rebases this bug on top of them
Created attachment 81993 [details] [review] Bug 20600: (follow-up) Fix errant tabs How on earth these tabs crept in I have no idea... <sigh>
Created attachment 82032 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82033 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82034 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82035 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82036 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82037 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82038 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected. Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 82039 [details] [review] Bug 20600: (follow-up) Rebase on current master Significant things have changed in master due to ILL enhancement patches being pushed. This patch rebases this bug on top of them
Created attachment 82062 [details] [review] Bug 20600: (follow-up) Restore patron name fix The fix for patron name display when no first name is specified was lost in the rebase. This patch restores it.
Created attachment 82170 [details] [review] Bug 20600: (follow-up) Fix API spec We now return placed_formatted and updated_formatted, the API spec should reflect this
Created attachment 82171 [details] [review] Bug 20600: (follow-up) Fix unit tests The unit tests now need to be aware of the calculated placed_formatted and updated_formatted properties in the API response object
Created attachment 82432 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82433 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82434 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82435 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82436 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82437 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 82438 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected. Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com>
Created attachment 82439 [details] [review] Bug 20600: (follow-up) Rebase on current master Significant things have changed in master due to ILL enhancement patches being pushed. This patch rebases this bug on top of them
Created attachment 82440 [details] [review] Bug 20600: (follow-up) Restore patron name fix The fix for patron name display when no first name is specified was lost in the rebase. This patch restores it.
Created attachment 82441 [details] [review] Bug 20600: (follow-up) Fix API spec We now return placed_formatted and updated_formatted, the API spec should reflect this
Created attachment 82442 [details] [review] Bug 20600: (follow-up) Fix unit tests The unit tests now need to be aware of the calculated placed_formatted and updated_formatted properties in the API response object
Hi Andrew, here some first findings: 1) Unit tests failing: t/db_dependent/api/v1/illrequests.t .. # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/illrequests.t line 108. # Structures begin differing at: # $got->[0]{id_prefix} = '' # $expected->[0]{id_prefix} = Does not exist # Failed test 'exact match for JSON Pointer ""' # at t/db_dependent/api/v1/illrequests.t line 143. # Structures begin differing at: # $got->[0]{id_prefix} = '' # $expected->[0]{id_prefix} = Does not exist # Looks like you failed 2 tests of 18. t/db_dependent/api/v1/illrequests.t .. 1/1 # Failed test 'list() tests' # at t/db_dependent/api/v1/illrequests.t line 155. # Looks like you failed 1 test of 1. 2) QA tool complaining: FAIL Koha/REST/V1/Illrequests.pm FAIL critic # Variables::ProhibitUnusedVariables: Got 1 violation(s). I am having some issues with my yarn... so couldn't test further yet. Can you please have a look at the above?
Created attachment 83882 [details] [review] Bug 20600: (follow-up) Fix tests Some silly variable name typos in the tests
(In reply to Katrin Fischer from comment #83) > > 1) Unit tests failing: Silly mistake in the test script, latest patch fixes it > 2) QA tool complaining: > > > FAIL Koha/REST/V1/Illrequests.pm > FAIL critic > # Variables::ProhibitUnusedVariables: Got 1 violation(s). > I can't replicate this, I ran: koha-qa.pl -v 2 -c 8 and didn't get any failures. What command did you run and did the error give any indication of what the unused variable was?
Created attachment 83887 [details] [review] Bug 20600: (follow-up) Fix critic error
Sorted the critic error for you.. setting back to SO
Created attachment 84736 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84737 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84738 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84739 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84740 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84741 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84742 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected. Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84743 [details] [review] Bug 20600: (follow-up) Rebase on current master Significant things have changed in master due to ILL enhancement patches being pushed. This patch rebases this bug on top of them Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84744 [details] [review] Bug 20600: (follow-up) Restore patron name fix The fix for patron name display when no first name is specified was lost in the rebase. This patch restores it. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84745 [details] [review] Bug 20600: (follow-up) Fix API spec We now return placed_formatted and updated_formatted, the API spec should reflect this Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84746 [details] [review] Bug 20600: (follow-up) Fix unit tests The unit tests now need to be aware of the calculated placed_formatted and updated_formatted properties in the API response object Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84747 [details] [review] Bug 20600: (follow-up) Fix tests Some silly variable name typos in the tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84748 [details] [review] Bug 20600: (follow-up) Fix critic error Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 84749 [details] [review] Bug 20600: (QA follow-up) Fix terminology and capitalization - Subject Line > Subject line - Patron barcode > Cardnumber
Hi Andrew, can you please rebase? One issue was from me adding the price filter, but was not sure about the second, so it would be great if you could take a look.
Even with the revert this still doesn't apply for me?
It's the same issue with the datatables code I had before it looks like, maybe the trouble was not the price patch? Please check.
Created attachment 85037 [details] [review] Bug 20600: Add filtering of ILL requests in list This patch adds the ability for the ILL request list in the staff interface to be filtered by a number of criteria: - Status - Date placed - Date modified - Pickup branch - Borrower card number To test: 1) Apply patch 2) Ensure you have a reasonable range of ILL requests created 3) Navigate to the "View ILL requests" page 4) Choose one or more filtering criteria 5) Click "Search" 6) Observe the results are filtered to match the selected criteria 7) Click "Clear" 8) Observe your results are returned to their initial state 9) Repeat steps 4 -> 6 until you are happy. Dates supplied by the /api/v1/illrequests API route were not conforming to the preference specified by the dateformat syspref. This patch addresses that. It has been addressed as part of this bug since we are adding filtering of requests by some date fields and, therefore, needed dates in a predictable format. To test: 1) Apply the patch 2) Ensure you have at least one ILL request created 3) Make a request to the /api/v1/illrequests endpoint 4) Observe that dates supplied for "placed_formatted" & "updated_formatted" conform to your dateformat syspref. 5) Change your date format syspref, repeat steps 3 & 4 Signed-off-by: Magnus Enger <magnus@libriotech.no> Works as advertised. Nice enh! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85038 [details] [review] Bug 20600: (follow-up) Do not use $_ in loop Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85039 [details] [review] Bug 20600: (follow-up) Remove date formatting It is not required Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85040 [details] [review] Bug 20600: (follow-up) Add date range filtering It makes more sense to be able to filter placed and updated dates by range. This patch adds that. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85041 [details] [review] Bug 20600: (follow-up) Fix filtering There was a bug whereby filtering by date would fail if the selected date syspref was set to dmydot or metric. Rather than use the display date value, We now use the non-display date value that is always in a predictable ISO8601 format to construct the Date objects used for comparison. Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85042 [details] [review] Bug 20600: (follow-up) Remove console.log! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85043 [details] [review] Bug 20600: (follow-up) Move template JavaScript to the footer This patch moves the ILL requests template's JavaScript to the footer. Also changed: - Removed unused jQuery checkboxes plugin - Some <input> tags have been corrected to be self-closing. - Title tag has been corrected - Table row limit of 10 has been removed in favor of accepting default used elsewhere. To test, apply the patch and follow the original test plan for this bug. Everything should work as expected. Signed-off-by: Andrew Isherwood <andrew.isherwood@ptfs-europe.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85044 [details] [review] Bug 20600: (follow-up) Rebase on current master Significant things have changed in master due to ILL enhancement patches being pushed. This patch rebases this bug on top of them Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85045 [details] [review] Bug 20600: (follow-up) Restore patron name fix The fix for patron name display when no first name is specified was lost in the rebase. This patch restores it. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85046 [details] [review] Bug 20600: (follow-up) Fix API spec We now return placed_formatted and updated_formatted, the API spec should reflect this Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85047 [details] [review] Bug 20600: (follow-up) Fix unit tests The unit tests now need to be aware of the calculated placed_formatted and updated_formatted properties in the API response object Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85048 [details] [review] Bug 20600: (follow-up) Fix tests Some silly variable name typos in the tests Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85049 [details] [review] Bug 20600: (follow-up) Fix critic error Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 85050 [details] [review] Bug 20600: (QA follow-up) Fix terminology and capitalization - Subject Line > Subject line - Patron barcode > Cardnumber
Hi Katrin - I've now fixed the merge conflict. It should apply cleanly
Awesome work all! Pushed to master for 19.05
Enhancement, will not be backported to 18.11.x series.