Bug 17530 - Don't show 'article request' link when no article requests are permitted
Summary: Don't show 'article request' link when no article requests are permitted
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on: 14610
Blocks: 21475 22735
  Show dependency treegraph
 
Reported: 2016-11-01 14:00 UTC by Katrin Fischer
Modified: 2020-11-12 15:07 UTC (History)
9 users (show)

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


Attachments
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes (9.80 KB, patch)
2018-03-14 13:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use may_article_request method in opac-search (4.10 KB, patch)
2018-03-14 13:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.11 KB, patch)
2018-03-14 13:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.17 KB, patch)
2018-03-14 13:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes (9.80 KB, patch)
2018-03-15 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use may_article_request method in opac-search (4.10 KB, patch)
2018-03-15 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.11 KB, patch)
2018-03-15 08:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.17 KB, patch)
2018-03-15 08:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes (9.83 KB, patch)
2018-03-15 09:39 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 17530: Use may_article_request method in opac-search (4.13 KB, patch)
2018-03-15 09:40 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.19 KB, patch)
2018-03-15 09:40 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace our variable by cached value (6.28 KB, patch)
2018-03-15 11:14 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace our variable by cached value (6.34 KB, patch)
2018-03-15 11:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace class_or_self variables (1.71 KB, patch)
2018-05-31 07:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Move may_article_request to ItemType (8.43 KB, patch)
2018-06-04 10:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes (9.85 KB, patch)
2018-07-02 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use may_article_request method in opac-search (4.17 KB, patch)
2018-07-02 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.23 KB, patch)
2018-07-02 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace our variable by cached value (6.34 KB, patch)
2018-07-02 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace class_or_self variables (1.71 KB, patch)
2018-07-02 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Move may_article_request to ItemType (8.43 KB, patch)
2018-07-02 11:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes (9.85 KB, patch)
2018-07-25 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use may_article_request method in opac-search (4.17 KB, patch)
2018-07-25 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.23 KB, patch)
2018-07-25 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace our variable by cached value (6.34 KB, patch)
2018-07-25 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace class_or_self variables (1.71 KB, patch)
2018-07-25 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Move may_article_request to ItemType (7.22 KB, patch)
2018-07-25 14:30 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes (9.86 KB, patch)
2018-08-21 09:20 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17530: Use may_article_request method in opac-search (4.19 KB, patch)
2018-08-21 09:20 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.25 KB, patch)
2018-08-21 09:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace our variable by cached value (6.35 KB, patch)
2018-08-21 09:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace class_or_self variables (1.73 KB, patch)
2018-08-21 09:21 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Move may_article_request to ItemType (7.27 KB, patch)
2018-08-21 09:22 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes (9.90 KB, patch)
2018-09-03 11:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use may_article_request method in opac-search (4.22 KB, patch)
2018-09-03 11:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: Use can_article_request to control sidebar link (4.29 KB, patch)
2018-09-03 11:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace our variable by cached value (6.39 KB, patch)
2018-09-03 11:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Replace class_or_self variables (1.76 KB, patch)
2018-09-03 11:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Move may_article_request to ItemType (8.48 KB, patch)
2018-09-03 11:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Add pref ArticleRequestsLinkControl (7.77 KB, patch)
2018-09-03 11:16 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17530: (QA follow-up) Fix caching error (1.21 KB, patch)
2018-09-07 06:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2016-11-01 14:00:51 UTC
Don't show the article request link on records where there is no article request possible.

At the moment the article request link shows on all records, no matter if the circulation conditions allow for an article request or not.

We should not show the link when an article request is in general not possible.
Comment 1 Kyle M Hall 2016-11-10 02:10:36 UTC
Please correct me if I'm wrong, but don't we always show the place hold link?
Comment 2 Kyle M Hall 2016-11-10 11:54:04 UTC
I've been looking at Search.pm, it looks like the "Place hold" links display unless the item is not lost, withdrawn, damaged or notforloan. I don't think we should limit it by notforloan for reasons I think are pretty clear. Other than that I'd say we follow the lost/withdrawn/damaged logic that exists. Would you agree?
Comment 3 Katrin Fischer 2016-11-10 12:02:18 UTC
Hi Kyle, please ignore the enh request for now - I will write up what I mean. We used to have better control of the place hold links in the results list - but we lost this a while ago with another development. I think the bugs with the tests and CSS need to be fixed first!
Comment 4 Marcel de Rooy 2018-03-01 14:44:42 UTC
Will provide an 'intelligent guess' at search results without the performance penalty of going through all items. Detail can be more precise.
Comment 5 Marcel de Rooy 2018-03-14 13:12:42 UTC
Created attachment 72857 [details] [review]
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes

This routine looks at the issuingrules and makes a fast 'intelligent
guess' if article requests may be allowed for item types while
branch code and patron category may not be available.

We also add Koha::Biblio->may_article_request using the routine based
on default item type or parameter. Implemented as both class and instance
method. Added a few tests.

Test plan:
[1] Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
[2] Run t/db_dependent/ArticleRequests.t to test Koha::Biblio changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2018-03-14 13:12:45 UTC
Created attachment 72858 [details] [review]
Bug 17530: Use may_article_request method in opac-search

If you enable pref ArticleRequests, until now all search results got the
'Request article' link. This patch tries to improve the situation by
using the new sub with the itemtype of the search result.

In most cases the number of links will drastically decrease. It may still
be possible sometimes that a link is shown while it effectively is not
possible, but we do not get the performance burden of determining that and
going through all items.

Test plan:
[1] Look for two borrowers P1 and P2 within categories C1 resp. C2.
[2] Look for two biblios B1 and B2 with default item types I1 resp. I2.
    (See 942c in case of MARC21.)
[3] Make sure that no circ rules allow article requests. Enable the pref.
[4] Add/modify circ rule category=C1, itemtype=I1, art_req=yes.
    Log out. Search for B1 and B2, verify that only B1 has AR link.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that no biblio has AR link.
[5] Add/modify circ rule category=C2, itemtype=I2, art_req=item_only.
    Log out. Search for B1 and B2, verify that both have AR links.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that only B2 has AR link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2018-03-14 13:12:49 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2018-03-14 13:13:56 UTC
Created attachment 72860 [details] [review]
Bug 17530: Use can_article_request to control sidebar link

Before this patch, the 'Request article' link is displayed whenever the
pref is enabled. In many cases this might be useless. Instead of a guess
as in opac-search, we now call can_article_request to know for sure.
Note: at least this is the case when a user has logged in.

Update sidebar template with template variable artreqpossible.
Add code in opac-detail, MARCdetail and ISBDdetail to fill it.

Test plan:
[1] Look for two biblios with items: one that should allow article requests
    and one that should not (respecting branch, patron, item type).
[2] Verify on detail, ISBD and MARC that the link is displayed for
    the first biblio and hidden for the second biblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2018-03-15 08:48:13 UTC
Created attachment 72909 [details] [review]
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes

This routine looks at the issuingrules and makes a fast 'intelligent
guess' if article requests may be allowed for item types while
branch code and patron category may not be available.

We also add Koha::Biblio->may_article_request using the routine based
on default item type or parameter. Implemented as both class and instance
method. Added a few tests.

Test plan:
[1] Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
[2] Run t/db_dependent/ArticleRequests.t to test Koha::Biblio changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2018-03-15 08:48:17 UTC
Created attachment 72910 [details] [review]
Bug 17530: Use may_article_request method in opac-search

If you enable pref ArticleRequests, until now all search results got the
'Request article' link. This patch tries to improve the situation by
using the new sub with the itemtype of the search result.

In most cases the number of links will drastically decrease. It may still
be possible sometimes that a link is shown while it effectively is not
possible, but we do not get the performance burden of determining that and
going through all items.

Test plan:
[1] Look for two borrowers P1 and P2 within categories C1 resp. C2.
[2] Look for two biblios B1 and B2 with default item types I1 resp. I2.
    (See 942c in case of MARC21.)
[3] Make sure that no circ rules allow article requests. Enable the pref.
[4] Add/modify circ rule category=C1, itemtype=I1, art_req=yes.
    Log out. Search for B1 and B2, verify that only B1 has AR link.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that no biblio has AR link.
[5] Add/modify circ rule category=C2, itemtype=I2, art_req=item_only.
    Log out. Search for B1 and B2, verify that both have AR links.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that only B2 has AR link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2018-03-15 08:48:21 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2018-03-15 08:51:34 UTC
Fixed a warning like:
Use of uninitialized value $itemtype in hash element at /usr/share/koha/shadowclone/Koha/Biblio.pm line 125.
Acquisition records may not yet have a 942c (default item type).
Comment 13 Marcel de Rooy 2018-03-15 08:53:17 UTC
Created attachment 72912 [details] [review]
Bug 17530: Use can_article_request to control sidebar link

Before this patch, the 'Request article' link is displayed whenever the
pref is enabled. In many cases this might be useless. Instead of a guess
as in opac-search, we now call can_article_request to know for sure.
Note: at least this is the case when a user has logged in.

Update sidebar template with template variable artreqpossible.
Add code in opac-detail, MARCdetail and ISBDdetail to fill it.

Test plan:
[1] Look for two biblios with items: one that should allow article requests
    and one that should not (respecting branch, patron, item type).
[2] Verify on detail, ISBD and MARC that the link is displayed for
    the first biblio and hidden for the second biblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Axelle Aix-Marseille Université 2018-03-15 08:56:06 UTC
Tested on sandbox Biblibre n°11

The patch doesn't work : the link remains on all pages (normal, ISBD, marc) and all biblios on the OPAC and in the staff client.
Comment 15 Marcel de Rooy 2018-03-15 08:59:35 UTC
(In reply to Axelle Aix-Marseille Université from comment #14)
> Tested on sandbox Biblibre n°11
> 
> The patch doesn't work : the link remains on all pages (normal, ISBD, marc)
> and all biblios on the OPAC and in the staff client.

Please explain what you did. Article requests / circ rules may not be trivial.
Comment 16 Brendan Gallagher 2018-03-15 09:39:55 UTC
Created attachment 72913 [details] [review]
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes

This routine looks at the issuingrules and makes a fast 'intelligent
guess' if article requests may be allowed for item types while
branch code and patron category may not be available.

We also add Koha::Biblio->may_article_request using the routine based
on default item type or parameter. Implemented as both class and instance
method. Added a few tests.

Test plan:
[1] Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
[2] Run t/db_dependent/ArticleRequests.t to test Koha::Biblio changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 17 Brendan Gallagher 2018-03-15 09:40:08 UTC
Created attachment 72914 [details] [review]
Bug 17530: Use may_article_request method in opac-search

If you enable pref ArticleRequests, until now all search results got the
'Request article' link. This patch tries to improve the situation by
using the new sub with the itemtype of the search result.

In most cases the number of links will drastically decrease. It may still
be possible sometimes that a link is shown while it effectively is not
possible, but we do not get the performance burden of determining that and
going through all items.

Test plan:
[1] Look for two borrowers P1 and P2 within categories C1 resp. C2.
[2] Look for two biblios B1 and B2 with default item types I1 resp. I2.
    (See 942c in case of MARC21.)
[3] Make sure that no circ rules allow article requests. Enable the pref.
[4] Add/modify circ rule category=C1, itemtype=I1, art_req=yes.
    Log out. Search for B1 and B2, verify that only B1 has AR link.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that no biblio has AR link.
[5] Add/modify circ rule category=C2, itemtype=I2, art_req=item_only.
    Log out. Search for B1 and B2, verify that both have AR links.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that only B2 has AR link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 18 Brendan Gallagher 2018-03-15 09:40:12 UTC
Created attachment 72915 [details] [review]
Bug 17530: Use can_article_request to control sidebar link

Before this patch, the 'Request article' link is displayed whenever the
pref is enabled. In many cases this might be useless. Instead of a guess
as in opac-search, we now call can_article_request to know for sure.
Note: at least this is the case when a user has logged in.

Update sidebar template with template variable artreqpossible.
Add code in opac-detail, MARCdetail and ISBDdetail to fill it.

Test plan:
[1] Look for two biblios with items: one that should allow article requests
    and one that should not (respecting branch, patron, item type).
[2] Verify on detail, ISBD and MARC that the link is displayed for
    the first biblio and hidden for the second biblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 19 Marcel de Rooy 2018-03-15 09:53:19 UTC
Note for QA: Saving state in Plack makes that changing the circ rules needs either restarting Plack or waiting until Plack served a configured number of requests per worker. Will still improve this.
Comment 20 Marcel de Rooy 2018-03-15 11:14:59 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2018-03-15 11:15:29 UTC
Created attachment 72931 [details] [review]
Bug 17530: (QA follow-up) Replace our variable by cached value

Instead of saving the state locally in a variable during Plack lifetime,
we move the saved hash to the cache. We clear the key when we enter
smart-rules.pl. This makes a change in circ rules immediately effective.

Test plan:
Run the modified tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2018-03-15 11:16:37 UTC
(In reply to Marcel de Rooy from comment #19)
> Note for QA: Saving state in Plack makes that changing the circ rules needs
> either restarting Plack or waiting until Plack served a configured number of
> requests per worker. Will still improve this.

QA: If you feel uncomfortable with the last follow-up, feel free to obsolete. I can move it to another report too.
Comment 23 Jonathan Druart 2018-05-28 19:20:44 UTC
I have not tested it, just had a quick look.

1.
This:
  Koha::Biblio->may_article_request
shows that Koha::Biblio is not the right place to put it. Koha::Object class should not have class methods.

2. In Koha::IssuingRules, $class_or_self must be named $class

3. The cache is cleared in a very lazy way :)

4. Maybe we do not need may_article_request, but only guess_article_requestable_itemtypes?
Comment 24 Marcel de Rooy 2018-05-31 07:56:34 UTC
(In reply to Jonathan Druart from comment #23)
> I have not tested it, just had a quick look.
Thats a good start ;)

> 1.
> This:
>   Koha::Biblio->may_article_request
> shows that Koha::Biblio is not the right place to put it. Koha::Object class
> should not have class methods.
Yes, this was arbitrary. I could move it to Koha::ItemType. What do you think?

> 2. In Koha::IssuingRules, $class_or_self must be named $class
Will fix that in a follow-up.

> 3. The cache is cleared in a very lazy way :)
Lazy is good enough here imo.

> 4. Maybe we do not need may_article_request, but only
> guess_article_requestable_itemtypes?
See point 1.
Comment 25 Marcel de Rooy 2018-05-31 07:57:32 UTC
Created attachment 75697 [details] [review]
Bug 17530: (QA follow-up) Replace class_or_self variables

See comment23. Requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Jonathan Druart 2018-05-31 20:09:51 UTC
(In reply to Marcel de Rooy from comment #24)
> (In reply to Jonathan Druart from comment #23)
> > I have not tested it, just had a quick look.
> Thats a good start ;)
> 
> > 1.
> > This:
> >   Koha::Biblio->may_article_request
> > shows that Koha::Biblio is not the right place to put it. Koha::Object class
> > should not have class methods.
> Yes, this was arbitrary. I could move it to Koha::ItemType. What do you
> think?
> > 4. Maybe we do not need may_article_request, but only
> > guess_article_requestable_itemtypes?
> See point 1.

If we remove it, there is no need to move it... I do not understand your point.


> > 3. The cache is cleared in a very lazy way :)
> Lazy is good enough here imo.

I do not think so, the cache is flushed every time the page is hit.
Comment 27 Marcel de Rooy 2018-06-01 06:37:36 UTC
(In reply to Jonathan Druart from comment #26)
> (In reply to Marcel de Rooy from comment #24)
> > (In reply to Jonathan Druart from comment #23)
> > > I have not tested it, just had a quick look.
> > Thats a good start ;)
> > 
> > > 1.
> > > This:
> > >   Koha::Biblio->may_article_request
> > > shows that Koha::Biblio is not the right place to put it. Koha::Object class
> > > should not have class methods.
> > Yes, this was arbitrary. I could move it to Koha::ItemType. What do you
> > think?
> > > 4. Maybe we do not need may_article_request, but only
> > > guess_article_requestable_itemtypes?
> > See point 1.
> 
> If we remove it, there is no need to move it... I do not understand your
> point.
> 

I do not want to remove it. I read: "I could move it to Koha::ItemType". Move is not remove?
Comment 28 Marcel de Rooy 2018-06-01 07:01:23 UTC
(In reply to Jonathan Druart from comment #26)
> > > 3. The cache is cleared in a very lazy way :)
> > Lazy is good enough here imo.
> 
> I do not think so, the cache is flushed every time the page is hit.

The L1 cache is cleared. I do not see the need to remove it frequently from memcached. If we change smart rules, it is cleared (even more strict than needed; read: programmer's lazy). This cache is just for faster presentation, no threat for bad data or so.
Comment 29 Jonathan Druart 2018-06-01 13:49:18 UTC
(In reply to Marcel de Rooy from comment #27)
> Move is not remove?

I suggested to remove it: "4. Maybe we do not need may_article_request, but only guess_article_requestable_itemtypes?"
Comment 30 Marcel de Rooy 2018-06-04 10:20:17 UTC
Created attachment 75751 [details] [review]
Bug 17530: (QA follow-up) Move may_article_request to ItemType

As requested by QA, we should move may_article_request out of Biblio.

For reasons of performance removed the wrapper layer of may_article_request
in opac-search. No need to look up all item types. For readability kept
the routine in the detail scripts.

Note for running ArticleRequests.t: A possible failure on the subtest
'search_limited' is addressed on bug 20866. So you can ignore that one.
As long as the subtest for may_article_request passes.

Test plan:
See previous patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2018-06-04 10:22:05 UTC
(In reply to Jonathan Druart from comment #29)
> (In reply to Marcel de Rooy from comment #27)
> > Move is not remove?
> 
> I suggested to remove it: "4. Maybe we do not need may_article_request, but
> only guess_article_requestable_itemtypes?"

Moved to Koha::ItemType and removed in opac-search.pl
Comment 32 Marcel de Rooy 2018-07-02 11:56:05 UTC
Created attachment 76617 [details] [review]
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes

This routine looks at the issuingrules and makes a fast 'intelligent
guess' if article requests may be allowed for item types while
branch code and patron category may not be available.

We also add Koha::Biblio->may_article_request using the routine based
on default item type or parameter. Implemented as both class and instance
method. Added a few tests.

Test plan:
[1] Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
[2] Run t/db_dependent/ArticleRequests.t to test Koha::Biblio changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 33 Marcel de Rooy 2018-07-02 11:56:10 UTC
Created attachment 76618 [details] [review]
Bug 17530: Use may_article_request method in opac-search

If you enable pref ArticleRequests, until now all search results got the
'Request article' link. This patch tries to improve the situation by
using the new sub with the itemtype of the search result.

In most cases the number of links will drastically decrease. It may still
be possible sometimes that a link is shown while it effectively is not
possible, but we do not get the performance burden of determining that and
going through all items.

Test plan:
[1] Look for two borrowers P1 and P2 within categories C1 resp. C2.
[2] Look for two biblios B1 and B2 with default item types I1 resp. I2.
    (See 942c in case of MARC21.)
[3] Make sure that no circ rules allow article requests. Enable the pref.
[4] Add/modify circ rule category=C1, itemtype=I1, art_req=yes.
    Log out. Search for B1 and B2, verify that only B1 has AR link.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that no biblio has AR link.
[5] Add/modify circ rule category=C2, itemtype=I2, art_req=item_only.
    Log out. Search for B1 and B2, verify that both have AR links.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that only B2 has AR link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 34 Marcel de Rooy 2018-07-02 11:56:15 UTC
Created attachment 76619 [details] [review]
Bug 17530: Use can_article_request to control sidebar link

Before this patch, the 'Request article' link is displayed whenever the
pref is enabled. In many cases this might be useless. Instead of a guess
as in opac-search, we now call can_article_request to know for sure.
Note: at least this is the case when a user has logged in.

Update sidebar template with template variable artreqpossible.
Add code in opac-detail, MARCdetail and ISBDdetail to fill it.

Test plan:
[1] Look for two biblios with items: one that should allow article requests
    and one that should not (respecting branch, patron, item type).
[2] Verify on detail, ISBD and MARC that the link is displayed for
    the first biblio and hidden for the second biblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 35 Marcel de Rooy 2018-07-02 11:56:20 UTC
Created attachment 76620 [details] [review]
Bug 17530: (QA follow-up) Replace our variable by cached value

Instead of saving the state locally in a variable during Plack lifetime,
we move the saved hash to the cache. We clear the key when we enter
smart-rules.pl. This makes a change in circ rules immediately effective.

Test plan:
Run the modified tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2018-07-02 11:56:24 UTC
Created attachment 76621 [details] [review]
Bug 17530: (QA follow-up) Replace class_or_self variables

See comment23. Requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2018-07-02 11:56:29 UTC
Created attachment 76622 [details] [review]
Bug 17530: (QA follow-up) Move may_article_request to ItemType

As requested by QA, we should move may_article_request out of Biblio.

For reasons of performance removed the wrapper layer of may_article_request
in opac-search. No need to look up all item types. For readability kept
the routine in the detail scripts.

Note for running ArticleRequests.t: A possible failure on the subtest
'search_limited' is addressed on bug 20866. So you can ignore that one.
As long as the subtest for may_article_request passes.

Test plan:
See previous patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2018-07-02 11:57:47 UTC
Jonathan: Did you see the last changes after your comment ?
Comment 39 Marcel de Rooy 2018-07-25 14:30:12 UTC
Created attachment 77258 [details] [review]
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes

This routine looks at the issuingrules and makes a fast 'intelligent
guess' if article requests may be allowed for item types while
branch code and patron category may not be available.

We also add Koha::Biblio->may_article_request using the routine based
on default item type or parameter. Implemented as both class and instance
method. Added a few tests.

Test plan:
[1] Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
[2] Run t/db_dependent/ArticleRequests.t to test Koha::Biblio changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 40 Marcel de Rooy 2018-07-25 14:30:17 UTC
Created attachment 77259 [details] [review]
Bug 17530: Use may_article_request method in opac-search

If you enable pref ArticleRequests, until now all search results got the
'Request article' link. This patch tries to improve the situation by
using the new sub with the itemtype of the search result.

In most cases the number of links will drastically decrease. It may still
be possible sometimes that a link is shown while it effectively is not
possible, but we do not get the performance burden of determining that and
going through all items.

Test plan:
[1] Look for two borrowers P1 and P2 within categories C1 resp. C2.
[2] Look for two biblios B1 and B2 with default item types I1 resp. I2.
    (See 942c in case of MARC21.)
[3] Make sure that no circ rules allow article requests. Enable the pref.
[4] Add/modify circ rule category=C1, itemtype=I1, art_req=yes.
    Log out. Search for B1 and B2, verify that only B1 has AR link.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that no biblio has AR link.
[5] Add/modify circ rule category=C2, itemtype=I2, art_req=item_only.
    Log out. Search for B1 and B2, verify that both have AR links.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that only B2 has AR link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 41 Marcel de Rooy 2018-07-25 14:30:23 UTC
Created attachment 77260 [details] [review]
Bug 17530: Use can_article_request to control sidebar link

Before this patch, the 'Request article' link is displayed whenever the
pref is enabled. In many cases this might be useless. Instead of a guess
as in opac-search, we now call can_article_request to know for sure.
Note: at least this is the case when a user has logged in.

Update sidebar template with template variable artreqpossible.
Add code in opac-detail, MARCdetail and ISBDdetail to fill it.

Test plan:
[1] Look for two biblios with items: one that should allow article requests
    and one that should not (respecting branch, patron, item type).
[2] Verify on detail, ISBD and MARC that the link is displayed for
    the first biblio and hidden for the second biblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 42 Marcel de Rooy 2018-07-25 14:30:28 UTC
Created attachment 77261 [details] [review]
Bug 17530: (QA follow-up) Replace our variable by cached value

Instead of saving the state locally in a variable during Plack lifetime,
we move the saved hash to the cache. We clear the key when we enter
smart-rules.pl. This makes a change in circ rules immediately effective.

Test plan:
Run the modified tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Marcel de Rooy 2018-07-25 14:30:33 UTC
Created attachment 77262 [details] [review]
Bug 17530: (QA follow-up) Replace class_or_self variables

See comment23. Requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 44 Marcel de Rooy 2018-07-25 14:30:38 UTC
Created attachment 77263 [details] [review]
Bug 17530: (QA follow-up) Move may_article_request to ItemType

As requested by QA, we should move may_article_request out of Biblio.

For reasons of performance removed the wrapper layer of may_article_request
in opac-search. No need to look up all item types. For readability kept
the routine in the detail scripts.

Note for running ArticleRequests.t: A possible failure on the subtest
'search_limited' is addressed on bug 20866. So you can ignore that one.
As long as the subtest for may_article_request passes.

Test plan:
See previous patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Marcel de Rooy 2018-07-25 14:32:14 UTC
Simple rebase on Koha/Biblio.pm
QA still needs to be finished ;)
Comment 46 Chris Cormack 2018-08-21 09:20:21 UTC
Created attachment 78030 [details] [review]
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes

This routine looks at the issuingrules and makes a fast 'intelligent
guess' if article requests may be allowed for item types while
branch code and patron category may not be available.

We also add Koha::Biblio->may_article_request using the routine based
on default item type or parameter. Implemented as both class and instance
method. Added a few tests.

Test plan:
[1] Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
[2] Run t/db_dependent/ArticleRequests.t to test Koha::Biblio changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 47 Chris Cormack 2018-08-21 09:20:45 UTC
Created attachment 78031 [details] [review]
Bug 17530: Use may_article_request method in opac-search

If you enable pref ArticleRequests, until now all search results got the
'Request article' link. This patch tries to improve the situation by
using the new sub with the itemtype of the search result.

In most cases the number of links will drastically decrease. It may still
be possible sometimes that a link is shown while it effectively is not
possible, but we do not get the performance burden of determining that and
going through all items.

Test plan:
[1] Look for two borrowers P1 and P2 within categories C1 resp. C2.
[2] Look for two biblios B1 and B2 with default item types I1 resp. I2.
    (See 942c in case of MARC21.)
[3] Make sure that no circ rules allow article requests. Enable the pref.
[4] Add/modify circ rule category=C1, itemtype=I1, art_req=yes.
    Log out. Search for B1 and B2, verify that only B1 has AR link.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that no biblio has AR link.
[5] Add/modify circ rule category=C2, itemtype=I2, art_req=item_only.
    Log out. Search for B1 and B2, verify that both have AR links.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that only B2 has AR link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 48 Chris Cormack 2018-08-21 09:21:05 UTC
Created attachment 78032 [details] [review]
Bug 17530: Use can_article_request to control sidebar link

Before this patch, the 'Request article' link is displayed whenever the
pref is enabled. In many cases this might be useless. Instead of a guess
as in opac-search, we now call can_article_request to know for sure.
Note: at least this is the case when a user has logged in.

Update sidebar template with template variable artreqpossible.
Add code in opac-detail, MARCdetail and ISBDdetail to fill it.

Test plan:
[1] Look for two biblios with items: one that should allow article requests
    and one that should not (respecting branch, patron, item type).
[2] Verify on detail, ISBD and MARC that the link is displayed for
    the first biblio and hidden for the second biblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 49 Chris Cormack 2018-08-21 09:21:29 UTC
Created attachment 78033 [details] [review]
Bug 17530: (QA follow-up) Replace our variable by cached value

Instead of saving the state locally in a variable during Plack lifetime,
we move the saved hash to the cache. We clear the key when we enter
smart-rules.pl. This makes a change in circ rules immediately effective.

Test plan:
Run the modified tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 50 Chris Cormack 2018-08-21 09:21:48 UTC
Created attachment 78034 [details] [review]
Bug 17530: (QA follow-up) Replace class_or_self variables

See comment23. Requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 51 Chris Cormack 2018-08-21 09:22:15 UTC
Created attachment 78035 [details] [review]
Bug 17530: (QA follow-up) Move may_article_request to ItemType

As requested by QA, we should move may_article_request out of Biblio.

For reasons of performance removed the wrapper layer of may_article_request
in opac-search. No need to look up all item types. For readability kept
the routine in the detail scripts.

Note for running ArticleRequests.t: A possible failure on the subtest
'search_limited' is addressed on bug 20866. So you can ignore that one.
As long as the subtest for may_article_request passes.

Test plan:
See previous patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 52 Marcel de Rooy 2018-08-22 11:00:40 UTC
(In reply to Chris Cormack from comment #51)
> Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>

Thx
Comment 53 Nick Clemens 2018-08-30 12:33:00 UTC
The code here seems solid, but I worry about false negatives when itemtypes/itypes might be inconsistent (i.e. initial result doesn't have a link so patron never clicks in to record to see that they can place a request)

Can a syspref be added to control this? Something like "ArticleRequests: Show article request link 'Always/Use best guess' "
Comment 54 Marcel de Rooy 2018-09-03 11:15:38 UTC
Created attachment 78362 [details] [review]
Bug 17530: Add Koha::IssuingRules->guess_article_requestable_itemtypes

This routine looks at the issuingrules and makes a fast 'intelligent
guess' if article requests may be allowed for item types while
branch code and patron category may not be available.

We also add Koha::Biblio->may_article_request using the routine based
on default item type or parameter. Implemented as both class and instance
method. Added a few tests.

Test plan:
[1] Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
[2] Run t/db_dependent/ArticleRequests.t to test Koha::Biblio changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 55 Marcel de Rooy 2018-09-03 11:15:44 UTC
Created attachment 78363 [details] [review]
Bug 17530: Use may_article_request method in opac-search

If you enable pref ArticleRequests, until now all search results got the
'Request article' link. This patch tries to improve the situation by
using the new sub with the itemtype of the search result.

In most cases the number of links will drastically decrease. It may still
be possible sometimes that a link is shown while it effectively is not
possible, but we do not get the performance burden of determining that and
going through all items.

Test plan:
[1] Look for two borrowers P1 and P2 within categories C1 resp. C2.
[2] Look for two biblios B1 and B2 with default item types I1 resp. I2.
    (See 942c in case of MARC21.)
[3] Make sure that no circ rules allow article requests. Enable the pref.
[4] Add/modify circ rule category=C1, itemtype=I1, art_req=yes.
    Log out. Search for B1 and B2, verify that only B1 has AR link.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that no biblio has AR link.
[5] Add/modify circ rule category=C2, itemtype=I2, art_req=item_only.
    Log out. Search for B1 and B2, verify that both have AR links.
    Log in as P1. Verify that only B1 has AR link.
    Log in as P2. Verify that only B2 has AR link.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 56 Marcel de Rooy 2018-09-03 11:15:49 UTC
Created attachment 78364 [details] [review]
Bug 17530: Use can_article_request to control sidebar link

Before this patch, the 'Request article' link is displayed whenever the
pref is enabled. In many cases this might be useless. Instead of a guess
as in opac-search, we now call can_article_request to know for sure.
Note: at least this is the case when a user has logged in.

Update sidebar template with template variable artreqpossible.
Add code in opac-detail, MARCdetail and ISBDdetail to fill it.

Test plan:
[1] Look for two biblios with items: one that should allow article requests
    and one that should not (respecting branch, patron, item type).
[2] Verify on detail, ISBD and MARC that the link is displayed for
    the first biblio and hidden for the second biblio.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 57 Marcel de Rooy 2018-09-03 11:15:56 UTC
Created attachment 78365 [details] [review]
Bug 17530: (QA follow-up) Replace our variable by cached value

Instead of saving the state locally in a variable during Plack lifetime,
we move the saved hash to the cache. We clear the key when we enter
smart-rules.pl. This makes a change in circ rules immediately effective.

Test plan:
Run the modified tests.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 58 Marcel de Rooy 2018-09-03 11:16:01 UTC
Created attachment 78366 [details] [review]
Bug 17530: (QA follow-up) Replace class_or_self variables

See comment23. Requested by QA.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 59 Marcel de Rooy 2018-09-03 11:16:06 UTC
Created attachment 78367 [details] [review]
Bug 17530: (QA follow-up) Move may_article_request to ItemType

As requested by QA, we should move may_article_request out of Biblio.

For reasons of performance removed the wrapper layer of may_article_request
in opac-search. No need to look up all item types. For readability kept
the routine in the detail scripts.

Note for running ArticleRequests.t: A possible failure on the subtest
'search_limited' is addressed on bug 20866. So you can ignore that one.
As long as the subtest for may_article_request passes.

Test plan:
See previous patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 60 Marcel de Rooy 2018-09-03 11:16:11 UTC
Created attachment 78368 [details] [review]
Bug 17530: (QA follow-up) Add pref ArticleRequestsLinkControl

As requested by RM on Bugzilla comment53.

This pref allows you to always display the link on search results.
Which is the previous behavior ;) It will raise false positives, whereas
the algorithm may result in false positives or negatives (depending on
default item type and circulation rules).

When upgrading the pref is set to always (current behavior), new installs
get the new calc value using the algorithm in may_article_request.

Test plan:
Run t/db_dependent/Koha/IssuingRules/guess_article_requestable_itemtypes.t
Run t/db_dependent/ArticleRequests.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 61 Marcel de Rooy 2018-09-03 11:17:46 UTC
Back in the PQA queue. Removed some lines from patch 6 and added patch 7 in order to meet comment53.
Comment 62 Nick Clemens 2018-09-06 11:51:59 UTC
(In reply to Marcel de Rooy from comment #61)
> Back in the PQA queue. Removed some lines from patch 6 and added patch 7 in
> order to meet comment53.

These lines don't seem correct, I keep getting a 'Can't use string ("1") as a hashref'
174     return $last_article_requestable_guesses->{$key}
175         if $last_article_requestable_guesses && exists $last_article_requestable_guesses->{$key};

Should that be "return $last_article_requestable_guesses"?
Comment 63 Marcel de Rooy 2018-09-06 13:45:56 UTC
(In reply to Nick Clemens from comment #62)
> (In reply to Marcel de Rooy from comment #61)
> > Back in the PQA queue. Removed some lines from patch 6 and added patch 7 in
> > order to meet comment53.
> 
> These lines don't seem correct, I keep getting a 'Can't use string ("1") as
> a hashref'
> 174     return $last_article_requestable_guesses->{$key}
> 175         if $last_article_requestable_guesses && exists
> $last_article_requestable_guesses->{$key};
> 
> Should that be "return $last_article_requestable_guesses"?

Did you flush the cache? Is memcache causing this problem ?
Comment 64 Marcel de Rooy 2018-09-07 06:13:24 UTC
This change in the 4th patch is the culprit:

-    $last_article_requestable_guesses->{$key} = $res;
+    $cache->set_in_cache(GUESSED_ITEMTYPES_KEY, $res);

We lost a hash level here.. This got mostly unnoticed since the routine just recalculates if the hash key was not found. So the tests just passed..
It seems that you found this error because you must have a match between patron categories and itemtypes. If the routine cached e.g. { A => 1 } for itemtype A and you ran it again with patron category A, opac-search did not get a hash but a scalar, etc.
Comment 65 Marcel de Rooy 2018-09-07 06:21:08 UTC
Created attachment 78499 [details] [review]
Bug 17530: (QA follow-up) Fix caching error

The fourth patch made this change:
-    $last_article_requestable_guesses->{$key} = $res;
+    $cache->set_in_cache(GUESSED_ITEMTYPES_KEY, $res);

This removed the category level in the cached hash. Adding it back here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested by verifying cached values, running opac-search with anonymous
session as well as user context.
Comment 66 Nick Clemens 2018-09-07 16:04:37 UTC
Awesome work all!

Pushed to master for 18.11
Comment 67 Martin Renvoize 2018-09-11 08:29:09 UTC
This could result in a surprising/unexpected change for the end user and as such I'm classifying it as an enhancement and will not be backporting to the 18.05.x series.