[Koha-bugs] [Bug 22569] Add a 'Transfers to send' report

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22569

--- Comment #46 from Jonathan Druart <jonathan.druart at bugs.koha-community.org> ---
Adding here some notes about the dependent tree (I've squashed the whole tree
for pre-review):

1. 
-        $messages->{'WasTransfered'} = 1;
+        $messages->{'WasTransfered'} = $tbr;

but

t/db_dependent/SIP/Transaction.t:    is_deeply($result,{ messages => {
'NotIssued' => $item->barcode, 'WasTransfered' => 1 } },"Messages show not
issued and transferred");                                       

So I ran the tests and there is a failure:

    #   Failed test 'Messages show not issued and transferred'
    #   at t/db_dependent/SIP/Transaction.t line 327.
    #     Structures begin differing at:
    #          $got->{messages}{TransferTrigger} = 'ReturnToHome'
    #     $expected->{messages}{TransferTrigger} = Does not exist
        # Looks like you planned 5 tests but ran 2.

2. 
-    my ($datesent,$frombranch,$tobranch) = GetTransfers( $item->itemnumber );
+    my $transfer = $item->get_transfer;

This GetTransfers is pretty bad, it could return several transfers, but callers
are not ready for that:
opac/opac-detail.pl:     my ( $transfertwhen, $transfertfrom, $transfertto ) =
GetTransfers($itm->{itemnumber});                                               

So that's definitely a good move to have a get_transfer method that will return
only 1, the current one.
However, cannot we enforce this constraint at DB level (DB unique key) and have
a ->find call in ->get_transfer to replace the ->first?

3. Shouldn't Koha::Exceptions::Item::Transfer::Found be actually
Koha::Exceptions::Item::Transfer::AlreadyInTransfer, to be more explicit?

4. Same, should we make Koha::Exceptions::Item::Transfer::Limit more explicit?
Its description is "Transfer not allowed" but it seems that we can make it more
exact.

5. in circ/transferstosend.pl
+    show_date    => output_pref(                                               
+        { dt => dt_from_string, dateformat => 'iso', dateonly => 1 }
+    )

I'd pass {today => dt_from_string}, it should be enough.

6. in circ/transferstosend.tt
[% FOREACH branchesloo IN branchesloop %]                                       
should be
[% FOREACH library IN libraries %]

7. There are some indentation inconsistencies in circ/transferstosend.tt

8. (not blocker) transferCollection.tt
+                        [%- SWITCH message.type -%]
+                            [%- CASE 'failure' %]
...
+                            [%- CASE 'enqueu' -%]

The usual pattern is
push @message, 
{
  type => 'error', # or message
  code => 'enqueu(ed?)',
  %more_variables 
}

-- 
You are receiving this mail because:
You are watching all bug changes.