Summary: | getAll not encoding URL params | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | ERM | Assignee: | Jonathan Druart <jonathan.druart> |
Status: | RESOLVED FIXED | QA Contact: | |
Severity: | major | ||
Priority: | P5 - low | CC: | jonathan.druart, jonathan.field, lucas, martin.renvoize, matt.blenkinsop, pedro.amorim |
Version: | unspecified | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
23.05.00,22.11.06
|
|
Bug Depends on: | |||
Bug Blocks: | 32474 | ||
Attachments: |
Bug 33623: Encode URL params for getAll
Bug 33623: Move ULSearchParams into http-client.js Bug 33623: Encode URL params for getAll Bug 33623: Move ULSearchParams into http-client.js Bug 33623: Encode URL params for getAll Bug 33623: Move ULSearchParams into http-client.js |
Description
Jonathan Druart
2023-04-27 06:41:33 UTC
Created attachment 150281 [details] [review] Bug 33623: Encode URL params for getAll We must encode properly URL parameters when building the REST API routes Can be tested easily using const client = APIClient.erm client.licenses.getAll({ "me.name:%ff%" }) Please make this patch part of master ASAP! Also contains some prettier changes: yarn run prettier --trailing-comma es5 --arrow-parens avoid --write koha-tmpl/intranet-tmpl/prog/js/vue/fetch/erm-api-client.js (yes, we are missing a QA test) (In reply to Jonathan Druart from comment #3) > Also contains some prettier changes: > yarn run prettier --trailing-comma es5 --arrow-parens avoid --write > koha-tmpl/intranet-tmpl/prog/js/vue/fetch/erm-api-client.js > > (yes, we are missing a QA test) Opened bug 33625. Created attachment 150362 [details] [review] Bug 33623: Move ULSearchParams into http-client.js Remove unused getAll methods in erm-api-client.js Make use of new getAll query in AgreementRelationships Created attachment 150498 [details] [review] Bug 33623: Encode URL params for getAll We must encode properly URL parameters when building the REST API routes Can be tested easily using const client = APIClient.erm client.licenses.getAll({ "me.name:%ff%" }) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 150499 [details] [review] Bug 33623: Move ULSearchParams into http-client.js Remove unused getAll methods in erm-api-client.js Make use of new getAll query in AgreementRelationships Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Great follow-up, Pedro, thanks! Created attachment 150506 [details] [review] Bug 33623: Encode URL params for getAll We must encode properly URL parameters when building the REST API routes Can be tested easily using const client = APIClient.erm client.licenses.getAll({ "me.name:%ff%" }) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Created attachment 150507 [details] [review] Bug 33623: Move ULSearchParams into http-client.js Remove unused getAll methods in erm-api-client.js Make use of new getAll query in AgreementRelationships Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> All working as expected in testing... Signing off. The code looks like it introduces a behaviour change to me.. but maybe I'm just not entirely close enough to it. endpoint: "agreements?" + (query || "_per_page=-1"), vs endpoint: "agreements?" + new URLSearchParams({ _per_page: -1, ...(query && { q: JSON.stringify(query) }), }), To me, that looks like before now we let the query govern the paging whereas after we're always setting _per_page=-1. Yes you are correct, but it was not used. We should actually allow the callers to pass more options. Like embed, for a follow-up bug I need to retrieve the items with the biblio info. I wont add "embed: biblio" to items.getAll(), but allow the caller to ask for it. It should to be done when we will need it (unless I missed something). OK, that makes sense to me.. upgrading my signoff to a PQA stamp. Thanks for the clarifications Jonathan. Pushed to master for 23.05. Nice work everyone, thanks! Nice work everyone! Pushed to stable for 22.11.x Missing dependencies for 22.05.x, no backport |