Bug 10558

Summary: Convert records table in manage-marc-import.pl to ajax DataTable
Product: Koha Reporter: Kyle M Hall <kyle>
Component: MARC Bibliographic record staging/importAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact:
Severity: enhancement    
Priority: P5 - low CC: dpk, fridolin.somers, gmcharlt, jonathan.druart, katrin.fischer
Version: Main   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11605
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 11923, 16033    
Attachments: WIP - Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
WIP - Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
[SIGNED-OFF] Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Ergonomic issue on wide screen
Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [QA Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [QA Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [QA Follow-up 2] - Fix JavaScript Error
Bug 10558 [QA Follow-up 3] - Change GPL version number and add POD
Bug 10558 [QA Follow-up 4] - switch $.getJSON to $.ajax
Bug 10558 - Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [QA Follow-up] Convert records table in manage-marc-import.pl to ajax DataTable
Bug 10558 [QA Follow-up 2] - Fix JavaScript Error
Bug 10558 [QA Follow-up 3] - Change GPL version number and add POD
Bug 10558 [QA Follow-up 4] - switch $.getJSON to $.ajax
Bug 10558 [QA Follow-up 5] - Add Tamil copyrightwq
Bug 10558 [QA Follow-up 6] - Use datatables.inc
Bug 10558 [QA Follow-up 7] - Make strings translatable
Bug 10558 [QA Follow-up 7] - Make strings translatable
Bug 10558 [QA Follow-up 8]
Bug 10558 [QA Follow-up]

Description Kyle M Hall 2013-07-09 16:30:22 UTC
Some libraries would like to sort by columns for the records of an import batch. This seems like a good use of ajax DataTables.
Comment 1 Kyle M Hall 2013-07-09 16:31:23 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-07-09 17:57:40 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2013-07-10 11:53:27 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2013-07-10 12:13:16 UTC Comment hidden (obsolete)
Comment 5 Kyle M Hall 2013-07-10 12:35:17 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2013-07-10 12:51:29 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2013-07-10 16:40:38 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2013-07-10 16:47:20 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2013-07-11 08:00:24 UTC
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 :)
Comment 10 Kyle M Hall 2013-07-15 15:43:50 UTC Comment hidden (obsolete)
Comment 11 Kyle M Hall 2013-07-15 15:45:30 UTC Comment hidden (obsolete)
Comment 12 Kyle M Hall 2013-07-15 15:47:52 UTC
I can replicate Jonathan's issue. Owen, do you think you can take a look at that?
Comment 13 Owen Leonard 2013-07-15 16:17:21 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2013-07-15 17:45:54 UTC
Owen, the quote of the day table looks good in FireFox, but still looks like Jonathan's screen cap in Chrome.
Comment 15 Kyle M Hall 2013-07-15 18:04:45 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-07-16 12:02:29 UTC
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.
Comment 17 Kyle M Hall 2013-07-16 12:33:39 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-07-16 12:37:05 UTC
> 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!
Comment 19 Owen Leonard 2013-07-16 12:57:30 UTC
(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.
Comment 20 Owen Leonard 2013-07-16 13:20:58 UTC
(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.
Comment 21 Jonathan Druart 2013-07-16 13:25:14 UTC
(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.
Comment 22 Katrin Fischer 2013-08-04 13:22:49 UTC
Switching this to failed QA because of Jonathan's last comment and the fact that it does not apply currently.
Comment 23 Kyle M Hall 2013-08-08 18:51:28 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2013-08-08 18:51:38 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2013-08-08 18:51:53 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2013-08-14 09:37:55 UTC
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.
Comment 27 Kyle M Hall 2013-08-14 13:30:00 UTC Comment hidden (obsolete)
Comment 28 Jonathan Druart 2013-08-14 14:41:33 UTC
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.
Comment 29 Kyle M Hall 2013-08-14 15:33:59 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2013-08-14 15:39:33 UTC
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.
Comment 31 Jonathan Druart 2013-08-16 07:56:56 UTC
(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.
Comment 32 Jonathan Druart 2013-08-16 08:04:38 UTC
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);
+                  }
+              });
Comment 33 Kyle M Hall 2013-08-16 10:47:55 UTC
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);
> +                  }
> +              });
Comment 34 Kyle M Hall 2013-08-16 10:48:11 UTC Comment hidden (obsolete)
Comment 35 Jonathan Druart 2013-08-16 11:39:33 UTC
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?
Comment 36 Jonathan Druart 2013-09-13 10:27:45 UTC
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?
Comment 37 Katrin Fischer 2013-09-22 20:38:22 UTC
(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! :)
Comment 38 Martin Renvoize 2013-10-17 23:05:31 UTC
*** Bug 11065 has been marked as a duplicate of this bug. ***
Comment 39 Doug Kingston 2013-10-17 23:14:54 UTC
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?
Comment 40 Kyle M Hall 2013-10-17 23:51:07 UTC
(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
Comment 41 Kyle M Hall 2013-10-18 16:31:30 UTC
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.
Comment 42 Kyle M Hall 2013-10-18 16:32:22 UTC
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>
Comment 43 Kyle M Hall 2013-10-18 16:32:55 UTC Comment hidden (obsolete)
Comment 44 Kyle M Hall 2013-10-18 16:33:15 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2013-10-18 16:33:39 UTC Comment hidden (obsolete)
Comment 46 Kyle M Hall 2013-10-18 16:33:57 UTC Comment hidden (obsolete)
Comment 47 Kyle M Hall 2013-10-18 16:34:39 UTC Comment hidden (obsolete)
Comment 48 Kyle M Hall 2013-10-18 16:36:40 UTC
Setting to Passed QA under the assumption that this was the only QA issue left. If it is not, then I am wrong.
Comment 49 Galen Charlton 2013-12-10 04:09:30 UTC
I like the general intent of this, but this patch causes an unnecessary regression of bug 10170.
Comment 50 Kyle M Hall 2014-01-07 19:44:10 UTC Comment hidden (obsolete)
Comment 51 Kyle M Hall 2014-01-07 20:06:35 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2014-01-07 20:08:52 UTC
Setting to Passed QA again. Regression of 10170 resolved!
Comment 53 Kyle M Hall 2014-01-07 20:13:19 UTC Comment hidden (obsolete)
Comment 54 Galen Charlton 2014-02-28 23:15:16 UTC
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.
Comment 55 Kyle M Hall 2014-03-07 15:42:58 UTC Comment hidden (obsolete)
Comment 56 Kyle M Hall 2014-03-07 15:46:53 UTC
Created attachment 25937 [details] [review]
Bug 10558 [QA Follow-up]
Comment 57 Kyle M Hall 2014-03-07 15:48:26 UTC
(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.
Comment 58 Galen Charlton 2014-03-10 15:52:04 UTC
Pushed to master, along with a tiny follow-up.  Thanks, Kyle!
Comment 59 Fridolin Somers 2014-05-27 13:19:02 UTC
Too big to push in 3.14.