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'
Nice idea, I'll try to get to this some time soon.
Created attachment 90458 [details] [review] Bug 22569: Proof of concept This patch adds a proof of concept 'Transfers to send' report to the circulation page.
Created attachment 90477 [details] [review] Bug 22569: Proof of concept This patch adds a proof of concept 'Transfers to send' report to the circulation page. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
This patch adds a 'Transfers to send' report to compliment the 'Transfers to receive' report. 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.
Created attachment 90486 [details] [review] Bug 22569: Proof of concept This patch adds a proof of concept 'Transfers to send' report to the circulation page. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90487 [details] [review] Bug 22569: Proof of concept This patch adds a proof of concept 'Transfers to send' report to the circulation page. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Work still to be done.. Unit tests.
Created attachment 94650 [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. 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. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 94651 [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>
Created attachment 94658 [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>
Created attachment 94659 [details] [review] Bug 22569: Add Unit Tests
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?
(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?
Created attachment 96471 [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>
Created attachment 96472 [details] [review] Bug 22569: Add Unit Tests
Created attachment 96473 [details] [review] Reverse transferstosend controller
Created attachment 96474 [details] [review] Add inbound_transfers to Koha::Library
Created attachment 96475 [details] [review] Use inbound_transfers in transferstosend template
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>
Created attachment 96479 [details] [review] Bug 22569: Add Unit Tests
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>
Created attachment 96517 [details] [review] Bug 22569: Add Unit Tests
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>
Created attachment 97393 [details] [review] Bug 22569: Add Unit Tests
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!
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>
Created attachment 107259 [details] [review] Bug 22569: Add Unit Tests
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.
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>
Created attachment 107307 [details] [review] Bug 22569: Add Unit Tests
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.
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>
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>
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>
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.
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>
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>
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>
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.
I'm looking for feedback for this tree on https://gitlab.com/mrenvoize/Koha/-/merge_requests/1
BLOCKED by dependent bug.
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>
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>
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.
Shouldn't we start with a good script/template name? transferstosend.pl => transfers_to_send.pl
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 }
(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 :)
(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'..
(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 ?
(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.
(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.
Updated the branch attached to the merge request here: https://gitlab.com/mrenvoize/Koha/-/merge_requests/1
(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
(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 :)
Thanks for clarifying.. couldn't see the wood for the trees.. Followups pushed to branch to deal with both issues :)
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
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>
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?
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. :)
Created attachment 117840 [details] [review] Bug 22569: (QA follow-up) Make 'reason' translatable
Created attachment 117841 [details] [review] Bug 22569: (QA follow-up) Use itemtype description
Created attachment 117842 [details] [review] Bug 22569: (QA follow-up) Make 'reason' translatable
Created attachment 117843 [details] [review] Bug 22569: (QA follow-up) Use itemtype description
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?
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.
Created attachment 117848 [details] [review] Bug 22569: (QA follow-up) Use itemtype description
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>
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>
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>
Created attachment 117881 [details] [review] Bug 22569: (QA follow-up) Use itemtype description Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
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 :)
Pushed to master for 21.05, thanks to everybody involved!
New feature not pushed to 20.11.x
Hi I don't find a description of this feature in Koha Manual. Is it described somewhere?
(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.
I found this report after breaking my head about how to add transfers without a date sent ;)
(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.
Assigning myself to documenting this as I was recently confused by it :)
Documented and now in the manual. Seeing as the test plan only relates to the stock rotation automation tool and the report is only shown when stock rotation automation tool is in use, I think the text to go in the release notes is misleading: "...lists all items that are set to transfer but not yet in transit" Well, it wouldn't list all items - only those in a stock rotation rota that are requested from your library. Correct me if I'm wrong or out of line here. Might be personal preference, but I also feel the report name itself is a little misleading as I would expect it to list all transfers, e.g. holds requested to be sent to a different library as well. Maybe something like "Stock rotation requests to send" would be better.