As bug 29083 does for Koha::Patron, this bug will deal the with Koha::Biblio counterpart.
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>
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
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>
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>
Another nice improvement.. no regressions found. Signing off
QA: Looking here
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.
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_*
(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
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>
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>
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
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.
Applying: Bug 29084: Unit tests error: sha1 information is lacking or useless (t/db_dependent/Koha/Biblio.t). error: could not build fake ancestor
(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.
Patches will be squashed when pushed.
Pushed to master for 21.11, thanks to everybody involved!