Some libraries would like to sort by columns for the records of an import batch. This seems like a good use of ajax DataTables.
Created attachment 19506 [details] [review] WIP - Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Created attachment 19507 [details] [review] WIP - Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Created attachment 19515 [details] [review] 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
Created attachment 19516 [details] [review] 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
Created attachment 19517 [details] [review] 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
Created attachment 19520 [details] [review] 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
Created attachment 19525 [details] [review] 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)
Created attachment 19526 [details] [review] [SIGNED-OFF] 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.
Created attachment 19547 [details] Ergonomic issue on wide screen Hi Owen and Kyle, I got an ergonomic issue on my wide screen. Except that, sounds good :)
Created attachment 19674 [details] [review] 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.
Created attachment 19675 [details] [review] 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) Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I can replicate Jonathan's issue. Owen, do you think you can take a look at that?
Created attachment 19681 [details] [review] 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
Owen, the quote of the day table looks good in FireFox, but still looks like Jonathan's screen cap in Chrome.
Created attachment 19682 [details] [review] 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>
QA comment: 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. 2/ I got another JS error clicking on the title SyntaxError: syntax error: void() Prefer a href="#" 3/ The DT aLengthMenu parameter should contain an "All" entry (git grep "aLengthMenu.*All") 4/ Why do you comment lines in tools/manage-marc-import.pl instead of deleting them? 5/ Why do you add a show_marc JS function instead of using the "classic" way (link to showmarc.pl)? 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? Marked as Failed QA.
Created attachment 19692 [details] [review] Bug 10558 [QA Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
> 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!
(In reply to Kyle M Hall from comment #18) > > 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. ...which is something I would like to be able to do if we could (See Bug 8290). I find that in this case the jQueryUI dialog works well. In other cases I have found that it isn't as easy to implement as Greybox, hence the lack of progress on that bug.
(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.
(In reply to Kyle M Hall from comment #18) > > 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! Still got the JS error.
Switching this to failed QA because of Jonathan's last comment and the fact that it does not apply currently.
Created attachment 20183 [details] [review] 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.
Created attachment 20184 [details] [review] 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>
Created attachment 20185 [details] [review] Bug 10558 [QA Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Kyle, the js error still occurred. To reproduce: - open firebug on the console tab - go to tools/tools-home.pl - click on the "Staged MARC record management" link caused by tools/manage-marc-import.tt l. 49: aoData.push( { "name": "import_batch_id", "value": [% import_batch_id %] } ); The import_batch_id var is not defined on this view. Marked as Failed QA.
Created attachment 20328 [details] [review] Bug 10558 [QA Follow-up 2] - Fix JavaScript Error
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.
Created attachment 20345 [details] [review] Bug 10558 [QA Follow-up 3] - Change GPL version number and add POD
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.
(In reply to Kyle M Hall from comment #30) > 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. The 2 calls get the records and return a JSON content.
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); + } + });
Thanks Jonathan! I'll implement this change and add another followup shortly. (In reply to Jonathan Druart from comment #32) > 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); > + } > + });
Created attachment 20395 [details] [review] Bug 10558 [QA Follow-up 4] - switch $.getJSON to $.ajax
Comment on attachment 20345 [details] [review] Bug 10558 [QA Follow-up 3] - Change GPL version number and add POD Review of attachment 20345 [details] [review]: ----------------------------------------------------------------- ::: tools/batch_records_ajax.pl @@ +2,4 @@ > > # This software is placed under the gnu General Public License, v2 (http://www.gnu.org/licenses/gpl.html) > > +# Copyright 2013 ByWater Solutions Does the original code is from Tamil or is it a wrong copy/paste?
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?
(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! :)
*** Bug 11065 has been marked as a duplicate of this bug. ***
This bug is now blocking a simpler fix (bug 11065) which will work and has no licensing issues. Lets get this one cleared to push or park it in favor of a simpler but less flexibe fix. This was a high priority issue coming out of the Cataloging roundtable at KohaCon13. What precisely do we need to do to clear this?
(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
Created attachment 22062 [details] [review] 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.
Created attachment 22063 [details] [review] 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>
Created attachment 22064 [details] [review] Bug 10558 [QA Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Created attachment 22065 [details] [review] Bug 10558 [QA Follow-up 2] - Fix JavaScript Error
Created attachment 22066 [details] [review] Bug 10558 [QA Follow-up 3] - Change GPL version number and add POD
Created attachment 22067 [details] [review] Bug 10558 [QA Follow-up 4] - switch $.getJSON to $.ajax
Created attachment 22068 [details] [review] Bug 10558 [QA Follow-up 5] - Add Tamil copyrightwq
Setting to Passed QA under the assumption that this was the only QA issue left. If it is not, then I am wrong.
I like the general intent of this, but this patch causes an unnecessary regression of bug 10170.
Created attachment 23993 [details] [review] Bug 10558 [QA Follow-up 6] - Use datatables.inc
Created attachment 23994 [details] [review] Bug 10558 [QA Follow-up 7] - Make strings translatable
Setting to Passed QA again. Regression of 10170 resolved!
Created attachment 23995 [details] [review] Bug 10558 [QA Follow-up 7] - Make strings translatable Removed trailing whitespace
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.
Created attachment 25936 [details] [review] Bug 10558 [QA Follow-up 8]
Created attachment 25937 [details] [review] Bug 10558 [QA Follow-up]
(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.
Pushed to master, along with a tiny follow-up. Thanks, Kyle!
Too big to push in 3.14.