Bug 29083

Summary: Update article requests-related Koha::Patron methods to use relationships
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, kyle, m.de.rooy, martin.renvoize, nick, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28684
Change sponsored?: --- Patch complexity: Trivial patch
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
Bug Depends on: 29082    
Bug Blocks: 29084    
Attachments: Bug 29083: Unit tests
Bug 29083: Update article requests-related Koha::Patron methods to use relationships
Bug 29083: (QA follow-up) Remove unused param
Bug 29083: Fix OPAC listing of article requests
Bug 29083: Unit tests
Bug 29083: Update article requests-related Koha::Patron methods to use relationships
Bug 29083: (QA follow-up) Remove unused param
Bug 29083: Fix OPAC listing of article requests
Bug 29083: Unit tests
Bug 29083: Update article requests-related Koha::Patron methods to use relationships
Bug 29083: (QA follow-up) Remove unused param
Bug 29083: Fix OPAC listing of article requests
Bug 29083: Unit tests
Bug 29083: Update article requests-related Koha::Patron methods to use relationships
Bug 29083: (QA follow-up) Remove unused param
Bug 29083: Fix OPAC listing of article requests
Bug 29083: Remove article_requests_finished and article_requests_current
Bug 29083: Unit tests
Bug 29083: Update article requests-related Koha::Patron methods to use relationships
Bug 29083: (QA follow-up) Remove unused param
Bug 29083: Fix OPAC listing of article requests
Bug 29083: Remove article_requests_finished and article_requests_current
Bug 29083: Remove article_requests_finished and article_requests_current
Bug 29083: Don't pass _count var

Description Tomás Cohen Arazi 2021-09-22 18:31:04 UTC

    
Comment 1 Tomás Cohen Arazi 2021-09-22 18:33:28 UTC
Added bug 27945 as dependency because it touches the same tests file.
Comment 2 Tomás Cohen Arazi 2021-09-22 18:55:26 UTC
Created attachment 125160 [details] [review]
Bug 29083: Unit tests

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

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/ArticleRequests.t \
           t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass!
Comment 3 Tomás Cohen Arazi 2021-09-22 18:55:31 UTC
Created attachment 125161 [details] [review]
Bug 29083: Update article requests-related Koha::Patron 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/Patron.t \
           t/db_dependent/ArticleRequests.t
=> SUCCESS: Tests pass!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass!
5. Sign off :-D
Comment 4 Tomás Cohen Arazi 2021-09-22 19:01:56 UTC
Created attachment 125162 [details] [review]
Bug 29083: (QA follow-up) Remove unused param

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2021-09-23 12:12:58 UTC
Created attachment 125186 [details] [review]
Bug 29083: Fix OPAC listing of article requests

This patch makes the OPAC template reuse a precalculated value for the
active article requests for the patron (and its count).

The original code relied on the methods returning a list, which is not
the case for _new_from_dbic until bug 28883 is pushed.

This patch fixes that.

Note: there was an odd behavior when ArticleRequests was enabled but no
active article requests were present: the tab wasn't rendered but the
'empty table' with the 'You have no article requests currently.' message
was displayed below the Checkouts tab. I'm not sure that was caused by
this patches, or other. Fixed on this patch.

To test:
1. In the OPAC, go to 'your summary'
=> FAIL: Things don't show for article requests
2. Add some article requests and repeat 1
=> FAIL: Something's wrong there
3. Apply this patch and repeat 1
=> Yes! Things show correctly!
4. Cancel all your article requests
=> SUCCESS: Things render as they should
5. Re-enter the 'your summary' page (to force re-rendering)
=> SUCCESS: Things render correctly for empty article requests
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 6 Martin Renvoize 2021-09-24 13:01:16 UTC
Created attachment 125240 [details] [review]
Bug 29083: Unit tests

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

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-09-24 13:01:20 UTC
Created attachment 125241 [details] [review]
Bug 29083: Update article requests-related Koha::Patron 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/Patron.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 8 Martin Renvoize 2021-09-24 13:01:24 UTC
Created attachment 125242 [details] [review]
Bug 29083: (QA follow-up) Remove unused param

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2021-09-24 13:01:28 UTC
Created attachment 125243 [details] [review]
Bug 29083: Fix OPAC listing of article requests

This patch makes the OPAC template reuse a precalculated value for the
active article requests for the patron (and its count).

The original code relied on the methods returning a list, which is not
the case for _new_from_dbic until bug 28883 is pushed.

This patch fixes that.

Note: there was an odd behavior when ArticleRequests was enabled but no
active article requests were present: the tab wasn't rendered but the
'empty table' with the 'You have no article requests currently.' message
was displayed below the Checkouts tab. I'm not sure that was caused by
this patches, or other. Fixed on this patch.

To test:
1. In the OPAC, go to 'your summary'
=> FAIL: Things don't show for article requests
2. Add some article requests and repeat 1
=> FAIL: Something's wrong there
3. Apply this patch and repeat 1
=> Yes! Things show correctly!
4. Cancel all your article requests
=> SUCCESS: Things render as they should
5. Re-enter the 'your summary' page (to force re-rendering)
=> SUCCESS: Things render correctly for empty article requests
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2021-09-24 13:01:59 UTC
Nice clear refactor.. signing off
Comment 11 Nick Clemens 2021-09-24 15:25:40 UTC
Created attachment 125268 [details] [review]
Bug 29083: Unit tests

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

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 12 Nick Clemens 2021-09-24 15:25:44 UTC
Created attachment 125269 [details] [review]
Bug 29083: Update article requests-related Koha::Patron 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/Patron.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>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2021-09-24 15:25:47 UTC
Created attachment 125270 [details] [review]
Bug 29083: (QA follow-up) Remove unused param

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Nick Clemens 2021-09-24 15:25:50 UTC
Created attachment 125271 [details] [review]
Bug 29083: Fix OPAC listing of article requests

This patch makes the OPAC template reuse a precalculated value for the
active article requests for the patron (and its count).

The original code relied on the methods returning a list, which is not
the case for _new_from_dbic until bug 28883 is pushed.

This patch fixes that.

Note: there was an odd behavior when ArticleRequests was enabled but no
active article requests were present: the tab wasn't rendered but the
'empty table' with the 'You have no article requests currently.' message
was displayed below the Checkouts tab. I'm not sure that was caused by
this patches, or other. Fixed on this patch.

To test:
1. In the OPAC, go to 'your summary'
=> FAIL: Things don't show for article requests
2. Add some article requests and repeat 1
=> FAIL: Something's wrong there
3. Apply this patch and repeat 1
=> Yes! Things show correctly!
4. Cancel all your article requests
=> SUCCESS: Things render as they should
5. Re-enter the 'your summary' page (to force re-rendering)
=> SUCCESS: Things render correctly for empty article requests
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Tomás Cohen Arazi 2021-10-01 22:28:22 UTC
Created attachment 125640 [details] [review]
Bug 29083: Unit tests

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

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Tomás Cohen Arazi 2021-10-01 22:28:27 UTC
Created attachment 125641 [details] [review]
Bug 29083: Update article requests-related Koha::Patron 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/Patron.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>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Tomás Cohen Arazi 2021-10-01 22:28:32 UTC
Created attachment 125642 [details] [review]
Bug 29083: (QA follow-up) Remove unused param

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Tomás Cohen Arazi 2021-10-01 22:28:37 UTC
Created attachment 125643 [details] [review]
Bug 29083: Fix OPAC listing of article requests

This patch makes the OPAC template reuse a precalculated value for the
active article requests for the patron (and its count).

The original code relied on the methods returning a list, which is not
the case for _new_from_dbic until bug 28883 is pushed.

This patch fixes that.

Note: there was an odd behavior when ArticleRequests was enabled but no
active article requests were present: the tab wasn't rendered but the
'empty table' with the 'You have no article requests currently.' message
was displayed below the Checkouts tab. I'm not sure that was caused by
this patches, or other. Fixed on this patch.

To test:
1. In the OPAC, go to 'your summary'
=> FAIL: Things don't show for article requests
2. Add some article requests and repeat 1
=> FAIL: Something's wrong there
3. Apply this patch and repeat 1
=> Yes! Things show correctly!
4. Cancel all your article requests
=> SUCCESS: Things render as they should
5. Re-enter the 'your summary' page (to force re-rendering)
=> SUCCESS: Things render correctly for empty article requests
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 19 Jonathan Druart 2021-10-04 10:48:02 UTC
Same as bug 29084 comment 8. I don't think we need $patron->article_requests_finished, callers can $patron->article_requests->filter_by_finished (or ->finished as the filter_by methods are not correctly named in this module).
Comment 20 Tomás Cohen Arazi 2021-10-06 13:49:04 UTC
(In reply to Jonathan Druart from comment #19)
> Same as bug 29084 comment 8. I don't think we need
> $patron->article_requests_finished, callers can
> $patron->article_requests->filter_by_finished (or ->finished as the
> filter_by methods are not correctly named in this module).

Until we sort bug 28883 and friends, can we keep the methods and allow this bug?
Comment 21 Jonathan Druart 2021-10-06 13:59:08 UTC
We don't need to wait for bug 28883, we have a workaround. Having scalar in include files it not horrible IMO.
Comment 22 Marcel de Rooy 2021-10-06 14:01:42 UTC
(In reply to Jonathan Druart from comment #21)
> We don't need to wait for bug 28883, we have a workaround. Having scalar in
> include files it not horrible IMO.

Smaken verschillen.
But you could also simply call your method outside the template and pass in the results..
Comment 23 Jonathan Druart 2021-10-06 14:17:08 UTC
(In reply to Marcel de Rooy from comment #22)
> (In reply to Jonathan Druart from comment #21)
> > We don't need to wait for bug 28883, we have a workaround. Having scalar in
> > include files it not horrible IMO.
> 
> Smaken verschillen.
> But you could also simply call your method outside the template and pass in
> the results..

Yes but you duplicate the code. It's not nice when you need to add it to 10 controllers.
Comment 24 Tomás Cohen Arazi 2021-10-06 16:09:18 UTC
Created attachment 125833 [details] [review]
Bug 29083: 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 patron with article requests
=> SUCCESS: All works
3. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass, less tests.
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 25 Tomás Cohen Arazi 2021-10-06 16:09:39 UTC
Done. Went the Context.Scalar way for now.
Comment 26 Tomás Cohen Arazi 2021-10-06 16:18:17 UTC
Rebased.
Comment 27 Tomás Cohen Arazi 2021-10-06 16:18:58 UTC
Created attachment 125834 [details] [review]
Bug 29083: Unit tests

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

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

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 28 Tomás Cohen Arazi 2021-10-06 16:19:04 UTC
Created attachment 125835 [details] [review]
Bug 29083: Update article requests-related Koha::Patron 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/Patron.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>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 29 Tomás Cohen Arazi 2021-10-06 16:19:09 UTC
Created attachment 125836 [details] [review]
Bug 29083: (QA follow-up) Remove unused param

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Tomás Cohen Arazi 2021-10-06 16:19:14 UTC
Created attachment 125837 [details] [review]
Bug 29083: Fix OPAC listing of article requests

This patch makes the OPAC template reuse a precalculated value for the
active article requests for the patron (and its count).

The original code relied on the methods returning a list, which is not
the case for _new_from_dbic until bug 28883 is pushed.

This patch fixes that.

Note: there was an odd behavior when ArticleRequests was enabled but no
active article requests were present: the tab wasn't rendered but the
'empty table' with the 'You have no article requests currently.' message
was displayed below the Checkouts tab. I'm not sure that was caused by
this patches, or other. Fixed on this patch.

To test:
1. In the OPAC, go to 'your summary'
=> FAIL: Things don't show for article requests
2. Add some article requests and repeat 1
=> FAIL: Something's wrong there
3. Apply this patch and repeat 1
=> Yes! Things show correctly!
4. Cancel all your article requests
=> SUCCESS: Things render as they should
5. Re-enter the 'your summary' page (to force re-rendering)
=> SUCCESS: Things render correctly for empty article requests
6. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Tomás Cohen Arazi 2021-10-06 16:19:19 UTC
Created attachment 125838 [details] [review]
Bug 29083: 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 patron with article requests
=> SUCCESS: All works
3. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass, less tests.
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi 2021-10-06 17:04:57 UTC
Created attachment 125840 [details] [review]
Bug 29083: 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 patron with article requests
=> SUCCESS: All works
3. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Patron.t
=> SUCCESS: Tests pass, less tests.
4. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Jonathan Druart 2021-10-07 07:38:15 UTC
Patches will be squashed when pushed.
Comment 34 Jonathan Druart 2021-10-07 08:15:07 UTC
Created attachment 125862 [details] [review]
Bug 29083: Don't pass _count var
Comment 35 Jonathan Druart 2021-10-07 08:15:24 UTC
(In reply to Jonathan Druart from comment #34)
> Created attachment 125862 [details] [review] [review]
> Bug 29083: Don't pass _count var

Something wrong with size?
Comment 36 Jonathan Druart 2021-10-07 09:21:03 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 37 Tomás Cohen Arazi 2021-10-07 11:09:14 UTC
(In reply to Jonathan Druart from comment #35)
> (In reply to Jonathan Druart from comment #34)
> > Created attachment 125862 [details] [review] [review] [review]
> > Bug 29083: Don't pass _count var
> 
> Something wrong with size?

It was used twice, not sure it was worth calculating twice. Don't you care about global warming? LOL
Comment 38 Jonathan Druart 2021-10-07 11:40:09 UTC
I am expecting it to be cached at some point at lower level (dbix or dbms).