Bug 18211 - Circulation.pl / Moremember.pl has errors in table row id
Summary: Circulation.pl / Moremember.pl has errors in table row id
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-04 19:21 UTC by Christopher Brannon
Modified: 2018-09-27 18:21 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18211: Corrected intended id's in jquery. (2.49 KB, patch)
2017-03-10 15:36 UTC, Christopher Brannon
Details | Diff | Splinter Review
Bug 18211: Corrected intended id's in jquery. (2.54 KB, patch)
2018-09-16 18:56 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 18211: Corrected intended id's in jquery. (2.60 KB, patch)
2018-09-16 20:39 UTC, Brendan Gallagher
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Christopher Brannon 2017-03-04 19:21:14 UTC
The table row showing the label "Today's checkouts" shows the id of "group-id-issues-table_-strong-today-s-checkouts-strong-"  The id is inaccurate and is taking on values from a child cell.
Comment 1 Christopher Brannon 2017-03-10 15:36:15 UTC
Created attachment 60997 [details] [review]
Bug 18211: Corrected intended id's in jquery.

To test:
1) Checkout items - if you can, have items checked out from previous day
too, but not necessary.
2) Review source code and see that the ids on the <tr> and <td> for
cells containing "Today's checkouts" and "Previous checkouts" labels
have some crazy ids with partial html tags (strong) and other
unnecessary elements.
3) Apply patch.
4) Examine source code again and see a better and smaller id.
Comment 2 Alex Buckley 2017-04-17 09:32:16 UTC
Hi Christopher

I am still seeing partial html tags in the ids for todays checkout and previous checkouts with your patch applied, for example:

  var RENEWALS_REMAINING = _("%s of %s renewals remaining");
    var HOLD_IS_SUSPENDED = _("Hold is <strong>suspended</strong>");
    var UNTIL = _("until %s");
    var NEXT_AVAILABLE_ITYPE = _("Next available %s item");
    var ITEM_IS_WAITING = _("Item is <strong>waiting</strong>");
    var ITEM_IS_WAITING_HERE = _("Item is <strong>waiting here</strong>");
    var AT = _("at %s");
    var ITEM_IS_IN_TRANSIT = _("Item is <strong>in transit</strong> from %s since %s");

Can you please clarify your test plan further to clarify if this is intended or not
Comment 3 Owen Leonard 2018-09-16 18:56:39 UTC Comment hidden (obsolete)
Comment 4 Brendan Gallagher 2018-09-16 20:39:13 UTC
Created attachment 79001 [details] [review]
Bug 18211: Corrected intended id's in jquery.

To test:
1) Checkout items - if you can, have items checked out from previous day
too, but not necessary.
2) Review source code and see that the ids on the <tr> and <td> for
cells containing "Today's checkouts" and "Previous checkouts" labels
have some crazy ids with partial html tags (strong) and other
unnecessary elements.
3) Apply patch.
4) Examine source code again and see a better and smaller id.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 5 Jonathan Druart 2018-09-16 21:25:57 UTC
Changes made to jquery.dataTables.rowGrouping.js will be overwritten when we will upgrade to a newer version, we should not modify these jQuery plugins.

What is your need exactly?
Comment 6 Christopher Brannon 2018-09-16 22:11:25 UTC
The need is to correctly apply an id to these rows.  They are malformed.
Comment 7 Jonathan Druart 2018-09-19 20:36:29 UTC
This is not clear to me, switching back to Signed off to get other opinions.
Comment 8 Katrin Fischer 2018-09-24 19:00:36 UTC
Adding Owen for an additional opinion on how to fix this.
Comment 9 Mark Tompsett 2018-09-24 19:06:19 UTC
(In reply to Jonathan Druart from comment #5)
> ... we should not modify these jQuery plugins.

I agree. If this is a dependency, then we shouldn't be tweaking it.
Comment 10 Marcel de Rooy 2018-09-24 19:43:29 UTC
(In reply to M. Tompsett from comment #9)
> (In reply to Jonathan Druart from comment #5)
> > ... we should not modify these jQuery plugins.
> 
> I agree. If this is a dependency, then we shouldn't be tweaking it.

True. If we pass the wrong values, correct them at the source.
Comment 11 Owen Leonard 2018-09-25 15:19:53 UTC
(In reply to Jonathan Druart from comment #5)
> Changes made to jquery.dataTables.rowGrouping.js will be overwritten when we
> will upgrade to a newer version, we should not modify these jQuery plugins.

Agreed. While I was focusing on the effect of the patch I wasn't looking at how the changes were made -- my bad.
Comment 12 Christopher Brannon 2018-09-25 16:05:13 UTC
I'll have to take a look at what I did and see if I can do it the correct way.  :/
Comment 13 Christopher Brannon 2018-09-27 18:10:03 UTC
I think I am going to scrap this bug altogether.  Id's seem to be created by jquery.dataTables.rowGrouping.js, which is an outside library that was written in 2013.  I am not seeing a newer version of it.  There are other scripts written for row grouping.  In particular, one that appears to be active is:

https://cdn.datatables.net/v/dt/dt-1.10.18/rg-1.0.3/datatables.min.js

This is a bit beyond me in tackling a project like this.  If someone else wants to see if this particular script would work and improve on the id's, that would be great.  For now, I am unable to do so.
Comment 14 Christopher Brannon 2018-09-27 18:16:14 UTC
Just so we are clear on the issue, it looks like jquery.dataTables.rowGrouping.js is adding an unneeded prefix "group-id-" to id's, and including html tags in the sGroup.  My solution was to remove the prefix and point the sGroup to the text only, and add another regex to remove some other random characters.

Unless we want to adopt this script into koha's libraries and maintain it ourselves, we should probably consider updating to a newer script at some point, like the one I suggested.  I haven't tested it, so I don't know if it is any better or worse that what is currently being used.