Bug 3701 - If the ReturnToShelvingCart syspref is on, and something needs to go in transit, the shelving cart setting is overriding the transit.
Summary: If the ReturnToShelvingCart syspref is on, and something needs to go in trans...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: 3.8
Hardware: PC All
: P5 - low minor (vote)
Assignee: Kyle M Hall
QA Contact: Mason James
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-10-09 12:37 UTC by Kyle M Hall
Modified: 2013-12-05 19:59 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Fixes the issue by checking to see if the item was transferred. If it was, we don't change the location. (2.30 KB, patch)
2009-10-09 12:39 UTC, Chris Cormack
Details | Diff | Splinter Review
BugFix - ReturnToShelvingCart (2.37 KB, patch)
2012-06-09 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
BugFix - ReturnToShelvingCart (3.25 KB, patch)
2012-07-16 13:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
Screenshot (64.88 KB, image/jpeg)
2012-07-16 13:23 UTC, Kyle M Hall
Details
BugFix - ReturnToShelvingCart (2.74 KB, patch)
2012-07-16 13:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
BugFix - ReturnToShelvingCart (2.80 KB, patch)
2012-07-16 14:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
BugFix - ReturnToShelvingCart (3.90 KB, patch)
2012-07-17 16:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
BugFix - ReturnToShelvingCart (4.00 KB, patch)
2012-07-23 21:10 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Cormack 2010-05-21 01:13:57 UTC


---- Reported by kyle.m.hall@gmail.com 2009-10-09 12:37:27 ----

What seems to be happening is this:
 * Item is checked in, and flagged as needing to go in transit from A to B.
 * Item is immediately flagged as shelving cart, though.  Current display says both shelving cart and in transit.
 * After the cart-to-shelf script runs, item comes out of transit and shows as being available at the destination library, although it hasn't arrived yet and no one has manually checked it in.
    
It looks like the "checkin" from the script is telling Koha that the in-transit has been received.



---- Additional Comments From kyle.m.hall@gmail.com 2009-10-09 12:39:41 ----

Created an attachment
Fixes the issue by checking to see if the item was transferred. If it was, we don't change the location.





--- Bug imported by chris@bigballofwax.co.nz 2010-05-21 01:13 UTC  ---

This bug was previously known as _bug_ 3701 at http://bugs.koha.org/cgi-bin/bugzilla3/show_bug.cgi?id=3701
Imported an attachment (id=1524)

Actual time not defined. Setting to 0.0
The original submitter of attachment 1524 [details] [review] is unknown.
   Reassigning to the person who moved it here: chris@bigballofwax.co.nz.

Comment 1 Kyle M Hall 2012-06-09 13:33:54 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2012-07-12 14:27:14 UTC
I think I don't understand. What is expected ?

I have the same behaviour with and without your patch.

Syspref configuration:
  AutomaticItemReturn = Don't
  ReturnToShelvingCart = Move

- I checkout an item from A
- I change my library for B
- I checkin the item
- The interface ask me if I want to tranfer now (Shelving location is "Book Cart"), I answer "Yes"
- I go on the catalogue detail page and the item informations are
  status = In transit from B, to A, since [date]
  homelibrary = A Book Cart
- I launch the cart_to_shelf script (with modifications for don't wait 1h) and the items.location is set to items.permanent_location.
- On the catalogue detail page :
  status = In transit from B, to A, since [date]
  homelibrary = A (without "Book Cart")
Comment 3 Kyle M Hall 2012-07-13 13:57:59 UTC
Have you tried it with AutomaticItemReturn enabled?

Kyle

(In reply to comment #2)
> I think I don't understand. What is expected ?
> 
> I have the same behaviour with and without your patch.
> 
> Syspref configuration:
>   AutomaticItemReturn = Don't
>   ReturnToShelvingCart = Move
> 
> - I checkout an item from A
> - I change my library for B
> - I checkin the item
> - The interface ask me if I want to tranfer now (Shelving location is "Book
> Cart"), I answer "Yes"
> - I go on the catalogue detail page and the item informations are
>   status = In transit from B, to A, since [date]
>   homelibrary = A Book Cart
> - I launch the cart_to_shelf script (with modifications for don't wait 1h)
> and the items.location is set to items.permanent_location.
> - On the catalogue detail page :
>   status = In transit from B, to A, since [date]
>   homelibrary = A (without "Book Cart")
Comment 4 Jonathan Druart 2012-07-16 11:57:08 UTC
(In reply to comment #3)
> Have you tried it with AutomaticItemReturn enabled?

If AutomaticItemReturn is enabled:

Without your patch:
- I checkout an item from A
- I change my library for B
- I checkin the item
- The interface tell me: "Please return BIBLIO_TITLE to A"
- I go on the catalogue detail page and the item informations are
  status = In transit from B, to A, since [date]
  homelibrary = A Book Cart
After returned to A, information become: 
  status = Available
  homelibrary = A A (permanent_location and location)

With your patch, I have
  status = In transit from B, to A, since [date]
  homelibrary = A A
After returned to A, information become: 
  status = Available
  homelibrary = A Book Cart

Is it the expected behaviour ? If it is, don't we want the same behaviour whatever the value of AutomaticItemReturn ?
Comment 5 Kyle M Hall 2012-07-16 13:20:06 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2012-07-16 13:23:07 UTC
Created attachment 10872 [details]
Screenshot

First line is automatic transfers without patch
Second line is automatic transfers with patch
Third line is manual transfers without patch
Fourth lines is manual transfers with patch ( fails )
Fifth line is manual transfers with *updated* patch ( succeeds )
Comment 7 Kyle M Hall 2012-07-16 13:26:49 UTC
Test Plan:

To replicate the bug:
1) Enable ReturnToShelvingCart
2) Checkout an item at LibraryA
3) Return the item to LibraryB
4) If AutomaticItemReturn is enabled, do nothing, otherwise click yes to begin transfer.
5) Check item record, you should see both a location of 'shelving cart' and the fact that the item is being transferred.
6) Apply this patch
7) Repeat steps 2 through 4
8) You should see the standard location ( if any ) along with the item being transferred message.
Comment 8 Kyle M Hall 2012-07-16 13:27:53 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2012-07-16 14:43:34 UTC Comment hidden (obsolete)
Comment 10 Mason James 2012-07-17 04:58:40 UTC
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

passing QA on this patch...

$ koha-qa.pl
- commit 7fcbf2e BugFix - ReturnToShelvingCart
        C4/Items.pm
        circ/returns.pl
- perlcritic-progressive tests... OK
- perl -c syntax tests... OK
- xt/tt_valid.t tests... OK
- xt/author/vaild-template.t tests... OK
Comment 11 Paul Poulain 2012-07-17 16:05:41 UTC
mmm... I don't like this patch, but still not sure it's wrong.

Here is why I don't like it:

You write:
+    if ( C4::Context->preference("ReturnToShelvingCart") && !$messages->{'WasTransfered'} ) {
+        my $item = GetItem( $itemnumber );
+        $item->{'location'} = 'CART';
+        ModItem( $item, $item->{'biblionumber'}, $item->{'itemnumber'} );
+    }

in return.pl

So, you manage the ShelvingCart:
 * outside from AddReturn sub => what would happen if I mark return from circulation.pl (I can to that by checking the checkbox on the right of any issued item, and "renew or return checked items") ? Same thing for moremember.pl. Overall, I feel it's unsafe to manage that outside AddReturn

 * you use $messages->{'WasTransfered'} to check if there has been a transfer. That's a little bit unsafe. If we remove the message, or change the use of the message, we may face side-effect

I don't failed QA this patch for now, please argue if you think i'm wrong.
Comment 12 Kyle M Hall 2012-07-17 16:55:16 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2012-07-17 16:59:11 UTC
I think you are absolutely right. I have just submitted a modified version of the patch that moves this logic in AddReturn(), rather than leaving it in returns.pl

I'm leaving the status as Passed QA for now. If you feel this new version needs to restart the  submission process, please go ahead and reset the status to "Needs Signoff".

(In reply to comment #11)
> mmm... I don't like this patch, but still not sure it's wrong.
> 
> Here is why I don't like it:
> 
> You write:
> +    if ( C4::Context->preference("ReturnToShelvingCart") &&
> !$messages->{'WasTransfered'} ) {
> +        my $item = GetItem( $itemnumber );
> +        $item->{'location'} = 'CART';
> +        ModItem( $item, $item->{'biblionumber'}, $item->{'itemnumber'} );
> +    }
> 
> in return.pl
> 
> So, you manage the ShelvingCart:
>  * outside from AddReturn sub => what would happen if I mark return from
> circulation.pl (I can to that by checking the checkbox on the right of any
> issued item, and "renew or return checked items") ? Same thing for
> moremember.pl. Overall, I feel it's unsafe to manage that outside AddReturn
> 
>  * you use $messages->{'WasTransfered'} to check if there has been a
> transfer. That's a little bit unsafe. If we remove the message, or change
> the use of the message, we may face side-effect
> 
> I don't failed QA this patch for now, please argue if you think i'm wrong.
Comment 14 Paul Poulain 2012-07-18 09:48:45 UTC
Seeing the change, yes, I think a new signoff is required
Comment 15 Jonathan Druart 2012-07-18 10:14:31 UTC
Same behaviour with my last tests.
Signed off
Comment 16 Paul Poulain 2012-07-23 15:17:42 UTC
QA comment:

The code now says
             ShelfToCart( $item->{'itemnumber'} );
=> shouldn't we deal with the syspref and say
            ShelfToCart( $item->{'itemnumber'} ) if C4::Context->preference("ReturnToShelvingCart");
?

2nd question: when the item reaches it's homebranch, a check-in is done, right ?
I tried that, and the location stayed to 'CART' instead of being switched back to items.permanentlocation.
Comment 17 Kyle M Hall 2012-07-23 21:09:45 UTC
(In reply to comment #16)
> QA comment:
> 
> The code now says
>              ShelfToCart( $item->{'itemnumber'} );
> => shouldn't we deal with the syspref and say
>             ShelfToCart( $item->{'itemnumber'} ) if
> C4::Context->preference("ReturnToShelvingCart");
> ?

You are right, I'll fix that.

> 2nd question: when the item reaches it's homebranch, a check-in is done,
> right ?
> I tried that, and the location stayed to 'CART' instead of being switched
> back to items.permanentlocation.

Yes, a check-in is done, but that sets the location to 'CART' if it is not already set. Later, the cronjob will change the status from 'CART' back to the item's permanent location.
Comment 18 Kyle M Hall 2012-07-23 21:10:14 UTC
Created attachment 11093 [details] [review]
BugFix - ReturnToShelvingCart

If the ReturnToShelvingCart syspref is on, and something needs to go in transit,
the shelving cart setting is overriding the transit.

What seems to be happening is this:
  * Item is checked in, and flagged as needing to go in transit from A to B.
  * Item is immediately flagged as shelving cart, though.  Current display says both shelving cart and in transit.
  * After the cart-to-shelf script runs, item comes out of transit and
    shows as being available at the destination library, although it hasn't
    arrived yet and no one has manually checked it in.

http://bugs.koha-community.org/show_bug.cgi?id=3701

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 19 Paul Poulain 2012-07-24 14:06:30 UTC
OK, this time, passed QA.

I also made many tests to check that, with without the syspref ReturnToShelvingCart set the behaviour is unchanged. I could not find any problem
Comment 20 Chris Cormack 2012-07-25 01:37:37 UTC
Pushed to 3.8.x, will be in 3.8.4