Summary: | admin/branches: DT search generates js error on col.data.split | ||
---|---|---|---|
Product: | Koha | Reporter: | Marcel de Rooy <m.de.rooy> |
Component: | Architecture, internals, and plumbing | Assignee: | Marcel de Rooy <m.de.rooy> |
Status: | CLOSED FIXED | QA Contact: | Katrin Fischer <katrin.fischer> |
Severity: | normal | ||
Priority: | P5 - low | CC: | jonathan.druart, lucas, oleonard, tomascohen |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
22.11.00
|
|
Circulation function: | |||
Bug Depends on: | 29144 | ||
Bug Blocks: | 31754 | ||
Attachments: |
Bug 31842: Move data function to render in branches.tt
Bug 31842: Move data function to render in branches.tt Bug 31842: Move data function to render in branches.tt Bug 31842: Move data function to render in branches.tt Bug 31842: Fix DT search error on col.data.split Bug 31842: Fix DT search error on col.data.split |
Description
Marcel de Rooy
2022-10-17 09:32:38 UTC
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 142144 [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> 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 data.col.split as a clear bug btw. Created attachment 142146 [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 data.col.split as a clear bug btw. 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 ;) |