Bug 19889

Summary: LocalHoldsPriority needs exclusions
Product: Koha Reporter: Christopher Brannon <cbrannon>
Component: Hold requestsAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: enhancement    
Priority: P1 - high CC: 1joynelson, gmcharlt, jonathan.druart, katrin.fischer, kyle, lucas, margaret, nick, tomascohen, wizzyrea
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26027
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26129
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00
Bug Depends on:    
Bug Blocks: 26047, 26167, 26513    
Attachments: Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Bug 19889: (follow-up) Fix overlapping blue box and message in batch item modification
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Bug 19889: (follow-up) Fix overlapping blue box and message in batch item modification
Bug 19889: (follow-up) Add tests
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema
Bug 19889: Add tests
Bug 19889: Make it possible to exclude items and categories from local holds priority
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Bug 19889: (follow-up) Fix overlapping blue box and message in batch item modification
Bug 19889: (follow-up) Add tests
Bug 19889: (follow-up) Fix few minor things
Bug 19889: (follow-up) Add tests
Bug 19889: (follow-up) Fix few minor things
Bug 19889: (QA follow-up) use em over i and update field if unset
Bug 19889: (follow-up) Fix few minor things
Bug 19889: (QA follow-up) use em over i and update field if unset
Bug 19889: (QA follow-up) Check that exclude from local holds priority is set and not a blank string
Bug 19889: (QA follow-up) Check that exclude from local holds priority is set and not a blank string
Bug 19889: (follow-up) Fix text in moredetail.tt
Bug 19889: (follow-up) Fix text in moredetail.tt
Bug 19889: Add select2 class to the new dropdown list
Bug 19889: Add new attribute to the REST API definition

Description Christopher Brannon 2017-12-28 18:39:47 UTC
localholdspriority is great, unless you have a unique or rare collection that is popular.  If you have such a collection, and ongoing holds are being placed on it to be picked up at several locations, only the patrons at that library will ever get their hands on it unless a patron is smart enough and willing to pickup the item at that location.

I would propose an exclusion preference, that would exclude listed patron categories, item types, formats, or specific items.  Any other item criteria or factors that I missed could be added to this list.

Currently we have patrons at other libraries that will never get the items they have requested.
Comment 1 Liz Rea 2019-09-24 15:35:43 UTC
Sponsored by CIN.
Comment 2 Tomás Cohen Arazi 2020-06-05 18:15:54 UTC
# Work to be done:

1 - Add a new column to the items table - 'exclude_from_local_holds_priority' or similar
2 - This field should be settable on the edit item form, and bath modifications
3 - Modify existing local holds priority code to skip items with column flag 'exclude_from_local_holds_priority' set for checking local hold priorities
4 - Add a column to 'categories' => 'excluded_from_localholds_priority'
5 - Alter code to ignore local holds priority for these
Comment 3 Agustín Moyano 2020-06-10 23:04:57 UTC
Created attachment 105715 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)
Comment 4 Agustín Moyano 2020-06-10 23:05:01 UTC
Created attachment 105716 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)
Comment 5 Agustín Moyano 2020-06-10 23:05:06 UTC
Created attachment 105717 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)
Comment 6 Agustín Moyano 2020-06-10 23:05:11 UTC
Created attachment 105718 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)
Comment 7 Christopher Brannon 2020-06-26 14:40:18 UTC
This is only one of four criteria that was specifically asked for in exclusion:  patron categories, item types, formats, or specific items.

I will test this patch, but it cannot be signed off as is.
Comment 8 Christopher Brannon 2020-06-26 14:43:12 UTC
(In reply to Christopher Brannon from comment #7)
> This is only one of four criteria that was specifically asked for in
> exclusion:  patron categories, item types, formats, or specific items.
> 
> I will test this patch, but it cannot be signed off as is.

Ignore this comment.  The description of the test wasn't clear.  I will test to see if all four criteria are met and works.
Comment 9 Christopher Brannon 2020-06-26 16:07:51 UTC
Agustin, Kidclamp had to rebase a minor conflict.  I don't know the details.

After that, this is what I found:

Everything works except the Batch Item Modification.  It did not change the setting for the items specified.  Might be because of the rebase.  Please check.

Also, the description on the patron category is confusing.  It should read "If Yes, holds placed by patrons of this category will not be given priority."

I am also unable to prove the test, step 21.  I am testing from a sandbox.
Comment 10 Agustín Moyano 2020-06-30 17:16:41 UTC
Created attachment 106435 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)
Comment 11 Agustín Moyano 2020-06-30 17:16:47 UTC
Created attachment 106436 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)
Comment 12 Agustín Moyano 2020-06-30 17:16:53 UTC
Created attachment 106437 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)
Comment 13 Agustín Moyano 2020-06-30 17:16:58 UTC
Created attachment 106438 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)
Comment 14 Agustín Moyano 2020-06-30 17:21:11 UTC
(In reply to Christopher Brannon from comment #9)
> Agustin, Kidclamp had to rebase a minor conflict.  I don't know the details.
> 
> After that, this is what I found:
> 
> Everything works except the Batch Item Modification.  It did not change the
> setting for the items specified.  Might be because of the rebase.  Please
> check.
> 
> Also, the description on the patron category is confusing.  It should read
> "If Yes, holds placed by patrons of this category will not be given
> priority."
> 
> I am also unable to prove the test, step 21.  I am testing from a sandbox.

Hello Christopher, I just updated and rebased this patches.

About the description you're right.. at first I understood patron's category exclusion backwards (as if you would exclude the patron that couldn't get the item because of localholdspriority), but later coded things right, and that description remained wrong.

Please test again.

Thanks.
Comment 15 ByWater Sandboxes 2020-06-30 18:39:31 UTC
Created attachment 106439 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 16 ByWater Sandboxes 2020-06-30 18:39:34 UTC
Created attachment 106440 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 17 ByWater Sandboxes 2020-06-30 18:39:36 UTC
Created attachment 106441 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 18 ByWater Sandboxes 2020-06-30 18:39:39 UTC
Created attachment 106442 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 19 Christopher Brannon 2020-06-30 18:58:22 UTC
Test passes, and everything works according to the test.  Let's discuss the following issues before I sign off, since we are funding this project:

* When you update priority in the item (yes/no), there is nothing that indicates that it is done or set.  It would be helpful if there was something that appeared to indicate that it took.  Maybe (SET/UNSET) next to the dropbox?

* The patron exclusion seems to be in an uncommon place.  Technically, it makes sense to have this in the patron category, but usually these kinds of conditions are setup in the preferences.pl.  I state this, because if we do some forward thinking here, if we want to do other exclusions in the future, it might be better to set this as a textbox pref, where you would set this exclusion with a patroncategory: PT, and then in the future we could exclude by itemtype, collectioncode, or other ways.  It just makes sense to keep the exclusions all in one place rather than spread out everywhere.  And if you do this, could biblio or barcode/itemnumber be set here as well, rather than in the item record itself?

* A couple of the criteria we had asked for in the exclusions was item types and formats.  Those were not addressed in these patches.  I would be willing to forget about formats at this point, but would like to see item types.  I am kicking myself for not including biblio in possible exclusions.  As is, we have to exclude individual items.  If that can be added easily, great.  If not, at least the item types needs to be, since it was in the original request.
Comment 20 Christopher Brannon 2020-07-06 17:13:30 UTC
Please the last paragraph in my comment about the criteria.  I was going off the original description in this bug rather than the signed contract.  This is what the contract called for:

* Add a new column to the items table - 'ExcludeFromLoaclHoldsPriority' or similar
* Addd the ability to mark the item from the items tab or the batch item modification page
* Add a system preference 'CategoriesToExcludeFromLocalHoldsPriority'
* This system preference would take a list of patron categories
* Alter code to ignore local holds priority for these

Talk to me about the reasoning for the placement of the patron exclusion in the patron category rather than a central preference that was outlined here.  I just want to understand before you do anything.

My comment about the priority for the item not showing as set still stands.  That is confusing.
Comment 21 Tomás Cohen Arazi 2020-07-06 17:32:15 UTC
(In reply to Christopher Brannon from comment #20)
> * This system preference would take a list of patron categories
> * Alter code to ignore local holds priority for these
> 
> Talk to me about the reasoning for the placement of the patron exclusion in
> the patron category rather than a central preference that was outlined here.
> I just want to understand before you do anything.

The original idea was to add a syspref, as you outline. I'm the one that decided it was better to make it a patron category attribute. This, because the syspref is basically a way to say something about the category, but in a place that is not tied to categories (and thus unnatural).

This is the reasoning:

From the coding point of view, we might need/want to ask if a patron belongs to a category that is excluded from localholdspriority, and we would do it like this:

    # Find the patron
    my $patron = Koha::Patrons->find($request->{borrowernumber});
    # skip the patron if its category is excluded
    next if $patron->category->exclude_from_local_holds_priority; 

if we go the sysprefs way it would read like:

    my @excluded_categories = split(/\|/, C4::Context->preference("CategoriesToExcludeFromLocalHoldsPriority")) // ();
    my $patron = Koha::Patrons->find($request->{borrowernumber});
    next if any { $_ eq $patron->categorycode } @excluded_categories;

So the result is similar, but ugly-ish. And involves an error prone free-text field with pipe-separated values. So I don't see any advantage on it. If we were to expose this condition on the API also, we would need to add methods to extract the boolean with this kind of comparison, instead of just reading what's on the category table.

That said, we can absolutely roll back to the syspref approach and come back to refactor this later. Once we have a more clear interface for setting all things related to local holds priorities in a single place. As I understand your point of the whole setting being sparse in different places being counter-intuitive.

> My comment about the priority for the item not showing as set still stands. 
> That is confusing.
Comment 22 Christopher Brannon 2020-07-09 14:28:58 UTC
(In reply to Tomás Cohen Arazi from comment #21)
 
> So the result is similar, but ugly-ish. And involves an error prone
> free-text field with pipe-separated values. So I don't see any advantage on
> it. If we were to expose this condition on the API also, we would need to
> add methods to extract the boolean with this kind of comparison, instead of
> just reading what's on the category table.

What if it wasn't a free-text field?  What if we did something similar to what is being done in https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22844?  Select the categories from a list rather than free typing?

I'd prefer to keep these settings together, but if you think it makes more sense to put exclusions in the patron categories, then that would be acceptable.  It works.  My biggest concern is trying to debug behavior.  It's one thing to have the items with their own exclusions in the record.  But trying to figure out why it excluding items in some cases and not others because of a patron category exclusion might be more challenging.  If there were some indicator when the exclusion is skipped somewhere, then I would have no problem with it.  If it is logged, then it would be easy to trace.

I'm just trying to make sure that our enhancement doesn't become a headache for those using it to try and understand system behavior.  Circulation rules alone are hard to follow when dealing with multiple branches.
Comment 23 Agustín Moyano 2020-07-17 19:25:58 UTC
(In reply to Christopher Brannon from comment #22)
> (In reply to Tomás Cohen Arazi from comment #21)
>  
> > So the result is similar, but ugly-ish. And involves an error prone
> > free-text field with pipe-separated values. So I don't see any advantage on
> > it. If we were to expose this condition on the API also, we would need to
> > add methods to extract the boolean with this kind of comparison, instead of
> > just reading what's on the category table.
> 
> What if it wasn't a free-text field?  What if we did something similar to
> what is being done in
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22844?  Select the
> categories from a list rather than free typing?
> 
> I'd prefer to keep these settings together, but if you think it makes more
> sense to put exclusions in the patron categories, then that would be
> acceptable.  It works.  My biggest concern is trying to debug behavior. 
> It's one thing to have the items with their own exclusions in the record. 
> But trying to figure out why it excluding items in some cases and not others
> because of a patron category exclusion might be more challenging.  If there
> were some indicator when the exclusion is skipped somewhere, then I would
> have no problem with it.  If it is logged, then it would be easy to trace.
> 
> I'm just trying to make sure that our enhancement doesn't become a headache
> for those using it to try and understand system behavior.  Circulation rules
> alone are hard to follow when dealing with multiple branches.

Hi Christopher, great idea about logging when local hold is excluded.

I'll get right into it.
Comment 24 Kyle M Hall 2020-07-20 13:52:51 UTC
(In reply to Christopher Brannon from comment #22)
> I'd prefer to keep these settings together, but if you think it makes more
> sense to put exclusions in the patron categories, then that would be
> acceptable.  It works.  My biggest concern is trying to debug behavior. 
> It's one thing to have the items with their own exclusions in the record. 
> But trying to figure out why it excluding items in some cases and not others
> because of a patron category exclusion might be more challenging.  If there
> were some indicator when the exclusion is skipped somewhere, then I would
> have no problem with it.  If it is logged, then it would be easy to trace.
> 
> I'm just trying to make sure that our enhancement doesn't become a headache
> for those using it to try and understand system behavior.  Circulation rules
> alone are hard to follow when dealing with multiple branches.

I think the feedback idea is both a great one, and also out of scope for this specific bug and development. What I'm imagining is inside the hold trapping code, every time we skip over a hold of higher priority, we add it to a list of skipped holds. This list then needs to be passed all the way back to the template where we can have some kind of 'reason' icon that when clicked will show the higher priority holds that were skipped, and the reason they were skipped. This would definitely be useful even excluding this specific development, and is only tangential to this bug report. I have filed bug 26027 for this new functionality.
Comment 25 Tomás Cohen Arazi 2020-07-20 17:42:50 UTC
(In reply to Christopher Brannon from comment #22)
> (In reply to Tomás Cohen Arazi from comment #21)
>  
> > So the result is similar, but ugly-ish. And involves an error prone
> > free-text field with pipe-separated values. So I don't see any advantage on
> > it. If we were to expose this condition on the API also, we would need to
> > add methods to extract the boolean with this kind of comparison, instead of
> > just reading what's on the category table.
> 
> What if it wasn't a free-text field?  What if we did something similar to
> what is being done in
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22844?  Select the
> categories from a list rather than free typing?

I wasn't aware of that dev. Nice!
Comment 26 Tomás Cohen Arazi 2020-07-20 17:45:29 UTC
(In reply to Tomás Cohen Arazi from comment #25)
> (In reply to Christopher Brannon from comment #22)
> > (In reply to Tomás Cohen Arazi from comment #21)
> >  
> > > So the result is similar, but ugly-ish. And involves an error prone
> > > free-text field with pipe-separated values. So I don't see any advantage on
> > > it. If we were to expose this condition on the API also, we would need to
> > > add methods to extract the boolean with this kind of comparison, instead of
> > > just reading what's on the category table.
> > 
> > What if it wasn't a free-text field?  What if we did something similar to
> > what is being done in
> > https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22844?  Select the
> > categories from a list rather than free typing?
> 
> I wasn't aware of that dev. Nice!

After reviewing the implementation I can say that we would still need a separate dev so we can make it handle data from the DB (the defined categories) as it is only using a hardcoded list of attributes...
Comment 27 Christopher Brannon 2020-07-21 18:12:43 UTC
(In reply to Christopher Brannon from comment #19)
> Test passes, and everything works according to the test.  Let's discuss the
> following issues before I sign off, since we are funding this project:
> 
> * When you update priority in the item (yes/no), there is nothing that
> indicates that it is done or set.  It would be helpful if there was
> something that appeared to indicate that it took.  Maybe (SET/UNSET) next to
> the dropbox?
> 
Okay, so that just leaves this issue.  Can this be addressed?
Comment 28 Tomás Cohen Arazi 2020-07-21 18:41:51 UTC
(In reply to Christopher Brannon from comment #27)
> (In reply to Christopher Brannon from comment #19)
> > Test passes, and everything works according to the test.  Let's discuss the
> > following issues before I sign off, since we are funding this project:
> > 
> > * When you update priority in the item (yes/no), there is nothing that
> > indicates that it is done or set.  It would be helpful if there was
> > something that appeared to indicate that it took.  Maybe (SET/UNSET) next to
> > the dropbox?
> > 
> Okay, so that just leaves this issue.  Can this be addressed?

That's what Kyle mentioned in comment 24. We are discussing this tomorrow.
Comment 29 Joy Nelson 2020-07-22 14:19:30 UTC
I've linked bug 26027 to this bugzilla ticket.  That work is outside the scope of this original contracted dev.  We will followup on that work after this development is completed.

Thanks
Joy

(In reply to Tomás Cohen Arazi from comment #28)
> (In reply to Christopher Brannon from comment #27)
> > (In reply to Christopher Brannon from comment #19)
> > > Test passes, and everything works according to the test.  Let's discuss the
> > > following issues before I sign off, since we are funding this project:
> > > 
> > > * When you update priority in the item (yes/no), there is nothing that
> > > indicates that it is done or set.  It would be helpful if there was
> > > something that appeared to indicate that it took.  Maybe (SET/UNSET) next to
> > > the dropbox?
> > > 
> > Okay, so that just leaves this issue.  Can this be addressed?
> 
> That's what Kyle mentioned in comment 24. We are discussing this tomorrow.
Comment 30 Christopher Brannon 2020-07-22 14:35:41 UTC
The issue in comment 27 still has not been addressed.  I consider this incomplete without that indicator in place.
Comment 31 Kyle M Hall 2020-07-22 15:21:57 UTC
Created attachment 107180 [details] [review]
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Comment 32 Christopher Brannon 2020-07-22 15:55:06 UTC
Whatever changed since the last sign-off is not working.  This crashes the sandbox.  Tested sandbox without patch, sandbox works.  Added patch, sandbox crashes.

RUNNING HANDLER [Run updatedatabase.pl in docker container] ********************
fatal: [localhost -> koha-bug19889]: FAILED! => {"changed": true, "cmd": "koha-shell -c '/kohadevbox/koha/installer/data/mysql/updatedatabase.pl' bug19889", "delta": "0:00:02.838032", "end": "2020-07-22 15:42:59.451294", "msg": "non-zero return code", "rc": 255, "start": "2020-07-22 15:42:56.613262", "stderr": "Number found where operator expected at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 301, near \"Bug 19889\"\n\t(Do you need to predeclare Bug?)\n{UNKNOWN}: syntax error at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 294, near \"==\"\nCompilation failed in require at /usr/share/perl5/Class/C3/Componentised.pm line 150. at /usr/share/perl5/Class/C3/Componentised.pm line 155\nCompilation failed in require at /kohadevbox/koha/Koha/Database.pm line 50.", "stderr_lines": ["Number found where operator expected at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 301, near \"Bug 19889\"", "\t(Do you need to predeclare Bug?)", "{UNKNOWN}: syntax error at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 294, near \"==\"", "Compilation failed in require at /usr/share/perl5/Class/C3/Componentised.pm line 150. at /usr/share/perl5/Class/C3/Componentised.pm line 155", "Compilation failed in require at /kohadevbox/koha/Koha/Database.pm line 50."], "stdout": "", "stdout_lines": []}

NO MORE HOSTS LEFT *************************************************************

PLAY RECAP *********************************************************************
localhost                  : ok=31   changed=22   unreachable=0    failed=1    skipped=1    rescued=0    ignored=0
Comment 33 Christopher Brannon 2020-07-22 15:55:17 UTC
Whatever changed since the last sign-off is not working.  This crashes the sandbox.  Tested sandbox without patch, sandbox works.  Added patch, sandbox crashes.

RUNNING HANDLER [Run updatedatabase.pl in docker container] ********************
fatal: [localhost -> koha-bug19889]: FAILED! => {"changed": true, "cmd": "koha-shell -c '/kohadevbox/koha/installer/data/mysql/updatedatabase.pl' bug19889", "delta": "0:00:02.838032", "end": "2020-07-22 15:42:59.451294", "msg": "non-zero return code", "rc": 255, "start": "2020-07-22 15:42:56.613262", "stderr": "Number found where operator expected at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 301, near \"Bug 19889\"\n\t(Do you need to predeclare Bug?)\n{UNKNOWN}: syntax error at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 294, near \"==\"\nCompilation failed in require at /usr/share/perl5/Class/C3/Componentised.pm line 150. at /usr/share/perl5/Class/C3/Componentised.pm line 155\nCompilation failed in require at /kohadevbox/koha/Koha/Database.pm line 50.", "stderr_lines": ["Number found where operator expected at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 301, near \"Bug 19889\"", "\t(Do you need to predeclare Bug?)", "{UNKNOWN}: syntax error at /kohadevbox/koha/Koha/Schema/Result/Category.pm line 294, near \"==\"", "Compilation failed in require at /usr/share/perl5/Class/C3/Componentised.pm line 150. at /usr/share/perl5/Class/C3/Componentised.pm line 155", "Compilation failed in require at /kohadevbox/koha/Koha/Database.pm line 50."], "stdout": "", "stdout_lines": []}

NO MORE HOSTS LEFT *************************************************************

PLAY RECAP *********************************************************************
localhost                  : ok=31   changed=22   unreachable=0    failed=1    skipped=1    rescued=0    ignored=0
Comment 34 Tomás Cohen Arazi 2020-07-22 17:39:06 UTC
Created attachment 107187 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 35 Tomás Cohen Arazi 2020-07-22 17:39:12 UTC
Created attachment 107188 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 36 Tomás Cohen Arazi 2020-07-22 17:39:19 UTC
Created attachment 107189 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 37 Tomás Cohen Arazi 2020-07-22 17:39:40 UTC
Created attachment 107190 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 38 Tomás Cohen Arazi 2020-07-22 17:39:58 UTC
Created attachment 107191 [details] [review]
Bug 19889: Add feedback when exclusion is updated on moredetail.pl
Comment 39 Christopher Brannon 2020-07-22 18:17:39 UTC
You really like to trip us up on the testing.  :)  There is no info about HOW the last patch is supposed to show that a change has been made.  I was expecting to see something similar to the statuses, where it puts a message next to the header of that section.  Instead, it puts an alert at the top of the page.  Had to look through the code to figure out what I was looking for.

So, it does show staff that something happened.

I think at some point we need to open a new bug and fix the consistency of the messages on this page.  Statuses put a message next to the section header, this puts an alert at the top of the page, and the notes don't do anything.  That's kind of wonky.

I would have opted to be consistent with the way statuses show a change rather than do it a whole new way, but it works, and it doesn't do nothing like the notes.  So, I'll work with that.  :)
Comment 40 ByWater Sandboxes 2020-07-22 18:18:32 UTC
Created attachment 107192 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 41 ByWater Sandboxes 2020-07-22 18:18:35 UTC
Created attachment 107193 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 42 ByWater Sandboxes 2020-07-22 18:18:38 UTC
Created attachment 107194 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 43 ByWater Sandboxes 2020-07-22 18:18:41 UTC
Created attachment 107195 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 44 ByWater Sandboxes 2020-07-22 18:18:44 UTC
Created attachment 107196 [details] [review]
Bug 19889: Add feedback when exclusion is updated on moredetail.pl

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 45 Nick Clemens 2020-08-06 11:49:40 UTC
Everything works as expected, tests pass, qa tools green.
Last patch needs a rebase

 - When updating an item on moredetail.pl the blue box can overlap with the title details https://snipboard.io/dKGf8y.jpg
 - When batch modifying items to change exclude... it registers no changes https://snipboard.io/i6GQ8x.jpg
- I think we must display on moremember.pl and detail.pl the info on when a patron/item is excluded, otherwise these changes may not be visible to staff who cannot edit items/patrons/categories and they will not understand what is happening to the holds
Comment 46 Agustín Moyano 2020-08-06 19:04:24 UTC
Created attachment 107908 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 47 Agustín Moyano 2020-08-06 19:04:31 UTC
Created attachment 107909 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 48 Agustín Moyano 2020-08-06 19:04:37 UTC
Created attachment 107910 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 49 Agustín Moyano 2020-08-06 19:04:43 UTC
Created attachment 107911 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 50 Agustín Moyano 2020-08-06 19:04:48 UTC
Created attachment 107912 [details] [review]
Bug 19889: Add feedback when exclusion is updated on moredetail.pl

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>
Comment 51 Agustín Moyano 2020-08-06 19:04:53 UTC
Created attachment 107913 [details] [review]
Bug 19889: (follow-up) Fix overlapping blue box and message in batch item modification
Comment 52 Nick Clemens 2020-08-06 23:52:29 UTC
Created attachment 107926 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 53 Nick Clemens 2020-08-06 23:52:33 UTC
Created attachment 107927 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Nick Clemens 2020-08-06 23:52:37 UTC
Created attachment 107928 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens 2020-08-06 23:52:40 UTC
Created attachment 107929 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2020-08-06 23:52:44 UTC
Created attachment 107930 [details] [review]
Bug 19889: Add feedback when exclusion is updated on moredetail.pl

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens 2020-08-06 23:52:48 UTC
Created attachment 107931 [details] [review]
Bug 19889: (follow-up) Fix overlapping blue box and message in batch item modification

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Jonathan Druart 2020-08-07 09:08:20 UTC
Few minor things found:

* I think "Batch item modification results" should display the value in the table if it has been modified

* https://snipboard.io/NWPmMg.jpg
Maybe we need a "legend" for the first fieldset and a less specific one for the one you add? What other field/attribute could we add to "Priority"?
Double check with a English native speaker but maybe we would prefer "MARC fields" and "Other attributes"?

* "Exclude from local holds priority" does not appear in the header of the "Patron categories" table

* moredetails.pl
+    updated_exclude_from_local_holds_priority => $query->param('updated_exclude_from_local_holds_priority')
=> Force scalar context is missing for ->param

* 
-print $cgi->redirect("moredetail.pl?biblionumber=$biblionumber&itemnumber=$itemnumber#item$itemnumber");
+print $cgi->redirect("moredetail.pl?" . $alerts . "biblionumber=$biblionumber&itemnumber=$itemnumber#item$itemnumber");

Put $alerts after existing params I'd say
Also maybe "$messages" would be more appropriated

* I think the following block is not translator friendly
+        <i>Exclude from local holds priority</i> updated to
+        [% IF updated_exclude_from_local_holds_priority %]<i>Yes</i>[% ELSE %]<i>No</i>[% END %]


* Not blocker, in the tests you could have set the default value for exclude_from_local_holds_priority in t::lib::TestBuilder
Comment 59 Jonathan Druart 2020-08-07 09:18:21 UTC
Created attachment 107948 [details] [review]
Bug 19889: (follow-up) Add tests
Comment 60 Jonathan Druart 2020-08-07 09:25:35 UTC
Scratching my head, is the following condition correct?

C4::HoldsQueue::MapItemsToHoldRequests

409                 next
410                   if ( !$item->{holdallowed} )
411                   || ( $item->{holdallowed} == 1
412                     && $item->{homebranch} ne $request->{borrowerbranch}
413                   || $item->{_object}->exclude_from_local_holds_priority );

Should not we exclude it in any cases?
It's correct but read it is confusing, I would expect it to be more explicit:

409                 next
410                   if !$item->{holdallowed}
411                   || ( $item->{holdallowed} == 1
412                     && $item->{homebranch} ne $request->{borrowerbranch} )
413                   || $item->{_object}->exclude_from_local_holds_priority
Comment 61 Jonathan Druart 2020-08-07 09:31:48 UTC
And, finally, why isn't it a circ rule actually?
Comment 62 Tomás Cohen Arazi 2020-08-07 12:08:35 UTC
(In reply to Jonathan Druart from comment #61)
> And, finally, why isn't it a circ rule actually?

That's because we don't have a suitable user interface for this, until the revamp is pushed.
Comment 63 Nick Clemens 2020-08-11 11:23:11 UTC
(In reply to Jonathan Druart from comment #61)
> And, finally, why isn't it a circ rule actually?

There was a desire to be able to mark individual items as well. Marking it per item allows it to show in reports and other places as well.
Comment 64 Agustín Moyano 2020-08-13 19:41:42 UTC
Created attachment 108213 [details] [review]
Bug 19889: Add exclude_from_local_holds_priority columns to item, deleteditems and categories tables

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 65 Agustín Moyano 2020-08-13 19:41:47 UTC
Created attachment 108214 [details] [review]
Bug 19889: set exclude_from_local_holds_priority columns as boolean in schema

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 66 Agustín Moyano 2020-08-13 19:41:51 UTC
Created attachment 108215 [details] [review]
Bug 19889: Add tests

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 67 Agustín Moyano 2020-08-13 19:41:57 UTC
Created attachment 108216 [details] [review]
Bug 19889: Make it possible to exclude items and categories from local holds priority

This patch adds the ability to exclude patrons (by category) from local
holds, and items, by editing the item itself or by batch item
modification tool.

To test:
1. apply patches
2. updatedatabase
3. Enable LocalHoldsPriority preference, and leave
   LocalHoldsPriorityPatronControl in pickup library, and
LocalHoldsPriorityItemControl in holding library.
4. Search for a biblio with one item.
5. Place a hold with a patron (patron1) and set pickup location to a different
   library of the item's home library
6. Place another hold with another patron (patron2) and set pickup location to be
   the same as the item's home library
7. ./misc/cronjobs/holds/build_holds_queue.pl
8. Go to circulation -> holds queue
9. Search by the item's home library
CHECK => only the hold for patron2 (with the pickup location the same as the
item's home library) appears in the table
10. Go back to the biblio details page and click on "Items" tab
CHECK => There is a new section in the item's details between "Statuses"
and "History" called "Priority"
11. Set exclude to "Yes" and update
12. repeat steps 7 to 9
SUCCESS => only the hold for patron1 now appears, even the other hold had local
hold priority
13. Repeat step 10 and 11 but this time set exclude to "No"
14. repeat steps 7 to 9
CHECK => the hold for patron2 is back
15. Edit patron2's category and set exclude from local holds priority to
    "Yes"
16. Repeat steps 7 to 9
SUCCESS => the hold for patron1 is back
17. Go to tools -> Batch item modification and in barcode list place
    several (existing) barcodes and press continue
CHECK => There is a new section in the bottom called "Priority"
18. Set exclude to "Yes" and save
SUCCESS => all items in the list now have exclude setted to "Yes"
19. Try to checkout the first item to a patron3
SUCCESS => Alert message appears saying that patron1 has a hold on that
item
20. Click on Yes and then checkin that item
SUCCESS => There is a modal window saying that a hold was found for
patron1
21. prove t/db_dependent/HoldsQueue.t t/db_dependent/Holds/LocalHoldsPriority.t
22. Sign off

Sponsored-by: Cooperative Information Network (CIN)

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 68 Agustín Moyano 2020-08-13 19:42:01 UTC
Created attachment 108217 [details] [review]
Bug 19889: Add feedback when exclusion is updated on moredetail.pl

Signed-off-by: Christopher Brannon <cbrannon@cdalibrary.org>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 69 Agustín Moyano 2020-08-13 19:42:06 UTC
Created attachment 108218 [details] [review]
Bug 19889: (follow-up) Fix overlapping blue box and message in batch item modification

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 70 Agustín Moyano 2020-08-13 19:42:11 UTC
Created attachment 108219 [details] [review]
Bug 19889: (follow-up) Add tests
Comment 71 Agustín Moyano 2020-08-13 19:42:15 UTC
Created attachment 108220 [details] [review]
Bug 19889: (follow-up) Fix few minor things
Comment 72 Agustín Moyano 2020-08-13 19:48:45 UTC
Hi Jonathan, fixed most things but.. 

> * 
> -print
> $cgi->redirect("moredetail.
> pl?biblionumber=$biblionumber&itemnumber=$itemnumber#item$itemnumber");
> +print $cgi->redirect("moredetail.pl?" . $alerts .
> "biblionumber=$biblionumber&itemnumber=$itemnumber#item$itemnumber");
> 
> Put $alerts after existing params I'd say

changed name to messages, but this cannot be placed at the end of the string. It needs to be before #item$itemnumber
Comment 73 Nick Clemens 2020-08-19 12:24:07 UTC
Created attachment 108593 [details] [review]
Bug 19889: (follow-up) Add tests

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 74 Nick Clemens 2020-08-19 12:24:11 UTC
Created attachment 108594 [details] [review]
Bug 19889: (follow-up) Fix few minor things

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 75 Nick Clemens 2020-08-19 12:24:14 UTC
Created attachment 108595 [details] [review]
Bug 19889: (QA follow-up) use em over i and update field if unset

em is better for accessibility

We should set the field to 0 when it was previously unset

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 76 Jonathan Druart 2020-08-20 10:07:10 UTC
(In reply to Nick Clemens from comment #74)
> Created attachment 108594 [details] [review] [review]
> Bug 19889: (follow-up) Fix few minor things
> 
> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Agustin, please do not introduce unnecessary indentation changes.
Comment 77 Jonathan Druart 2020-08-20 10:07:38 UTC
Created attachment 108725 [details] [review]
Bug 19889: (follow-up) Fix few minor things

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

JD amended patch: remove unecessary indentation changes
Comment 78 Jonathan Druart 2020-08-20 10:07:59 UTC
Created attachment 108726 [details] [review]
Bug 19889: (QA follow-up) use em over i and update field if unset

em is better for accessibility

We should set the field to 0 when it was previously unset

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 79 Jonathan Druart 2020-08-20 10:16:34 UTC
I think there is something wrong in the batch mod tool, the value is set to 0 even if we don't select "Yes" or "No"

To recreate
have item bc_1 with value set to Yes
Edit in batch, don't modify anything, submit
=> bc_1 has now "No" (0)
Comment 80 Jonathan Druart 2020-08-20 10:21:25 UTC
(In reply to Jonathan Druart from comment #58)
> * I think the following block is not translator friendly
> +        <i>Exclude from local holds priority</i> updated to
> +        [% IF updated_exclude_from_local_holds_priority %]<i>Yes</i>[% ELSE
> %]<i>No</i>[% END %]

You replaced it with:

-        [% IF updated_exclude_from_local_holds_priority %]<i>Yes</i>[% ELSE %]<i>No</i>[% END %]
+        [% IF updated_exclude_from_local_holds_priority %]
+            <i>Yes</i>
+        [% ELSE %]
+            <i>No</i>
+        [% END %]

What I meant is that it will be displayed with the TT block in the po files.
I think it would be better:
        [% IF updated_exclude_from_local_holds_priority %]
            <i>Exclude from local holds priority</i> updated to <i>Yes</i>
        [% ELSE %]
            <i>Exclude from local holds priority</i> updated to <i>No</i>
        [% END %]

Asking confirmation to Katrin.
Comment 81 Nick Clemens 2020-08-20 10:36:52 UTC
Created attachment 108728 [details] [review]
Bug 19889: (QA follow-up) Check that exclude from local holds priority is set and not a blank string
Comment 82 Nick Clemens 2020-08-20 10:38:52 UTC
(In reply to Nick Clemens from comment #81)
> Created attachment 108728 [details] [review] [review]
> Bug 19889: (QA follow-up) Check that exclude from local holds priority is
> set and not a blank string

I thought it was my follow-up that messed things up so followed up here. Seems we need to test if defined and set to something.  Agustin feel free to comment or update my follow-ups if needed
Comment 83 Agustín Moyano 2020-08-20 14:19:47 UTC
(In reply to Nick Clemens from comment #82)
> (In reply to Nick Clemens from comment #81)
> > Created attachment 108728 [details] [review] [review] [review]
> > Bug 19889: (QA follow-up) Check that exclude from local holds priority is
> > set and not a blank string
> 
> I thought it was my follow-up that messed things up so followed up here.
> Seems we need to test if defined and set to something.  Agustin feel free to
> comment or update my follow-ups if needed

Thanks Nick for catching this up.. I'll check it out, but I don't think I will need to update your follow-up. It seems ok.
Comment 84 Katrin Fischer 2020-08-20 19:40:39 UTC
(In reply to Jonathan Druart from comment #80)
> (In reply to Jonathan Druart from comment #58)
> > * I think the following block is not translator friendly
> > +        <i>Exclude from local holds priority</i> updated to
> > +        [% IF updated_exclude_from_local_holds_priority %]<i>Yes</i>[% ELSE
> > %]<i>No</i>[% END %]
> 
> You replaced it with:
> 
> -        [% IF updated_exclude_from_local_holds_priority %]<i>Yes</i>[% ELSE
> %]<i>No</i>[% END %]
> +        [% IF updated_exclude_from_local_holds_priority %]
> +            <i>Yes</i>
> +        [% ELSE %]
> +            <i>No</i>
> +        [% END %]
> 
> What I meant is that it will be displayed with the TT block in the po files.
> I think it would be better:
>         [% IF updated_exclude_from_local_holds_priority %]
>             <i>Exclude from local holds priority</i> updated to <i>Yes</i>
>         [% ELSE %]
>             <i>Exclude from local holds priority</i> updated to <i>No</i>
>         [% END %]
> 
> Asking confirmation to Katrin.

I think I'd avoid the <i> - as HTML tags they break up the strings. I haven't tested this, but with your suggestion I think we could end up with:

Exclude from local holds priority
updated to
Yes

(Best is always to confirm by updating the po files if unsure)

Better, I think, would be something like:

Exclude from local holds priority updated to ... [IF ELSE for yes and no, maybe with apostropes instad of italic? 'Yes']
Comment 85 Agustín Moyano 2020-08-25 19:04:39 UTC
(In reply to Jonathan Druart from comment #76)
> (In reply to Nick Clemens from comment #74)
> > Created attachment 108594 [details] [review] [review] [review]
> > Bug 19889: (follow-up) Fix few minor things
> > 
> > Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
> 
> Agustin, please do not introduce unnecessary indentation changes.

Ups.. Sorry Jonathan.. seems my perltidy is too eager. I'll check him out next time :P
Comment 86 Agustín Moyano 2020-08-25 20:15:49 UTC
Created attachment 109131 [details] [review]
Bug 19889: (QA follow-up) Check that exclude from local holds priority is set and not a blank string

Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 87 Agustín Moyano 2020-08-25 20:15:55 UTC
Created attachment 109132 [details] [review]
Bug 19889: (follow-up) Fix text in moredetail.tt
Comment 88 Nick Clemens 2020-08-28 14:14:39 UTC
Created attachment 109285 [details] [review]
Bug 19889: (follow-up) Fix text in moredetail.tt

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 89 Nick Clemens 2020-08-28 14:15:11 UTC
Katrin said the last patch should be okay for translations, moving to PQA
Comment 90 Jonathan Druart 2020-08-31 12:45:13 UTC
Created attachment 109364 [details] [review]
Bug 19889: Add select2 class to the new dropdown list

This is needed since bug 25727.
Comment 91 Jonathan Druart 2020-08-31 14:20:27 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 92 Jonathan Druart 2020-09-01 10:23:01 UTC
Created attachment 109419 [details] [review]
Bug 19889: Add new attribute to the REST API definition

It fixes t/db_dependent/api/v1/items.t
Comment 93 Jonathan Druart 2020-09-01 10:25:34 UTC
(In reply to Jonathan Druart from comment #92)
> Created attachment 109419 [details] [review] [review]
> Bug 19889: Add new attribute to the REST API definition
> 
> It fixes t/db_dependent/api/v1/items.t

Pushed to master for 20.11.
Comment 94 Lucas Gass 2020-09-04 21:11:06 UTC
enhancement will not be backported to 20.05.x
Comment 95 Nick Clemens 2020-09-21 13:20:04 UTC
Cannot upgrade to master from earlier versions

DBRev 20.06.00.032:
ALTER TABLE `categories` ADD COLUMN `exclude_from_local_holds_priority` tinyint(1) default NULL AFTER `require_strong_password`

DBRev 20.06.00.037:
ALTER TABLE categories ADD COLUMN `require_strong_password` TINYINT(1) NULL DEFAULT NULL AFTER `min_password_length` -- set required password strength for patrons in this category

We cannot add a column after another before we add that column, just need to swap these revisions
Comment 96 Tomás Cohen Arazi 2020-09-23 14:11:24 UTC
(In reply to Nick Clemens from comment #95)
> Cannot upgrade to master from earlier versions
> 
> DBRev 20.06.00.032:
> ALTER TABLE `categories` ADD COLUMN `exclude_from_local_holds_priority`
> tinyint(1) default NULL AFTER `require_strong_password`
> 
> DBRev 20.06.00.037:
> ALTER TABLE categories ADD COLUMN `require_strong_password` TINYINT(1) NULL
> DEFAULT NULL AFTER `min_password_length` -- set required password strength
> for patrons in this category
> 
> We cannot add a column after another before we add that column, just need to
> swap these revisions

Bug 26513
Comment 97 Jonathan Druart 2020-09-23 20:17:20 UTC
Should be fixed now

  commit d45a699975b9857990e19bb9ddc12267ebd05334
  Bug 19889: (follow-up 2) update DB adjustments
Comment 98 Lucas Gass 2020-10-16 15:56:47 UTC
enhancement will not be backported to 20.05.x