Bug 15481 - Remove dead code in datatables.js
Summary: Remove dead code in datatables.js
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-01-06 14:35 UTC by Julian Maurice
Modified: 2017-06-14 22:05 UTC (History)
4 users (show)

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


Attachments
Bug 15481: Remove dead code in datatables.js (5.25 KB, patch)
2016-01-06 14:36 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 15481: Remove dead code in datatables.js (5.38 KB, patch)
2016-01-06 14:45 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 15481: Remove dead code in datatables.js (5.45 KB, patch)
2016-01-08 10:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2016-01-06 14:35:50 UTC
Remove dead code in datatables.js
Comment 1 Julian Maurice 2016-01-06 14:36:28 UTC Comment hidden (obsolete)
Comment 2 Marc Véron 2016-01-06 14:45:31 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-01-07 12:17:04 UTC
Julian,
IIRC, these functions were originally used by an alternative patch of bug 5342, never pushed (see attachment 8910 [details] [review], Followup : DataTables server-side processing).
Are there no plan to rebase these changes?
Comment 4 Julian Maurice 2016-01-07 21:18:13 UTC
I'm not sure if there are plans to rebase this patch, but in this case I'm sure it could be rewritten differently (i.e. better) without these functions. And let's face it, replace_html is such a bad name for a JS function :)
Comment 5 Jonathan Druart 2016-01-08 10:24:43 UTC
Created attachment 46417 [details] [review]
Bug 15481: Remove dead code in datatables.js

This removes the following functions that are not used anywhere:
- dt_add_rangedate_filter
- dt_overwrite_string_sorting_localeCompare
- replace_html
- replace_html_date

Test plan:
1. Search for calls to the above functions in the code. There should be
   none.
2. Check that existing tables using DataTables are not broken.

Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Marcel de Rooy 2016-02-26 07:44:13 UTC
I am having the impression that some of this dead code was still used.
And this patch broke some datatables functionality.
For instance, look at some tables in Administration: Libraries, Item types.
Where is the search bar?
Comment 8 Marcel de Rooy 2016-02-26 07:46:17 UTC
Should we revert this one?
Comment 9 Julian Maurice 2016-02-26 08:34:22 UTC
(In reply to Marcel de Rooy from comment #7)
> I am having the impression that some of this dead code was still used.
> And this patch broke some datatables functionality.
> For instance, look at some tables in Administration: Libraries, Item types.
> Where is the search bar?

(In reply to Marcel de Rooy from comment #8)
> Should we revert this one?

If you revert it, does the search bar reappear ?
Comment 10 Owen Leonard 2016-02-26 14:01:36 UTC
> For instance, look at some tables in Administration: Libraries, Item types.
> Where is the search bar?

Running git bisect to diagnose a similar problem with the holds to pull report led to Bug 15916 - Regression: Many tables' sorting broken by JavaScript error. Perhaps the same issue?(In reply to Marcel de Rooy from comment #7)
Comment 11 Brendan Gallagher 2016-02-27 15:32:36 UTC
Will definitely look at this again - thanks.
Comment 12 Marcel de Rooy 2016-02-29 09:02:13 UTC
(In reply to Marcel de Rooy from comment #8)
> Should we revert this one?

No. Bug 15916 solved the issue. So this patch was fine after all :)
Comment 13 Brendan Gallagher 2016-02-29 15:18:15 UTC
(In reply to Marcel de Rooy from comment #12)
> (In reply to Marcel de Rooy from comment #8)
> > Should we revert this one?
> 
> No. Bug 15916 solved the issue. So this patch was fine after all :)

Awesome :)