http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|gmcharlt@gmail.com |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19506|0 |1 is obsolete| | --- Comment #2 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19507 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19507&action=edit WIP - Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19507|0 |1 is obsolete| | --- Comment #3 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19515 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19515&action=edit Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable Some libraries would like to sort by columns for the records of an import batch. This seems like a good use of ajax DataTables. Test plan: 1) Apply this patch 2) Import a record batch into Koha a) Use some form of matching b) Have some records that will match and some that won't c) Have at least 30 records so you can test the pager 3) Verify the new table is functionally equivilent to the old static one -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19516|0 |1 is obsolete| | --- Comment #5 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19517 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19517&action=edit Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable Some libraries would like to sort by columns for the records of an import batch. This seems like a good use of ajax DataTables. Test plan: 1) Apply this patch 2) Import a record batch into Koha a) Use some form of matching b) Have some records that will match and some that won't c) Have at least 30 records so you can test the pager 3) Verify the new table is functionally equivilent to the old static one -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #7 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 19525 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19525&action=edit Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable This patch makes improvements to the default DataTables CSS and corrects a couple of errors in the original patch. CSS for the "full numbers" type pagination, used by this and the quotes editor, has been cleaned up and made more visually consistent with the brief pagination style. Also changed: - removed some hard-coded paths containing "http://staff.kohadev..." - Added a column header to the column showing record match details. This provides both information and a clearer target for clicking to resort - Expanding the default table pager controls for this page and the quotes editor (for consistency) -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19526|0 |1 is obsolete| | --- Comment #10 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19674 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19674&action=edit Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable Some libraries would like to sort by columns for the records of an import batch. This seems like a good use of ajax DataTables. Test plan: 1) Apply this patch 2) Import a record batch into Koha a) Use some form of matching b) Have some records that will match and some that won't c) Have at least 30 records so you can test the pager 3) Verify the new table is functionally equivilent to the old static one Signed-off-by: Owen Leonard <oleonard@myacpl.org> Tests fine and looks good with the exception of the corrections I put in a follow-up. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #14 from Kyle M Hall <kyle@bywatersolutions.com> --- Owen, the quote of the day table looks good in FireFox, but still looks like Jonathan's screen cap in Chrome. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19681|0 |1 is obsolete| | --- Comment #15 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 19682 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=19682&action=edit Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable This patch makes improvements to the default DataTables CSS and corrects a couple of errors in the original patch. CSS for the "full numbers" type pagination, used by this and the quotes editor, has been cleaned up and made more visually consistent with the brief pagination style. Also changed: - removed some hard-coded paths containing "http://staff.kohadev..." - Added a column header to the column showing record match details. This provides both information and a clearer target for clicking to resort - Expanding the default table pager controls for this page and the quotes editor (for consistency) - Correction to quotes.css to fix pager display problem on wide screens Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #18 from Kyle M Hall <kyle@bywatersolutions.com> ---

1/ On tools/manage-marc-import.pl, I got a JS error: aoData.push( { "name": "import_batch_id", "value": } ); The table should not be displayed if there is no import_batch_id parameter.

Fixed!

2/ I got another JS error clicking on the title SyntaxError: syntax error: void() Prefer a href="#"

By bad, the call is supposed to be void(0). This appears to be the preferred practice these days. The problem with href="#" is that it causes the page to jump back to the top when clicked. This isn't a problem when the page does not scroll vertically, but in this case it often will.

3/ The DT aLengthMenu parameter should contain an "All" entry (git grep "aLengthMenu.*All")

Fixed!

4/ Why do you comment lines in tools/manage-marc-import.pl instead of deleting them?

Fixed!

5/ Why do you add a show_marc JS function instead of using the "classic" way (link to showmarc.pl)?

The "classic" way uses greybox, which isn't compatible with DataTables built from an ajax source. On the plus side, I'm using jQuery UI instead, and if we switched all of them to jQuery UI, we could drop greybox as a dependency.

6/ Permissions: manage-marc-import.pl needs the tools=>manage_staged_marc permission and the ajax script needs editcatalogue. Don't you think we should have the same one in both?

Fixed! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #20 from Owen Leonard <oleonard@myacpl.org> --- (In reply to Kyle M Hall from comment #18)

My bad, the call is supposed to be void(0). This appears to be the preferred practice these days. The problem with href="#"

No, using "javascript:" is not the preferred practice. More frequently you'll see "return false" added to the onclick (at least in cases where "return false" is what you want). The ideal way to handle something like this is to design the inserted markup so that the click event can be moved into the main JavaScript block (even better, into a separate JavaScript file). Then the outcome of the click event can be handled by something like this: http://css-tricks.com/return-false-and-prevent-default/ This isn't an issue that should block QA, but is something to think about next time. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |katrin.fischer@bsz-bw.de --- Comment #22 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Switching this to failed QA because of Jonathan's last comment and the fact that it does not apply currently. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19682|0 |1 is obsolete| | --- Comment #24 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20184 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20184&action=edit Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable This patch makes improvements to the default DataTables CSS and corrects a couple of errors in the original patch. CSS for the "full numbers" type pagination, used by this and the quotes editor, has been cleaned up and made more visually consistent with the brief pagination style. Also changed: - removed some hard-coded paths containing "http://staff.kohadev..." - Added a column header to the column showing record match details. This provides both information and a clearer target for clicking to resort - Expanding the default table pager controls for this page and the quotes editor (for consistency) - Correction to quotes.css to fix pager display problem on wide screens Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #19692|0 |1 is obsolete| | --- Comment #25 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20185 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20185&action=edit Bug 10558 [QA Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Jonathan Druart <jonathan.druart@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #28 from Jonathan Druart <jonathan.druart@biblibre.com> --- QA comment: 1/ There is no pod in the new file (yes I know, I am not the best person to make this remark...). 2/ The license version should be replaced with the GPLv3. 3/ I don't know why but the ajax script is called twice! (maybe can you have a look at my patch for bug 9811 where I use a script to populate a table with the server side processing method). Marked as Failed QA. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Signed Off --- Comment #30 from Kyle M Hall <kyle@bywatersolutions.com> --- Is this followup 3 what you are looking for? I examined the network access of the page and did find two calls to batch_records_ajax.pl, but the first was a 302 redirect that did not send any content, with the second one being a 200 that does send content. (In reply to Jonathan Druart from comment #28)

QA comment:

1/ There is no pod in the new file (yes I know, I am not the best person to make this remark...).

2/ The license version should be replaced with the GPLv3.

3/ I don't know why but the ajax script is called twice! (maybe can you have a look at my patch for bug 9811 where I use a script to populate a table with the server side processing method).

Marked as Failed QA.

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #32 from Jonathan Druart <jonathan.druart@biblibre.com> --- Using $.ajax instead of $.getJSON (2 GET), there is only one call (1 POST) to the ajax script: - $.getJSON( sSource, aoData, function (json) { - fnCallback(json) - } ); + $.ajax({ + 'dataType': 'json', + 'type': 'POST', + 'url': sSource, + 'data': aoData, + 'success': function(json){ + fnCallback(json); + } + }); -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #34 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 20395 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=20395&action=edit Bug 10558 [QA Follow-up 4] - switch $.getJSON to $.ajax -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #36 from Jonathan Druart <jonathan.druart@biblibre.com> --- ping Kyle, we are close to finish! Is a rebased code from Tamil or the first patch contains a bad c/p of the license? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dpk@randomnotes.org --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- *** Bug 11065 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #40 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Katrin Fischer from comment #37)

(In reply to Jonathan Druart from comment #36)

...

ping Kyle, we are close to finish! Is a rebased code from Tamil or the first patch contains a bad c/p of the license?

Kyle, we need your feedback on this! :)

The file in question is based on a file Tamil wrote that I copied and then completely re-wrote. However, if someone wants to add a followup to add a copyright for Tamil to my the new file, I would be fine with that. Kyle -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20184|0 |1 is obsolete| | --- Comment #42 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 22063 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22063&action=edit Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable This patch makes improvements to the default DataTables CSS and corrects a couple of errors in the original patch. CSS for the "full numbers" type pagination, used by this and the quotes editor, has been cleaned up and made more visually consistent with the brief pagination style. Also changed: - removed some hard-coded paths containing "http://staff.kohadev..." - Added a column header to the column showing record match details. This provides both information and a clearer target for clicking to resort - Expanding the default table pager controls for this page and the quotes editor (for consistency) - Correction to quotes.css to fix pager display problem on wide screens Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20328|0 |1 is obsolete| | --- Comment #44 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 22065 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22065&action=edit Bug 10558 [QA Follow-up 2] - Fix JavaScript Error -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #20395|0 |1 is obsolete| | --- Comment #46 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 22067 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22067&action=edit Bug 10558 [QA Follow-up 4] - switch $.getJSON to $.ajax -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #47 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 22068 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=22068&action=edit Bug 10558 [QA Follow-up 5] - Add Tamil copyrightwq -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |gmcharlt@gmail.com --- Comment #49 from Galen Charlton <gmcharlt@gmail.com> --- I like the general intent of this, but this patch causes an unnecessary regression of bug 10170. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #50 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 23993 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=23993&action=edit Bug 10558 [QA Follow-up 6] - Use datatables.inc -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA --- Comment #52 from Kyle M Hall <kyle@bywatersolutions.com> --- Setting to Passed QA again. Regression of 10170 resolved! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #54 from Galen Charlton <gmcharlt@gmail.com> --- Close, but there is a serious issue left: a regression on bug 2060. In particular, this patch regresses on the ability to properly display authority record batches. Instead of linking to the authority details page for imported records, for example, it links to the *bibliographic* details page. Sorting by citation for authority batches is also broken -- GetImportRecordsRange() is asked to sort by title when it should be sorting by authorized_heading. By the way, it would be preferable to me if GetImportRecordsRange() did not rely on dbh_quote_identifier() to sanitize the sorting column, which at present is passed directly from a CGI parameter. It should instead set $order_by to one of a known set of values, similar to how you deal with $order_by_direction. A much more minor issue: tools/batch_records_ajax.pl asserts that it's both GPL2 and GPL3+. Please squash your QA follow-ups (but not Owen's) when you resubmit. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 --- Comment #57 from Kyle M Hall <kyle@bywatersolutions.com> --- (In reply to Galen Charlton from comment #54)

Close, but there is a serious issue left: a regression on bug 2060.

In particular, this patch regresses on the ability to properly display authority record batches. Instead of linking to the authority details page for imported records, for example, it links to the *bibliographic* details page.

Fixed!

Sorting by citation for authority batches is also broken -- GetImportRecordsRange() is asked to sort by title when it should be sorting by authorized_heading.

Fixed!

By the way, it would be preferable to me if GetImportRecordsRange() did not rely on dbh_quote_identifier() to sanitize the sorting column, which at present is passed directly from a CGI parameter. It should instead set $order_by to one of a known set of values, similar to how you deal with $order_by_direction.

Fixed!

A much more minor issue: tools/batch_records_ajax.pl asserts that it's both GPL2 and GPL3+.

Fixed!

Please squash your QA follow-ups (but not Owen's) when you resubmit.

Done! New fixes published in followup 8 for easy analysis. All followups squashed and resubmitted as one patch. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |fridolyn.somers@biblibre.co | |m Patch complexity|--- |Medium patch -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=10558 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11605 -- You are receiving this mail because: You are watching all bug changes.