Bug 20469

Summary: Add item status to staff article requests form
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: CirculationAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: gmcharlt, jonathan.druart, kyle.m.hall, lucas, martin.renvoize
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17530
Change 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: 20468    
Bug Blocks: 20472    
Attachments: Bug 20469: Add item status to staff article request form
Bug 20469: Add item status to staff article request form
Bug 20469: (Follow-up) Make translation of status possible
Bug 20469: Add item status to staff article request form
Bug 20469: (Follow-up) Make translation of status possible
Bug 20469: Add item status to staff article request form
Bug 20469: (Follow-up) Make translation of status possible
Bug 20469: Add item status to staff article request form
Bug 20469: (Follow-up) Make translation of status possible
Bug 20469: Use DBIC rs and return ->count
Bug 20469: (follow-up) Add Koha::Holds->waiting_or_transit
Bug 20469: Add item status to staff article request form
Bug 20469: (follow-up) Make translation of status possible
Bug 20469: Add item status to staff article request form
Bug 20469: Add test for ItemTypes plugin
Bug 20469: Add Koha::Item->itemtype
Bug 20469: Add item status to staff article request form
Bug 20469: Add Koha::Item->itemtype
Bug 20469: Remove effective parameter
Bug 20469: Add item status to staff article request form
Bug 20469: Add Koha::Item->itemtype
Bug 20469: Remove effective parameter
Bug 20469: Add item status to staff article request form

Description Marcel de Rooy 2018-03-26 11:20:41 UTC
The Article Requests feature does not look at item status. When you want to process a request, it is useful to show a few fundamental item statuses like Checked out, On hold or Available.
Comment 1 Marcel de Rooy 2018-03-26 11:44:21 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2018-03-26 11:48:18 UTC Comment hidden (obsolete)
Comment 3 Mark Tompsett 2018-03-28 03:41:26 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2018-03-28 13:00:09 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2018-03-28 13:12:13 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2018-07-02 12:22:22 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2018-07-02 12:22:26 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2018-07-02 15:17:37 UTC
Created attachment 76630 [details] [review]
Bug 20469: Add item status to staff article request form

A method Koha::Item->is_waiting_or_transit is added (with tests).
The template checks if an item is checked out, on hold (waiting or
transit), or is available. (Note: This can be extended in the future
in a general include as we have at opac side.)

Test plan:
[1] Run t/db_dependent/Koha/Items.t
[2] Place an article request on an item on loan. Verify status on form.
[3] Place an article request on a waiting item. Check status again.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Owen Leonard 2018-07-02 15:17:39 UTC
Created attachment 76631 [details] [review]
Bug 20469: (Follow-up) Make translation of status possible

As Mark noticed, adding a span for each status will allow for translation.
Note that the menu options in the block before are inside html tags like
<a><i>..</i>..</a> and will be translated (as long as spaces match).

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Jonathan Druart 2018-07-23 15:40:12 UTC
Why do you return an empty string instead of "0"?
IMO we should return ->count, and use the DBIC rs.

I will provide a patch and you will tell me what you think about it.
Comment 11 Jonathan Druart 2018-07-23 15:52:55 UTC
Created attachment 77184 [details] [review]
Bug 20469: Add item status to staff article request form

A method Koha::Item->is_waiting_or_transit is added (with tests).
The template checks if an item is checked out, on hold (waiting or
transit), or is available. (Note: This can be extended in the future
in a general include as we have at opac side.)

Test plan:
[1] Run t/db_dependent/Koha/Items.t
[2] Place an article request on an item on loan. Verify status on form.
[3] Place an article request on a waiting item. Check status again.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 12 Jonathan Druart 2018-07-23 15:52:59 UTC
Created attachment 77185 [details] [review]
Bug 20469: (Follow-up) Make translation of status possible

As Mark noticed, adding a span for each status will allow for translation.
Note that the menu options in the block before are inside html tags like
<a><i>..</i>..</a> and will be translated (as long as spaces match).

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 13 Jonathan Druart 2018-07-23 15:53:03 UTC
Created attachment 77186 [details] [review]
Bug 20469: Use DBIC rs and return ->count
Comment 14 Jonathan Druart 2018-07-23 15:54:19 UTC
Koha::Item->is_waiting_or_transit sounds wrong to me, this logic belongs to Koha::Hold
Waiting for another QA opinion.
Comment 15 Marcel de Rooy 2018-07-24 12:27:52 UTC
(In reply to Jonathan Druart from comment #10)
> Why do you return an empty string instead of "0"?
> IMO we should return ->count, and use the DBIC rs.
> 
> I will provide a patch and you will tell me what you think about it.

Empty string is the perl default for false. I have seen its use more in the Koha codebase. But I have no objection to your follow-up. Thanks.
Comment 16 Marcel de Rooy 2018-07-24 12:30:51 UTC
(In reply to Jonathan Druart from comment #14)
> Koha::Item->is_waiting_or_transit sounds wrong to me, this logic belongs to
> Koha::Hold
> Waiting for another QA opinion.

Yeah, I could agree too. Having another look..
Comment 17 Marcel de Rooy 2018-07-24 14:49:37 UTC
Created attachment 77222 [details] [review]
Bug 20469: (follow-up) Add Koha::Holds->waiting_or_transit

Adding this variant next to existing ->waiting and ->unfilled methods.
The method will be used in the next patch in Koha::Item.
Trivial unit test added in Holds.t

Test plan:
Run t/db_dependent/Holds.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2018-07-24 14:49:42 UTC
Created attachment 77223 [details] [review]
Bug 20469: Add item status to staff article request form

A method Koha::Item->has_waiting_or_transit_hold is added (with tests).
The template checks if an item is checked out, on hold (waiting or
transit), or is available. (Note: This can be extended in the future
in a general include as we have at opac side.)

Test plan:
[1] Run t/db_dependent/Koha/Items.t
[2] Place an article request on an item on loan. Verify status on form.
[3] Place an article request on a waiting item. Check status again.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

EDIT (July 24, 2018):
Method renamed to has_waiting_or_transit_hold. Part of the logic moved to
Koha::Holds (see first patch now). Replaced storing undef to found by
deleting hold in corresponding subtest of Items.t.
Added an additional check in the template for article requests on record
level that have no itemnumber (in that case no item status).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2018-07-24 14:49:46 UTC
Created attachment 77224 [details] [review]
Bug 20469: (follow-up) Make translation of status possible

As Mark noticed, adding a span for each status will allow for translation.
Note that the menu options in the block before are inside html tags like
<a><i>..</i>..</a> and will be translated (as long as spaces match).

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 20 Marcel de Rooy 2018-07-24 14:54:49 UTC
Core change is now:
+sub has_waiting_or_transit_hold {
+    my ( $self ) = @_;
+    my $rs = $self->_result->reserves;
+    return Koha::Holds->_new_from_dbic($rs)->waiting_or_transit->count;
+}

But I am still not completely happy about it. Fetching a result set, wrapping it in Koha object and searching again..

Another approach would be to get the "found => [W, T] " attribute from a class method in Koha Holds and pass it into the DBIx call in Koha Item. Resulting in one search. But also not very nice..

Or add something like Koha::Holds->search_waiting_or_transit({ itemnumber => $x })->count..

What do you think?
Comment 21 Marcel de Rooy 2018-07-24 14:55:42 UTC
Other candidates for a future change?

C4/Items.pm:            WHERE (found = 'W' OR found = 'T')
C4/Members.pm:    my $waiting_holds = $patron->holds->search({ found => 'W' });
circ/returns.pl:            my $waiting_holds = $patron->holds->search({ found => 'W', branchcode => $userenv_branch })->count;
installer/data/mysql/updatedatabase.pl:    my $waiting_holds = Koha::Holds->search({ found => 'W', priority => 0 });
Comment 22 Jonathan Druart 2018-07-24 18:59:55 UTC
The "correct" way to do it (regarding the discussion on bug 11983) would be:

  $patron->holds->filter_by_in_transit->filter_by_waiting

But it is not as simple to implement as it sounds.
Comment 23 Marcel de Rooy 2018-07-25 11:22:08 UTC
(In reply to Jonathan Druart from comment #22)
> The "correct" way to do it (regarding the discussion on bug 11983) would be:
> 
>   $patron->holds->filter_by_in_transit->filter_by_waiting
> 
> But it is not as simple to implement as it sounds.

Yeah, this sounds more like transit AND waiting instead of transit OR waiting.
And the idea of two filters applied in a row does not look good in terms of performance.

I will leave this patch in BLOCKED for the time being.
Comment 24 Jonathan Druart 2018-07-25 13:06:42 UTC
(In reply to Marcel de Rooy from comment #23)
> (In reply to Jonathan Druart from comment #22)
> > The "correct" way to do it (regarding the discussion on bug 11983) would be:
> > 
> >   $patron->holds->filter_by_in_transit->filter_by_waiting
> > 
> > But it is not as simple to implement as it sounds.
> 
> Yeah, this sounds more like transit AND waiting instead of transit OR
> waiting.

Yes you are right, it's not equivalent.

> And the idea of two filters applied in a row does not look good in terms of
> performance.

No it will not. DBIx::Class is building the ResultSet, nothing is fetch at this point.
Comment 25 Martin Renvoize 2018-08-06 14:31:39 UTC
This feels like it doesn't fit in either Koha::Items or Koha::Holds to me and rather, as per my suggestion in bug 11983 comment 8 (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=11983#c8) a Koha::Article::Requests module should be factored out.. we currently have too much business logic directly tied up in the model classes.

I don't really see this as a case for filter_by_x and filter_by_y methods actually as the filter, in my opinion, doesn't fit the 'involving complex joins and stuff' category.. rather it's a simple dbic query.

I would probably subclass the class most appropriate (Holds or Items.. I'm not sure here) and add the special accessor there.

Does this sound sane/make sense or do you want me to attempt to create a counter patch as example code?
Comment 26 Martin Renvoize 2018-08-08 10:43:33 UTC
https://wiki.koha-community.org/wiki/Koha_Objects#Subclassing_Koha::Object_and_Object_.22factories.22 for guidance around the sort of thing I'm thinking.
Comment 27 Marcel de Rooy 2020-07-23 07:42:03 UTC
This got stuck two years ago on a discussion on another report that did not get further either.
And now what?
Comment 28 Jonathan Druart 2020-07-23 08:02:43 UTC
Dependency removed. We more or less agreed on what has been discussed in bug 11983, even if not widely used yet we have some filter_by_* methods in Koha::

Can you please rebase, I will have another look.
Comment 29 Marcel de Rooy 2020-07-23 12:43:12 UTC
Created attachment 107235 [details] [review]
Bug 20469: Add item status to staff article request form

The template checks if an item is checked out, on hold (waiting or
transit), not for loan. (Note: This can be extended in the future
in a general include as we have at opac side.)

Test plan:
[1] Place an article request on an item on loan. Verify status on form.
[2] Place an article request on a waiting item. Check status again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

[EDIT] Simplified on 2020-07-23.
Removed Koha::Item->is_waiting_or_transit.
Added template plugin call ItemTypes->Notforloan.
Adjusted commit message accordingly.

Additional test:
[3] Place an article request on a not for loan item. Check status.

NOTE: Not for loan is informational too. It does not say that this item
is not available for an article request. Depends on local situation.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 30 Marcel de Rooy 2020-07-23 12:43:18 UTC
Created attachment 107236 [details] [review]
Bug 20469: Add test for ItemTypes plugin

Test plan:
Run the test.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 31 Marcel de Rooy 2020-07-23 12:44:34 UTC
Back to SO queue
The patch has been simplified. And actually no longer part of the former discussion..
Comment 32 Jonathan Druart 2020-07-27 10:19:47 UTC
Would not it be better to introduce Koha::Item->itemtype that would return a Koha::ItemType object?
Comment 33 Marcel de Rooy 2020-07-27 11:29:02 UTC
(In reply to Jonathan Druart from comment #32)
> Would not it be better to introduce Koha::Item->itemtype that would return a
> Koha::ItemType object?

Yes, good idea.
Comment 34 Marcel de Rooy 2020-07-27 12:48:18 UTC
Created attachment 107412 [details] [review]
Bug 20469: Add Koha::Item->itemtype

Also needed to add a missing rollback to preceding subtest.

Test plan:
Run t/db_dependent/Koha/Item.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2020-07-27 12:48:23 UTC
Created attachment 107413 [details] [review]
Bug 20469: Add item status to staff article request form

The template checks if an item is checked out, on hold (waiting or
transit), not for loan. (Note: This can be extended in the future
in a general include as we have at opac side.)

Test plan:
[1] Place an article request on an item on loan. Verify status on form.
[2] Place an article request on a waiting item. Check status again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

[EDIT] Simplified on 2020-07-27
Removed Koha::Item->is_waiting_or_transit.
Use Koha::Item->itemtype to check notforloan on itemtype level.
Adjusted commit message accordingly.

Additional test:
[3] Place an article request on a not for loan item. Check status.

NOTE: Not for loan is informational too. It does not say that this item
is not available for an article request. Depends on local situation.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Jonathan Druart 2020-07-27 13:25:05 UTC
I'd remove the effective param, I don't see a case where we would not need it, effective_itemtype will always return the correct value (depending on the pref)
Comment 37 Marcel de Rooy 2020-07-27 13:37:45 UTC
Created attachment 107417 [details] [review]
Bug 20469: Add Koha::Item->itemtype

Also needed to add a missing rollback to preceding subtest.

Test plan:
Run t/db_dependent/Koha/Item.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2020-07-27 13:37:50 UTC
Created attachment 107418 [details] [review]
Bug 20469: Remove effective parameter

Test plan:
Run test Koha/Item.t again

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Marcel de Rooy 2020-07-27 13:37:55 UTC
Created attachment 107419 [details] [review]
Bug 20469: Add item status to staff article request form

The template checks if an item is checked out, on hold (waiting or
transit), not for loan. (Note: This can be extended in the future
in a general include as we have at opac side.)

Test plan:
[1] Place an article request on an item on loan. Verify status on form.
[2] Place an article request on a waiting item. Check status again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

[EDIT] Simplified on 2020-07-27
Removed Koha::Item->is_waiting_or_transit.
Use Koha::Item->itemtype to check notforloan on itemtype level.
Adjusted commit message accordingly.

Additional test:
[3] Place an article request on a not for loan item. Check status.

NOTE: Not for loan is informational too. It does not say that this item
is not available for an article request. Depends on local situation.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Jonathan Druart 2020-07-27 14:01:57 UTC
Thanks Marcel, the patches look great now! :)
Comment 41 Katrin Fischer 2020-08-16 09:11:49 UTC
Created attachment 108345 [details] [review]
Bug 20469: Add Koha::Item->itemtype

Also needed to add a missing rollback to preceding subtest.

Test plan:
Run t/db_dependent/Koha/Item.t

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 42 Katrin Fischer 2020-08-16 09:11:55 UTC
Created attachment 108346 [details] [review]
Bug 20469: Remove effective parameter

Test plan:
Run test Koha/Item.t again

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 43 Katrin Fischer 2020-08-16 09:12:02 UTC
Created attachment 108347 [details] [review]
Bug 20469: Add item status to staff article request form

The template checks if an item is checked out, on hold (waiting or
transit), not for loan. (Note: This can be extended in the future
in a general include as we have at opac side.)

Test plan:
[1] Place an article request on an item on loan. Verify status on form.
[2] Place an article request on a waiting item. Check status again.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

[EDIT] Simplified on 2020-07-27
Removed Koha::Item->is_waiting_or_transit.
Use Koha::Item->itemtype to check notforloan on itemtype level.
Adjusted commit message accordingly.

Additional test:
[3] Place an article request on a not for loan item. Check status.

NOTE: Not for loan is informational too. It does not say that this item
is not available for an article request. Depends on local situation.

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

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 44 Katrin Fischer 2020-08-16 09:12:17 UTC
Nice clean patch. Something similar would be really nice for the item search results.
Comment 45 Marcel de Rooy 2020-08-17 06:11:20 UTC
(In reply to Katrin Fischer from comment #43)
> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Thanks !
Comment 46 Jonathan Druart 2020-08-18 15:46:01 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 47 Lucas Gass 2020-09-04 15:35:07 UTC
enhancement will not be backported to 20.05.x