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.
Created attachment 103693 [details] [review] Bug 25279: Add general query parameters to the /cities route Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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>
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>
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>
Created attachment 103804 [details] [review] Bug 25279: Fix edit and delete links Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
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&cityid='+ row.id +'"><i class="fa fa-pencil"></i> Edit</a>'; The button label is not translatable
> > + > > + var result = '<a class="btn btn-default btn-xs" href="/cgi-bin/koha/admin/cities.pl?op=add_form&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.
Created attachment 104065 [details] [review] Bug 25279: (QA follow-up) Make button texts translatable Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Josef Moravec from comment #7) > > The button label is not translatable Done
Created attachment 104072 [details] [review] Bug 25279: (QA follow-up) Make button texts translatable Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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&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
(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&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.
That does not work, please restest. https://snipboard.io/PR9nxJ.jpg SyntaxError: unexpected token: string literal
Created attachment 104359 [details] [review] Bug 25279: (QA follow-up) Make button texts translatable Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(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.
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?
(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.
(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.
(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.
(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?)
(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.
(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!
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>
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>
(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.
(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.
https://snipboard.io/0VXrTZ.jpg https://snipboard.io/ho6wZf.jpg
When searching for something that does not return any results: https://snipboard.io/TGwL4v.jpg
(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.
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
(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.
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
Created attachment 104435 [details] [review] Bug 25279: Replace encodeURI with encodeURIComponent Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 104510 [details] [review] Bug 25279: (QA follow-up) Use .escapeHtml Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Nice work everyone! Pushed to master for 20.05
enhancement not backported to 19.11.x
*** Bug 14974 has been marked as a duplicate of this bug. ***