Bug 25279 - Make the cities list use the API
Summary: Make the cities list use the API
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on: 24561
Blocks: 25288
  Show dependency treegraph
 
Reported: 2020-04-24 14:43 UTC by Tomás Cohen Arazi
Modified: 2020-05-18 09:49 UTC (History)
9 users (show)

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


Attachments
Bug 25279: Add general query parameters to the /cities route (5.75 KB, patch)
2020-04-24 14:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: Use the API to list cities (5.78 KB, patch)
2020-04-24 14:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: Add general query parameters to the /cities route (5.82 KB, patch)
2020-04-27 19:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25279: Use the API to list cities (5.85 KB, patch)
2020-04-27 19:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25279: Fix edit and delete links (1.62 KB, patch)
2020-04-27 19:36 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25279: (QA follow-up) Make button texts translatable (1.77 KB, patch)
2020-05-01 05:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: (QA follow-up) Make button texts translatable (1.72 KB, patch)
2020-05-01 05:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: (QA follow-up) Make button texts translatable (1.72 KB, patch)
2020-05-05 12:07 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: (QA follow-up) Original behaviour (5.42 KB, patch)
2020-05-05 22:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: HTML escape cells (1.42 KB, patch)
2020-05-06 11:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: Use another var name for the filter (2.67 KB, patch)
2020-05-06 13:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: Use another var name for the filter (2.72 KB, patch)
2020-05-06 13:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: URI-escape city_id in links (2.27 KB, patch)
2020-05-06 13:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: URI-escape city_id in links (2.29 KB, patch)
2020-05-06 14:02 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25279: Add general query parameters to the /cities route (5.88 KB, patch)
2020-05-06 14:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: Use the API to list cities (5.90 KB, patch)
2020-05-06 14:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: Fix edit and delete links (1.68 KB, patch)
2020-05-06 14:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: (QA follow-up) Make button texts translatable (1.79 KB, patch)
2020-05-06 14:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: (QA follow-up) Original behaviour (5.49 KB, patch)
2020-05-06 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: HTML escape cells (1.49 KB, patch)
2020-05-06 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: Use another var name for the filter (2.79 KB, patch)
2020-05-06 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: URI-escape city_id in links (2.36 KB, patch)
2020-05-06 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: Replace encodeURI with encodeURIComponent (1.10 KB, patch)
2020-05-06 14:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25279: (QA follow-up) Use .escapeHtml (1.75 KB, patch)
2020-05-07 13:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-04-24 14:43:52 UTC

    
Comment 1 Tomás Cohen Arazi 2020-04-24 14:48:10 UTC
My idea is to highlight how cool and easy is to convert our datatables using the API wrapper introduced by bug 24561.

The two examples we had in bugzilla are taking a bit long because they are rather complex cases:
- Holds history: stuck in QA because of the reserves+old_reserves issue, and no feedback from other devs on ways out of the FQA status.
- Slowliness on acquisitions is probably the most ocmplex example I could've picked. Still a few use cases that I haven't coveder

So I submit this one, so other devs know they can use it, and know how to do it. The only missing piece on this one is embedding related objects and using them for rendering. But that doesn't block this one to be useful anyways.
Comment 2 Tomás Cohen Arazi 2020-04-24 14:52:26 UTC
Created attachment 103693 [details] [review]
Bug 25279: Add general query parameters to the /cities route

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 Tomás Cohen Arazi 2020-04-24 14:52:30 UTC
Created attachment 103694 [details] [review]
Bug 25279: Use the API to list cities

This patch makes the general cities datatable use the API for rendering.
To test:
1. Test the datatable behaviour
2. Apply this patch
3. Repeat your tests
=> SUCCESS: Things work! Filtering and sorting specially

Bonus: Use the browser inspector to notice each interaction with the
datatable triggers an API call with the right query parameters

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Kyle M Hall 2020-04-27 19:36:21 UTC
Created attachment 103802 [details] [review]
Bug 25279: Add general query parameters to the /cities route

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Kyle M Hall 2020-04-27 19:36:48 UTC
Created attachment 103803 [details] [review]
Bug 25279: Use the API to list cities

This patch makes the general cities datatable use the API for rendering.
To test:
1. Test the datatable behaviour
2. Apply this patch
3. Repeat your tests
=> SUCCESS: Things work! Filtering and sorting specially

Bonus: Use the browser inspector to notice each interaction with the
datatable triggers an API call with the right query parameters

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 6 Kyle M Hall 2020-04-27 19:36:51 UTC
Created attachment 103804 [details] [review]
Bug 25279: Fix edit and delete links

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 7 Josef Moravec 2020-04-30 12:02:54 UTC
Comment on attachment 103803 [details] [review]
Bug 25279: Use the API to list cities

Review of attachment 103803 [details] [review]:
-----------------------------------------------------------------

Looks great, only one little issue (see below)

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/cities.tt
@@ +196,5 @@
> +                    },
> +                    {
> +                        "data": function( row, type, val, meta ) {
> +
> +                            var result = '<a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/cities.pl?op=add_form&amp;cityid='+ row.id +'"><i class="fa fa-pencil"></i> Edit</a>';

The button label is not translatable
Comment 8 Katrin Fischer 2020-04-30 22:22:12 UTC
> > +
> > +                            var result = '<a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/cities.pl?op=add_form&amp;cityid='+ row.id +'"><i class="fa fa-pencil"></i> Edit</a>';
> 
> The button label is not translatable

Not sure if the QA tools catches it, but the icon it should also have the aria attribute I think.
Comment 9 Tomás Cohen Arazi 2020-05-01 05:40:44 UTC
Created attachment 104065 [details] [review]
Bug 25279: (QA follow-up) Make button texts translatable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Tomás Cohen Arazi 2020-05-01 05:41:51 UTC
(In reply to Josef Moravec from comment #7)
> 
> The button label is not translatable

Done
Comment 11 Tomás Cohen Arazi 2020-05-01 05:46:54 UTC
Created attachment 104072 [details] [review]
Bug 25279: (QA follow-up) Make button texts translatable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 12 Tomás Cohen Arazi 2020-05-01 05:48:50 UTC
(In reply to Katrin Fischer from comment #8)
> > > +
> > > +                            var result = '<a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/cities.pl?op=add_form&amp;cityid='+ row.id +'"><i class="fa fa-pencil"></i> Edit</a>';
> > 
> > The button label is not translatable
> 
> Not sure if the QA tools catches it, but the icon it should also have the
> aria attribute I think.

There should be (at least) a coding guideline. I read the Mozilla page about it, and some tutorials, plus looked at what we do, and we are really heterogeneous regarding this.

I've added the aria-hidden=true to the icons (y), and there's also the aria-label or aria-labelledby which are (sometimes) used, but my intuition says in this  case it is not required as we are just saying this is a button, and when reading it the icon should be hidden. Am I right? I'm learning new things :-D
Comment 13 Katrin Fischer 2020-05-01 10:42:39 UTC
(In reply to Tomás Cohen Arazi from comment #12)
> (In reply to Katrin Fischer from comment #8)
> > > > +
> > > > +                            var result = '<a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/cities.pl?op=add_form&amp;cityid='+ row.id +'"><i class="fa fa-pencil"></i> Edit</a>';
> > > 
> > > The button label is not translatable
> > 
> > Not sure if the QA tools catches it, but the icon it should also have the
> > aria attribute I think.
> 
> There should be (at least) a coding guideline. I read the Mozilla page about
> it, and some tutorials, plus looked at what we do, and we are really
> heterogeneous regarding this.
> 
> I've added the aria-hidden=true to the icons (y), and there's also the
> aria-label or aria-labelledby which are (sometimes) used, but my intuition
> says in this  case it is not required as we are just saying this is a
> button, and when reading it the icon should be hidden. Am I right? I'm
> learning new things :-D

I asked Owen and we agreed that aria-hidden=true should be used on the buttons when there is an additional text. I fully agree with you about the coding guideline, adding it to next dev meeting.
Comment 14 Jonathan Druart 2020-05-05 11:56:42 UTC
That does not work, please restest.

https://snipboard.io/PR9nxJ.jpg

SyntaxError: unexpected token: string literal
Comment 15 Tomás Cohen Arazi 2020-05-05 12:07:59 UTC
Created attachment 104359 [details] [review]
Bug 25279: (QA follow-up) Make button texts translatable

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2020-05-05 12:08:54 UTC
(In reply to Jonathan Druart from comment #14)
> That does not work, please restest.
> 
> https://snipboard.io/PR9nxJ.jpg
> 
> SyntaxError: unexpected token: string literal

Thanks, I've now fixed the small typo I made.
Comment 17 Jonathan Druart 2020-05-05 14:28:02 UTC
1. Small UI changes (not advertised)

Before: https://snipboard.io/cRdEWA.jpg
After : https://snipboard.io/qep6rS.jpg

a) No more pagination (should be restored imo)
b) Missing space between action buttons
c) Default sort order is not the name instead of the ID
d) Table is displayed even if empty
e) Default number of entries was 10, not 20 (better as it)

2. "Search cities" in the header does not longer works.

3. Why the indentation changes in cities.json?

4. We reached a point where we don't have XSS issue in our templates, all variables are correctly escaped.
With this adding more JS code we should continue to enforce the rule, all the variables must be correctly escaped.

5. How does it work with the column settings?
Comment 18 Tomás Cohen Arazi 2020-05-05 14:38:53 UTC
(In reply to Jonathan Druart from comment #17)
> 1. Small UI changes (not advertised)
> 
> Before: https://snipboard.io/cRdEWA.jpg
> After : https://snipboard.io/qep6rS.jpg
> 
> a) No more pagination (should be restored imo)

Pagination works, it just doesn't have pages.

> b) Missing space between action buttons

Will take a look

> c) Default sort order is not the name instead of the ID

Will take a look

> d) Table is displayed even if empty

I don't think this is a problem. The only solution is to count them in the controller .pl before rendering the page. Would you agree with such approach?

> e) Default number of entries was 10, not 20 (better as it)

Will fix, this is configurable.

> 2. "Search cities" in the header does not longer works.

I didn't look at that. Well spotted

> 3. Why the indentation changes in cities.json?

I think my editor did that when I intended to re-indent the portion I added. Is it a problem?

> 4. We reached a point where we don't have XSS issue in our templates, all
> variables are correctly escaped.
> With this adding more JS code we should continue to enforce the rule, all
> the variables must be correctly escaped.

You mean data that comes from the API????

> 5. How does it work with the column settings?

Take a look at bug 25287 and bug 25288.
Comment 19 Jonathan Druart 2020-05-05 15:25:30 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> (In reply to Jonathan Druart from comment #17)
> > 1. Small UI changes (not advertised)
> > 
> > Before: https://snipboard.io/cRdEWA.jpg
> > After : https://snipboard.io/qep6rS.jpg
> > 
> > a) No more pagination (should be restored imo)
> 
> Pagination works, it just doesn't have pages.

Yes, I did not say it did not work. The pages should be displayed imo.

> > d) Table is displayed even if empty
> 
> I don't think this is a problem. The only solution is to count them in the
> controller .pl before rendering the page. Would you agree with such approach?

Maybe something DT-side to handle that?

> > e) Default number of entries was 10, not 20 (better as it)
> 
> Will fix, this is configurable.

No, 20 is correct.

> > 3. Why the indentation changes in cities.json?
> 
> I think my editor did that when I intended to re-indent the portion I added.
> Is it a problem?

No, I wanted to know if the existing syntax was a problem for something.
We should prevent such unnecessary changes (no blocker here).

> > 4. We reached a point where we don't have XSS issue in our templates, all
> > variables are correctly escaped.
> > With this adding more JS code we should continue to enforce the rule, all
> > the variables must be correctly escaped.
> 
> You mean data that comes from the API????

I did not investigate it, I don't know where it's best to escape them.
Comment 20 Tomás Cohen Arazi 2020-05-05 18:34:18 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Tomás Cohen Arazi from comment #18)
> > (In reply to Jonathan Druart from comment #17)
> > > 1. Small UI changes (not advertised)
> > > 
> > > Before: https://snipboard.io/cRdEWA.jpg
> > > After : https://snipboard.io/qep6rS.jpg
> > > 
> > > a) No more pagination (should be restored imo)
> > 
> > Pagination works, it just doesn't have pages.
> 
> Yes, I did not say it did not work. The pages should be displayed imo.

I will dig a bit about it and fix it.

> > > d) Table is displayed even if empty
> > 
> > I don't think this is a problem. The only solution is to count them in the
> > controller .pl before rendering the page. Would you agree with such approach?
> 
> Maybe something DT-side to handle that?

The only thing I found DataTables provided was a way to put a string on the rows place, stating there are no rows. But maybe we can overload the rendering code so it hides the table. Will take a look as well.

My next submission will just use a count for now, so there's no behaviour change and this is not blocked until we find a better solution.

> > > e) Default number of entries was 10, not 20 (better as it)
> > 
> > Will fix, this is configurable.
> 
> No, 20 is correct.

Ok!

> > > 3. Why the indentation changes in cities.json?
> > 
> > I think my editor did that when I intended to re-indent the portion I added.
> > Is it a problem?
> 
> No, I wanted to know if the existing syntax was a problem for something.
> We should prevent such unnecessary changes (no blocker here).

To be honest, I always hated the original indentation, and the cities one has diverted from newer ones when it comes to arrays. (single line vs. multi line).

> > > 4. We reached a point where we don't have XSS issue in our templates, all
> > > variables are correctly escaped.
> > > With this adding more JS code we should continue to enforce the rule, all
> > > the variables must be correctly escaped.
> > 
> > You mean data that comes from the API????
> 
> I did not investigate it, I don't know where it's best to escape them.

We will take a look, ideally we would do something DT-side, so this doesn't get borked with calls to URI/HTML encoding methods all over the place.

Thanks Jonathan for your valuable feedback.
Comment 21 Jonathan Druart 2020-05-05 19:38:31 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> (In reply to Jonathan Druart from comment #19)
> > > > 4. We reached a point where we don't have XSS issue in our templates, all
> > > > variables are correctly escaped.
> > > > With this adding more JS code we should continue to enforce the rule, all
> > > > the variables must be correctly escaped.
> > > 
> > > You mean data that comes from the API????
> > 
> > I did not investigate it, I don't know where it's best to escape them.
> 
> We will take a look, ideally we would do something DT-side, so this doesn't
> get borked with calls to URI/HTML encoding methods all over the place.

Keep in mind that "sometimes" we want to display raw data, but I don't think it should be a problem: we could unescape in that case.

"Escape them all at the source" is maybe the way to go here but you will certainly need to loop over all the values of the objects, even when we will display only few of them (perf consequences?)
Comment 22 Tomás Cohen Arazi 2020-05-05 19:50:00 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Tomás Cohen Arazi from comment #20)
> > (In reply to Jonathan Druart from comment #19)
> > > > > 4. We reached a point where we don't have XSS issue in our templates, all
> > > > > variables are correctly escaped.
> > > > > With this adding more JS code we should continue to enforce the rule, all
> > > > > the variables must be correctly escaped.
> > > > 
> > > > You mean data that comes from the API????
> > > 
> > > I did not investigate it, I don't know where it's best to escape them.
> > 
> > We will take a look, ideally we would do something DT-side, so this doesn't
> > get borked with calls to URI/HTML encoding methods all over the place.
> 
> Keep in mind that "sometimes" we want to display raw data, but I don't think
> it should be a problem: we could unescape in that case.
> 
> "Escape them all at the source" is maybe the way to go here but you will
> certainly need to loop over all the values of the objects, even when we will
> display only few of them (perf consequences?)

Yeah, we certainly need:
- html escaped representations
- URI escaped representations

so we at least need a seamless way to request those without obfuscating the syntax. Maybe add data.url and data.html methods... We'll see.
Comment 23 Tomás Cohen Arazi 2020-05-05 21:32:50 UTC
(In reply to Tomás Cohen Arazi from comment #20)
> (In reply to Jonathan Druart from comment #19)
> > (In reply to Tomás Cohen Arazi from comment #18)
> > > (In reply to Jonathan Druart from comment #17)
> > > > 1. Small UI changes (not advertised)
> > > > 
> > > > Before: https://snipboard.io/cRdEWA.jpg
> > > > After : https://snipboard.io/qep6rS.jpg
> > > > 
> > > > a) No more pagination (should be restored imo)
> > > 
> > > Pagination works, it just doesn't have pages.
> > 
> > Yes, I did not say it did not work. The pages should be displayed imo.
> 
> I will dig a bit about it and fix it.

Submitted a follow-up patch to bug 24561 as we just overlooked that value. Solved!
Comment 24 Tomás Cohen Arazi 2020-05-05 22:02:25 UTC
Created attachment 104387 [details] [review]
Bug 25279: (QA follow-up) Original behaviour

This patch does the following things:
- Uses a call to Koha::Cities->search(*)->count to display the same
  message we displayed before when no city was found/defined (datatable
  not rendered at all).
- Restores the main search box functionality, the passed param is used
  to query on the city name with wildcards on both sides, for (a)
  counting results and for (b) apending to the API call with the same
  behaviour.

The only missing bit from QA is HTML/URI escaping values from cells, but
this is going to happen at DT level most sure.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2020-05-06 11:30:16 UTC
Created attachment 104416 [details] [review]
Bug 25279: HTML escape cells

This patch makes HTML entities be escaped. It is done in a simple way.
We need to explore ways to do it at datatables.js level but it deserves
its own bug.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 26 Tomás Cohen Arazi 2020-05-06 11:37:54 UTC
(In reply to Jonathan Druart from comment #21)
> (In reply to Tomás Cohen Arazi from comment #20)
> > (In reply to Jonathan Druart from comment #19)
> > > > > 4. We reached a point where we don't have XSS issue in our templates, all
> > > > > variables are correctly escaped.
> > > > > With this adding more JS code we should continue to enforce the rule, all
> > > > > the variables must be correctly escaped.
> > > > 
> > > > You mean data that comes from the API????
> > > 
> > > I did not investigate it, I don't know where it's best to escape them.
> > 
> > We will take a look, ideally we would do something DT-side, so this doesn't
> > get borked with calls to URI/HTML encoding methods all over the place.
> 
> Keep in mind that "sometimes" we want to display raw data, but I don't think
> it should be a problem: we could unescape in that case.
> 
> "Escape them all at the source" is maybe the way to go here but you will
> certainly need to loop over all the values of the objects, even when we will
> display only few of them (perf consequences?)

The way datatables work, we can escape HTML entities 'only for rendering', the way I did it. I also tried adding my own render on a specific column, and DT correctly uses it and not the 'global' one, so no double escaping happens. So I think it is a nice implementation.

This is lazy evaluated, so no need to loop on any row/value globally. I would prefer something in datatables.js only to ease coders life, but I also think it should be on another bug report.
Comment 27 Tomás Cohen Arazi 2020-05-06 11:39:22 UTC
(In reply to Tomás Cohen Arazi from comment #26)
> (In reply to Jonathan Druart from comment #21)
> > (In reply to Tomás Cohen Arazi from comment #20)
> > > (In reply to Jonathan Druart from comment #19)
> > > > > > 4. We reached a point where we don't have XSS issue in our templates, all
> > > > > > variables are correctly escaped.
> > > > > > With this adding more JS code we should continue to enforce the rule, all
> > > > > > the variables must be correctly escaped.
> > > > > 
> > > > > You mean data that comes from the API????
> > > > 
> > > > I did not investigate it, I don't know where it's best to escape them.
> > > 
> > > We will take a look, ideally we would do something DT-side, so this doesn't
> > > get borked with calls to URI/HTML encoding methods all over the place.
> > 
> > Keep in mind that "sometimes" we want to display raw data, but I don't think
> > it should be a problem: we could unescape in that case.
> > 
> > "Escape them all at the source" is maybe the way to go here but you will
> > certainly need to loop over all the values of the objects, even when we will
> > display only few of them (perf consequences?)
> 
> The way datatables work, we can escape HTML entities 'only for rendering',
> the way I did it. I also tried adding my own render on a specific column,
> and DT correctly uses it and not the 'global' one, so no double escaping
> happens. So I think it is a nice implementation.
> 
> This is lazy evaluated, so no need to loop on any row/value globally. I
> would prefer something in datatables.js only to ease coders life, but I also
> think it should be on another bug report.

I forgot the point of that comment: so the data is escaped at rendering time, but if we want to render raw data for some reason, we can add a renderer that does that for the specific column. And the data is available as raw JSON data all the time.
Comment 29 Jonathan Druart 2020-05-06 12:53:18 UTC
When searching for something that does not return any results: https://snipboard.io/TGwL4v.jpg
Comment 30 Jonathan Druart 2020-05-06 12:53:55 UTC
(In reply to Jonathan Druart from comment #29)
> When searching for something that does not return any results:
> https://snipboard.io/TGwL4v.jpg

Same behaviour in master.
Comment 31 Jonathan Druart 2020-05-06 13:00:55 UTC
Created attachment 104417 [details] [review]
Bug 25279: Use another var name for the filter

It prevents:
* The screen to display "no city, create one" if we are
searching for a string that does not return any results.
* The list to display only the city we have just created
Comment 32 Jonathan Druart 2020-05-06 13:08:23 UTC
(In reply to Jonathan Druart from comment #31)
> Created attachment 104417 [details] [review] [review]
> Bug 25279: Use another var name for the filter
> 
> It prevents:
> * The screen to display "no city, create one" if we are
> searching for a string that does not return any results.
> * The list to display only the city we have just created

As a side-effect it also fixes comment 28.
Comment 33 Tomás Cohen Arazi 2020-05-06 13:36:23 UTC
Created attachment 104420 [details] [review]
Bug 25279: Use another var name for the filter

It prevents:
* The screen to display "no city, create one" if we are
searching for a string that does not return any results.
* The list to display only the city we have just created

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2020-05-06 13:36:29 UTC
Created attachment 104421 [details] [review]
Bug 25279: URI-escape city_id in links

This patch escapes city_id uses in URLs, and also removes redundant dom
entry as this is inherited from the global datatables configuration
anyway.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Tomás Cohen Arazi 2020-05-06 14:02:26 UTC
Created attachment 104423 [details] [review]
Bug 25279: URI-escape city_id in links

This patch escapes city_id uses in URLs, and also removes redundant dom
entry as this is inherited from the global datatables configuration
anyway.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Jonathan Druart 2020-05-06 14:12:45 UTC
Created attachment 104427 [details] [review]
Bug 25279: Add general query parameters to the /cities route

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 37 Jonathan Druart 2020-05-06 14:12:49 UTC
Created attachment 104428 [details] [review]
Bug 25279: Use the API to list cities

This patch makes the general cities datatable use the API for rendering.
To test:
1. Test the datatable behaviour
2. Apply this patch
3. Repeat your tests
=> SUCCESS: Things work! Filtering and sorting specially

Bonus: Use the browser inspector to notice each interaction with the
datatable triggers an API call with the right query parameters

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

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 38 Jonathan Druart 2020-05-06 14:12:52 UTC
Created attachment 104429 [details] [review]
Bug 25279: Fix edit and delete links

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 39 Jonathan Druart 2020-05-06 14:12:56 UTC
Created attachment 104430 [details] [review]
Bug 25279: (QA follow-up) Make button texts translatable

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 40 Jonathan Druart 2020-05-06 14:13:00 UTC
Created attachment 104431 [details] [review]
Bug 25279: (QA follow-up) Original behaviour

This patch does the following things:
- Uses a call to Koha::Cities->search(*)->count to display the same
  message we displayed before when no city was found/defined (datatable
  not rendered at all).
- Restores the main search box functionality, the passed param is used
  to query on the city name with wildcards on both sides, for (a)
  counting results and for (b) apending to the API call with the same
  behaviour.

The only missing bit from QA is HTML/URI escaping values from cells, but
this is going to happen at DT level most sure.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 41 Jonathan Druart 2020-05-06 14:13:03 UTC
Created attachment 104432 [details] [review]
Bug 25279: HTML escape cells

This patch makes HTML entities be escaped. It is done in a simple way.
We need to explore ways to do it at datatables.js level but it deserves
its own bug.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 42 Jonathan Druart 2020-05-06 14:13:07 UTC
Created attachment 104433 [details] [review]
Bug 25279: Use another var name for the filter

It prevents:
* The screen to display "no city, create one" if we are
searching for a string that does not return any results.
* The list to display only the city we have just created

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 43 Jonathan Druart 2020-05-06 14:13:11 UTC
Created attachment 104434 [details] [review]
Bug 25279: URI-escape city_id in links

This patch escapes city_id uses in URLs, and also removes redundant dom
entry as this is inherited from the global datatables configuration
anyway.

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

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 44 Jonathan Druart 2020-05-06 14:13:14 UTC
Created attachment 104435 [details] [review]
Bug 25279: Replace encodeURI with encodeURIComponent

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 45 Tomás Cohen Arazi 2020-05-07 13:11:15 UTC
Created attachment 104510 [details] [review]
Bug 25279: (QA follow-up) Use .escapeHtml

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Martin Renvoize 2020-05-12 10:47:53 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 47 Joy Nelson 2020-05-13 19:08:38 UTC
enhancement not backported to 19.11.x