Bug 31692 - Let librarians change item level holds to record level holds when possible
Summary: Let librarians change item level holds to record level holds when possible
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Lari Taskula
QA Contact: Testopia
URL:
Keywords: release-notes-needed, rel_23_11_candidate
Depends on:
Blocks: 35483
  Show dependency treegraph
 
Reported: 2022-10-05 16:17 UTC by Lari Taskula
Modified: 2024-02-28 14:36 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact: Philip Orr
Documentation submission: https://gitlab.com/koha-community/koha-manual/-/merge_requests/832
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 31692: Add Koha::Hold::change_type and unit tests (4.23 KB, patch)
2022-10-05 16:22 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (6.07 KB, patch)
2022-10-05 16:23 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.19 KB, patch)
2022-10-18 21:35 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (6.06 KB, patch)
2022-10-18 21:35 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Add tooltip for changing hold type (1.40 KB, patch)
2022-10-18 21:35 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (6.33 KB, patch)
2022-10-18 22:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Add tooltip for changing hold type (1.35 KB, patch)
2022-10-18 22:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Use fa-chain and fa-chain-broken icons (2.19 KB, patch)
2022-10-18 22:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Rewording tooltip (1.56 KB, patch)
2022-10-18 22:16 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (6.14 KB, patch)
2022-10-18 22:57 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Add tooltip for changing hold type (1.50 KB, patch)
2022-10-18 22:58 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Use fa-chain and fa-chain-broken icons (2.35 KB, patch)
2022-10-18 22:58 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Rewording tooltip (1.71 KB, patch)
2022-10-18 22:58 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (alternative look) Use select dropdown (4.59 KB, patch)
2022-10-18 22:58 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.19 KB, patch)
2022-10-24 23:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (5.79 KB, patch)
2022-10-24 23:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.24 KB, patch)
2022-10-25 01:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (5.83 KB, patch)
2022-10-25 01:57 UTC, David Nind
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.30 KB, patch)
2022-10-30 12:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (5.89 KB, patch)
2022-10-30 12:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31692: (follow-up) Adjust reserves.item_level_hold (2.49 KB, patch)
2022-11-03 20:52 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.34 KB, patch)
2022-11-03 23:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (5.94 KB, patch)
2022-11-03 23:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 31692: (follow-up) Adjust reserves.item_level_hold (2.54 KB, patch)
2022-11-03 23:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.22 KB, patch)
2022-11-10 02:17 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (5.79 KB, patch)
2022-11-10 02:17 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Adjust reserves.item_level_hold (2.48 KB, patch)
2022-11-10 02:17 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.29 KB, patch)
2022-11-10 02:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (5.93 KB, patch)
2022-11-10 02:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: (follow-up) Adjust reserves.item_level_hold (2.53 KB, patch)
2022-11-10 02:29 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 31692: Add Koha::Hold::change_type and unit tests (4.34 KB, patch)
2022-11-10 09:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31692: Allow librarians to change hold type in staff client (5.98 KB, patch)
2022-11-10 09:36 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 31692: (follow-up) Adjust reserves.item_level_hold (2.58 KB, patch)
2022-11-10 09:36 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lari Taskula 2022-10-05 16:17:11 UTC
Koha has item level holds (only a specific item was requested) and record level holds (any next available item).

There can be a case when a librarian wishes to change an item level hold to a record level hold. This is currently not (easily) possible, as they would have to cancel the hold and then set another type of hold and place it to the previous point in hold queue.

This Bug aims to add an easy way to change hold types.

Only one type of hold is allowed simultaneously for a record and a patron. The two types cannot be mixed. In other words a patron can't have both item level and record level holds on a specific record at the same time. This must be kept in mind when designing such functionality.
Comment 1 Lari Taskula 2022-10-05 16:22:56 UTC Comment hidden (obsolete)
Comment 2 Lari Taskula 2022-10-05 16:23:01 UTC Comment hidden (obsolete)
Comment 3 Lucas Gass 2022-10-11 22:47:40 UTC
I think the use of another red X on the page to change the item level hold into a bib level one is confusing. There are already a few red X's on the page and that particular one offers no explanation to the user as to what it will do. 

Maybe we need a dropdown? I'm not sure if that is the proper solution either.
Comment 4 Katrin Fischer 2022-10-14 19:52:57 UTC
(In reply to Lucas Gass from comment #3)
> I think the use of another red X on the page to change the item level hold
> into a bib level one is confusing. There are already a few red X's on the
> page and that particular one offers no explanation to the user as to what it
> will do. 
> 
> Maybe we need a dropdown? I'm not sure if that is the proper solution either.

Lucas is making a good point, could you please have a look Lari? Maybe some other icon could be used? I have to admit my first idea was some kind of upwards arrow for (level upwards) but... there are also a lot of arrows already. A tooltip for sure makes sense.
Comment 5 Lari Taskula 2022-10-18 21:35:49 UTC Comment hidden (obsolete)
Comment 6 Lari Taskula 2022-10-18 21:35:53 UTC Comment hidden (obsolete)
Comment 7 Lari Taskula 2022-10-18 21:35:57 UTC Comment hidden (obsolete)
Comment 8 Lari Taskula 2022-10-18 21:45:23 UTC
(In reply to Lucas Gass from comment #3)
> I think the use of another red X on the page to change the item level hold
> into a bib level one is confusing. There are already a few red X's on the
> page and that particular one offers no explanation to the user as to what it
> will do. 
> 
> Maybe we need a dropdown? I'm not sure if that is the proper solution either.

(In reply to Katrin Fischer from comment #4)
> Lucas is making a good point, could you please have a look Lari? Maybe some
> other icon could be used? I have to admit my first idea was some kind of
> upwards arrow for (level upwards) but... there are also a lot of arrows
> already. A tooltip for sure makes sense.

Thanks for the feedback, Lucas and Katrin.

I've added a tooltip in the form of "title" attribute - if Koha has another way of showing tooltips then please correct me. Great suggestion!

About the X, (by the way, it just became green with the new theme) my reasoning for choosing it was to represent something to "remove" the item from the hold. Perhaps an "unlinking" icon could work too. https://fontawesome.com/v4/icon/chain-broken

Dropdown is also a nice idea in the sense that it could be extended to show other items as well and let librarians change the item if necessary. But that is unfortunately out of scope for what I am trying to achieve here.

I have also rebased to resolve some git conflicts and patches can now be tested again.
Comment 9 Lari Taskula 2022-10-18 22:16:45 UTC Comment hidden (obsolete)
Comment 10 Lari Taskula 2022-10-18 22:16:50 UTC Comment hidden (obsolete)
Comment 11 Lari Taskula 2022-10-18 22:16:54 UTC Comment hidden (obsolete)
Comment 12 Lari Taskula 2022-10-18 22:16:58 UTC Comment hidden (obsolete)
Comment 13 Lari Taskula 2022-10-18 22:57:59 UTC Comment hidden (obsolete)
Comment 14 Lari Taskula 2022-10-18 22:58:04 UTC Comment hidden (obsolete)
Comment 15 Lari Taskula 2022-10-18 22:58:08 UTC Comment hidden (obsolete)
Comment 16 Lari Taskula 2022-10-18 22:58:13 UTC Comment hidden (obsolete)
Comment 17 Lari Taskula 2022-10-18 22:58:22 UTC Comment hidden (obsolete)
Comment 18 Lari Taskula 2022-10-18 23:03:04 UTC
In reply to Lucas Gass from comment #3)
> Maybe we need a dropdown? I'm not sure if that is the proper solution either.

I actually wanted to see what it looks like so I implemented patch:

Bug 31692: (alternative look) Use select dropdown

I think it looks quite okay. It also allows me to get rid of all extra JavaScript and tooltip issues. Great idea Lucas. Thoughts?
Comment 19 Lucas Gass 2022-10-20 15:30:49 UTC
(In reply to Lari Taskula from comment #18)
> In reply to Lucas Gass from comment #3)
> > Maybe we need a dropdown? I'm not sure if that is the proper solution either.
> 
> I actually wanted to see what it looks like so I implemented patch:
> 
> Bug 31692: (alternative look) Use select dropdown
> 
> I think it looks quite okay. It also allows me to get rid of all extra
> JavaScript and tooltip issues. Great idea Lucas. Thoughts?

Lari,

I do like that much better! Everything works great.

I do wonder if there is still going to be some confusion on what the dropdown does. Do we need a hint or a tip to help describe what the dropdown is going to do?
Comment 20 Lari Taskula 2022-10-24 23:39:33 UTC
Created attachment 142586 [details] [review]
Bug 31692: Add Koha::Hold::change_type and unit tests

To test:
1. prove t/db_dependent/Koha/Hold.t
Comment 21 Lari Taskula 2022-10-24 23:39:38 UTC
Created attachment 142587 [details] [review]
Bug 31692: Allow librarians to change hold type in staff client

To test:
1. Apply patch
2. Add item level hold to a record/item, make sure patron has no other
   holds on that record
3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is
   the record you placed the hold for
4. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>" dropdown
5. Select "Next available" from the dropdown
6. Click Update hold(s)
7. Observe dropdown value has changed from "Only item <barcode>" to
   "Next available"
8. Cancel the hold and add two item level holds
9. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>", but no select dropdown
Comment 22 Lari Taskula 2022-10-24 23:49:35 UTC
(In reply to Lucas Gass from comment #19)
> I do wonder if there is still going to be some confusion on what the
> dropdown does. Do we need a hint or a tip to help describe what the dropdown
> is going to do?

I think the dropdown values themselves explain what it does. If you think more detailed description is needed, feel free to make a suggestion.

On another note I've squashed the follow-ups into the main patch and updated test plan.
Comment 23 David Nind 2022-10-25 01:56:55 UTC
Created attachment 142595 [details] [review]
Bug 31692: Add Koha::Hold::change_type and unit tests

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2022-10-25 01:57:01 UTC
Created attachment 142596 [details] [review]
Bug 31692: Allow librarians to change hold type in staff client

To test:
1. Apply patch
2. Add item level hold to a record/item, make sure patron has no other
   holds on that record
3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is
   the record you placed the hold for
4. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>" dropdown
5. Select "Next available" from the dropdown
6. Click Update hold(s)
7. Observe dropdown value has changed from "Only item <barcode>" to
   "Next available"
8. Cancel the hold and add two item level holds
9. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>", but no select dropdown

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2022-10-25 02:02:19 UTC
I've signed off, as everything works as per the test plan.

For step 8, I added two item level holds for the same patron - I hope that is what was required.

My only suggestion would be to change the "Details" column label to "Hold type".
Comment 26 Lari Taskula 2022-10-26 11:40:38 UTC
(In reply to David Nind from comment #25)
> For step 8, I added two item level holds for the same patron - I hope that
> is what was required.

Yes that's correct.

> My only suggestion would be to change the "Details" column label to "Hold
> type".

It looks like the details column can also contain a "Non priority hold" text. If we were to rename the column, can we consider "non priority hold" as a "hold type", or does hold type just mean an item/biblio hold? https://github.com/Koha-Community/Koha/blob/48bf9b1d91dbbc336f859ac1a83d7d25b0e6a3c1/koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc#L189-L191
Comment 27 David Nind 2022-10-26 20:17:00 UTC
(In reply to Lari Taskula from comment #26)
> > My only suggestion would be to change the "Details" column label to "Hold
> > type".
> 
> It looks like the details column can also contain a "Non priority hold"
> text. If we were to rename the column, can we consider "non priority hold"
> as a "hold type", or does hold type just mean an item/biblio hold?
> https://github.com/Koha-Community/Koha/blob/
> 48bf9b1d91dbbc336f859ac1a83d7d25b0e6a3c1/koha-tmpl/intranet-tmpl/prog/en/
> includes/holds_table.inc#L189-L191

I wasn't aware of that :D 

Also noticed that the details column already exists, so happy to leave the same without any changes.
Comment 28 Katrin Fischer 2022-10-30 12:43:52 UTC
Created attachment 142818 [details] [review]
Bug 31692: Add Koha::Hold::change_type and unit tests

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 29 Katrin Fischer 2022-10-30 12:43:57 UTC
Created attachment 142819 [details] [review]
Bug 31692: Allow librarians to change hold type in staff client

To test:
1. Apply patch
2. Add item level hold to a record/item, make sure patron has no other
   holds on that record
3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is
   the record you placed the hold for
4. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>" dropdown
5. Select "Next available" from the dropdown
6. Click Update hold(s)
7. Observe dropdown value has changed from "Only item <barcode>" to
   "Next available"
8. Cancel the hold and add two item level holds
9. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>", but no select dropdown

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 30 Katrin Fischer 2022-10-30 12:45:14 UTC
I was starting to doubt myself, but this patch broke my evil "FQA" streak.
Thanks Lari, I think the discussion resulted in a nice clean GUI here!
Comment 31 Katrin Fischer 2022-10-30 12:49:19 UTC
Oh no... I found an issue :(

You remove the itemnumber, which would have been enough on old versions, but now we also need to change reserves.item_level_hold

Should be an easy fix - please follow-up.
Comment 32 Lari Taskula 2022-11-03 20:12:46 UTC
(In reply to Katrin Fischer from comment #31)
> Oh no... I found an issue :(
> 
> You remove the itemnumber, which would have been enough on old versions, but
> now we also need to change reserves.item_level_hold
> 
> Should be an easy fix - please follow-up.

Good catch. Although it has been there for 3 years I didn't know of this "new" column :) Fix will follow.
Comment 33 Lari Taskula 2022-11-03 20:52:04 UTC
Created attachment 143151 [details] [review]
Bug 31692: (follow-up) Adjust reserves.item_level_hold

Original patch missed handling reserves.item_level_hold column.

To test:
1. prove t/db_dependent/Koha/Hold.t
Comment 34 David Nind 2022-11-03 22:57:19 UTC
The patches still apply, but something seems changed:

1. Step 7 - There is now no dropdown list (before there was a drop down list with Only item <barcode> and Next available); selecting next available and updating removes the drop down list).

2. Step 9 - There is still a dropdown list with the two options (Only item <barcode> and Next available).

Happy to sign off again if that is what is expected.

Tests pass both before and after patches are applied.
Comment 35 Lari Taskula 2022-11-03 23:20:25 UTC
(In reply to David Nind from comment #34)
> The patches still apply, but something seems changed:
> 
> 1. Step 7 - There is now no dropdown list (before there was a drop down list
> with Only item <barcode> and Next available); selecting next available and
> updating removes the drop down list).

The wording in this step is a bit silly, it appears I didn't properly rewrite it when I squashed the new dropdown menu with the original patch.

What you describe seems expected behavior to me. After changing from item level hold to record level hold there should no longer be a dropdown. So at step 7 if you see just the text "Next available" then that is to be expected.

This is because the patches here do not add GUI support for going from record level to item level hold, but only the other way around.

> 2. Step 9 - There is still a dropdown list with the two options (Only item
> <barcode> and Next available).

This is not expected, did you add the two item level holds for the same patron? I noticed the test plan does not clarify that this is required.
Comment 36 David Nind 2022-11-03 23:45:26 UTC
(In reply to Lari Taskula from comment #35)
> (In reply to David Nind from comment #34)
> > The patches still apply, but something seems changed:
> > 
> > 1. Step 7 - There is now no dropdown list (before there was a drop down list
> > with Only item <barcode> and Next available); selecting next available and
> > updating removes the drop down list).
> 
> The wording in this step is a bit silly, it appears I didn't properly
> rewrite it when I squashed the new dropdown menu with the original patch.
> 
> What you describe seems expected behavior to me. After changing from item
> level hold to record level hold there should no longer be a dropdown. So at
> step 7 if you see just the text "Next available" then that is to be expected.
> 
> This is because the patches here do not add GUI support for going from
> record level to item level hold, but only the other way around.

Thanks for clarifying!

> > 2. Step 9 - There is still a dropdown list with the two options (Only item
> > <barcode> and Next available).
> 
> This is not expected, did you add the two item level holds for the same
> patron? I noticed the test plan does not clarify that this is required.

I added for different patrons, so will use the same patron.

Just re-testing, so should be signed off shortly!
Comment 37 David Nind 2022-11-03 23:51:20 UTC
Created attachment 143160 [details] [review]
Bug 31692: Add Koha::Hold::change_type and unit tests

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: David Nind <david@davidnind.com>
Comment 38 David Nind 2022-11-03 23:51:25 UTC
Created attachment 143161 [details] [review]
Bug 31692: Allow librarians to change hold type in staff client

To test:
1. Apply patch
2. Add item level hold to a record/item, make sure patron has no other
   holds on that record
3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is
   the record you placed the hold for
4. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>" dropdown
5. Select "Next available" from the dropdown
6. Click Update hold(s)
7. Observe dropdown value has changed from "Only item <barcode>" to
   "Next available"
8. Cancel the hold and add two item level holds
9. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>", but no select dropdown

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: David Nind <david@davidnind.com>
Comment 39 David Nind 2022-11-03 23:51:30 UTC
Created attachment 143162 [details] [review]
Bug 31692: (follow-up) Adjust reserves.item_level_hold

Original patch missed handling reserves.item_level_hold column.

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 David Nind 2022-11-04 06:43:15 UTC
I've ended up adding sign off lines for the first two patches - not sure how to remove those!
Comment 41 Katrin Fischer 2022-11-10 00:39:15 UTC
There is a conflict in the .t, can you please fix?
Comment 42 Lari Taskula 2022-11-10 02:17:23 UTC Comment hidden (obsolete)
Comment 43 Lari Taskula 2022-11-10 02:17:28 UTC Comment hidden (obsolete)
Comment 44 Lari Taskula 2022-11-10 02:17:33 UTC Comment hidden (obsolete)
Comment 45 Lari Taskula 2022-11-10 02:24:13 UTC Comment hidden (obsolete)
Comment 46 Lari Taskula 2022-11-10 02:29:11 UTC
Created attachment 143617 [details] [review]
Bug 31692: Add Koha::Hold::change_type and unit tests

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 47 Lari Taskula 2022-11-10 02:29:18 UTC
Created attachment 143618 [details] [review]
Bug 31692: Allow librarians to change hold type in staff client

To test:
1. Apply patch
2. Add item level hold to a record/item, make sure patron has no other
   holds on that record
3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is
   the record you placed the hold for
4. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>" dropdown
5. Select "Next available" from the dropdown
6. Click Update hold(s)
7. Observe dropdown is gone and cell value has changed from
   "Only item <barcode>" to "Next available"
8. Cancel the hold and add two item level holds for the same patron
9. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>", but no select dropdown

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 48 Lari Taskula 2022-11-10 02:29:24 UTC
Created attachment 143619 [details] [review]
Bug 31692: (follow-up) Adjust reserves.item_level_hold

Original patch missed handling reserves.item_level_hold column.

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 49 Lari Taskula 2022-11-10 02:30:12 UTC
(In reply to David Nind from comment #40)
> I've ended up adding sign off lines for the first two patches - not sure how
> to remove those!

I've removed those for you.

(In reply to Katrin Fischer from comment #41)
> There is a conflict in the .t, can you please fix?

Fixed!
Comment 50 Katrin Fischer 2022-11-10 09:36:03 UTC
Created attachment 143627 [details] [review]
Bug 31692: Add Koha::Hold::change_type and unit tests

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 51 Katrin Fischer 2022-11-10 09:36:23 UTC
Created attachment 143628 [details] [review]
Bug 31692: Allow librarians to change hold type in staff client

To test:
1. Apply patch
2. Add item level hold to a record/item, make sure patron has no other
   holds on that record
3. Go to /cgi-bin/koha/reserve/request.pl?biblionumber=xxx where xxx is
   the record you placed the hold for
4. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>" dropdown
5. Select "Next available" from the dropdown
6. Click Update hold(s)
7. Observe dropdown is gone and cell value has changed from
   "Only item <barcode>" to "Next available"
8. Cancel the hold and add two item level holds for the same patron
9. Under "Existing holds" table, in "Details" column you should see
   "Only item <barcode>", but no select dropdown

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 52 Katrin Fischer 2022-11-10 09:36:32 UTC
Created attachment 143629 [details] [review]
Bug 31692: (follow-up) Adjust reserves.item_level_hold

Original patch missed handling reserves.item_level_hold column.

To test:
1. prove t/db_dependent/Koha/Hold.t

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 53 Katrin Fischer 2022-11-10 09:38:58 UTC
Thx for the quick fix, Lari. This has some string changes, so not sure if we can still make it.
Comment 54 Tomás Cohen Arazi 2023-03-14 13:45:41 UTC
Can we please change the method signature? I'd expect

$hold->change_type(
    {
        type => [biblio|item],
      [ item_id => $item->id, ]
    }
);

Or even split it in two methods:

$hold->make_biblio_level
$hold->make_item_level( $item->id )

each of them checking the right things and throwing the required exceptions.
Comment 55 Katrin Fischer 2023-03-14 14:01:36 UTC
(In reply to Tomás Cohen Arazi from comment #54)
> Can we please change the method signature? I'd expect
> 
> $hold->change_type(
>     {
>         type => [biblio|item],
>       [ item_id => $item->id, ]
>     }
> );
> 
> Or even split it in two methods:
> 
> $hold->make_biblio_level
> $hold->make_item_level( $item->id )
> 
> each of them checking the right things and throwing the required exceptions.

Maybe use switch? switch_to_record_level (we want to avoid biblio) and switch_to_item_level
Comment 56 Tomás Cohen Arazi 2023-04-06 15:01:30 UTC
Lari, are you still willing to work on this? Thanks
Comment 57 Tomás Cohen Arazi 2023-10-10 13:02:20 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 58 Fridolin Somers 2023-10-12 20:52:38 UTC
Enhancement not pushed to 23.05.x
Comment 59 Katrin Fischer 2023-12-04 12:38:37 UTC
Testing with a superlibrarian, but I don't get a pull down in the details column on my item level holds. Any idea what could go wrong there?
Comment 60 David Nind 2023-12-04 18:58:08 UTC
(In reply to Katrin Fischer from comment #59)
> Testing with a superlibrarian, but I don't get a pull down in the details
> column on my item level holds. Any idea what could go wrong there?

I'm not seeing it either...
Comment 61 Lari Taskula 2023-12-04 21:18:38 UTC
(In reply to Katrin Fischer from comment #59)
> Testing with a superlibrarian, but I don't get a pull down in the details
> column on my item level holds. Any idea what could go wrong there?

Looks like the change introduced to koha-tmpl/intranet-tmpl/prog/en/includes/holds_table.inc in "Allow librarians to change hold type in staff client" patch was reverted by Bug 33575.

So this feature is no longer working as of now.
Comment 62 Lari Taskula 2023-12-04 23:13:58 UTC
See Bug 35483 for restoring the missing feature.

(In reply to Tomás Cohen Arazi from comment #56)
> Lari, are you still willing to work on this? Thanks

Sorry I had either missed or forgot to answer your comment!

(In reply to Tomás Cohen Arazi from comment #54)
> Can we please change the method signature? I'd expect
> 
> $hold->change_type(
>     {
>         type => [biblio|item],
>       [ item_id => $item->id, ]
>     }
> );
> 
> Or even split it in two methods:
> 
> $hold->make_biblio_level
> $hold->make_item_level( $item->id )

Well, why not, if you feel it's still important even after pushing the Bug (sorry again for the late answer). I'm not extremely attached or strongly opinionated about the choice I made - I suppose I was thinking of it more as a toggle hence not introducing two methods. The itemnumber param in my sense specifies the type already, if given => item, if not given => biblio.