Some issues I see:

- checkouts.js contains English strings. They're escaped for translation, but the translation script doesn't touch js file at all. Translatable strings must be in a .tt or .inc file.

- Does this eliminate use of the UseTablesortForCirc system preference altogether? If so it should be removed.

- I disagree that submit buttons for circulation actions need to be restyled with Bootstrap styles and icons. I'm in favor of pretty conservative use of button icons. I don't think they lend added meaning in this case, just visual clutter.

- This patch eliminates the separation of today's checkouts from previous days' checkouts. I don't think that's a good thing. I think maintaining that separation is important to the way the circulation staff does their job.

- It looks like tables don't load with any particular sort by default? I would like to see it stick with the previous behavior of default sorting.

- I think it's problematic that the "processing" overlay obscures the page, especially on the patron detail page where there is more to obscure. Can we get some kind of loading indicator within the table itself instead? That would give a clearer indication to the user what they're waiting for.

Overall this looks like it works well, but I'm concerned that the performance hit we get for full page render, including tables. I tested with a patron who had only 10 checkouts and the AJAX version was a few extra seconds to fully load the table of checkouts.

Testing a patron with 250 checkouts I found the time to page render with non-AJAX was about 20 seconds, time with AJAX was 45 seconds.

On the other hand full page render is only part of what the staff is looking for with this page. We also want to see reduced time between one item barcode scan and the next. In my tests with the 250-checkout patron I found the AJAX response time to be mixed, with some faster than standard and some not. I think this requires more testing.

One worrisome thing: A couple of times in my test submitting a barcode in the AJAX version resulted in me getting kicked back to a blank checkout screen as if I had submitted a blank barcode. Other testers please watch for that.

I'm marking Failed QA so that those bullet points can be considered, but I hope others will continue to test this.

Also: A couple of fails from the QA test tool but looks like it's just GPL version issues.

(In reply to Owen Leonard from comment #9)
> Some issues I see:
> 
> - checkouts.js contains English strings. They're escaped for translation,
> but the translation script doesn't touch js file at all. Translatable
> strings must be in a .tt or .inc file.

Noted
 
> - Does this eliminate use of the UseTablesortForCirc system preference
> altogether? If so it should be removed.

Yep, I had forgot to take care of that.

> - I disagree that submit buttons for circulation actions need to be restyled
> with Bootstrap styles and icons. I'm in favor of pretty conservative use of
> button icons. I don't think they lend added meaning in this case, just
> visual clutter.

Fair enough. I feel like they have better "weight" considering every other button on the page has an icon, but that's just my personal opinion. I can remove the icon.

> - This patch eliminates the separation of today's checkouts from previous
> days' checkouts. I don't think that's a good thing. I think maintaining that
> separation is important to the way the circulation staff does their job.

I was waiting to see if this would come up. I'm not surprised. I should be able to add that back in.

> - It looks like tables don't load with any particular sort by default? I
> would like to see it stick with the previous behavior of default sorting.

The default sort is issuedate descending, or at least it should be!

> - I think it's problematic that the "processing" overlay obscures the page,
> especially on the patron detail page where there is more to obscure. Can we
> get some kind of loading indicator within the table itself instead? That
> would give a clearer indication to the user what they're waiting for.

I completely agree. We can easily turn it off, but it would be much better to have it float over top the table itself.

> Overall this looks like it works well, but I'm concerned that the
> performance hit we get for full page render, including tables. I tested with
> a patron who had only 10 checkouts and the AJAX version was a few extra
> seconds to fully load the table of checkouts.
> 
> Testing a patron with 250 checkouts I found the time to page render with
> non-AJAX was about 20 seconds, time with AJAX was 45 seconds.
> 
> On the other hand full page render is only part of what the staff is looking
> for with this page. We also want to see reduced time between one item
> barcode scan and the next. In my tests with the 250-checkout patron I found
> the AJAX response time to be mixed, with some faster than standard and some
> not. I think this requires more testing.

I'm a bit surprised by your results. Mine has showed a steady improvement. A patron with 200+ checkouts went from 40 seconds to 1.5 seconds for rendering. The fact that you had results switching sides is interesting. There must be something else going on. However, the primary purpose of this patch is to improve the responsiveness of the circulation system, rather than the overall page load time. The total time to load including the checkouts table will likely be slightly higher, but the I think the increased speed at which librarians will be able to check out books will be well worth it. In addition, we can later tune the ajax script to be as fast as possible. I've found that about half the time is spent on GetIssuingCharges, CanBookBeRenewed, and GetRenewCount.
 
> One worrisome thing: A couple of times in my test submitting a barcode in
> the AJAX version resulted in me getting kicked back to a blank checkout
> screen as if I had submitted a blank barcode. Other testers please watch for
> that.

That is odd, I haven't encountered that particular issue.

> I'm marking Failed QA so that those bullet points can be considered, but I
> hope others will continue to test this.

Agreed! Keep on testing!

Kyle, we already have the svc dir, which is our restful api, could we put these files there instead of a new api dir?

(In reply to Chris Cormack from comment #21)
> Kyle, we already have the svc dir, which is our restful api, could we put
> these files there instead of a new api dir?

Excellent point. I'll add a followup for that.

This patch works for me well, but I dislike it's size: since code was moving from api to svc patch is very complex to review (I did read through it).

I would prefer is we can squash it somewhat before it gets into master, but if nobody else objects on it I will sign it off as-is.

Aside from cosmetic notes, I have funny smell about this patch.

When I open the page, I can see 6 ajax requests which seem to return same JSON data. First three of them return 302 and other three return 200.

However, only difference is in sEcho parameter for which I can't find use in code itself. My worry is that we are creating too much unneeded requests to the server.

Below are relevant lines from access log:

193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET /cgi-bin/koha/svc/checkouts.pl?sEcho=1&iColumns=12&sColumns=&iDisplayStart=0&iDisplayLength=-1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnumber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=function&mDataProp_11=function&sSearch=&bRegex=false&sSearch_0=&bRegex_0=false&bSearchable_0=true&sSearch_1=&bRegex_1=false&bSearchable_1=true&sSearch_2=&bRegex_2=false&bSearchable_2=true&sSearch_3=&bRegex_3=false&bSearchable_3=true&sSearch_4=&bRegex_4=false&bSearchable_4=true&sSearch_5=&bRegex_5=false&bSearchable_5=true&sSearch_6=&bRegex_6=false&bSearchable_6=true&sSearch_7=&bRegex_7=false&bSearchable_7=true&sSearch_8=&bRegex_8=false&bSearchable_8=true&sSearch_9=&bRegex_9=false&bSearchable_9=true&sSearch_10=&bRegex_10=false&bSearchable_10=true&sSearch_11=&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSortDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1" 302 558
193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET /cgi-bin/koha/svc/checkouts.pl?sEcho=2&iColumns=12&sColumns=&iDisplayStart=0&iDisplayLength=-1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnumber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=function&mDataProp_11=function&sSearch=&bRegex=false&sSearch_0=&bRegex_0=false&bSearchable_0=true&sSearch_1=&bRegex_1=false&bSearchable_1=true&sSearch_2=&bRegex_2=false&bSearchable_2=true&sSearch_3=&bRegex_3=false&bSearchable_3=true&sSearch_4=&bRegex_4=false&bSearchable_4=true&sSearch_5=&bRegex_5=false&bSearchable_5=true&sSearch_6=&bRegex_6=false&bSearchable_6=true&sSearch_7=&bRegex_7=false&bSearchable_7=true&sSearch_8=&bRegex_8=false&bSearchable_8=true&sSearch_9=&bRegex_9=false&bSearchable_9=true&sSearch_10=&bRegex_10=false&bSearchable_10=true&sSearch_11=&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSortDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1" 302 558
193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET /cgi-bin/koha/svc/checkouts.pl?sEcho=3&iColumns=12&sColumns=&iDisplayStart=0&iDisplayLength=-1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnumber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=function&mDataProp_11=function&sSearch=&bRegex=false&sSearch_0=&bRegex_0=false&bSearchable_0=true&sSearch_1=&bRegex_1=false&bSearchable_1=true&sSearch_2=&bRegex_2=false&bSearchable_2=true&sSearch_3=&bRegex_3=false&bSearchable_3=true&sSearch_4=&bRegex_4=false&bSearchable_4=true&sSearch_5=&bRegex_5=false&bSearchable_5=true&sSearch_6=&bRegex_6=false&bSearchable_6=true&sSearch_7=&bRegex_7=false&bSearchable_7=true&sSearch_8=&bRegex_8=false&bSearchable_8=true&sSearch_9=&bRegex_9=false&bSearchable_9=true&sSearch_10=&bRegex_10=false&bSearchable_10=true&sSearch_11=&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSortDir_0=desc&iSortCol_1=0&sSortDir_1=desc&iSortingCols=2&bSortable_0=true&bSortable_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1" 302 564
193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET /intranet-tmpl/prog/img/ascdesc.gif HTTP/1.1" 304 -
193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET /cgi-bin/koha/svc/checkouts.pl?sEcho=1&iColumns=12&iDisplayStart=0&iDisplayLength=-1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnumber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=function&mDataProp_11=function&bRegex=false&bRegex_0=false&bSearchable_0=true&bRegex_1=false&bSearchable_1=true&bRegex_2=false&bSearchable_2=true&bRegex_3=false&bSearchable_3=true&bRegex_4=false&bSearchable_4=true&bRegex_5=false&bSearchable_5=true&bRegex_6=false&bSearchable_6=true&bRegex_7=false&bSearchable_7=true&bRegex_8=false&bSearchable_8=true&bRegex_9=false&bSearchable_9=true&bRegex_10=false&bSearchable_10=true&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSortDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1" 200 1939
193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET /cgi-bin/koha/svc/checkouts.pl?sEcho=2&iColumns=12&iDisplayStart=0&iDisplayLength=-1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnumber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=function&mDataProp_11=function&bRegex=false&bRegex_0=false&bSearchable_0=true&bRegex_1=false&bSearchable_1=true&bRegex_2=false&bSearchable_2=true&bRegex_3=false&bSearchable_3=true&bRegex_4=false&bSearchable_4=true&bRegex_5=false&bSearchable_5=true&bRegex_6=false&bSearchable_6=true&bRegex_7=false&bSearchable_7=true&bRegex_8=false&bSearchable_8=true&bRegex_9=false&bSearchable_9=true&bRegex_10=false&bSearchable_10=true&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSortDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1" 200 1940
193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET /cgi-bin/koha/svc/checkouts.pl?sEcho=3&iColumns=12&iDisplayStart=0&iDisplayLength=-1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnumber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=function&mDataProp_11=function&bRegex=false&bRegex_0=false&bSearchable_0=true&bRegex_1=false&bSearchable_1=true&bRegex_2=false&bSearchable_2=true&bRegex_3=false&bSearchable_3=true&bRegex_4=false&bSearchable_4=true&bRegex_5=false&bSearchable_5=true&bRegex_6=false&bSearchable_6=true&bRegex_7=false&bSearchable_7=true&bRegex_8=false&bSearchable_8=true&bRegex_9=false&bSearchable_9=true&bRegex_10=false&bSearchable_10=true&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSortDir_0=desc&iSortCol_1=0&sSortDir_1=desc&iSortingCols=2&bSortable_0=true&bSortable_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1" 200 1940


Patch is quite huge, so it's possible that I missed something obvious, so I would really appreciate clarification on this.

Dobrica, sEcho is a parameter datatables sends when using an ajax source. It is expected that the contents of the parameter be sent back just as it was received.

Kyle

(In reply to Dobrica Pavlinusic from comment #40)
> Aside from cosmetic notes, I have funny smell about this patch.
> 
> When I open the page, I can see 6 ajax requests which seem to return same
> JSON data. First three of them return 302 and other three return 200.
> 
> However, only difference is in sEcho parameter for which I can't find use in
> code itself. My worry is that we are creating too much unneeded requests to
> the server.
> 
> Below are relevant lines from access log:
> 
> 193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET
> /cgi-bin/koha/svc/checkouts.
> pl?sEcho=1&iColumns=12&sColumns=&iDisplayStart=0&iDisplayLength=-
> 1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=
> itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnum
> ber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=
> function&mDataProp_11=function&sSearch=&bRegex=false&sSearch_0=&bRegex_0=fals
> e&bSearchable_0=true&sSearch_1=&bRegex_1=false&bSearchable_1=true&sSearch_2=&
> bRegex_2=false&bSearchable_2=true&sSearch_3=&bRegex_3=false&bSearchable_3=tru
> e&sSearch_4=&bRegex_4=false&bSearchable_4=true&sSearch_5=&bRegex_5=false&bSea
> rchable_5=true&sSearch_6=&bRegex_6=false&bSearchable_6=true&sSearch_7=&bRegex
> _7=false&bSearchable_7=true&sSearch_8=&bRegex_8=false&bSearchable_8=true&sSea
> rch_9=&bRegex_9=false&bSearchable_9=true&sSearch_10=&bRegex_10=false&bSearcha
> ble_10=true&sSearch_11=&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSor
> tDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSortable_2=true
> &bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortabl
> e_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11
> =false&borrowernumber=1 HTTP/1.1" 302 558
> 193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET
> /cgi-bin/koha/svc/checkouts.
> pl?sEcho=2&iColumns=12&sColumns=&iDisplayStart=0&iDisplayLength=-
> 1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=
> itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnum
> ber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=
> function&mDataProp_11=function&sSearch=&bRegex=false&sSearch_0=&bRegex_0=fals
> e&bSearchable_0=true&sSearch_1=&bRegex_1=false&bSearchable_1=true&sSearch_2=&
> bRegex_2=false&bSearchable_2=true&sSearch_3=&bRegex_3=false&bSearchable_3=tru
> e&sSearch_4=&bRegex_4=false&bSearchable_4=true&sSearch_5=&bRegex_5=false&bSea
> rchable_5=true&sSearch_6=&bRegex_6=false&bSearchable_6=true&sSearch_7=&bRegex
> _7=false&bSearchable_7=true&sSearch_8=&bRegex_8=false&bSearchable_8=true&sSea
> rch_9=&bRegex_9=false&bSearchable_9=true&sSearch_10=&bRegex_10=false&bSearcha
> ble_10=true&sSearch_11=&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSor
> tDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSortable_2=true
> &bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=true&bSortabl
> e_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&bSortable_11
> =false&borrowernumber=1 HTTP/1.1" 302 558
> 193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET
> /cgi-bin/koha/svc/checkouts.
> pl?sEcho=3&iColumns=12&sColumns=&iDisplayStart=0&iDisplayLength=-
> 1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=
> itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnum
> ber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=
> function&mDataProp_11=function&sSearch=&bRegex=false&sSearch_0=&bRegex_0=fals
> e&bSearchable_0=true&sSearch_1=&bRegex_1=false&bSearchable_1=true&sSearch_2=&
> bRegex_2=false&bSearchable_2=true&sSearch_3=&bRegex_3=false&bSearchable_3=tru
> e&sSearch_4=&bRegex_4=false&bSearchable_4=true&sSearch_5=&bRegex_5=false&bSea
> rchable_5=true&sSearch_6=&bRegex_6=false&bSearchable_6=true&sSearch_7=&bRegex
> _7=false&bSearchable_7=true&sSearch_8=&bRegex_8=false&bSearchable_8=true&sSea
> rch_9=&bRegex_9=false&bSearchable_9=true&sSearch_10=&bRegex_10=false&bSearcha
> ble_10=true&sSearch_11=&bRegex_11=false&bSearchable_11=true&iSortCol_0=0&sSor
> tDir_0=desc&iSortCol_1=0&sSortDir_1=desc&iSortingCols=2&bSortable_0=true&bSor
> table_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=t
> rue&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable_9=false&bS
> ortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1" 302 564
> 193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET
> /intranet-tmpl/prog/img/ascdesc.gif HTTP/1.1" 304 -
> 193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET
> /cgi-bin/koha/svc/checkouts.
> pl?sEcho=1&iColumns=12&iDisplayStart=0&iDisplayLength=-
> 1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=
> itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnum
> ber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=
> function&mDataProp_11=function&bRegex=false&bRegex_0=false&bSearchable_0=true
> &bRegex_1=false&bSearchable_1=true&bRegex_2=false&bSearchable_2=true&bRegex_3
> =false&bSearchable_3=true&bRegex_4=false&bSearchable_4=true&bRegex_5=false&bS
> earchable_5=true&bRegex_6=false&bSearchable_6=true&bRegex_7=false&bSearchable
> _7=true&bRegex_8=false&bSearchable_8=true&bRegex_9=false&bSearchable_9=true&b
> Regex_10=false&bSearchable_10=true&bRegex_11=false&bSearchable_11=true&iSortC
> ol_0=0&sSortDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSort
> able_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=tr
> ue&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&b
> Sortable_11=false&borrowernumber=1 HTTP/1.1" 200 1939
> 193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET
> /cgi-bin/koha/svc/checkouts.
> pl?sEcho=2&iColumns=12&iDisplayStart=0&iDisplayLength=-
> 1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=
> itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnum
> ber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=
> function&mDataProp_11=function&bRegex=false&bRegex_0=false&bSearchable_0=true
> &bRegex_1=false&bSearchable_1=true&bRegex_2=false&bSearchable_2=true&bRegex_3
> =false&bSearchable_3=true&bRegex_4=false&bSearchable_4=true&bRegex_5=false&bS
> earchable_5=true&bRegex_6=false&bSearchable_6=true&bRegex_7=false&bSearchable
> _7=true&bRegex_8=false&bSearchable_8=true&bRegex_9=false&bSearchable_9=true&b
> Regex_10=false&bSearchable_10=true&bRegex_11=false&bSearchable_11=true&iSortC
> ol_0=0&sSortDir_0=desc&iSortingCols=1&bSortable_0=true&bSortable_1=true&bSort
> able_2=true&bSortable_3=true&bSortable_4=true&bSortable_5=true&bSortable_6=tr
> ue&bSortable_7=false&bSortable_8=false&bSortable_9=false&bSortable_10=false&b
> Sortable_11=false&borrowernumber=1 HTTP/1.1" 200 1940
> 193.198.212.4 - - [11/Mar/2014:09:44:07 +0100] "GET
> /cgi-bin/koha/svc/checkouts.
> pl?sEcho=3&iColumns=12&iDisplayStart=0&iDisplayLength=-
> 1&mDataProp_0=function&mDataProp_1=function&mDataProp_2=function&mDataProp_3=
> itemtype&mDataProp_4=issuedate&mDataProp_5=branchname&mDataProp_6=itemcallnum
> ber&mDataProp_7=function&mDataProp_8=price&mDataProp_9=function&mDataProp_10=
> function&mDataProp_11=function&bRegex=false&bRegex_0=false&bSearchable_0=true
> &bRegex_1=false&bSearchable_1=true&bRegex_2=false&bSearchable_2=true&bRegex_3
> =false&bSearchable_3=true&bRegex_4=false&bSearchable_4=true&bRegex_5=false&bS
> earchable_5=true&bRegex_6=false&bSearchable_6=true&bRegex_7=false&bSearchable
> _7=true&bRegex_8=false&bSearchable_8=true&bRegex_9=false&bSearchable_9=true&b
> Regex_10=false&bSearchable_10=true&bRegex_11=false&bSearchable_11=true&iSortC
> ol_0=0&sSortDir_0=desc&iSortCol_1=0&sSortDir_1=desc&iSortingCols=2&bSortable_
> 0=true&bSortable_1=true&bSortable_2=true&bSortable_3=true&bSortable_4=true&bS
> ortable_5=true&bSortable_6=true&bSortable_7=false&bSortable_8=false&bSortable
> _9=false&bSortable_10=false&bSortable_11=false&borrowernumber=1 HTTP/1.1"
> 200 1940
> 
> 
> Patch is quite huge, so it's possible that I missed something obvious, so I
> would really appreciate clarification on this.

From hackfest, we got a bug on the holds tab:

<h1>Software error:</h1>
<pre>Can't locate object method &quot;effective_itemtype&quot; via package &quot;Koha::Schema::Result::Item&quot; at /home/jonathan/workspace/versions/koha_coderun/svc/holds.pl line 115.
</pre>

Oops, sorry it works with bug 11518 :)

(In reply to Kyle M Hall from comment #46)
> Created attachment 26115 [details] [review] [review]
> Bug 11703 [QA Followup 12] - Fix problem detected by koha-qa.pl

 [% INCLUDE 'datatables-strings.inc' %]
Should be removed too.

Replacement followup 12 addresses this.

(In reply to Jonathan Druart from comment #47)
> (In reply to Kyle M Hall from comment #46)
> > Created attachment 26115 [details] [review] [review] [review]
> > Bug 11703 [QA Followup 12] - Fix problem detected by koha-qa.pl
> 
>  [% INCLUDE 'datatables-strings.inc' %]
> Should be removed too.

When renewing item, message (1 of 2 renewals remaining) doesn't update to 0 of 2 renewals.

I didn't manage to break this patch any further.

However, there are two visual differences compared to old page:

1) table column widths seems to be different, e.g. title always get too little space and looks somewhat squashed
2) on book check-in row which was checked in doesn't disappear (which makes sense because we are not reloading the page) -- I would like it to disappear, maybe after short time-out

having said that, none of these are a reason not to sign-off this patch, good work Kyle.

p.s. I would also love to see ajax check-in in future ;-)

Awesome to see more services being added :D

Kyle,
One question after a quick test: after checking one item out, the line on the checkout table in displayed under the "Previous checkouts", it is expected?

(In reply to Jonathan Druart from comment #56)
> Kyle,
> One question after a quick test: after checking one item out, the line on
> the checkout table in displayed under the "Previous checkouts", it is
> expected?

Jonathan, I can't reproduce this issue. Do you think you could post a screenshot?

(In reply to Kyle M Hall from comment #58)
> Jonathan, I can't reproduce this issue. Do you think you could post a
> screenshot?

I added 2 screenshots, with and without the patch.
Note that the titles were bold, not with the patch.
The table width has been changed too. It seems too narrow to me.

(In reply to Jonathan Druart from comment #61)
> (In reply to Kyle M Hall from comment #58)
> > Jonathan, I can't reproduce this issue. Do you think you could post a
> > screenshot?
> 
> I added 2 screenshots, with and without the patch.
> Note that the titles were bold, not with the patch.
> The table width has been changed too. It seems too narrow to me.

Thanks! Yes, that is the way it should look.

(In reply to Kyle M Hall from comment #62)
> Thanks! Yes, that is the way it should look.

The width, the bold ou the "Previous checkouts" line?

1/ The previous checkouts appears but todays checkouts are put in it. Not logical.
2/ The bold: it is a regression, in my opinion.
3/ The width: the table is, now, really narrow on wide screen.

Marked as Failed QA.

> 1/ The previous checkouts appears but todays checkouts are put in it. Not
> logical.

Ok, I see this now. I didn't notice it the first time I looked at your screen caps. I have been unable to reproduce this behavior, try as I might!

> 2/ The bold: it is a regression, in my opinion.

Fixed!

> 3/ The width: the table is, now, really narrow on wide screen.

Fixed!

(In reply to Kyle M Hall from comment #65)
> > 1/ The previous checkouts appears but todays checkouts are put in it. Not
> > logical.
> 
> Ok, I see this now. I didn't notice it the first time I looked at your
> screen caps. I have been unable to reproduce this behavior, try as I might!

It's odd because I don't do something special. I just check an item out to a patron (tested with marc21/unimarc and dateformat us/metric). What is your display? You get the 2 "Previous checkouts" and "Today's checkouts" lines?

> > 2/ The bold: it is a regression, in my opinion.
> 
> Fixed!

Maybe a span with a class would be better than using the b tag.

> It's odd because I don't do something special. I just check an item out to a
> patron (tested with marc21/unimarc and dateformat us/metric). What is your
> display? You get the 2 "Previous checkouts" and "Today's checkouts" lines?

I've attached a screenshot to show what I see. I've tried all the different date formats and they all work for me.

> > > 2/ The bold: it is a regression, in my opinion.
> > 
> > Fixed!
> 
> Maybe a span with a class would be better than using the b tag.

I've updated the followup to use a span with a class.

Screenshot: http://i.imgur.com/S01Sv0V.png

The QA script reveals a small problem:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt
   FAIL	  forbidden patterns
		forbidden pattern: To include datatables files, please use the include file (see bug 10868) (line 17)

I see the same behaviour that Jonathan noted - I checked out an item to an account with previous checkouts. The new checkout shows with the Previous checkouts, instead of in its own section.

This is still the case after reloading the page.

I found there are 2 preferences that should influence the sequence in which the issues are displayed here: todaysIssuesDefaultSortOrder,  previousIssuesDefaultSortOrder - it seems like the page is not loading with the configured default order.
The checked out on date doesn't follow the dateformat preference.

Hi Kyle,

as promised, here my first test results:

GENERAL
- New jQuery plugin: jquery.dataTables.grouping.js
  Should we add a note to the about page about this?
- Translation:
    +    var HOLD_IS = _("Hold is");
    +    var SUSPENDED = _("suspended");
    +    var UNTIL = _("until");
    +    var ITEM_IS = _("Item is");
    +    var WAITING = _("waiting");
    +    var AT = _("at");
    +    var FROM = _("from");
   Would be much nicer as complete sentences with placeholders - this is
    really hard to translate grammatically correct. 

CHECKOUTS
- Sorting table
  > OK. Sorting the table was not possible before patch. Sorting
    will remove the Today's and Previous checkouts differentiation.
    Works ok.
- Column price and charge
  > Bug: columns price and charge are not sortable.
- Column Due date
  > Ok, only shows time now when it's an hourly loan. Nice!
- Column Item type
  > Shows the code instead of description, something for another day.
- Column Checked out on
  > Bug: Shows seconds, but those are always 00 so I guess we don't record
    the issue by second?

Checking out - still to be tested
- 'normal' check out
  > OK
- hourly check out (with specify due date)
  > OK
- with rental fee
- notforloan item
- too many items
  > OK, override possible.
- item on hold for someone else
- item with material note

Renewing items
- Check out with an already checked out barcode again
  > OK, item is renewed
- renewal using checkboxes
  > Concern: It's only possible to do one renewal at a time.
    After renewing once the checkbox is gone and then you will have
    to reload the page to bring the checkboxes back.
- renewal override
  > OK. If the item is on hold, the renewal will fail. I wonder if that's 
    always been the case, but it's behaviour pre-patches.
- too early renewal (bug 6779)
  > Bug: Shows too_soon instead of a nice message for why the renewal
    is not yet allowed. Should show the date - compare to master.

System preferences
- UseTableSortForCirc
  > Is removed, sorting will be always available
  > Bug: Should be removed properly with updatedatabase.pl as well
- [prefs for sorting by checkout date] - still to be tested

RELATIVE's CHECKOUTS
- releative's checkouts tab is only shown for the guarantee (child),
  but not for the parent - something seems messed up here?
  > Bug: My patron has 2 children, one of them has an item checked out.
    It doesn't show up, the relative's checkouts tab is empty.

HOLDS
- Sorting table
  > Sorting the holds table was not possible pre-patches.
    Concern: The sorting and page load seems a bit slow for 4 holds
- Columns: Title, Hold date, expiration date, Delete
  > OK
- Barcode column
  > Ok, now offers a link to the item
- Delete column
- Suspending and unsuspending holds
  > OK
- Cancelling holds
  > OK, possible enhancement: Redirect back to the holds tab after 
    cancelling a hold instead of sending user to the checkouts tab. 
    Exists pre-patch.

Concern: Performance testing / benchmarking
- I would be really interested to see a comparison on how fast the
  complete page loads in comparison to before and similar. Is this possible?

Concern: Accessibility
- We currently have no feedback about accessibility issues this might
  cause in circulation. I want to note that there is no non-Javascript
  fallback here. In case the Ajax proves to be a problem, we'd have
  a regression as currenct circulation works nicely.

I really like he idea of this, but I think I'd like it a bit better if
it had been implemented as an alternative at first - but that sure is
debatable :)

(In reply to Katrin Fischer from comment #79)
> Hi Kyle,
> 
> as promised, here my first test results:
> 
> GENERAL
> - New jQuery plugin: jquery.dataTables.grouping.js
>   Should we add a note to the about page about this?

Will do.

> - Translation:
>     +    var HOLD_IS = _("Hold is");
>     +    var SUSPENDED = _("suspended");
>     +    var UNTIL = _("until");
>     +    var ITEM_IS = _("Item is");
>     +    var WAITING = _("waiting");
>     +    var AT = _("at");
>     +    var FROM = _("from");
>    Would be much nicer as complete sentences with placeholders - this is
>     really hard to translate grammatically correct. 

I don't think this is possible with JavaScript, as it has no string interpolation.

> - renewal using checkboxes
>   > Concern: It's only possible to do one renewal at a time.
>     After renewing once the checkbox is gone and then you will have
>     to reload the page to bring the checkboxes back.

This is by design. I think in those rare cases reloading the page would be a fair resolution for this.

> - renewal override
>   > OK. If the item is on hold, the renewal will fail. I wonder if that's 
>     always been the case, but it's behaviour pre-patches.

Yes, that has always been the case.

> - too early renewal (bug 6779)
>   > Bug: Shows too_soon instead of a nice message for why the renewal
>     is not yet allowed. Should show the date - compare to master.

Will fix

> System preferences
> - UseTableSortForCirc
>   > Is removed, sorting will be always available
>   > Bug: Should be removed properly with updatedatabase.pl as well
> - [prefs for sorting by checkout date] - still to be tested

Will fix

 
> RELATIVE's CHECKOUTS
> - releative's checkouts tab is only shown for the guarantee (child),
>   but not for the parent - something seems messed up here?
>   > Bug: My patron has 2 children, one of them has an item checked out.
>     It doesn't show up, the relative's checkouts tab is empty.

I'll look into this
 
> HOLDS
> - Sorting table
>   > Sorting the holds table was not possible pre-patches.
>     Concern: The sorting and page load seems a bit slow for 4 holds

The sorting is done server side. We can either leave it as is, since there was no sorting to begin with, make the sorting client side ( which will break sorting on some columns possibly ), or just disable sorting altogether ( ie pre-patch behavior )

> Concern: Performance testing / benchmarking
> - I would be really interested to see a comparison on how fast the
>   complete page loads in comparison to before and similar. Is this possible?

Not easily. The idea is to get the circulation screen to be as *responsive* as possible. The more checkouts and holds a patron has, the less responsive circulation.pl becomes. This patch obviates that issue be loading the page, and fetching that data afterward.
 
> Concern: Accessibility
> - We currently have no feedback about accessibility issues this might
>   cause in circulation. I want to note that there is no non-Javascript
>   fallback here. In case the Ajax proves to be a problem, we'd have
>   a regression as currenct circulation works nicely.

These tables should be just as accessible as any other datatable in Koha, some of which are also ajax based such as this.

(In reply to Kyle M Hall from comment #80)
> (In reply to Katrin Fischer from comment #79)
> > Hi Kyle,
> > 
> > as promised, here my first test results:
> > 
> > GENERAL
> > - New jQuery plugin: jquery.dataTables.grouping.js
> >   Should we add a note to the about page about this?
> 
> Will do.

On further review most of our datatables plugins are not listed on the about page. Only the star rating one is listed.

I checked with Galen, please add to the about page.

> - renewal using checkboxes
>   > Concern: It's only possible to do one renewal at a time.
>     After renewing once the checkbox is gone and then you will have
>     to reload the page to bring the checkboxes back.

I've been tracking this bug, as one of the libraries backing the development, and Kyle, I'm quite concerned by Katrin's comment above, and then your response below, "This is by design. I think in those rare cases reloading the page would be a fair resolution for this."

Select renewal isn't rare at circulation desks. Asking a librarian to refresh a page for each select renewal -- especially when there's three or more select renewals, is not good practice. 

I haven't had a chance to see this development in action yet, so I'm basing my comments and concerns on the descriptions, but I would hate for the system to lose the multiple select renewal option.

> - renewal override
>   > OK. If the item is on hold, the renewal will fail. I wonder if that's 
>     always been the case, but it's behaviour pre-patches.

Katrin, I can also confirm that this behavior has always been the case.

And Katrin, thanks for your thorough testing!

Hi Heather, select renewal still works - my description was not very good there :) You can only renew each item once - then you have to refresh the page, if you want to do a second renewal on the same item.

Katrin, thank you for the clarification. That helps a lot and it's no longer an issue! :)

Running out of time here this morning - will try to continue asap. Patches apply, relative's checkouts are working, but my holds (shows 2 on the tab) have gone missing. The table loads empty.

I also see a syntax error in Firebug:
unterminated string literal:  data += ITEM_IS_WAITING";

Switching to 'FAILED QA' - Kyle, can you check?

(In reply to Heather Braum from comment #86)
> Select renewal isn't rare at circulation desks. Asking a librarian to
> refresh a page for each select renewal -- especially when there's three or
> more select renewals, is not good practice. 
> 
> I haven't had a chance to see this development in action yet, so I'm basing
> my comments and concerns on the descriptions, but I would hate for the
> system to lose the multiple select renewal option.

Heather, I think you mis-understood the issue. You don't have to reload the page to renew multiple items. You would need to reload the page to renew one item multiple times! If a patron has 20 items checked out, you are still able to renew them all with a couple clicks.

Kyle

Hi Kyle,

NEEDS REBASE!

I tried to fix the conflicts, but wasn't sure I got it right so continued testing the patches on top of 2a7e71554b534f8d79cdbdc9552bc78bba9c4408 - the patches apply on this commit without conflicts.

1) In the holds table, the replacing technique is not working correctly:
   Die Räuber by _AUTHOR_ Schiller, Friedrich

2) On the relative's checkouts tab the price is not formatted correctly:
   0.000. Maybe 0 shouldn't show at all?

3) On the checkuts tab the checkout date shows in 'database' format:
   2014-04-27 21:10:00. It also still shows the seconds, although those
   are not recorded and the time is always :00.

X) It doesn't feel faster than before :( It could just be my installation, but
   hard to tell for me.
   I'd really like to get another opinion on this.

(In reply to Katrin Fischer from comment #94)
> Hi Kyle,
> 
> NEEDS REBASE!

Done!

> 1) In the holds table, the replacing technique is not working correctly:
>    Die Räuber by _AUTHOR_ Schiller, Friedrich

Should now be fixed!
 
> 2) On the relative's checkouts tab the price is not formatted correctly:
>    0.000. Maybe 0 shouldn't show at all?

Also feexed!

> 3) On the checkuts tab the checkout date shows in 'database' format:
>    2014-04-27 21:10:00. It also still shows the seconds, although those
>    are not recorded and the time is always :00.

This one too

> X) It doesn't feel faster than before :( It could just be my installation,
> but hard to tell for me

If you haven't cleared your browser cache you might still be using the js file with the ajax sorting. The js sorting should be way faster.

I've also squashed all of my new qa followups into one qa followup patch for simplicity.

Kyle

GENERAL
- New jQuery plugin: jquery.dataTables.grouping.js
  > Added to About page.

CHECKOUTS
- Sorting table
  > OK. Sorting the table was not possible before patch. Sorting
    will remove the Today's and Previous checkouts differentiation.
    Works ok.
- Column price and charge
  > OK.
- Column Due date
  > OK, only shows time now when it's an hourly loan. Nice!
- Column Item type
  > Shows the code instead of description, something for another day.
- Column Checked out on
  > OK.

Checking out - still to be tested
- 'normal' check out
  > OK.
- hourly check out (with specify due date)
  > OK.
- with rental fee
  > OK, charge shows up in table.
- notforloan item
  > OK, can be overriden or not, depending on configuration.
- too many items
  > OK, override possible.
> Some more done along the way, all message/alerts showing correctly.

Renewing items
- Check out with an already checked out barcode again
  > OK, item is renewed
- renewal using checkboxes
  > OK. Note: It's only possible to do one renewal at a time.
    After renewing once the checkbox is gone and then you will have
    to reload the page to bring the checkboxes back.
- renewal override
  > OK. Note: If the item is on hold, the renewal will fail.
    Behaviour is the same before applying the patch.
- too early renewal (bug 6779)
  > Bug: When the renewal is too soon, in master we show the first 
   possible renwal date, with the patches only a note is shown, that
   the renwal is premature.

System preferences
- UseTableSortForCirc 
  > Pref was removed, it's not longer needed as sorting is possible 
    by default now.
- todaysIssuesDefaultSortOrder 
  > OK.
- previousIssuesDefaultSortOrder
  > OK.
  Note: I wonder why we don't have a sorting by checkout date, so that
  the newest checkout always shows at the top or bottom. But that's been
  like that before.

RELATIVE's CHECKOUTS
> OK, still working nicely.

HOLDS
- Sorting table
  > OK, sorting the holds table was not possible pre-patches, it is now.
- Columns: Title, Hold date, expiration date, Delete
  > OK
- Barcode column
  > Ok, now offers a link to the item
- Delete column
- Suspending and unsuspending holds
  > OK
- Cancelling holds
  > OK, possible enhancement: Redirect back to the holds tab after 
    cancelling a hold instead of sending user to the checkouts tab. 
    Exists pre-patch.

Concern: Accessibility - I hope we don't make it harder with this patch.

Upon review:

- this definitely causes a regression on bug 12107
- rather than the approach you've used for putting placeholders in JavaScript strings to translate, the approach in bug 12138 should be used instead.

I suspect regressions of the following bugs may be possible

- 7413
- 11486
- 11711

The main reason, however, is that this is too large to safely put in this close to release.  I'm reasonably confident it can make it in early in the 3.18 cycle, however.

(In reply to Galen Charlton from comment #103)
> Upon review:
> 
> - this definitely causes a regression on bug 12107

I've filed another qa followup to take care of this.

> - rather than the approach you've used for putting placeholders in
> JavaScript strings to translate, the approach in bug 12138 should be used
> instead.

Also has a followup
 
> I suspect regressions of the following bugs may be possible
> 
> - 7413

Katrin filed a followup bug for this since it was not a regression from a previous release ( i.e. no loss of functionality that 3.14 had ). However, I've added a followup to take of this.

> - 11486

I can find no regression of this bug. I see the number of renewals and remaining renewals just fine.

> - 11711

I can find no regression for this issue either. I use the datatables include for both circulation.tt and moremember.tt

I'm very happy to see this patch coming along - great work! I did some quick tests in a sandbox (Unimarc / database 2) and here are my two cents: 

The sandbox seems slower than my regular live and demo servers and this causes some weird situations: 
* When loading "Check out" for a patron I see "3 Checkout(s)" but it sometimes takes a good 10 seconds to actually see the old checkouts. This might not be avoidable since fetching those three checkouts is what takes time. Suggestion: Change the message "no data available in table" to "Loading previous checkouts" as long as the Ajax script is accessing data. You could even add a progress indicator of some sort after the message to indicate communication with the server being active. 

* When I check out books the entire table clears every time. If possible I'd like the already checked out books to remain visible and just add the new book. I'd even suggest that every barcode you scan becomes immediately visible in the checkouts table but with the status "processing" as long as the script is working. Presently I have no way of knowing it that last barcode I scanned just disappeared into the abyss or is being processed. 

These issues might not be showstoppers on a fast server, but they made me a bit nervous when using the sandbox and I'd imagine that regular librarians would se it as an actual problem. Asynchronous communication is tricky to communicate to people who are used to the old behavior of the whole page loading at the same time.

Idea for a follow up in the future: Currently the system locks me out from entering additional barcodes while processing the one just scanned. I'd love to be able to just scan away at a pile of books and have the system take care of them asynchronously at its own pace without me having to wait for input to become available. That would of course require that you could light up multiple warning messages either together of after one another. This is not as much of an issue with Plack active of course but would still save us from dropped barcodes when you happen to scan when something took longer than expected.

I have been thinking about this patch some more and I wondered why my impression in testing was sometimes, that it didn't really 'feel' a lot faster. On loading the page, yes, but not for checking out. I think it's because the error/confirmation messages are not ajaxified - we are speeding up the initial load of the page by using datatables, but is the page not reloaded when you run into a message/dialog? It might be a misunderstanding on my side, please feel free to correct me :)

+1 to Viktor's comments!

(In reply to Viktor Sarge from comment #110)
> The sandbox seems slower than my regular live and demo servers and this
> causes some weird situations: 

I agree, it is kinda slow on the sandboxes.

> * When I check out books the entire table clears every time. If possible I'd
> like the already checked out books to remain visible and just add the new
> book. I'd even suggest that every barcode you scan becomes immediately
> visible in the checkouts table but with the status "processing" as long as
> the script is working. Presently I have no way of knowing it that last
> barcode I scanned just disappeared into the abyss or is being processed.

I agree with this as well.  If is strange for the table to clear with each group of scans, and a little disconcerting.

Also, while testing on a sandbox, I checked out all 10 test items, in order (1-10), and when I was done, the order of the items I checked out from top to bottom was:
05/30/2014	Sandbox test biblio 10	BK	05/09/2014 23:35	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 2	BK	05/09/2014 23:34	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 3	BK	05/09/2014 23:34	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 4	BK	05/09/2014 23:34	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 5	BK	05/09/2014 23:34	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 6	BK	05/09/2014 23:35	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 7	BK	05/09/2014 23:35	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 8	BK	05/09/2014 23:35	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 9	BK	05/09/2014 23:35	Centerville		0.00	0.00	0Not renewable	
05/30/2014	Sandbox test biblio 1	BK	05/09/2014 23:34	Centerville		0.00	0.00	0Not renewable

	Centerville		0.00	0.00	0Not renewable	

Not sure what the default sort is going off here.

Otherwise, I think it looks great.  I especially love returning books.  It didn't have to reload the table at all.  But that was what I was looking for in the checkout process too.  I just expected to see information stay put and a new row added in the order things were checked out in.  Maybe I'm just not understanding.

Christopher

Christopher

(In reply to Viktor Sarge from comment #111)
> Idea for a follow up in the future: Currently the system locks me out from
> entering additional barcodes while processing the one just scanned. I'd love
> to be able to just scan away at a pile of books and have the system take
> care of them asynchronously at its own pace without me having to wait for
> input to become available. That would of course require that you could light
> up multiple warning messages either together of after one another. This is
> not as much of an issue with Plack active of course but would still save us
> from dropped barcodes when you happen to scan when something took longer
> than expected.

This is merely the first step in fully ajaxifying Koha's circulation system. What you are describing is exactly where we plan to go in the future with circulation development.

> This is merely the first step in fully ajaxifying Koha's circulation system.
> What you are describing is exactly where we plan to go in the future with
> circulation development.

You are making me very happy with those two sentences Kyle :)

There should be independent QA of the last couple follow-ups; I'm setting the status accordingly.  Note that I may well be doing that independent QA myself tomorrow.

(In reply to Kyle M Hall from comment #109)

QA comments:

> (In reply to Galen Charlton from comment #103)
> > I suspect regressions of the following bugs may be possible
> > 
> > - 7413
> Katrin filed a followup bug for this since it was not a regression from a
> previous release ( i.e. no loss of functionality that 3.14 had ). However,
> I've added a followup to take of this.

I confirm it fixes the regression.

> > - 11486
> 
> I can find no regression of this bug. I see the number of renewals and
> remaining renewals just fine.

No regression found.

> > - 11711
> 
> I can find no regression for this issue either. I use the datatables include
> for both circulation.tt and moremember.tt

No regression found.

I didn't found any regression on testing this patch set.

I focused on performance:

A. 1 Checkout
  1. Using the Firebug's network tab
    a. without the patches
GET circulation.pl?borrowernumber=7853 9,0 KB 7,46s
1 requests
8,21s (onload: 14,24s)

    b. with the patches
GET circulation.pl?borrowernumber=7852 8,6 KB 7,53s
GET checkouts.pl?borrowernumber=7852 423 B 5,23s
2 requests
12,76s (onload: 14,88s)

  2. Using NytProf
    a. without the patches
Profile of /home/koha/src/circ/circulation.pl for 2.76s (of 3.94s), executing 476604 statements and 60325 subroutine calls in 150 source files and 36 string evals.
75% C4::Template::Output
9% get_template_and_user
5% build_issue_data
4% GetPendingIssue

    b. with the patches
Profile of /home/koha/src/circ/circulation.pl for 2.93s (of 4.21s), executing 397152 statements and 61459 subroutine calls in 170 source files and 82 string evals.
57% C4::Template::Output
25% Koha::Schema::Database
7% get_template_and_user
5% DBIx::Class::ResultSet::count

Profile of /home/koha/src/svc/checkouts.pl for 296ms (of 345ms), executing 11346 statements and 5019 subroutine calls in 89 source files and 14 string evals.
44% C4::Circulation::CanBookBeRenewed
12% C4::Biblio::GetMarcBiblio

B. 10 Checkouts
  1. Using the Firebug's network tab
    a. without the patches
GET circulation.pl?borrowernumber=7853 9,9 KB 8,12s
1 request
8,12s (onload: 14,85s)

    b. with the patches
GET circulation.pl?borrowernumber=7853 8,8 KB 7,11s
GET checkouts.pl?borrowernumber=7853 1,0 KB 5,77s
2 requests
12,88s (onload: 14,73s)

  2. Using NytProf
    a. without the patches
Profile of /home/koha/src/circ/circulation.pl for 4.82s (of 6.83s), executing 561240 statements and 94492 subroutine calls in 150 source files and 36 string evals.
62% C4::Template::Output
25% build_issue_data


    b. with the patches
Profile of /home/koha/src/circ/circulation.pl for 3.59s (of 5.00s), executing 397152 statements and 61459 subroutine calls in 170 source files and 80 string evals.
57% C4::Template::Output
25% Koha::Schema::Database

Profile of /home/koha/src/svc/checkouts.pl for 986ms (of 1.22s), executing 60681 statements and 25687 subroutine calls in 89 source files and 14 string evals.
45% C4::Circulation::CanBookBeRenewed
20% Getmarcbiblio


=>
I used a small vm for benchmarking, it's why the loads are quite high.
Looking at the results, this new interface won't make waste time to librarians on loading the checkouts page.
On checkin/renew, the page won't be reloaded entirely and the time will be divided (at least) by 2 (I let you make the benchmarks).
But the "Holds" tab will time consuming, since it will be loaded at the request (on clicking on the tab).

I found a small problem on testing, on the "Details" tab (members/moremember.pl), the checkout table don't have the "Loading..." popup, a line "No data available" is displayed. That should be fix, but I don't want to block this feature longer.
Kyle, could you please submit a patch on the bug I am going to open please?

Katrin has some concerns about accessibility. I don't know if screen readers support this Ajax new screen. It would be great to fix any potential further problem related to accessibility.

Marked as Passed QA.

(In reply to Jonathan Druart from comment #123)
>     b. with the patches
> Profile of /home/koha/src/circ/circulation.pl for 2.93s (of 4.21s),
> executing 397152 statements and 61459 subroutine calls in 170 source files

>     b. with the patches
> Profile of /home/koha/src/circ/circulation.pl for 3.59s (of 5.00s),
> executing 397152 statements and 61459 subroutine calls in 170 source files

Note:
A.2.b. and B.2.b. should be the same, it's the interest of this patch set.
The benchmark does not highlight that.

(In reply to Jonathan Druart from comment #123)
> I found a small problem on testing, on the "Details" tab
> (members/moremember.pl), the checkout table don't have the "Loading..."
> popup, a line "No data available" is displayed. That should be fix, but I
> don't want to block this feature longer.
> Kyle, could you please submit a patch on the bug I am going to open please?

See bug 12489.

I was worried that these changes might have an impact on accessibility and asked one of our libraries with a blind staff member to test this on a sandbox.

The test has turned up 2 major problems for her:

1) In IE checking out books is broken. Under some circumstances, if you checkout a book, it won't show up on the patron account. If it happens, it doesn't matter how often you reload the page or switch tabs or whatever, it won't show up. The number of checkouts on top of the tab changes and is always correct.  (Version: IE 9)

To test:
- Use IE.
- check out book
- check out a book that is checked out to someone else, confirm with yes
- Verify this book doesn't show up in the list of checkouts
- check out a book that is on hold for someone else, confirm with yes
> Check for each of those, if the list of checkouts is correctly displayed. For me, normal checkouts worked sometimes, but the other 2 cases never worked. Often things got stuck after the first successful checkout.

If you have an account with a wrong list of checkouts, try:
- Switching to the details tab and back to checkouts
- Shift/ctrl+reload
- Verify the list is still not correct.

Sometimes, when you go back to an account after having checked in all items, the old checkouts still show up!

I know we are not officially supporting IE on the staff side, but I think this problem is major enough that it really needs to be fixed. Also, switching browsers is not an option for this library as there are existing problems in using FF.

2) In IE and FF if you use keyboard shortcuts to move between the tabs, the tables for holds and relative's checkouts are not loaded. All you see is the table headers, but no contents. Only when you can click on the tab, the tables are loaded. So if you don't have a mouse, you can't see the holds and relative's checkouts. 

To test: 
- Click on the  "checkouts" tab, then use arrow keys to move between them.
- Verify that the relatives checkouts and holds tables don't display the entries, but just the table header.

I am setting to 'In Discussion' for the moment.

Katrin,
IE9 is two major versions back.  IE10 introduced a big jump in fixing a lot of issues and being more compliant than previous versions.

I'm wondering if these issues are persistent with current versions.

Because this is an accessibility discussion, is there a reason they need to stick with an older version of IE?  I really don't recommend use of anything earlier than IE10 here.

Christopher

Hi Christopher, I am normally not using IE - IE9 is what we appear to have available at work. I will check with the library for the version they use.

(In reply to Christopher Brannon from comment #134)

> IE9 is two major versions back.

...and we have never guaranteed that the staff client would work in Internet Explorer. I think that this shouldn't be a blocking issue for this bug unless there is good evidence that some users have to use Internet Explorer for accessibility reasons.

I know I am biased here, but we have a library where this is the case.
I will hear back from them tomorrow about the exact browser version they tested with. But I think between not officially supporting a browser and making a whole module unusuable is quite a big step and we should at least discuss this.

Aside from my previous comment, I can confirm the problem where the checkout list does not refresh in IE11 either.  So it isn't just an IE9 thing.  Does not seem to be related to whether the item is already checked out.  IE just does not refresh.

I can confirm that the holds list does not appear in FF or IE unless you physically click on the tab.

Christopher

Hi Christopher, thx for taking the time to confirm the problem in the current IE version!

(In reply to Katrin Fischer from comment #137)
> I know I am biased here, but we have a library where this is the case.
> I will hear back from them tomorrow about the exact browser version they
> tested with. But I think between not officially supporting a browser and
> making a whole module unusuable is quite a big step and we should at least
> discuss this.

This was a huge issue for our library when we moved to Koha.  Being a city owned and managed library, we had to convince IT to let us use FF.  They don't want to maintain multiple browsers, and they don't want to make the entire city change to FF because the library needs it for one application.

I am sure this is a discussion for another place and another time, but IE comes with Windows and is a default (and sometimes only supported) browser in many places.  Ignoring that doesn't make it go away.

I have to agree with Katrin...leaving the module unusable in any browser is an issue.  If it is cosmetics, that's one thing.  But not unusable.  We've had to address and correct issues with other browsers, and IE should be no different.  Especially IE.

That's my opinion.  Sorry if this comes off too strong.  It's been a harsh week so far.

Christopher

I can confirm that tables fail to populate in Chrome as well, unless clicked on.

Christopher

(In reply to Katrin Fischer from comment #132)
> 1) In IE checking out books is broken. Under some circumstances, if you
> checkout a book, it won't show up on the patron account. If it happens, it
> doesn't matter how often you reload the page or switch tabs or whatever, it
> won't show up. The number of checkouts on top of the tab changes and is
> always correct.  (Version: IE 9)

It seems the problem is bigger than items not showing up for IE.  In IE:

- Check out 3 items.
- Refresh the screen.  You can't get an accurate table in IE without refreshing the page (F5).  It doesn't matter if you navigate away from the patron and load the patron again.  It is as if an ajax table won't even be touched unless the page is refreshed from the browser refresh button.  (I don't claim to know anything about how the table is populated via ajax.)
- Check the items in.
- Now check the first item out.  It loads the table that was showing all three items before.  However, looking at the account from another browser shows that just the one item is checked out.  You have to refresh the page in IE for the correct information to come up.  Of course, you run into problems where it will resubmit checkouts and ask if you want to check it out again.  :/

If this were just a cosmetic thing, I would be on the fence about it.  But this needs to function properly, even in IE.

Christopher

Hi Christopher, thx again for giving this more testing!

I tried again in the old version and maybe there is some difference - for me the first item checks out ok (visible in table), but the second doesn't show up.
Then hitting F5 or clicking on the reload icon doesn't help it. I just can't get the correct list to show. The only indication is the number in the tab description - that is always correct.

Kyle,
Thanks for addressing both issues Katrin brought up so quickly.  Looks great in IE11.  :)

As a side note, there is only a couple cosmetic things I have a comment about.

When the checkout tab is being loaded, the table reads: "No data available in table"

When the holds tab is being loaded, it reads: "Loading..."

I like the "Loading...".  Could you do that for any other tables?

Also, I notice that printing a slip prints current, up-to-date information, despite what the table might be currently showing (i.e. someone checked in an item from the book drop in the back room).  Do you think this might lead to confusion?  I'm wondering if the tables should be updated if information is being printed, for more consistency.  Just a thought.

Other than those things, it is looking really good.

Christopher

Last 2 patches need a signoff, I'm changing the status according that.

(In reply to Christopher Brannon from comment #146)
> When the checkout tab is being loaded, the table reads: "No data available
> in table"
> 
> When the holds tab is being loaded, it reads: "Loading..."
> 
> I like the "Loading...".  Could you do that for any other tables?

I have added a followup for this.
 
> Also, I notice that printing a slip prints current, up-to-date information,
> despite what the table might be currently showing (i.e. someone checked in
> an item from the book drop in the back room).  Do you think this might lead
> to confusion?  I'm wondering if the tables should be updated if information
> is being printed, for more consistency.  Just a thought.

This is no less in issue with Koha currently than it is with these patches. This is definitely outside the scope of this bug.

I did not found any regression with the 3 last patches (using FF).
I don't have any IE versions to test if they fix the described issues.

Patch tested with a sandbox, by Christopher Brannon <cbrannon@cdalibrary.org>

Comment on attachment 29444 [details] [review]
Bug 11810: Input fields in OPAC suggestion form are a bit short (Bootstrap)

Wrong bug report.

Comment on attachment 29445 [details] [review]
Bug 11810: (followup) regenerate CSS due to conflict

Wrong bug report.

Patches pushed to master.

Thanks Kyle and everyone involved!

Comment on attachment 29472 [details] [review]
Bug 11703 - Convert checkouts table to ajax datatable

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

::: svc/checkouts.pl
@@ +3,5 @@
> +# This software is placed under the gnu General Public License, v2 (http://www.gnu.org/licenses/gpl.html)
> +
> +# Copyright 2014 ByWater Solutions
> +#
> +# This file is part of Koha.

What does this service do exactly? 

Does it just list current checkouts? Is Jesse going to develop this service further to perform actual checkouts or is he creating a new service for that? If so, I think this service name might be confusing...

@@ +19,5 @@
> +# with Koha; if not, write to the Free Software Foundation, Inc.,
> +# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> +
> +use strict;
> +use warnings;

Probably should've used Modern::Perl here instead.

@@ +94,5 @@
> +if ( @borrowernumber == 1 ) {
> +    $sql .= '= ?';
> +}
> +else {
> +    $sql = ' IN (' . join( ',', ('?') x @borrowernumber ) . ') ';

If @borrowernumber is empty and the environmental variable DEBUG is active, this SQL query would trigger a fatal software error in MySQL/MariaDB.

I doubt that many people run with DEBUG on, and this service "should" always be called with at least one borrowernumber parameter, but I think it makes sense to remove the possibility of generating invalid SQL in the first place. 

Even without DEBUG on, the logs will still contain MySQL errors for invalid SQL syntax.

My question about what this service does was supported to segue into asking for some internal documentation explaining what this service does so that other developers don't necessarily have to read through the code to know its purpose.

I don't mean to sound snarky. I need to include more inline comments as well. I just thought I'd point out that that the services could use some more comments :).

Created attachment 30228 [details] [review]
Bug 11703 - Convert checkouts table to ajax datatable [3.16.x]

When a patron has many checked out items, circulation.pl can take a very
long time to load ( on the order of minutes in some cases ). This is
primarily due to the processing of the previous checkouts list. If we
convert to this table to a datatable that fetches its data via ajax, we
can make circulation.pl far more responsive. The same should be done
with relative's checkouts as well.

Test Plan:
1) Apply this patch
2) Observe that the checkouts and relatives' checkouts tables
   are now loaded asynchronously
3) Observe and verify the renew and return actions are now
   ajax based and function in a manner equivilent to how they
   used to.

This bug had quite a few followups, so I squashed all of them into one
change so that code is easier to follow. Original commit messages are below:

Bug 11703 - Use the ajax datatables on patron details page

Bug 11703 - Convert holds tables to ajax datatables

Bug 11703 [QA Followup 1] - Center bProcessing message over table

Bug 11703 [QA Followup 2] - Remove icons from checkout and clear buttons

Bug 11703 [QA Followup 3] - Remove references to UseTablesortForCirc

Bug 11703 [QA Followup 4] - Add back in Today's checkouts/Previous checkouts rows

Bug 11703 [QA Followup 5]

Bug 11703 [QA Followup 6] - Move strings to an include file for translation purposes

Bug 11703 [QA Followup 7] - Fix issues spotted by koha-qa.pl

Bug 11703 [QA Followup 8] - Speed up api/checkouts.pl as much as possible

Bug 11703 [QA Followup 9] - Move scripts from api directory to svc directory

Bug 11703 [QA Followup 10] - Fix errors caused by rebase

Bug 11703 [QA Followup 11] - Prevent multiple fetchs from ajax source

Bug 11703 [QA Followup 12] - Fix problem detected by koha-qa.pl

Bug 11703 [QA Followup 13] - Removed uneccessary data from renewal box during renewal

Bug 11703 [QA Followup 14] - Fix table column span

Signed-off-by: Dobrica Pavlinusic <dpavlin@rot13.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Passes all tests and QA script.
Test plan on bug report:
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11703#c98

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703 [QA Followup]

* Bold record title, fix table width
* Change output_pref_due to output_pref
* Retain functionality of IssuesDefaultSortOrder system preferences
* Use datatables.inc in circulation.tt
* Fix up holds table details
* Add plugin to about
* fix relatives' checkouts
* add too_many string
* remove dead syspref from db
* Sort relatives' checkouts and holds tables client side
* Provide context for translation of strings
* Fix unterminated string literal, add missing paren
* Add replacement of _AUTHOR_ with title's author for holds
* Format prices correctly
* Format checkout dates correctly

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

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703 [QA Followup] - Use format instead of replace for string translation

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703 [QA Followup] - Use interface instead of themelang for checkboxes plugin

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703 [QA Followup] - Restore showing earliest renewal date for 'too early' renewals

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703: (qa-followup) fix update database entry

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703 [QA Followup] - Stop IE from caching ajax request

Signed-off-by: Britta Cramer <britta.cramer@inm-gmbh.de>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703 [QA Followup] - Allow keyboard navigation of tabs

Signed-off-by: Britta Cramer <britta.cramer@inm-gmbh.de>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703 [QA Followup] - Change "No data available in table" to "Loading..."

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Bug 11703: (qa followup) consistency in svc

This patch removes the trailing .pl from the introduced svc scripts.
Also removes a leftover (wrong license text)

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Pushed to 3.16.x for inclusion in 3.16.3.

The red highlighting for overdue items seems to be broken - see bug 12729.

There is a problem with the holds tab on 3.16.3, see bug 12883.

Pushed the missed patch to 3.16.x for inclusion in 3.16.4.

This feature introduces a bug: it's not possible to export the checkout list.
See bug 13190 for a fix.

Wondering if the dependency on 11518 is still valid?

*** Bug 11305 has been marked as a duplicate of this bug. ***

Fix for red highlighting for overdue items

file checkouts.js:
In tag $("#issues-table").dataTable... date form mySQL database is not parsed. Quick fix for it is change 
  var due = new Date( oObj.date_due )
into
  var t = oObj.date_due.split(/[- :]/);
  var due = new Date(t[0], t[1]-1, t[2], t[3], t[4], t[5]);

Hi M3ntor, I think this has already been fixed with bug 12729.

Thanks. I'm not very familiar with buzilla and miss this.

This fix needs to be applied to 3.16.x, also

Marked for inclusion in 3.16.7

keyword = rel_3_16_7_candidate

(In reply to Mason James from comment #194)
> This fix needs to be applied to 3.16.x, also
> 
> Marked for inclusion in 3.16.7

oops, ignore this comment ^

patch is already applied to 3.16.x