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.
Created attachment 73281 [details] [review] Bug 20469: Add item status to staff article request form
Created attachment 73282 [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>
Comment on attachment 73282 [details] [review] Bug 20469: Add item status to staff article request form Review of attachment 73282 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/article-requests.tt @@ +42,5 @@ > [% END %] > +[% BLOCK item_status %][%# pass Koha::Item into myitem %] > + [% IF myitem.onloan %]Checked out > + [% ELSIF myitem.is_waiting_or_transit %]On hold > + [% ELSE %]Available Just curious about the lack of <span>'s. Does this translate?
(In reply to M. Tompsett from comment #3) > Comment on attachment 73282 [details] [review] [review] > Bug 20469: Add item status to staff article request form > > Review of attachment 73282 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/article-requests.tt > @@ +42,5 @@ > > [% END %] > > +[% BLOCK item_status %][%# pass Koha::Item into myitem %] > > + [% IF myitem.onloan %]Checked out > > + [% ELSIF myitem.is_waiting_or_transit %]On hold > > + [% ELSE %]Available > > Just curious about the lack of <span>'s. Does this translate? Hmm It doesnt. Will add a follow-up. Need them too in the other block. Thx.
Created attachment 73388 [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>
Created attachment 76627 [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>
Created attachment 76628 [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>
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>
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>
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.
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>
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>
Created attachment 77186 [details] [review] Bug 20469: Use DBIC rs and return ->count
Koha::Item->is_waiting_or_transit sounds wrong to me, this logic belongs to Koha::Hold Waiting for another QA opinion.
(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.
(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..
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>
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>
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>
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?
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 });
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.
(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.
(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.
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?
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.
This got stuck two years ago on a discussion on another report that did not get further either. And now what?
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.
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>
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>
Back to SO queue The patch has been simplified. And actually no longer part of the former discussion..
Would not it be better to introduce Koha::Item->itemtype that would return a Koha::ItemType object?
(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.
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>
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>
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)
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>
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>
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>
Thanks Marcel, the patches look great now! :)
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>
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>
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>
Nice clean patch. Something similar would be really nice for the item search results.
(In reply to Katrin Fischer from comment #43) > Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Thanks !
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x