Bug 31842 - admin/branches: DT search generates js error on col.data.split
Summary: admin/branches: DT search generates js error on col.data.split
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Katrin Fischer
URL:
Keywords:
Depends on: 29144
Blocks: 31754
  Show dependency treegraph
 
Reported: 2022-10-17 09:32 UTC by Marcel de Rooy
Modified: 2023-12-28 20:42 UTC (History)
4 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:
22.11.00


Attachments
Bug 31842: Move data function to render in branches.tt (3.94 KB, patch)
2022-10-17 10:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31842: Move data function to render in branches.tt (3.95 KB, patch)
2022-10-17 15:17 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 31842: Move data function to render in branches.tt (4.00 KB, patch)
2022-10-17 21:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31842: Move data function to render in branches.tt (1.68 KB, patch)
2022-10-19 11:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31842: Fix DT search error on col.data.split (1.60 KB, patch)
2022-10-19 11:23 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 31842: Fix DT search error on col.data.split (1.60 KB, patch)
2022-10-19 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2022-10-17 09:32:38 UTC
See comment73 on bug 29144. Looks like a rebase issue. Addressing it here.
Comment 1 Marcel de Rooy 2022-10-17 09:44:49 UTC
It is no rebase issue after all.
Comment 2 Marcel de Rooy 2022-10-17 10:39:54 UTC
Created attachment 141953 [details] [review]
Bug 31842: Move data function to render in branches.tt

This applies to dataTable columns: library_info, actions and
opac_info.
Set searchable to false for opac_info too.

Test plan:
Check if DT search for libraries works again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 3 Jonathan Druart 2022-10-17 14:07:06 UTC
+                        "data": "actions",

There is some magic in datatables.js that is using this (for sorting, searching, etc.) and expect it to be an attribute of library. I would not use "actions".

Actually it's the same of the other occurrences.

It's not a problem to have a function defined for 'data', it just should not be searchable.
Comment 4 Jonathan Druart 2022-10-17 14:07:42 UTC
(In reply to Jonathan Druart from comment #3)
> It's not a problem to have a function defined for 'data', it just should not
> be searchable.

Do you agree with that?
Comment 5 Owen Leonard 2022-10-17 15:17:54 UTC
Created attachment 141993 [details] [review]
Bug 31842: Move data function to render in branches.tt

This applies to dataTable columns: library_info, actions and
opac_info.
Set searchable to false for opac_info too.

Test plan:
Check if DT search for libraries works again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Katrin Fischer 2022-10-17 21:02:08 UTC
Created attachment 142032 [details] [review]
Bug 31842: Move data function to render in branches.tt

This applies to dataTable columns: library_info, actions and
opac_info.
Set searchable to false for opac_info too.

Test plan:
Check if DT search for libraries works again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 7 Marcel de Rooy 2022-10-18 12:51:38 UTC
(In reply to Jonathan Druart from comment #4)
> (In reply to Jonathan Druart from comment #3)
> > It's not a problem to have a function defined for 'data', it just should not
> > be searchable.
> 
> Do you agree with that?

Hmm. It looks a bit inconsistent. I prefer the move to render now. And it works.
Comment 8 Marcel de Rooy 2022-10-18 12:52:21 UTC
(In reply to Jonathan Druart from comment #3)
> +                        "data": "actions",
> 
> There is some magic in datatables.js that is using this (for sorting,
> searching, etc.) and expect it to be an attribute of library. I would not
> use "actions".

Where can I find that? We can change these "labels" where needed..
Comment 9 Marcel de Rooy 2022-10-18 12:53:37 UTC
I do see an action function but I dont see actions in datatables.js
Comment 10 Jonathan Druart 2022-10-19 08:43:56 UTC
(In reply to Marcel de Rooy from comment #7)
> (In reply to Jonathan Druart from comment #4)
> > (In reply to Jonathan Druart from comment #3)
> > > It's not a problem to have a function defined for 'data', it just should not
> > > be searchable.
> > 
> > Do you agree with that?
> 
> Hmm. It looks a bit inconsistent. I prefer the move to render now. And it
> works.

Ok but if you keep data:function and set searchable to false, it also works. "It works" is not enough ;)

(In reply to Marcel de Rooy from comment #8)
> (In reply to Jonathan Druart from comment #3)
> > +                        "data": "actions",
> > 
> > There is some magic in datatables.js that is using this (for sorting,
> > searching, etc.) and expect it to be an attribute of library. I would not
> > use "actions".
> 
> Where can I find that? We can change these "labels" where needed..

"data" is not a label
https://datatables.net/reference/option/columns.data
vs
https://datatables.net/reference/option/columns.render

`data: "actions"` does not mean anything.

The magic is: the DT REST API wrapper is using the "data" value to build the table, it must be a value returned by the REST API response.

(In reply to Marcel de Rooy from comment #9)
> I do see an action function but I dont see actions in datatables.js

See above. Occurrences are:
583                                     var attributes = col.data.split(':');
607                                     return col.bSearchable && typeof col.data == 'string' && data.columns[col.idx].search.value != ''
685                                         var order_by       = options.columns[order_col].data;
Comment 11 Marcel de Rooy 2022-10-19 11:02:32 UTC
(In reply to Jonathan Druart from comment #10)
> Ok but if you keep data:function and set searchable to false, it also works.
> "It works" is not enough ;)
I will revert most changes now. But imo dataTables or the additional customizations are buggy and/or inconsistent.
If data may be a function, you simply need to check col.data.split.. Definitely a bug.
Comment 12 Marcel de Rooy 2022-10-19 11:20:04 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2022-10-19 11:23:11 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2022-10-19 11:24:42 UTC
Created attachment 142148 [details] [review]
Bug 31842: Fix DT search error on col.data.split

Set searchable to false for opac_info.

Test plan:
Check if DT search for libraries works again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Reverted the data/render function change as requested
by Jonathan. Considering the fact that dataTables or custom
extensions do not check col.data.split as a clear bug btw.
Comment 15 Marcel de Rooy 2022-10-19 11:25:06 UTC
Edited the commit message ;)
Comment 16 Tomás Cohen Arazi 2022-10-20 14:44:53 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 17 Tomás Cohen Arazi 2022-10-20 18:56:26 UTC
(In reply to Marcel de Rooy from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > Ok but if you keep data:function and set searchable to false, it also works.
> > "It works" is not enough ;)
> I will revert most changes now. But imo dataTables or the additional
> customizations are buggy and/or inconsistent.
> If data may be a function, you simply need to check col.data.split..
> Definitely a bug.

Are you going to file a bug about that?
Comment 18 Tomás Cohen Arazi 2022-10-20 23:19:51 UTC
Hi, after pushing this I noticed the conversation between Jonathan and Marcel is not entirely closed.

My feeling now is that we have hidden the problem by making the field not searchable, but a proper solution would be a bit more complex.

It this is the case, please file a bug to discuss it an I'm willing to add my two cents for the solution.
Comment 19 Marcel de Rooy 2022-10-21 06:05:54 UTC
The bug has been addressed now. I will not file another bug, but I gave my observations here. We have more code issues. Cant solve them all ;)