Bug 23493 - jquery.dataTables.rowGrouping.js is no longer maintained, but there is an official DataTables version we could switch to
Summary: jquery.dataTables.rowGrouping.js is no longer maintained, but there is an off...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact:
URL:
Keywords: additional_work_needed
Depends on: 23013
Blocks: 23355 24533
  Show dependency treegraph
 
Reported: 2019-08-22 14:58 UTC by Martin Renvoize
Modified: 2020-06-04 20:38 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00


Attachments
Bug 23493: Replace rowGrouping with rowGroup (1.67 KB, patch)
2019-08-23 10:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23493: Replace rowGrouping with rowGroup (5.15 KB, patch)
2019-08-23 11:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23493: Replace rowGrouping with rowGroup (5.13 KB, patch)
2020-01-13 16:22 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23493: Replace rowGrouping with rowGroup (5.20 KB, patch)
2020-01-14 11:54 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 23493: Replace rowGrouping with rowGroup (5.30 KB, patch)
2020-01-22 09:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23493: Replace rowGrouping with rowGroup (46.17 KB, patch)
2020-01-22 12:36 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23493: Replace rowGrouping with rowGroup (46.17 KB, patch)
2020-01-28 17:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23493: (follow-up) Switch to aDataSort for multi-column sorting (5.31 KB, patch)
2020-01-28 17:20 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23493: Replace rowGrouping with rowGroup (46.27 KB, patch)
2020-01-29 09:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23493: Remove commented lines (1.18 KB, patch)
2020-01-29 09:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23493: Fix selenium/regressions.t (1.58 KB, patch)
2020-01-30 09:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23493: (RM follow-up) Let datatables calculate colspan for us (1.52 KB, patch)
2020-01-30 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-08-22 14:58:03 UTC
We currently sparsely use this plugin on the checkouts table.

It would be nice to switch to the equivalent plugin by the datatables authors and bundle it into our package.

Doing so would allow other developers to take advantage of the feature as that version is well documented and supported.
Comment 1 Martin Renvoize 2019-08-23 10:21:48 UTC Comment hidden (obsolete)
Comment 2 Martin Renvoize 2019-08-23 10:23:47 UTC
This is a somewhat rough and ready attempt at replacement to get someone started.

I've directly included the new plugin via a CDN link, this would clearly need to be bundled in our datatables bundle, but I didn't want to add all there here in the first instance and confuse reviewers.

I'm sure there's still more code that could be removed from the js file but it would be good to get some initial feedback or perhaps for someone to take it on.
Comment 3 Kyle M Hall 2019-08-23 10:47:06 UTC
Is this patch complete? I'm not seeing the CDN import line among other things :)

(In reply to Martin Renvoize from comment #2)
> This is a somewhat rough and ready attempt at replacement to get someone
> started.
> 
> I've directly included the new plugin via a CDN link, this would clearly
> need to be bundled in our datatables bundle, but I didn't want to add all
> there here in the first instance and confuse reviewers.
> 
> I'm sure there's still more code that could be removed from the js file but
> it would be good to get some initial feedback or perhaps for someone to take
> it on.
Comment 4 Martin Renvoize 2019-08-23 11:11:30 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2019-08-23 11:11:54 UTC
Oops, forgot to squash  before uploading to BZ... should be better now.
Comment 6 Martin Renvoize 2020-01-13 16:22:24 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2020-01-14 11:54:38 UTC
Created attachment 97344 [details] [review]
Bug 23493: Replace rowGrouping with rowGroup

This is an initial attempt to get some feedback.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2020-01-14 11:56:32 UTC
Looks great to me! I also find the styling to be a bit improved.
Comment 9 Katrin Fischer 2020-01-15 23:04:11 UTC
I think this should be signed off - changing status.
Comment 10 Martin Renvoize 2020-01-22 09:20:24 UTC
Created attachment 97694 [details] [review]
Bug 23493: Replace rowGrouping with rowGroup

This patch replaces the unmaintained third party rowGrouping datatables
plugin with the supported core rowGroup replacement.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Martin Renvoize 2020-01-22 09:20:45 UTC
Simple rebase performed.
Comment 12 Jonathan Druart 2020-01-22 09:36:20 UTC
I am expecting this bug report to add the rowGroup plugin, not bug 23355.
It seems easy to make the dependency the other way around.
Comment 13 Martin Renvoize 2020-01-22 12:36:02 UTC
Created attachment 97704 [details] [review]
Bug 23493: Replace rowGrouping with rowGroup

This patch replaces the unmaintained third party rowGrouping datatables
plugin with the supported core rowGroup replacement.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 14 Martin Renvoize 2020-01-22 12:37:22 UTC
Dependency re-ordered as requested and suitable re-base undertaken.
Comment 15 Jonathan Druart 2020-01-23 09:56:48 UTC
 609             "fnInitComplete": function(oSettings, json) {
 610                 // Disable rowGroup plugin after first use
 611                 // so any sorting on the table doesn't use it
 612                 //var oSettings = issuesTable.fnSettings();

Should we remove those 3 commented lines?
Comment 16 Jonathan Druart 2020-01-23 10:03:04 UTC
One change I noticed: when the column are sorted the "grouped tr" are displayed with this new version:
https://snipboard.io/HzXJ4l.jpg (before)
https://snipboard.io/zRb4sk.jpg (after)
Comment 17 Martin Renvoize 2020-01-28 10:10:25 UTC
Hmm, 

Removing the commented lines seems sensible.. I'll do that now..

As for the display discrepancy you mention.. I can't seem to replicate that.

https://snipboard.io/1DOacC.jpg (before)
https://snipboard.io/gOLAkD.jpg (after)

There are subtle styling differences (which I'm sure can be resolved at the CSS level if we want to do so?), but I can't replicate the 'before' you show without grouping or the after with two "Today's" groupings.
Comment 18 Jonathan Druart 2020-01-28 10:20:38 UTC
I bet you did not sort the column by "due date".
Take a look at the "checked out on" and "due date" column on my screenshots.
Comment 19 Martin Renvoize 2020-01-28 17:20:43 UTC
Created attachment 98041 [details] [review]
Bug 23493: Replace rowGrouping with rowGroup

This patch replaces the unmaintained third party rowGrouping datatables
plugin with the supported core rowGroup replacement.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Martin Renvoize 2020-01-28 17:20:46 UTC
Created attachment 98042 [details] [review]
Bug 23493: (follow-up) Switch to aDataSort for multi-column sorting

This patch adds aDataSort configurations for each sortable column such
that we always maintain the group sorting primarily and apply a
secondary sort using the column in question.
Comment 21 Martin Renvoize 2020-01-28 17:21:30 UTC
OK, I see what you mean now.. good catch.

See followup which I believe addresses the issue.
Comment 22 Jonathan Druart 2020-01-29 09:23:22 UTC
I do not think it's correct, now we cannot really sort by due date:
https://snipboard.io/TRx43j.jpg
Comment 23 Jonathan Druart 2020-01-29 09:32:01 UTC
Created attachment 98055 [details] [review]
Bug 23493: Replace rowGrouping with rowGroup

This patch replaces the unmaintained third party rowGrouping datatables
plugin with the supported core rowGroup replacement.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 24 Jonathan Druart 2020-01-29 09:32:05 UTC
Created attachment 98056 [details] [review]
Bug 23493: Remove commented lines

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 25 Jonathan Druart 2020-01-29 09:34:00 UTC
Ok so I confused Martin with my previous comments.

I wanted to highlight the fact that there was a behavior's change, but did not consider it a regression or blocker. I am happy with how it works as it.

I have attached a follow-up to remove the 3 commented lines.

Marked as Passed QA!
Comment 26 Martin Renvoize 2020-01-29 11:32:54 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 27 Jonathan Druart 2020-01-29 14:31:49 UTC
(In reply to Martin Renvoize from comment #26)
> Nice work everyone!
> 
> Pushed to master for 20.05

There is something very wrong here Martin. You pushed:
Bug 23493: (follow-up) Disable rowGroup on due date sort
Bug 23493: (follow-up) Switch to aDataSort for multi-column sorting
Bug 23493: Replace rowGrouping with rowGroup

They all have my signed off. However I disagreed with the last 2. Additionally the last one has never been published here.
Comment 28 Martin Renvoize 2020-01-29 16:47:12 UTC
Corrected by reverting the errantly pushed patches and applying the followup here instead.
Comment 29 Jonathan Druart 2020-01-30 09:22:59 UTC
Created attachment 98118 [details] [review]
Bug 23493: Fix selenium/regressions.t

One test wants to make sure thead, tfood and tbody will have th/td's
with the same size/number.
Existing test for tbody was retrieving the grouped td, which has a
colspan of 100%. Note that this 100% value seems to be wrong as it is
interpreted like "100" by the browser. We do not want to test this row
anyway, but the real row, the one with all the td's.

This patch makes it retrieve the second tr and count the number of td it
has.
Comment 30 Martin Renvoize 2020-01-30 09:41:03 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 31 Martin Renvoize 2020-01-30 09:44:52 UTC
Created attachment 98132 [details] [review]
Bug 23493: (RM follow-up) Let datatables calculate colspan for us

Prior to this we arbitrarily set a colspan of 100 for the rowgroup
header row.  If we remove the td wrapper from the render function then
datatables will automatically wrap the output in an appropraite td with
cell with fullwidth colspan correctly set.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>