|
Description
Nick Clemens (kidclamp)
2020-06-01 10:48:05 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 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> 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> Convince me the "replace" calls are useful. (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 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
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. Created attachment 107440 [details] [review] Bug 25639: (follow-up) Remove replacements from querystring (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.) (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. Created attachment 107486 [details] [review] Bug 25639: (alternate) make query strings global variables (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 (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 [U+1F605] 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 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> 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> Created attachment 107714 [details] [review] Bug 25639: (follow-up) Remove replacements from querystring Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 107715 [details] [review] Bug 25639: (alternate) make query strings global variables Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Created attachment 107745 [details] [review] Bug 25639: (alternate) make query strings global variables Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Hi Nick, can you please rebase and adjust the test plan in the commit message? Created attachment 109120 [details] [review] Bug 25639: (alternate) make query strings global variables Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> This overall looks good, but can you please pretty up the commit message and add an updated test plan? 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> 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> Pushed to master for 20.11, thanks to everybody involved! seems very handy and is a simple enhancement, backported to 20.05.x for 20.05.04 enhancement, not backported to 19.11.x |