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")

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")

(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 ?

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 )

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.

> 
> 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

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.

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.

Seeing the change, yes, I think a new signoff is required

Same behaviour with my last tests.
Signed off

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.

(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.

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>

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

Pushed to 3.8.x, will be in 3.8.4