Bug 25639 - Add search queries to HTML so queries can be retrieved via JS
Summary: Add search queries to HTML so queries can be retrieved via JS
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-06-01 10:48 UTC by Nick Clemens
Modified: 2020-11-19 18:30 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
This patch adds global JS variables for the prepared search forms: query_desc_query_cgi, and query These are useful for plugins or custom JS wishing to perform searches outside of Koha and incorporate results.
Version(s) released in:
20.11.00, 20.05.04


Attachments
Bug 25639: Add query strings to html (1.64 KB, patch)
2020-06-01 10:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25639: Add query strings to html (1.70 KB, patch)
2020-06-02 13:41 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25639: Add query strings to html (1.76 KB, patch)
2020-06-21 14:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 25639: (follow-up) Remove replacements from querystring (1.13 KB, patch)
2020-07-27 15:37 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25639: (alternate) make query strings global variables (2.03 KB, patch)
2020-07-29 10:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25639: Add query strings as global JS variables (2.46 KB, patch)
2020-08-03 14:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26119: Enable patron's attribute edition when PatronSelfRegistrationVerifyByEmail (3.51 KB, patch)
2020-08-03 14:54 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25639: Add query strings to html (1.82 KB, patch)
2020-08-03 14:54 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25639: (follow-up) Remove replacements from querystring (1.20 KB, patch)
2020-08-03 14:54 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25639: (alternate) make query strings global variables (2.10 KB, patch)
2020-08-03 14:54 UTC, ByWater Sandboxes
Details | Diff | Splinter Review
Bug 25639: (alternate) make query strings global variables (2.10 KB, patch)
2020-08-04 11:33 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25639: (alternate) make query strings global variables (2.02 KB, patch)
2020-08-25 17:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25639: Add search query strings as global javascript variables (2.47 KB, patch)
2020-08-27 10:14 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 25639: Add search query strings as global javascript variables (2.52 KB, patch)
2020-08-27 18:16 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-06-01 10:48:05 UTC
For integrations or custom code it would be nice to have access to the search strings used in Koha to the html, like loggedinbranch etc.
Comment 1 Nick Clemens 2020-06-01 10:51:39 UTC
Created attachment 105458 [details] [review]
Bug 25639: Add query strings to html

This development simply adds the contructed queries generated by Koha
to the HTML source. This is useful for integrations like the Hoopla plugin:
https://github.com/bywatersolutions/koha-plugin-hoopla

To test:
1 - Search on OPAC for "qwertyuiop"
2 - View the HTML source
3 - Find 'qwertyuiop' in the source, it is not easily accessible via JS
4 - Apply patch
5 - Repeat
6 - Note that now there are spans with data elements containing the queries used by Koha
7 - Sign off
Comment 2 ByWater Sandboxes 2020-06-02 13:41:14 UTC
Created attachment 105487 [details] [review]
Bug 25639: Add query strings to html

This development simply adds the contructed queries generated by Koha
to the HTML source. This is useful for integrations like the Hoopla plugin:
https://github.com/bywatersolutions/koha-plugin-hoopla

To test:
1 - Search on OPAC for "qwertyuiop"
2 - View the HTML source
3 - Find 'qwertyuiop' in the source, it is not easily accessible via JS
4 - Apply patch
5 - Repeat
6 - Note that now there are spans with data elements containing the queries used by Koha
7 - Sign off

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 3 Katrin Fischer 2020-06-21 14:01:32 UTC
Created attachment 106112 [details] [review]
Bug 25639: Add query strings to html

This development simply adds the contructed queries generated by Koha
to the HTML source. This is useful for integrations like the Hoopla plugin:
https://github.com/bywatersolutions/koha-plugin-hoopla

To test:
1 - Search on OPAC for "qwertyuiop"
2 - View the HTML source
3 - Find 'qwertyuiop' in the source, it is not easily accessible via JS
4 - Apply patch
5 - Repeat
6 - Note that now there are spans with data elements containing the queries used by Koha
7 - Sign off

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Jonathan Druart 2020-06-24 13:04:25 UTC
Convince me the "replace" calls are useful.
Comment 5 Nick Clemens 2020-07-15 19:22:34 UTC
(In reply to Jonathan Druart from comment #4)
> Convince me the "replace" calls are useful.

They copy what we do elsewhere in the code and what we do to pass searches to overdrive and recordedbooks currently
Comment 6 Jonathan Druart 2020-07-20 09:09:48 UTC
Maybe they are not needed in other places?
If I search for "a\n'", I get:
    var query_desc = "kw,wrdl: a\n'";
    var querystring = "a\n'";
with and without the replaces
Comment 7 David Cook 2020-07-22 00:28:41 UTC
Here's a thought:

Does it have to be in the HTML? (We're not displaying the HTML nor are we POSTing a hidden field, so this just seems a bit hacky.)

Since we know what the search query is when we're building the HTTP response, we could possibly just put it in a Javascript variable. 

You could store the 3 strings in a JSON object, and since we'd build the JSON server-side, it would be easy to validate the data to prevent XSS vulnerabilities. 

Anyway, just a thought. It could be a cleaner way of doing it.
Comment 8 Nick Clemens 2020-07-27 15:37:09 UTC
Created attachment 107440 [details] [review]
Bug 25639: (follow-up) Remove replacements from querystring
Comment 9 Nick Clemens 2020-07-27 15:38:59 UTC
(In reply to David Cook from comment #7)
> Here's a thought:
> 
> Does it have to be in the HTML? (We're not displaying the HTML nor are we
> POSTing a hidden field, so this just seems a bit hacky.)
> 
> Since we know what the search query is when we're building the HTTP
> response, we could possibly just put it in a Javascript variable. 
> 
> You could store the 3 strings in a JSON object, and since we'd build the
> JSON server-side, it would be easy to validate the data to prevent XSS
> vulnerabilities. 
> 
> Anyway, just a thought. It could be a cleaner way of doing it.

In javascript we would need to make it a global variable, we have other instances in the code of placing useful information in the HTML (loggedinbranch etc.)
Comment 10 David Cook 2020-07-29 01:22:27 UTC
(In reply to Nick Clemens from comment #9)
> In javascript we would need to make it a global variable

Yes, but that's not bad.

Traditionally, Javascript only had Global Scope and Function Scope. They've only added Block Scope recently via the "let" and "const" keywords with ES2015. 

We have lots of global variables in our Javascript.

Technically speaking, an HTML element with an ID element is accessible as a Javascript global variable. (If you want to test that, open your F12 tools, go to console, and type in "console.log(add_comment)" on this page. It'll return a div element. 

> we have other
> instances in the code of placing useful information in the HTML
> (loggedinbranch etc.)

I don't see any examples of loggedinbranch in HTML specifically, but happy to look at examples. Perhaps it's needed in the HTML to be POSTed elsewhere. 

If it isn't needed in the HTML per se, I'm not sure inertia is the best reason to continue doing something.

But I don't think it's a blocker. Just a thought.
Comment 11 Nick Clemens 2020-07-29 10:56:59 UTC
Created attachment 107486 [details] [review]
Bug 25639: (alternate) make query strings global variables
Comment 12 Nick Clemens 2020-07-29 11:00:21 UTC
(In reply to David Cook from comment #10)
> I don't see any examples of loggedinbranch in HTML specifically, but happy
> to look at examples. Perhaps it's needed in the HTML to be POSTed elsewhere. 

bug 23648 and bug 20921 - it was borrower data, not branch info

> If it isn't needed in the HTML per se, I'm not sure inertia is the best
> reason to continue doing something.

I am not tied to either method - the html felt cleaner to me, it's available but not set, but I see your points. 

I posted a patch adding the data as global variables - OVerdrive and highlighting continue to work so either should be fine
Comment 13 David Cook 2020-07-31 01:43:31 UTC
(In reply to Nick Clemens from comment #12)
> bug 23648 and bug 20921 - it was borrower data, not branch info
> 

Thanks for those references! That's helpful!

I'd actually argue that those two aren't great uses of data-* attributes either heh.

I like the examples used here though: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/data-*

In that first example, it stores an ID for each list item. You could take that data and do an API lookup. 

In the second example, you could store state information in the data-* attributes.

In these Koha cases, it seems like we're putting data into HTML elements, which doesn't have anything to do with the HTML elements. I suppose you could argue that data-borrowernumber could be used to determine whether or not a user was logged in... but there's only 1 user so that would be trivial to store in the Javascript space. I could understand storing data-borrowernumber if you had a list of borrowers in a table for instance...

Anyway, thanks for indulging me 😅
Comment 14 Nick Clemens 2020-08-03 14:02:30 UTC
Created attachment 107711 [details] [review]
Bug 25639: Add query strings as global JS variables

To test:
1 - Search on OPAC for "qwertyuiop"
2 - View the HTML source
3 - Find 'qwertyuiop' in the source, it is not easily accessible via JS
4 - Apply patch
5 - Repeat
6 - Note that now there are global variables with the query strings
7 - Add to OpacUserJs:
    console.log( query_desc );
    console.log( querystring );
    console.log( query_cgi );
8 - Reload page and confirm variables are warned in the JS console (Press F12)
9 - Sign off
Comment 15 ByWater Sandboxes 2020-08-03 14:54:20 UTC
Created attachment 107712 [details] [review]
Bug 26119: Enable patron's attribute edition when PatronSelfRegistrationVerifyByEmail

There is no good reason remove the ability to edit patron's attributes
if PatronSelfRegistrationVerifyByEmail is set.

We cannot have them in the self-registration form (because we don't
store patron's attributes during self-ref), but we can have them when
a patron show/edit their information.

Test plan:
0. Enable PatronSelfRegistrationVerifyByEmail and create patron's
attributes that can be edited at the OPAC
1. Create a new patron using the self-reg feature
=> You don't have the ability to fill in the patron's attribute
2. Edit your personal details (OPAC)
=> Without this patch you still don't see the patron's attribute that
can be edited at the OPAC
=> With this patch applied you can!
3. Notice the wording change on the "Modify patron attribute type" form
and confirm it makes sense

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 16 ByWater Sandboxes 2020-08-03 14:54:23 UTC
Created attachment 107713 [details] [review]
Bug 25639: Add query strings to html

This development simply adds the contructed queries generated by Koha
to the HTML source. This is useful for integrations like the Hoopla plugin:
https://github.com/bywatersolutions/koha-plugin-hoopla

To test:
1 - Search on OPAC for "qwertyuiop"
2 - View the HTML source
3 - Find 'qwertyuiop' in the source, it is not easily accessible via JS
4 - Apply patch
5 - Repeat
6 - Note that now there are spans with data elements containing the queries used by Koha
7 - Sign off

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 17 ByWater Sandboxes 2020-08-03 14:54:25 UTC
Created attachment 107714 [details] [review]
Bug 25639: (follow-up) Remove replacements from querystring

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 18 ByWater Sandboxes 2020-08-03 14:54:28 UTC
Created attachment 107715 [details] [review]
Bug 25639: (alternate) make query strings global variables

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 19 Nick Clemens 2020-08-04 11:33:49 UTC
Created attachment 107745 [details] [review]
Bug 25639: (alternate) make query strings global variables

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 20 Katrin Fischer 2020-08-23 21:24:19 UTC
Hi Nick, can you please rebase and adjust the test plan in the commit message?
Comment 21 Nick Clemens 2020-08-25 17:02:06 UTC
Created attachment 109120 [details] [review]
Bug 25639: (alternate) make query strings global variables

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 22 Katrin Fischer 2020-08-26 22:27:15 UTC
This overall looks good, but can you please pretty up the commit message and add an updated test plan?
Comment 23 Nick Clemens 2020-08-27 10:14:46 UTC
Created attachment 109221 [details] [review]
Bug 25639: Add search query strings as global javascript variables

To test:
1 - Search on OPAC for "qwertyuiop"
2 - View the HTML source
3 - Find 'qwertyuiop' in the source, it is not easily accessible via JS
4 - Apply patch
5 - Repeat
6 - Note that now there are global variables with the query strings
7 - Add to OpacUserJs:
    console.log( query_desc );
    console.log( querystring );
    console.log( query_cgi );
8 - Reload page and confirm variables are warned in the JS console (Press F12)
9 - Sign off

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Comment 24 Katrin Fischer 2020-08-27 18:16:32 UTC
Created attachment 109258 [details] [review]
Bug 25639: Add search query strings as global javascript variables

To test:
1 - Search on OPAC for "qwertyuiop"
2 - View the HTML source
3 - Find 'qwertyuiop' in the source, it is not easily accessible via JS
4 - Apply patch
5 - Repeat
6 - Note that now there are global variables with the query strings
7 - Add to OpacUserJs:
    console.log( query_desc );
    console.log( querystring );
    console.log( query_cgi );
8 - Reload page and confirm variables are warned in the JS console (Press F12)
9 - Sign off

Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Jonathan Druart 2020-08-31 14:20:54 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 26 Lucas Gass 2020-09-04 20:31:32 UTC
seems very handy and is a simple enhancement, backported to 20.05.x for 20.05.04
Comment 27 Aleisha Amohia 2020-09-10 04:44:24 UTC
enhancement, not backported to 19.11.x