Bug 29408 - The datatables api wrapper is ambiguously named
Summary: The datatables api wrapper is ambiguously named
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 28854 29302 29896
  Show dependency treegraph
 
Reported: 2021-11-03 16:24 UTC by Martin Renvoize
Modified: 2022-01-17 15:23 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
This patch 1) renames the Koha REST JS dataTables wrapper from the ambiguous 'api' to the clearer 'kohaTable' 2) goes through the codebase and updates existing relevant calls to .api referencing the Koha REST dataTables wrapper to use the name 'kohaTable', and 3) adds JSDoc formatted parameter documentation for the kohaTable function.
Version(s) released in:
21.11.00,21.05.05


Attachments
Bug 29408: Rename Koha dataTables wrapper (1.08 KB, patch)
2021-11-03 16:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29408: Update all existing uses of .api to .kohaTable (4.78 KB, patch)
2021-11-03 16:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29408: Begginings of JSDoc documentation (1.37 KB, patch)
2021-11-03 16:54 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29408: Rename Koha dataTables wrapper (1.13 KB, patch)
2021-11-03 18:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29408: Update all existing uses of .api to .kohaTable (4.84 KB, patch)
2021-11-03 18:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29408: Begginings of JSDoc documentation (1.43 KB, patch)
2021-11-03 18:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29408: Rename Koha dataTables wrapper (1.19 KB, patch)
2021-11-04 10:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29408: Update all existing uses of .api to .kohaTable (4.89 KB, patch)
2021-11-04 10:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29408: Beginnings of JSDoc documentation (1.48 KB, patch)
2021-11-04 10:46 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29408: Rename Koha dataTables wrapper (1.19 KB, patch)
2021-11-04 12:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29408: Update all existing uses of .api to .kohaTable (4.89 KB, patch)
2021-11-04 12:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29408: Add JSDoc documentation for kohaTable function (1.98 KB, patch)
2021-11-04 12:10 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 2021-11-03 16:24:20 UTC
The dataTables Koha REST wrapper we added recently is badly named.. currently, it's the very unspecific 'api'.. we should start using a consistent naming convention for such JS wrapper functions.

In bug 29397 we introduce a minimal select2 wrapper and call it kohaSelect.. I propose we take that as inspiration and rename the dataTables function from 'api' to 'kohaTable'.

Thus, creating a new API driven dataTable would become:

$(".selector").kohaTable(usual stuff here);
Comment 1 Martin Renvoize 2021-11-03 16:34:35 UTC
Created attachment 127272 [details] [review]
Bug 29408: Rename Koha dataTables wrapper

This patch renames the Koha REST JS dataTables wrapper from the
ambigious 'api' to the clearer 'kohaTable'.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Martin Renvoize 2021-11-03 16:34:38 UTC
Created attachment 127273 [details] [review]
Bug 29408: Update all existing uses of .api to .kohaTable

This patch goes through the codebase and updates existing revelant calls
to .api referencing the Koha REST dataTables wrapper to use the name
name 'kohaTable'.

Test plan.
1) Ensure the tables on the following pages all continue to work as
   expected
   a) acqui/parcel
   b) admin/branches
   c) admin/cities
   d) admin/smtp_servers
   e) pos/register
   f) tools/quotes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2021-11-03 16:54:05 UTC
Created attachment 127274 [details] [review]
Bug 29408: Begginings of JSDoc documentation

I'd love to see the parameters clearly documented... I've started it off
but need some help.
Comment 4 Tomás Cohen Arazi 2021-11-03 18:05:48 UTC
Created attachment 127282 [details] [review]
Bug 29408: Rename Koha dataTables wrapper

This patch renames the Koha REST JS dataTables wrapper from the
ambigious 'api' to the clearer 'kohaTable'.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2021-11-03 18:05:56 UTC
Created attachment 127283 [details] [review]
Bug 29408: Update all existing uses of .api to .kohaTable

This patch goes through the codebase and updates existing revelant calls
to .api referencing the Koha REST dataTables wrapper to use the name
name 'kohaTable'.

Test plan.
1) Ensure the tables on the following pages all continue to work as
   expected
   a) acqui/parcel
   b) admin/branches
   c) admin/cities
   d) admin/smtp_servers
   e) pos/register
   f) tools/quotes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Tomás Cohen Arazi 2021-11-03 18:06:03 UTC
Created attachment 127284 [details] [review]
Bug 29408: Begginings of JSDoc documentation

I'd love to see the parameters clearly documented... I've started it off
but need some help.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 7 Tomás Cohen Arazi 2021-11-03 18:08:25 UTC
I did

git grep '\.api('

and the cases I found are correct (i.e. JS accessing the .api() method from DataTables.

All looks good and works as before this patchset.
Comment 8 Martin Renvoize 2021-11-03 18:28:11 UTC
Thanks for testing.. yup, those two calls to .api( that still exist are one of the reasons I was really keen to make this change :-).
Comment 9 Kyle M Hall 2021-11-04 10:46:33 UTC
Created attachment 127290 [details] [review]
Bug 29408: Rename Koha dataTables wrapper

This patch renames the Koha REST JS dataTables wrapper from the
ambigious 'api' to the clearer 'kohaTable'.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 10 Kyle M Hall 2021-11-04 10:46:49 UTC
Created attachment 127291 [details] [review]
Bug 29408: Update all existing uses of .api to .kohaTable

This patch goes through the codebase and updates existing revelant calls
to .api referencing the Koha REST dataTables wrapper to use the name
name 'kohaTable'.

Test plan.
1) Ensure the tables on the following pages all continue to work as
   expected
   a) acqui/parcel
   b) admin/branches
   c) admin/cities
   d) admin/smtp_servers
   e) pos/register
   f) tools/quotes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 11 Kyle M Hall 2021-11-04 10:46:52 UTC
Created attachment 127292 [details] [review]
Bug 29408: Beginnings of JSDoc documentation

I'd love to see the parameters clearly documented... I've started it off
but need some help.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 12 Jonathan Druart 2021-11-04 11:49:50 UTC
We already have "KohaTable", isn't this more ambiguous?
Comment 13 Martin Renvoize 2021-11-04 11:59:11 UTC
(In reply to Jonathan Druart from comment #12)
> We already have "KohaTable", isn't this more ambiguous?

I think the difference between `KohaTable('selector', settings)` and `$('selector').kohaTable(settings)` is clearer than the current `$('selector').api(settings)`.. what is `.api()`.. the jQuery API, the selected elements api.. it also doesn't help that datatables itself has an api() method to get back the datatables api object.

I think we should slowly be migrating from template-driven datatables (KohaTable) to api driven datatable (kohaTable).. so to me, the similarity is deliberate and the distinction is clear.

Also, by starting to add JSDoc, my hope is people will reach for kohaTable first in the future ;)
Comment 14 Martin Renvoize 2021-11-04 11:59:55 UTC
I do have a couple of improvements to make on that last patch though.. I clarified a few things with Tomas this morning, but Kyle was super quick to review what was already there before I got to update it ;)
Comment 15 Martin Renvoize 2021-11-04 12:10:24 UTC
Created attachment 127313 [details] [review]
Bug 29408: Rename Koha dataTables wrapper

This patch renames the Koha REST JS dataTables wrapper from the
ambigious 'api' to the clearer 'kohaTable'.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Martin Renvoize 2021-11-04 12:10:29 UTC
Created attachment 127314 [details] [review]
Bug 29408: Update all existing uses of .api to .kohaTable

This patch goes through the codebase and updates existing revelant calls
to .api referencing the Koha REST dataTables wrapper to use the name
name 'kohaTable'.

Test plan.
1) Ensure the tables on the following pages all continue to work as
   expected
   a) acqui/parcel
   b) admin/branches
   c) admin/cities
   d) admin/smtp_servers
   e) pos/register
   f) tools/quotes

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Martin Renvoize 2021-11-04 12:10:33 UTC
Created attachment 127315 [details] [review]
Bug 29408: Add JSDoc documentation for kohaTable function

This patch adds some JSDoc formatted parameter documentation for the
kohaTable function.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Martin Renvoize 2021-11-04 12:11:20 UTC
Amendments to documentation made, with agreements from Tomas.. back to PQA
Comment 19 Jonathan Druart 2021-11-15 11:40:27 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 20 Tomás Cohen Arazi 2021-11-15 16:55:31 UTC
Steps forward!

\o/
Comment 21 David Cook 2021-11-15 23:07:06 UTC
(In reply to Martin Renvoize from comment #13)
> 
> I think we should slowly be migrating from template-driven datatables
> (KohaTable) to api driven datatable (kohaTable).. so to me, the similarity
> is deliberate and the distinction is clear.
> 

Agreed and much appreciated! The .api() method name was driving me crazy.
Comment 22 Kyle M Hall 2021-11-19 16:39:22 UTC
Pushed to 21.05.x for 21.05.05
Comment 23 Fridolin Somers 2021-11-20 00:23:24 UTC
Shouldn't we also change in koha-tmpl/opac-tmpl/bootstrap/js/datatables.js ?
Comment 24 Martin Renvoize 2021-11-20 13:41:23 UTC
Good call... I didn't actually realise we had the api wrapper on the opac as yet...

We need a better way of keeping files like this in sync.. I'd put money on there being other bits of the wrapper that are already out of sync
Comment 25 Victor Grousset/tuxayo 2021-12-14 20:41:30 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed. (and if the above comments are followed upon)