Bug 32807 - No need to fetch all if we need to know if one exist
Summary: No need to fetch all if we need to know if one exist
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:
URL:
Keywords:
: 32494 (view as bug list)
Depends on: 32030 32991
Blocks:
  Show dependency treegraph
 
Reported: 2023-02-02 20:42 UTC by Jonathan Druart
Modified: 2023-10-03 06:56 UTC (History)
8 users (show)

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


Attachments
Bug 32807: Fetch count instead of all entries for agreements and licenses (8.29 KB, patch)
2023-02-22 07:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32807: Fetch count instead of all entries for agreements and licenses (8.38 KB, patch)
2023-02-22 10:00 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 32807: Fetch count instead of all entries for agreements and licenses (8.41 KB, patch)
2023-03-24 11:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 32807: Fix cypress tests (5.55 KB, patch)
2023-03-29 07:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 32807: Fix cypress tests (5.60 KB, patch)
2023-03-29 09:28 UTC, Pedro Amorim
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-02-02 20:42:10 UTC
In (at least) AgreementsList we are calling fetchAgreements which fetch all the agreements, to know if we display the table (that will fetch again the first X agreements).
We need a fetchCountAgreements (similar to fetchCountLocalPackages) instead.

I think we discussed that already somewhere else.

Anyone willing to provide a patch to make their grip on Vue and the ERM module?
Comment 1 David Cook 2023-02-02 22:43:21 UTC
(In reply to Jonathan Druart from comment #0)
> Anyone willing to provide a patch to make their grip on Vue and the ERM
> module?

I don't really know anything about the ERM, but one of these days I do plan to get to grips with Vue. Probably not this patch though 😅
Comment 2 Jonathan Druart 2023-02-22 07:51:53 UTC
Created attachment 147111 [details] [review]
Bug 32807: Fetch count instead of all entries for agreements and licenses

To list agreements and licenses we are retrieving the whole list to
simply know if at least one exists (and display the table that will
fetch the X first elements to display).

We should call count instead.

Test plan:
List agreements and licenses.
If none exists, the table is not displayed but a "There are no...
defined" message instead.
If at least one exists the table must be there
Comment 3 Matt Blenkinsop 2023-02-22 10:00:38 UTC
Created attachment 147115 [details] [review]
Bug 32807: Fetch count instead of all entries for agreements and licenses

To list agreements and licenses we are retrieving the whole list to
simply know if at least one exists (and display the table that will
fetch the X first elements to display).

We should call count instead.

Test plan:
List agreements and licenses.
If none exists, the table is not displayed but a "There are no...
defined" message instead.
If at least one exists the table must be there

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 4 Kyle M Hall 2023-03-24 11:04:29 UTC
Created attachment 148654 [details] [review]
Bug 32807: Fetch count instead of all entries for agreements and licenses

To list agreements and licenses we are retrieving the whole list to
simply know if at least one exists (and display the table that will
fetch the X first elements to display).

We should call count instead.

Test plan:
List agreements and licenses.
If none exists, the table is not displayed but a "There are no...
defined" message instead.
If at least one exists the table must be there

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 5 Tomás Cohen Arazi 2023-03-27 10:47:33 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 6 Jonathan Druart 2023-03-29 07:40:20 UTC
Created attachment 148851 [details] [review]
Bug 32807: Fix cypress tests

Cypress tests were failing, not sure I understand why exactly as I don't
recreate the failure when using the interface. We could simplify the if
logic anyway.
Comment 7 Pedro Amorim 2023-03-29 09:28:35 UTC
Created attachment 148894 [details] [review]
Bug 32807: Fix cypress tests

Cypress tests were failing, not sure I understand why exactly as I don't
recreate the failure when using the interface. We could simplify the if
logic anyway.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>
Comment 8 Tomás Cohen Arazi 2023-03-29 10:33:19 UTC
Follow-up pushed to master. Thanks for the quick fix, guys!
Comment 9 Matt Blenkinsop 2023-04-11 10:55:08 UTC
Nice work everyone!

Pushed to stable for 22.11.x
Comment 10 Lucas Gass 2023-04-24 17:06:43 UTC
Missing dependencies for 22.05.x, no backport
Comment 11 Jonathan Druart 2023-10-03 06:56:06 UTC
*** Bug 32494 has been marked as a duplicate of this bug. ***