Bug 34219 - getAll not allowing additional parameters
Summary: getAll not allowing additional parameters
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: ERM (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Martin Renvoize
URL:
Keywords:
Depends on:
Blocks: 32474 36392
  Show dependency treegraph
 
Reported: 2023-07-06 14:09 UTC by Jonathan Druart
Modified: 2024-03-21 20:54 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04,22.11.10


Attachments
Bug 34219: Allow getAll to receive additional URL parameters (4.05 KB, patch)
2023-07-06 14:14 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 34219: Allow getAll to receive additional URL parameters (4.10 KB, patch)
2023-07-19 09:41 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 34219: Allow getAll to receive additional URL parameters (4.17 KB, patch)
2023-08-17 10:28 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2023-07-06 14:09:31 UTC
It's not possible to pass additional URL parameters. We need it at least for bug 32474.
Comment 1 Jonathan Druart 2023-07-06 14:14:36 UTC
Created attachment 153097 [details] [review]
Bug 34219: Allow getAll to receive additional URL parameters

It's not possible to pass additional URL parameters. We need it at least for bug 32474.
Comment 2 Jonathan Druart 2023-07-06 14:14:51 UTC
Pedro, this is one for you ;)
Comment 3 Pedro Amorim 2023-07-07 14:52:14 UTC
getAll allows for additional parameters, e.g. query (see AgreementRelationships.vue), problem is it doesn't allow for additional URL parameters!

We have to:
- Add URLparams as a parameter to getAll
- Have a clear distinction between query parameters and URL parameters in http-client.js (and maybe also somewhere else)
- Rename getAll to search
- Handle default _per_page value
- Likely rename this bug

I hope to revisit this soon.
Comment 4 Jonathan Druart 2023-07-11 12:14:02 UTC
(In reply to Pedro Amorim from comment #3)
> - Add URLparams as a parameter to getAll

Do you mean we should rename params.params to params.URLparams?

> - Have a clear distinction between query parameters and URL parameters in
> http-client.js (and maybe also somewhere else)

2 separate parameters, not params.URLparams and params.query, is that what you mean?

> - Rename getAll to search

On a separate bug please.
get    => find
getAll => search

> - Handle default _per_page value

This is done already.

> - Likely rename this bug

It depends if we let it sit down or move on fast. We need it quite quickly in master I think, we will certainly need it soon (we need already on bug 32474).
Comment 5 Pedro Amorim 2023-07-19 09:41:53 UTC
Created attachment 153631 [details] [review]
Bug 34219: Allow getAll to receive additional URL parameters

It's not possible to pass additional URL parameters. We need it at least for bug 32474.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 6 Katrin Fischer 2023-07-29 21:56:38 UTC
Please add a test plan to the commit message.
Comment 7 Pedro Amorim 2023-07-31 08:46:51 UTC
(In reply to Katrin Fischer from comment #6)
> Please add a test plan to the commit message.

I believe this bug only serves as preparation to 32474, it doesn't implement anything, hence not having a test plan (sorry!).

A couple things that can be done though:
1) Run cypress tests - make sure all pass
2) Verify for yourself that everywhere getAll is used, is still working fine, those are:
-- Agreements form - Related licenses
-- Agreements form - Related agreements
-- Local packages form - Related agreements
-- Local titles form - Related packages
-- Titles - Import from list
Comment 8 Katrin Fischer 2023-07-31 11:41:49 UTC
I should probably not admit that, but how do I run the Cypress tests?
Comment 9 Jonathan Druart 2023-07-31 14:21:25 UTC
(In reply to Katrin Fischer from comment #8)
> I should probably not admit that, but how do I run the Cypress tests?

The easiest is:

  % perl /kohadevbox/misc4dev/run_tests.pl --run-cypress-tests-only
Comment 10 Victor Grousset/tuxayo 2023-08-08 00:10:05 UTC
Confirmed that tests pass after patch :)

To run one test (in case of flaky tests): 
yarn cypress run --spec t/cypress/integration/ERM/Agreements_spec.ts
Comment 11 Martin Renvoize 2023-08-17 10:28:36 UTC
Created attachment 154517 [details] [review]
Bug 34219: Allow getAll to receive additional URL parameters

It's not possible to pass additional URL parameters. We need it at least for bug 32474.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2023-08-17 10:29:09 UTC
Code makes sense and all works as expected here.. unit tests are passing still and qa script is happy.

Passing QA
Comment 13 Tomás Cohen Arazi 2023-08-29 17:36:20 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Fridolin Somers 2023-08-30 06:51:50 UTC
Pushed to 23.05.x for 23.05.04
Comment 15 Matt Blenkinsop 2023-08-31 10:24:12 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x