Bug 24839 - branchtransfers does not deal with holds
Summary: branchtransfers does not deal with holds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 24620
  Show dependency treegraph
 
Reported: 2020-03-10 13:37 UTC by Nick Clemens
Modified: 2020-11-30 21:46 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:
20.05.00, 19.11.05


Attachments
Bug 24839: Return hold info to transfer script and set borrowernumber (1.19 KB, patch)
2020-03-10 13:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24839: unit tests (2.94 KB, patch)
2020-03-10 13:59 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24839: Return hold info to transfer script and set borrowernumber (1.44 KB, patch)
2020-03-10 14:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24839: unit tests (2.98 KB, patch)
2020-03-10 19:56 UTC, David Nind
Details | Diff | Splinter Review
Bug 24839: Return hold info to transfer script and set borrowernumber (1.49 KB, patch)
2020-03-10 19:57 UTC, David Nind
Details | Diff | Splinter Review
Bug 24839: unit tests (3.04 KB, patch)
2020-03-11 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24839: Return hold info to transfer script and set borrowernumber (1.56 KB, patch)
2020-03-11 11:24 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24839: (QA follow-up) Remove unused variables (981 bytes, patch)
2020-03-11 11:28 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-03-10 13:37:14 UTC
This code looks to have been broken for some time, we check reserves, but we don't pass the variable back to the script
Comment 1 Nick Clemens 2020-03-10 13:59:45 UTC
Created attachment 100459 [details] [review]
Bug 24839: Return hold info to transfer script and set borrowernumber
Comment 2 Nick Clemens 2020-03-10 13:59:48 UTC
Created attachment 100460 [details] [review]
Bug 24839: unit tests
Comment 3 Nick Clemens 2020-03-10 14:02:38 UTC
Created attachment 100461 [details] [review]
Bug 24839: Return hold info to transfer script and set borrowernumber

To test:
1 - Place a hold on an item
2 - Go to Circulation->Transfers
3 - Attempt to transfer item to a branch it is not expected at
4 - No warning
5 - Apply patches
6 - Repeate
7 - You get a notice that there is a hold and must deal with the hold (or ignore)
Comment 4 David Nind 2020-03-10 19:56:59 UTC
Created attachment 100481 [details] [review]
Bug 24839: unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 David Nind 2020-03-10 19:57:03 UTC
Created attachment 100482 [details] [review]
Bug 24839: Return hold info to transfer script and set borrowernumber

To test:
1 - Place a hold on an item
2 - Go to Circulation->Transfers
3 - Attempt to transfer item to a branch it is not expected at
4 - No warning
5 - Apply patches
6 - Repeate
7 - You get a notice that there is a hold and must deal with the hold (or ignore)

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 Martin Renvoize 2020-03-11 11:23:13 UTC
transferbook is also called in rotating collections.. should the return value be used there too?
Comment 7 Martin Renvoize 2020-03-11 11:24:32 UTC
Created attachment 100542 [details] [review]
Bug 24839: unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2020-03-11 11:24:35 UTC
Created attachment 100543 [details] [review]
Bug 24839: Return hold info to transfer script and set borrowernumber

To test:
1 - Place a hold on an item
2 - Go to Circulation->Transfers
3 - Attempt to transfer item to a branch it is not expected at
4 - No warning
5 - Apply patches
6 - Repeate
7 - You get a notice that there is a hold and must deal with the hold (or ignore)

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-03-11 11:26:04 UTC
(In reply to Martin Renvoize from comment #6)
> transferbook is also called in rotating collections.. should the return
> value be used there too?

I think this can be handled in a follow-up bug report as currently Rotating Collections just ignores the response from transfer book entirely.

At some point soon I'd really like to actually see transferbook removed and the Koha Objects alternative used.. but that's down the line..

Passing QA
Comment 10 Martin Renvoize 2020-03-11 11:28:46 UTC
Created attachment 100544 [details] [review]
Bug 24839: (QA follow-up) Remove unused variables

Whilst QAing I spotted a couple of unused variables in the test.. no
harm in removing them ;)

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-03-11 13:49:58 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 12 Joy Nelson 2020-04-02 20:39:29 UTC
Backported to 19.11.x for 19.11.05
Comment 13 Lucas Gass 2020-04-10 15:32:31 UTC
tests would need to be rebased if this is needed in 19.05.x