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.
Please correct me if I'm wrong, but don't we always show the place hold link?
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?
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!
Will provide an 'intelligent guess' at search results without the performance penalty of going through all items. Detail can be more precise.
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>
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>
Created attachment 72859 [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.
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>
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>
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>
Created attachment 72911 [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.
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).
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>
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.
(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.
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>
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>
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>
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.
Created attachment 72930 [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.
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>
(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.
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?
(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.
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>
(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.
(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?
(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.
(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?"
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>
(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
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>
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>
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>
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>
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>
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>
Jonathan: Did you see the last changes after your comment ?
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>
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>
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>
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>
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>
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>
Simple rebase on Koha/Biblio.pm QA still needs to be finished ;)
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>
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>
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>
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>
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>
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>
(In reply to Chris Cormack from comment #51) > Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Thx
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' "
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>
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>
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>
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>
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>
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>
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>
Back in the PQA queue. Removed some lines from patch 6 and added patch 7 in order to meet comment53.
(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"?
(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 ?
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.
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.
Awesome work all! Pushed to master for 18.11
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.