It is no rebase issue after all.

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>

+                        "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.

(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?

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>

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>

(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.

(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..

I do see an action function but I dont see actions in datatables.js

(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;

(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.

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.

Edited the commit message ;)

Pushed to master for 22.11.

Nice work everyone, thanks!

(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?

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.

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 ;)