Bug 33623

Summary: getAll not encoding URL params
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: ERMAssignee: 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

    
Comment 1 Jonathan Druart 2023-04-27 06:49:35 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%" })
Comment 2 Jonathan Druart 2023-04-27 06:50:06 UTC
Please make this patch part of master ASAP!
Comment 3 Jonathan Druart 2023-04-27 06:57:02 UTC
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)
Comment 4 Jonathan Druart 2023-04-27 09:12:38 UTC
(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.
Comment 5 Pedro Amorim 2023-04-28 10:45:14 UTC
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
Comment 6 Jonathan Druart 2023-05-02 07:01:54 UTC
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>
Comment 7 Jonathan Druart 2023-05-02 07:01:56 UTC
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>
Comment 8 Jonathan Druart 2023-05-02 07:03:03 UTC
Great follow-up, Pedro, thanks!
Comment 9 Martin Renvoize 2023-05-02 09:04:31 UTC
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>
Comment 10 Martin Renvoize 2023-05-02 09:04:33 UTC
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>
Comment 11 Martin Renvoize 2023-05-02 09:06:09 UTC
All working as expected in testing... Signing off.
Comment 12 Martin Renvoize 2023-05-02 09:24:02 UTC
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.
Comment 13 Jonathan Druart 2023-05-02 10:18:59 UTC
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).
Comment 14 Martin Renvoize 2023-05-02 10:38:16 UTC
OK, that makes sense to me.. upgrading my signoff to a PQA stamp. Thanks for the clarifications Jonathan.
Comment 15 Tomás Cohen Arazi 2023-05-05 12:18:26 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 16 Matt Blenkinsop 2023-05-15 14:12:56 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 17 Lucas Gass 2023-06-15 19:42:05 UTC
Missing dependencies for 22.05.x, no backport