Bug 22569

Summary: Add a 'Transfers to send' report
Product: Koha Reporter: Nick Clemens <nick>
Component: Staff interfaceAssignee: Martin Renvoize <martin.renvoize>
Status: Needs documenting --- QA Contact: Katrin Fischer <katrin.fischer>
Severity: new feature    
Priority: P5 - low CC: bugzilla, fridolin.somers, gmcharlt, jonathan.druart, joonas.kylmala, katrin.fischer, m.de.rooy, martin.renvoize, mathsabypro, nick, philip.orr, sally.healey
Version: masterKeywords: Manual
Hardware: All   
OS: All   
URL: https://gitlab.com/mrenvoize/Koha/-/tree/bug_22569
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35388
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35389
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Philip Orr Documentation submission:
Text to go in the release notes:
This enhancement adds a 'Transfers to send' report to the circulation module. This is a parallel for the 'Transfers to receive' report and lists all items that are set to transfer but not yet in transit (along with their transfer reason and whether they're available to be picked from the shelves or currently checked out).
Version(s) released in:
21.05.00
Bug Depends on: 26618    
Bug Blocks: 22160, 35389    
Attachments: Bug 22569: Proof of concept
Bug 22569: Proof of concept
Bug 22569: Proof of concept
Bug 22569: Proof of concept
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Reverse transferstosend controller
Add inbound_transfers to Koha::Library
Use inbound_transfers in transferstosend template
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: (QA follow-up) Make 'reason' translatable
Bug 22569: (QA follow-up) Use itemtype description
Bug 22569: (QA follow-up) Make 'reason' translatable
Bug 22569: (QA follow-up) Use itemtype description
Bug 22569: (QA follow-up) Make 'reason' translatable
Bug 22569: (QA follow-up) Use itemtype description
Bug 22569: Add a 'Transfers to send' report
Bug 22569: Add Unit Tests
Bug 22569: (QA follow-up) Make 'reason' translatable
Bug 22569: (QA follow-up) Use itemtype description

Description Nick Clemens 2019-03-22 17:49:52 UTC
The cronjob provides a report email tot he branch, however, there is no way on the  staff client to view current items needing checked in/advanced at the current branch. Perhaps a 'transfers to send' report like 'transfers to receive'
Comment 1 Martin Renvoize 2019-06-10 09:31:55 UTC
Nice idea, I'll try to get to this some time soon.
Comment 2 Martin Renvoize 2019-06-10 14:18:51 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize 2019-06-11 10:56:05 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize 2019-06-11 11:02:11 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2019-06-11 13:31:35 UTC Comment hidden (obsolete)
Comment 6 Martin Renvoize 2019-06-11 13:33:05 UTC Comment hidden (obsolete)
Comment 7 Martin Renvoize 2019-06-11 13:34:40 UTC
Work still to be done.. Unit tests.
Comment 8 Martin Renvoize 2019-10-24 10:16:39 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2019-10-24 10:22:06 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2019-10-24 12:18:22 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize 2019-10-24 12:18:26 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2019-11-03 19:11:08 UTC
Hi Martin, I am not sure about how the report is supposed to be used - can you explain?

A bit worried that this will be the crucial bit:
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent.

So you won't see what has been put into transfer already?
Comment 13 Martin Renvoize 2019-11-04 14:40:07 UTC
(In reply to Katrin Fischer from comment #12)
> Hi Martin, I am not sure about how the report is supposed to be used - can
> you explain?
> 
> A bit worried that this will be the crucial bit:
> 2) There is no way to differentiate between a transfer that has been
>    requested and a transfer than has been actually sent.
> 
> So you won't see what has been put into transfer already?

Well, it's as an alternative to the stock rotation email reports which send this same data via email as a pick list for staff.

I can see where the confusion could come in.. perhaps bug 23092 needs to be done first if you think this will be more confusing as opposed to less?
Comment 14 Martin Renvoize 2019-12-19 06:53:09 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2019-12-19 06:53:12 UTC Comment hidden (obsolete)
Comment 16 Martin Renvoize 2019-12-19 06:53:15 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize 2019-12-19 06:53:18 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize 2019-12-19 06:53:21 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize 2019-12-19 07:09:02 UTC
Created attachment 96478 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer by the stock
rotation subsystem.

Caveats:
1) It is currently limited to transfers prompted by stockrotation
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent. (but we do
   filter out transfers that have been marked as received)
3) Both 'Advance' and 'Repatriate' actions will be listed and the
   'Advance' listings should have the option to set the item as 'In
   demand' at the current branch and thus cancel the transfer and
   make the item wait an additional stage period before being
   automatically set to transfer again.

   I believe we should enhance the transfers system as a whole to
   allow this report to be more meaningful and include transfer
   initiated via other means (along with appropriate actions to be
   taken upon them)

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appears in the new 'Transfers
   to send' page.

It does not yet catch items that are due for transfer by any other means
as at this time we do not store that sort of detail for other mechanisms.
See bug 23092 for one proposed resolution to that.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2019-12-19 07:09:05 UTC
Created attachment 96479 [details] [review]
Bug 22569: Add Unit Tests
Comment 21 Martin Renvoize 2019-12-20 11:53:57 UTC
Created attachment 96516 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer by the stock
rotation subsystem.

Caveats:
1) It is currently limited to transfers prompted by stockrotation
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent. (but we do
   filter out transfers that have been marked as received)
3) Both 'Advance' and 'Repatriate' actions will be listed and the
   'Advance' listings should have the option to set the item as 'In
   demand' at the current branch and thus cancel the transfer and
   make the item wait an additional stage period before being
   automatically set to transfer again.

   I believe we should enhance the transfers system as a whole to
   allow this report to be more meaningful and include transfer
   initiated via other means (along with appropriate actions to be
   taken upon them)

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appears in the new 'Transfers
   to send' page.

It does not yet catch items that are due for transfer by any other means
as at this time we do not store that sort of detail for other mechanisms.
See bug 23092 for one proposed resolution to that.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Martin Renvoize 2019-12-20 11:54:00 UTC
Created attachment 96517 [details] [review]
Bug 22569: Add Unit Tests
Comment 23 Martin Renvoize 2020-01-15 13:34:51 UTC
Created attachment 97392 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer by the stock
rotation subsystem.

Caveats:
1) It is currently limited to transfers prompted by stockrotation
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent. (but we do
   filter out transfers that have been marked as received)
3) Both 'Advance' and 'Repatriate' actions will be listed and the
   'Advance' listings should have the option to set the item as 'In
   demand' at the current branch and thus cancel the transfer and
   make the item wait an additional stage period before being
   automatically set to transfer again.

   I believe we should enhance the transfers system as a whole to
   allow this report to be more meaningful and include transfer
   initiated via other means (along with appropriate actions to be
   taken upon them)

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appears in the new 'Transfers
   to send' page.

It does not yet catch items that are due for transfer by any other means
as at this time we do not store that sort of detail for other mechanisms.
See bug 23092 for one proposed resolution to that.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Martin Renvoize 2020-01-15 13:34:55 UTC
Created attachment 97393 [details] [review]
Bug 22569: Add Unit Tests
Comment 25 Katrin Fischer 2020-01-15 22:57:17 UTC
This has a dependency on bug 23092 that doesn't seem right. 

We are also failing the QA script:

 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/circ/transferstosend.tt
   FAIL	  filters
		wrong_html_filter at line 59 (                        <a href="/cgi-bin/koha/catalogue/stockrotation.pl?op=toggle_in_demand&stage_id=4&item_id=[% transfer.itemnumber %]&biblionumber=[% transfer.item.biblionumber %]" class="btn btn-default btn-xs"><i class="fa fa-fire"></i> Mark "In demand"</a>)
		wrong_html_filter at line 59 (                        <a href="/cgi-bin/koha/catalogue/stockrotation.pl?op=toggle_in_demand&stage_id=4&item_id=[% transfer.itemnumber %]&biblionumber=[% transfer.item.biblionumber %]" class="btn btn-default btn-xs"><i class="fa fa-fire"></i> Mark "In demand"</a>)
		missing_filter at line 59 (                        <a href="/cgi-bin/koha/catalogue/stockrotation.pl?op=toggle_in_demand&stage_id=4&item_id=[% transfer.itemnumber %]&biblionumber=[% transfer.item.biblionumber %]" class="btn btn-default btn-xs"><i class="fa fa-fire"></i> Mark "In demand"</a>)
		missing_filter at line 59 (                        <a href="/cgi-bin/koha/catalogue/stockrotation.pl?op=toggle_in_demand&stage_id=4&item_id=[% transfer.itemnumber %]&biblionumber=[% transfer.item.biblionumber %]" class="btn btn-default btn-xs"><i class="fa fa-fire"></i> Mark "In demand"</a>)
   FAIL	  forbidden patterns
		forbidden pattern: tab char (line 31)


And the tests are not passing for me?

Please fix!
Comment 26 Martin Renvoize 2020-07-23 14:38:10 UTC
Created attachment 107258 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer by the stock
rotation subsystem.

Caveats:
1) It is currently limited to transfers prompted by stockrotation
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent. (but we do
   filter out transfers that have been marked as received)
3) Both 'Advance' and 'Repatriate' actions will be listed and the
   'Advance' listings should have the option to set the item as 'In
   demand' at the current branch and thus cancel the transfer and
   make the item wait an additional stage period before being
   automatically set to transfer again.

   I believe we should enhance the transfers system as a whole to
   allow this report to be more meaningful and include transfer
   initiated via other means (along with appropriate actions to be
   taken upon them)

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appears in the new 'Transfers
   to send' page.

It does not yet catch items that are due for transfer by any other means
as at this time we do not store that sort of detail for other mechanisms.
See bug 23092 for one proposed resolution to that.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2020-07-23 14:38:15 UTC
Created attachment 107259 [details] [review]
Bug 22569: Add Unit Tests
Comment 28 Martin Renvoize 2020-07-23 14:38:19 UTC
Created attachment 107260 [details] [review]
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers

GetTransfersFromTo is used solely by the transferstorecieve page which
assumes items have already been sent. With the introduction of
daterequested we allow for a state where a transfer has been scheduled
but not yet sent (See stockrotation for such a case) and so we need to
filter these transfers out from the transferstorecieve report.
Comment 29 Martin Renvoize 2020-07-24 10:19:41 UTC
Created attachment 107306 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer by the stock
rotation subsystem.

Caveats:
1) It is currently limited to transfers prompted by stockrotation
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent. (but we do
   filter out transfers that have been marked as received)
3) Both 'Advance' and 'Repatriate' actions will be listed and the
   'Advance' listings should have the option to set the item as 'In
   demand' at the current branch and thus cancel the transfer and
   make the item wait an additional stage period before being
   automatically set to transfer again.

   I believe we should enhance the transfers system as a whole to
   allow this report to be more meaningful and include transfer
   initiated via other means (along with appropriate actions to be
   taken upon them)

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appears in the new 'Transfers
   to send' page.

It does not yet catch items that are due for transfer by any other means
as at this time we do not store that sort of detail for other mechanisms.
See bug 23092 for one proposed resolution to that.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2020-07-24 10:19:44 UTC
Created attachment 107307 [details] [review]
Bug 22569: Add Unit Tests
Comment 31 Martin Renvoize 2020-07-24 10:19:48 UTC
Created attachment 107308 [details] [review]
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers

GetTransfersFromTo is used solely by the transferstorecieve page which
assumes items have already been sent. With the introduction of
daterequested we allow for a state where a transfer has been scheduled
but not yet sent (See stockrotation for such a case) and so we need to
filter these transfers out from the transferstorecieve report.
Comment 32 Martin Renvoize 2020-09-17 16:28:35 UTC
Created attachment 110284 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer by the stock
rotation subsystem.

Caveats:
1) It is currently limited to transfers prompted by stockrotation
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent. (but we do
   filter out transfers that have been marked as received)
3) Both 'Advance' and 'Repatriate' actions will be listed and the
   'Advance' listings should have the option to set the item as 'In
   demand' at the current branch and thus cancel the transfer and
   make the item wait an additional stage period before being
   automatically set to transfer again.

   I believe we should enhance the transfers system as a whole to
   allow this report to be more meaningful and include transfer
   initiated via other means (along with appropriate actions to be
   taken upon them)

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appears in the new 'Transfers
   to send' page.

It does not yet catch items that are due for transfer by any other means
as at this time we do not store that sort of detail for other mechanisms.
See bug 23092 for one proposed resolution to that.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 33 Martin Renvoize 2020-09-17 16:28:40 UTC
Created attachment 110285 [details] [review]
Bug 22569: Add Unit Tests

Test plan:
1/ Run the updated unit tests

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 34 Martin Renvoize 2020-09-17 16:28:47 UTC
Created attachment 110286 [details] [review]
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers

GetTransfersFromTo is used solely by the transferstorecieve page which
assumes items have already been sent. With the introduction of
daterequested we allow for a state where a transfer has been scheduled
but not yet sent (See stockrotation for such a case) and so we need to
filter these transfers out from the transferstorecieve report.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 35 Martin Renvoize 2020-09-17 16:30:28 UTC
This development has been in use for considerable time on site and I have been given approval by he users there to add their signoff.
Comment 36 Martin Renvoize 2020-09-22 12:19:24 UTC
Created attachment 110543 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer by the stock
rotation subsystem.

Caveats:
1) It is currently limited to transfers prompted by stockrotation
2) There is no way to differentiate between a transfer that has been
   requested and a transfer than has been actually sent. (but we do
   filter out transfers that have been marked as received)
3) Both 'Advance' and 'Repatriate' actions will be listed and the
   'Advance' listings should have the option to set the item as 'In
   demand' at the current branch and thus cancel the transfer and
   make the item wait an additional stage period before being
   automatically set to transfer again.

   I believe we should enhance the transfers system as a whole to
   allow this report to be more meaningful and include transfer
   initiated via other means (along with appropriate actions to be
   taken upon them)

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appears in the new 'Transfers
   to send' page.

It does not yet catch items that are due for transfer by any other means
as at this time we do not store that sort of detail for other mechanisms.
See bug 23092 for one proposed resolution to that.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 37 Martin Renvoize 2020-09-22 12:19:31 UTC
Created attachment 110544 [details] [review]
Bug 22569: Add Unit Tests

Test plan:
1/ Run the updated unit tests

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 38 Martin Renvoize 2020-09-22 12:19:36 UTC
Created attachment 110545 [details] [review]
Bug 22569: Refine GetTransfersFromTo to filter out unsent transfers

GetTransfersFromTo is used solely by the transferstorecieve page which
assumes items have already been sent. With the introduction of
daterequested we allow for a state where a transfer has been scheduled
but not yet sent (See stockrotation for such a case) and so we need to
filter these transfers out from the transferstorecieve report.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 39 Martin Renvoize 2020-10-20 13:25:26 UTC
OK.. pretty much the entire tree beneath this has now been tested and either already QA'd or SO'd in isolation, but it's time to QA the whole patchset.

I've set all preceeding bugs to BLOCKED to prevent duplication of work and obsoleted all patches so we can instead checkout the 'definitive' branch which contains all the patches.

I'm currently working through the creation of a complete test plan to take account of all area's modified.
Comment 40 Martin Renvoize 2021-01-12 11:29:08 UTC
I'm looking for feedback for this tree on https://gitlab.com/mrenvoize/Koha/-/merge_requests/1
Comment 41 Katrin Fischer 2021-02-07 13:42:00 UTC
BLOCKED by dependent bug.
Comment 42 Martin Renvoize 2021-02-09 11:20:08 UTC
Created attachment 116572 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer but not
marked as sent.

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appear in the new 'Transfers to send'
   page.
4) Confirm that items marked with 'StockRotationAdvance' provide the
   option to mark the item as 'In demand' (Which will cancel the
   transfer and mark the item as in demand such that it will wait at the
   current branch for an additional stage period before being
   automatically picke for transfer again)
5) Setup a rotating collection
6) Trigger a collection transfer for the above collection
7) Check that the expected items appear in the new 'Transfers to send'
   page'.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 43 Martin Renvoize 2021-02-09 11:20:12 UTC
Created attachment 116573 [details] [review]
Bug 22569: Add Unit Tests

Test plan:
1/ Run the updated unit tests

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 44 Jonathan Druart 2021-02-16 09:57:40 UTC
Is this store call needed?

+            $self->stage_id($new_stage->stage_id)->store;        # Revert stage change

There are also several places in the tests where ->store calls are unnecessary.
Comment 45 Jonathan Druart 2021-02-16 09:59:51 UTC
Shouldn't we start with a good script/template name?
transferstosend.pl => transfers_to_send.pl
Comment 46 Jonathan Druart 2021-02-16 10:39:36 UTC
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 
}
Comment 47 Martin Renvoize 2021-02-18 09:24:24 UTC
(In reply to Jonathan Druart from comment #46)
> 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.

Fixed on the gitlab branch: Two follow-ups on bug 24446, the first fixes the test above by adding the 'TransferTrigger' into the is_deeply comparison.  I've checked in SIP and the message is silently ignored further down the call chain.

Second follow-up fixes a logical issue that was highlighted by the new test that was introduced since I wrote the code.. nice catch :)
Comment 48 Martin Renvoize 2021-02-18 09:30:10 UTC
(In reply to Jonathan Druart from comment #46)
> 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?

Internally '->find' theoretically checks for constraints (Unique, Primary Key, etc).. if it doesn't find any, it falls back to search and throws an error if more than one row is found.. So in reality one would need to pass the order_by and rows attributes to get the correct single row.. So in effect they're the code results in the same thing...  

As for adding a unique constraint at the DB... I can't see what constraint would work here...  We're looking for the first with 'datesent' OR if there isn't a sent one, then the oldest 'daterequested'..
Comment 49 Martin Renvoize 2021-02-18 10:47:53 UTC
(In reply to Jonathan Druart from comment #46)
> 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.

I've come up with the followup alternatives.. but I'm struggling for the 'Limit' case.

::Found   -> ::InQueue
::Transit -> ::InTransit
::Out     -> ::OnLoan
::Limit   -> ? # This is when the branch transfer limits would ordinarily prevent transfer.. ::BreaksLimits, ::NotAllowed, ::Dissallowed ?
Comment 50 Martin Renvoize 2021-02-18 10:51:12 UTC
(In reply to Jonathan Druart from comment #45)
> Shouldn't we start with a good script/template name?
> transferstosend.pl => transfers_to_send.pl

More than happy for that change.. I was just being consistent with the existing 'transferstoreceive.pl' naming, but certainly, prefer snake_case.
Comment 51 Martin Renvoize 2021-02-18 10:58:41 UTC
(In reply to Jonathan Druart from comment #44)
> Is this store call needed?
> 
> +            $self->stage_id($new_stage->stage_id)->store;        # Revert
> stage change
> 
> There are also several places in the tests where ->store calls are
> unnecessary.

Corrected for the 'advance' method in StockRotationItem.. looks like there's a few places in SR this could be done.. mostly these predate this tree but I'm happy to fix them inline as followups to 24446 if you prefer.
Comment 52 Martin Renvoize 2021-02-18 10:58:57 UTC
Updated the branch attached to the merge request here: https://gitlab.com/mrenvoize/Koha/-/merge_requests/1
Comment 53 Martin Renvoize 2021-02-18 15:06:39 UTC
(In reply to Jonathan Druart from comment #46)
> 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.

Done in followup on branch

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

Done in followup on branch

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

Not sure I'm seeing the same thing you are.. need a bit of clarification

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

Struggling to find an example.. happy to change but struggling to understand the minute
Comment 54 Jonathan Druart 2021-02-18 15:14:33 UTC
(In reply to Martin Renvoize from comment #53)
> (In reply to Jonathan Druart from comment #46)

> > 7. There are some indentation inconsistencies in circ/transferstosend.tt
> 
> Not sure I'm seeing the same thing you are.. need a bit of clarification

 69                                     </tr>
 70                                     [% END %]
 71                                     [% END %]
 72                                 </tbody>
 73                             </table>
 74                             [% END %]
 75                         [% END %]
 76                         </div>
 77                         [% ELSE %]
 78                         <p>No transfers to send</p>
 79                         [% END %]


> > 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 
> > }
> 
> Struggling to find an example.. happy to change but struggling to understand
> the minute

% git grep 'push @messages'
will return a lot of examples :)
Comment 55 Martin Renvoize 2021-02-18 15:28:03 UTC
Thanks for clarifying.. couldn't see the wood for the trees..

Followups pushed to branch to deal with both issues :)
Comment 56 Jonathan Druart 2021-03-03 14:33:59 UTC
Created attachment 117612 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer but not
marked as sent.

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appear in the new 'Transfers to send'
   page.
4) Confirm that items marked with 'StockRotationAdvance' provide the
   option to mark the item as 'In demand' (Which will cancel the
   transfer and mark the item as in demand such that it will wait at the
   current branch for an additional stage period before being
   automatically picke for transfer again)
5) Setup a rotating collection
6) Trigger a collection transfer for the above collection
7) Check that the expected items appear in the new 'Transfers to send'
   page'.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>

Bug 22569: (QA follow-up) Rename script to snake_case

As requested, we simply rename the script, and references to it, to use
snake_case to make it clearer.

Bug 22569: (QA follow-up) Rename branchesloop to libraries

Bug 22569: (QA follow-up) Fix indentation in template
Comment 57 Jonathan Druart 2021-03-03 14:34:03 UTC
Created attachment 117613 [details] [review]
Bug 22569: Add Unit Tests

Test plan:
1/ Run the updated unit tests

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>
Comment 58 Katrin Fischer 2021-03-05 12:15:03 UTC
1) I notice that the reason in the table appears to be taken directly from the database. "AdvanceStockRotation" is not easy to understand untranslated. Can you please deal with this in the template?
Comment 59 Katrin Fischer 2021-03-05 12:19:20 UTC
2) Please also show the item type description instead of the code to keep consistent with our other displays:
+                                                    [% IF ( transfer.item.effective_itemtype ) %] (<b>[% transfer.item.effective_itemtype | html %]</b>)[% END %]

Otherwise this appears to work very well and I feel it's close to PQA. :)
Comment 60 Martin Renvoize 2021-03-05 12:30:17 UTC
Created attachment 117840 [details] [review]
Bug 22569: (QA follow-up) Make 'reason' translatable
Comment 61 Martin Renvoize 2021-03-05 12:34:07 UTC
Created attachment 117841 [details] [review]
Bug 22569: (QA follow-up) Use itemtype description
Comment 62 Martin Renvoize 2021-03-05 12:35:32 UTC
Created attachment 117842 [details] [review]
Bug 22569: (QA follow-up) Make 'reason' translatable
Comment 63 Martin Renvoize 2021-03-05 12:35:35 UTC
Created attachment 117843 [details] [review]
Bug 22569: (QA follow-up) Use itemtype description
Comment 64 Katrin Fischer 2021-03-05 12:40:10 UTC
I think you fell of IRC, so asking here: I am happy with the translations patch now, but can you explain how Hold lost can come to be? If I mark an item lost... how can it be in transfer?
Comment 65 Martin Renvoize 2021-03-05 13:15:26 UTC
Created attachment 117847 [details] [review]
Bug 22569: (QA follow-up) Make 'reason' translatable

Add a transfer_reasons include file for easy translation of the transer
reason codes.
Comment 66 Martin Renvoize 2021-03-05 13:15:30 UTC
Created attachment 117848 [details] [review]
Bug 22569: (QA follow-up) Use itemtype description
Comment 67 Katrin Fischer 2021-03-06 12:05:37 UTC
Created attachment 117878 [details] [review]
Bug 22569: Add a 'Transfers to send' report

This patch adds a 'Transfers to send' report to the circulation page
which displays items which have been triggered for transfer but not
marked as sent.

Test plan
1) Setup a rotation plan and add some items to it as per the
   manual
2) Run the stockrotation cronjob with the --execute flag
3) Check that the expected items appear in the new 'Transfers to send'
   page.
4) Confirm that items marked with 'StockRotationAdvance' provide the
   option to mark the item as 'In demand' (Which will cancel the
   transfer and mark the item as in demand such that it will wait at the
   current branch for an additional stage period before being
   automatically picke for transfer again)
5) Setup a rotating collection
6) Trigger a collection transfer for the above collection
7) Check that the expected items appear in the new 'Transfers to send'
   page'.

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>

Bug 22569: (QA follow-up) Rename script to snake_case

As requested, we simply rename the script, and references to it, to use
snake_case to make it clearer.

Bug 22569: (QA follow-up) Rename branchesloop to libraries

Bug 22569: (QA follow-up) Fix indentation in template

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 68 Katrin Fischer 2021-03-06 12:05:42 UTC
Created attachment 117879 [details] [review]
Bug 22569: Add Unit Tests

Test plan:
1/ Run the updated unit tests

Signed-off-by: Kathleen Milne <kathleen.milne@cne-siar.gov.uk>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 69 Katrin Fischer 2021-03-06 12:05:47 UTC
Created attachment 117880 [details] [review]
Bug 22569: (QA follow-up) Make 'reason' translatable

Add a transfer_reasons include file for easy translation of the transer
reason codes.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 70 Katrin Fischer 2021-03-06 12:05:51 UTC
Created attachment 117881 [details] [review]
Bug 22569: (QA follow-up) Use itemtype description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 71 Katrin Fischer 2021-03-06 12:06:28 UTC
I think we should use this report to advertise on the hard work that has gone into cleaning up the transfers code that has led up to here :)
Comment 72 Jonathan Druart 2021-03-11 15:37:41 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 73 Fridolin Somers 2021-03-19 14:22:56 UTC
New feature not pushed to 20.11.x
Comment 74 Mathieu Saby 2023-01-12 13:16:12 UTC
Hi

I don't find a description of this feature in Koha Manual. Is it described somewhere?
Comment 75 Katrin Fischer 2023-01-12 17:09:09 UTC
(In reply to mathieu saby from comment #74)
> Hi
> 
> I don't find a description of this feature in Koha Manual. Is it described
> somewhere?

It still has the keyword Manual, so it might not have been documented yet. The team removes it once that has been done.

The page is relate to the stock rotation tool that is described. I think before the page was added you got an email to alert you about items that needed progressing, but now the report can be used as well.

Martin might be able to explain more.
Comment 76 Marcel de Rooy 2023-11-23 08:56:05 UTC
I found this report after breaking my head about how to add transfers without a date sent ;)
Comment 77 Marcel de Rooy 2023-11-23 09:13:35 UTC
(In reply to Marcel de Rooy from comment #76)
> I found this report after breaking my head about how to add transfers
> without a date sent ;)

Added two reports to lower possible confusion. Bug 35388 and bug 35389.
Comment 78 Philip Orr 2024-03-28 12:56:23 UTC
Assigning myself to documenting this as I was recently confused by it :)