Bug 29084 - Update article requests-related Koha::Biblio methods to use relationships
Summary: Update article requests-related Koha::Biblio methods to use relationships
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 29083
Blocks: 29086
  Show dependency treegraph
 
Reported: 2021-09-22 19:09 UTC by Tomás Cohen Arazi
Modified: 2022-06-06 20:25 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Provides a small performance enhancement and allows prefetching and embedding to work in the API
Version(s) released in:
21.11.00


Attachments
Bug 29084: Unit tests (7.45 KB, patch)
2021-09-22 19:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29084: Update article requests-related Koha::Biblio methods to use relationships (3.64 KB, patch)
2021-09-22 19:26 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29084: Unit tests (7.52 KB, patch)
2021-09-24 13:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29084: Update article requests-related Koha::Biblio methods to use relationships (3.71 KB, patch)
2021-09-24 13:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29084: Unit tests (7.50 KB, patch)
2021-10-06 17:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29084: Update article requests-related Koha::Biblio methods to use relationships (3.70 KB, patch)
2021-10-06 17:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29084: Remove article_requests_finished and article_requests_current (6.95 KB, patch)
2021-10-06 17:06 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2021-09-22 19:09:59 UTC
As bug 29083 does for Koha::Patron, this bug will deal the with Koha::Biblio counterpart.
Comment 1 Tomás Cohen Arazi 2021-09-22 19:26:15 UTC
Created attachment 125163 [details] [review]
Bug 29084: Unit tests

This patch adds missing tests for Koha::Biblio->article_requests and
reorganizes (and extends) the tests for 'article_requests_current' and
'article_requests_finished' that were originally in ArticleRequests.t
into Koha/Biblio.t as we do now.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/ArticleRequests.t \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-09-22 19:26:20 UTC
Created attachment 125164 [details] [review]
Bug 29084: Update article requests-related Koha::Biblio methods to use relationships

This patch makes Koha::Patron->article_requests use the underlying DBIC
relationship and _new_from_dbic instead of a plain search. It also
refactors 'article_requests_current' and 'article_requests_finished' to
use ->article_requests, as well as the new methods introduced by bug
29082 for filtering.

No behavior change should take place.

To test:
1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t \
           t/db_dependent/ArticleRequests.t
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 3 Martin Renvoize 2021-09-24 13:04:47 UTC
Created attachment 125244 [details] [review]
Bug 29084: Unit tests

This patch adds missing tests for Koha::Biblio->article_requests and
reorganizes (and extends) the tests for 'article_requests_current' and
'article_requests_finished' that were originally in ArticleRequests.t
into Koha/Biblio.t as we do now.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/ArticleRequests.t \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 4 Martin Renvoize 2021-09-24 13:04:50 UTC
Created attachment 125245 [details] [review]
Bug 29084: Update article requests-related Koha::Biblio methods to use relationships

This patch makes Koha::Patron->article_requests use the underlying DBIC
relationship and _new_from_dbic instead of a plain search. It also
refactors 'article_requests_current' and 'article_requests_finished' to
use ->article_requests, as well as the new methods introduced by bug
29082 for filtering.

No behavior change should take place.

To test:
1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t \
           t/db_dependent/ArticleRequests.t
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 5 Martin Renvoize 2021-09-24 13:05:54 UTC
Another nice improvement.. no regressions found.

Signing off
Comment 6 Marcel de Rooy 2021-09-27 07:48:40 UTC
QA: Looking here
Comment 7 Marcel de Rooy 2021-09-27 08:07:38 UTC
I followed the chain up till here and somewhere it goes wrong. But since I discover it here, I will blame this report :)

I have a patron with 10 article requests.
OPAC shows 10 using [% current_article_requests_count %] Fine!

Intranet patron details shows None!
What does it use?
koha-tmpl/intranet-tmpl/prog/en/includes/patron-article-requests.inc
[% IF patron.article_requests_current.count %]

There is our famous bottleneck from bug 28883. TT runs the article_requests_current call in LIST context although you think that you are chaining.
So the above count does NOT work !

If I do [% SET x = patron.article_requests_current %] [% x.size %]
then I see that x contains 10 entries.

The problem is somewhere earlier. But I wouldnt mind correcting it here, and not disturb all previous QAs.
Comment 8 Jonathan Druart 2021-10-01 14:22:21 UTC
sub article_requests_current {
return $self->article_requests->filter_by_current;

sub article_requests_finished {
return $self->article_requests->filter_by_finished;

How are they useful? Callers can call $biblio->article_requests->filter_by_*
Comment 9 Tomás Cohen Arazi 2021-10-01 19:28:52 UTC
(In reply to Jonathan Druart from comment #8)
> sub article_requests_current {
> return $self->article_requests->filter_by_current;
> 
> sub article_requests_finished {
> return $self->article_requests->filter_by_finished;
> 
> How are they useful? Callers can call $biblio->article_requests->filter_by_*

Agreed. My plan was to remove them. But it felt like it was better to keep them to prove no behavior change. And then remove them while refactoring the controllers. And why not just remove the controllers if I had the chance :-D
Comment 10 Tomás Cohen Arazi 2021-10-06 17:05:51 UTC
Created attachment 125841 [details] [review]
Bug 29084: Unit tests

This patch adds missing tests for Koha::Biblio->article_requests and
reorganizes (and extends) the tests for 'article_requests_current' and
'article_requests_finished' that were originally in ArticleRequests.t
into Koha/Biblio.t as we do now.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/ArticleRequests.t \
           t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Tomás Cohen Arazi 2021-10-06 17:06:00 UTC
Created attachment 125842 [details] [review]
Bug 29084: Update article requests-related Koha::Biblio methods to use relationships

This patch makes Koha::Patron->article_requests use the underlying DBIC
relationship and _new_from_dbic instead of a plain search. It also
refactors 'article_requests_current' and 'article_requests_finished' to
use ->article_requests, as well as the new methods introduced by bug
29082 for filtering.

No behavior change should take place.

To test:
1. Apply the unit tests patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t \
           t/db_dependent/ArticleRequests.t
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Tomás Cohen Arazi 2021-10-06 17:06:08 UTC
Created attachment 125843 [details] [review]
Bug 29084: Remove article_requests_finished and article_requests_current

This patch removes those methods that are not really needed. Templates
are adjusted to use the expected combination of
->article_requests->filter_by_current.

To test:
1. Apply this patch
2. Visit a biblio with article requests
=> SUCCESS: All works
3. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Biblio.t
=> SUCCESS: Tests pass, less tests.
4. Sign off :-D
Comment 13 Tomás Cohen Arazi 2021-10-06 17:08:12 UTC
Went the Context.Scalar way on this one as well. Back to PQA.
Please note that the unit tests need the latest follow-up I submitted on bug 27526.
Comment 14 Marcel de Rooy 2021-10-07 06:57:48 UTC
Applying: Bug 29084: Unit tests
error: sha1 information is lacking or useless (t/db_dependent/Koha/Biblio.t).
error: could not build fake ancestor
Comment 15 Jonathan Druart 2021-10-07 07:20:17 UTC
(In reply to Marcel de Rooy from comment #14)
> Applying: Bug 29084: Unit tests
> error: sha1 information is lacking or useless (t/db_dependent/Koha/Biblio.t).
> error: could not build fake ancestor

You need the last patch from bug 27526.
Comment 16 Jonathan Druart 2021-10-07 07:38:20 UTC
Patches will be squashed when pushed.
Comment 17 Jonathan Druart 2021-10-07 09:21:10 UTC
Pushed to master for 21.11, thanks to everybody involved!