Bug 23695 - Items holdingbranch should be set to the originating library when generating a manual transfer
Summary: Items holdingbranch should be set to the originating library when generating ...
Status: Pushed to master
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: 23129
Blocks:
  Show dependency treegraph
 
Reported: 2019-09-30 09:49 UTC by Katrin Fischer
Modified: 2020-10-09 11:02 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
20.11.00


Attachments
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer (7.53 KB, patch)
2019-09-30 10:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Alter transferbook to take a hash of params and specify from_branch (14.28 KB, patch)
2020-02-14 13:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer (7.51 KB, patch)
2020-03-02 17:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Alter transferbook to take a hash of params and specify from_branch (8.55 KB, patch)
2020-03-02 17:22 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer (7.57 KB, patch)
2020-03-11 14:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23695: Alter transferbook to take a hash of params and specify from_branch (8.62 KB, patch)
2020-03-11 14:01 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer (7.74 KB, patch)
2020-03-23 12:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Alter transferbook to take a hash of params and specify from_branch (8.65 KB, patch)
2020-03-23 12:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: (follow-up) Adjust for addition of transfer trigger (9.46 KB, patch)
2020-03-23 12:51 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer (7.74 KB, patch)
2020-07-28 12:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Alter transferbook to take a hash of params and specify from_branch (7.30 KB, patch)
2020-07-28 12:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: (follow-up) Adjust for addition of transfer trigger (9.45 KB, patch)
2020-07-28 12:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer (7.80 KB, patch)
2020-08-13 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23695: Alter transferbook to take a hash of params and specify from_branch (7.34 KB, patch)
2020-08-13 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23695: (follow-up) Adjust for addition of transfer trigger (9.51 KB, patch)
2020-08-13 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23695: (QA follow-up) Fix Copy/Paste (1.64 KB, patch)
2020-08-13 13:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23695: (follow-up) Add exceptions for missing branch parameters (2.88 KB, patch)
2020-08-14 12:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 23695: (follow-up) Add exceptions for missing branch parameters (3.12 KB, patch)
2020-08-18 12:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23695: (QA follow-up) Revert 'Orginating library' option (3.30 KB, patch)
2020-10-09 10:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23695: (QA follow-up) Revert 'Orginating library' option (3.37 KB, patch)
2020-10-09 10:44 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2019-09-30 09:49:06 UTC
Very similar to bug 23129, just for manual tranfers from the circulation start page:

- Pick an item's barcode
- Go to circulation start page
- "Transfer"
- Transfer the item to another library
- Verify holding library is not updated to 'sending library'

Added difficulty:
- Pick an item from library A
- Switch to library B
- Initiate transfer from B to C
- Verify the item claims holding library A still 

So we don't get to see that the item was actually sent from B where it might have ended up accidentally.
Comment 1 Nick Clemens 2019-09-30 10:44:55 UTC Comment hidden (obsolete)
Comment 2 Martin Renvoize 2019-10-15 16:03:48 UTC
Comment on attachment 93231 [details] [review]
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer

Review of attachment 93231 [details] [review]:
-----------------------------------------------------------------

::: circ/branchtransfers.pl
@@ +127,5 @@
>  if ($barcode) {
>  
>      ( $transferred, $messages ) =
> +        transferbook({
> +            from_branch => C4::Context->userenv->{'branch'},

Hmm, not sure this is right.. you might be transferring from the branch you're logged in at.. but you may also be transferring from elsewhere to you logged in branch... I'd say we probably need to add a field to the form and default to the items current holdingbranch if the user doesn't explicitly state the items in their branch?
Comment 3 Nick Clemens 2020-02-14 13:46:09 UTC Comment hidden (obsolete)
Comment 4 Lisette Scheer 2020-02-14 18:58:01 UTC
Applying: Bug 23695: Set holding branch to transferring branch when manually initiating a transfer
Using index info to reconstruct a base tree...
M       C4/Circulation.pm
M       circ/branchtransfers.pl
M       t/db_dependent/Circulation.t
M       t/db_dependent/Koha/Items.t
Falling back to patching base and 3-way merge...
Auto-merging t/db_dependent/Koha/Items.t
Auto-merging t/db_dependent/Circulation.t
CONFLICT (content): Merge conflict in t/db_dependent/Circulation.t
Auto-merging circ/branchtransfers.pl
Auto-merging C4/Circulation.pm
error: Failed to merge in the changes.
Patch failed at 0001 Bug 23695: Set holding branch to transferring branch when manually initiating a transfer
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort"
Comment 5 Nick Clemens 2020-03-02 17:22:30 UTC
Created attachment 99955 [details] [review]
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer

To test:
 1 - Go to Circulation->Transfer
 2 - Note your signed in branch
 3 - Find an item from your branch and create a transfer to branch B
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to brnach B
 7 - Confirm the item is held at your current branch and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t
Comment 6 Nick Clemens 2020-03-02 17:22:33 UTC
Created attachment 99956 [details] [review]
Bug 23695: Alter transferbook to take a hash of params and specify from_branch

To test:
 1 - Go to Circulation->Transfer
 2 - Find an item from your branch and create a transfer to branch B
 3 - Note you can specify the origin and default is 'Item's holding library'
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to branch B from branch D
 7 - Confirm the item is held at branch D and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t
Comment 7 Owen Leonard 2020-03-11 14:01:23 UTC
Created attachment 100567 [details] [review]
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer

To test:
 1 - Go to Circulation->Transfer
 2 - Note your signed in branch
 3 - Find an item from your branch and create a transfer to branch B
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to brnach B
 7 - Confirm the item is held at your current branch and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 8 Owen Leonard 2020-03-11 14:01:26 UTC
Created attachment 100568 [details] [review]
Bug 23695: Alter transferbook to take a hash of params and specify from_branch

To test:
 1 - Go to Circulation->Transfer
 2 - Find an item from your branch and create a transfer to branch B
 3 - Note you can specify the origin and default is 'Item's holding library'
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to branch B from branch D
 7 - Confirm the item is held at branch D and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Nick Clemens 2020-03-23 12:51:50 UTC
Created attachment 101441 [details] [review]
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer

To test:
 1 - Go to Circulation->Transfer
 2 - Note your signed in branch
 3 - Find an item from your branch and create a transfer to branch B
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to brnach B
 7 - Confirm the item is held at your current branch and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Nick Clemens 2020-03-23 12:51:53 UTC
Created attachment 101442 [details] [review]
Bug 23695: Alter transferbook to take a hash of params and specify from_branch

To test:
 1 - Go to Circulation->Transfer
 2 - Find an item from your branch and create a transfer to branch B
 3 - Note you can specify the origin and default is 'Item's holding library'
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to branch B from branch D
 7 - Confirm the item is held at branch D and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Nick Clemens 2020-03-23 12:51:56 UTC
Created attachment 101443 [details] [review]
Bug 23695: (follow-up) Adjust for addition of transfer trigger
Comment 12 Katrin Fischer 2020-05-02 02:37:50 UTC
Hi Nick, I've switched this to be a bug, as bug 23129 was also considered one. Sadly it does on longer apply, can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23695: Set holding branch to transferring branch when manually initiating a transfer
Applying: Bug 23695: Alter transferbook to take a hash of params and specify from_branch
error: sha1 information is lacking or useless (circ/branchtransfers.pl).
error: could not build fake ancestor
Comment 13 Nick Clemens 2020-07-28 12:17:10 UTC
Created attachment 107463 [details] [review]
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer

To test:
 1 - Go to Circulation->Transfer
 2 - Note your signed in branch
 3 - Find an item from your branch and create a transfer to branch B
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to brnach B
 7 - Confirm the item is held at your current branch and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Nick Clemens 2020-07-28 12:17:13 UTC
Created attachment 107464 [details] [review]
Bug 23695: Alter transferbook to take a hash of params and specify from_branch

To test:
 1 - Go to Circulation->Transfer
 2 - Find an item from your branch and create a transfer to branch B
 3 - Note you can specify the origin and default is 'Item's holding library'
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to branch B from branch D
 7 - Confirm the item is held at branch D and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Nick Clemens 2020-07-28 12:17:16 UTC
Created attachment 107465 [details] [review]
Bug 23695: (follow-up) Adjust for addition of transfer trigger
Comment 16 Martin Renvoize 2020-08-13 13:37:22 UTC
Comment on attachment 107464 [details] [review]
Bug 23695: Alter transferbook to take a hash of params and specify from_branch

Review of attachment 107464 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/branchtransfers.tt
@@ +72,4 @@
>                              <input type="hidden" name="biblionumber" value="[% biblionumber | html %]" />
>                              <input type="hidden" name="borrowernumber" value="[% borrowernumber | html %]" />
>                              <input type="hidden" name="tobranchcd" value="[% tobranchcd | html %]" />
> +                            <input type="hidden" name="frombranchcd" value="[% tobranchcd | html %]" />

At first glance, the 'tobranch' value here seems incorrect..
Comment 17 Martin Renvoize 2020-08-13 13:45:01 UTC
Created attachment 108183 [details] [review]
Bug 23695: Set holding branch to transferring branch when manually initiating a transfer

To test:
 1 - Go to Circulation->Transfer
 2 - Note your signed in branch
 3 - Find an item from your branch and create a transfer to branch B
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to brnach B
 7 - Confirm the item is held at your current branch and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize 2020-08-13 13:45:05 UTC
Created attachment 108184 [details] [review]
Bug 23695: Alter transferbook to take a hash of params and specify from_branch

To test:
 1 - Go to Circulation->Transfer
 2 - Find an item from your branch and create a transfer to branch B
 3 - Note you can specify the origin and default is 'Item's holding library'
 4 - Confirm the item is marked as held at your current branch and is being transferred to B
 5 - Find an item from a third branch, branch C
 6 - Transfer that item to branch B from branch D
 7 - Confirm the item is held at branch D and is being transferred to B
 8 - prove -v t/db_dependent/Circulation.t
 9 - prove -v t/db_dependent/Koha/Items.t
10 - prove -v t/db_dependent/RotatingCollections.t

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2020-08-13 13:45:08 UTC
Created attachment 108185 [details] [review]
Bug 23695: (follow-up) Adjust for addition of transfer trigger

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2020-08-13 13:45:11 UTC
Created attachment 108186 [details] [review]
Bug 23695: (QA follow-up) Fix Copy/Paste

The value remained as 'tobranchcd' and needed to be updated to
'frombranchcd' to allow reservered transfers to be correctly set.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2020-08-13 13:45:43 UTC
After the QA fix, this all works well, is well tested and passes the QA scripts.

Passing QA
Comment 22 Jonathan Druart 2020-08-14 10:55:38 UTC
What about raising an exception if to_branch or from_branch are missing?
Comment 23 Nick Clemens 2020-08-14 12:58:05 UTC
Created attachment 108273 [details] [review]
Bug 23695: (follow-up) Add exceptions for missing branch parameters
Comment 24 Jonathan Druart 2020-08-18 12:51:42 UTC
Created attachment 108495 [details] [review]
Bug 23695: (follow-up) Add exceptions for missing branch parameters

JD: amended patch
Remove QA issues:
 FAIL   t/db_dependent/Circulation.t
   FAIL   valid
                "my" variable $doreturn masks earlier declaration in same scope
                "my" variable $messages masks earlier declaration in same scope
Comment 25 Jonathan Druart 2020-08-18 15:46:16 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 26 Lucas Gass 2020-09-04 14:24:51 UTC
patchset does not apply cleanly to 20.05.x, no backport

please rebase if needed
Comment 27 Martin Renvoize 2020-10-08 15:21:14 UTC
Hmmm.. I think there's something wrong here re-reading the code..

We appear to expose the option in the UI to pick the 'Originating branch', but we don't pass that through to transferbook and instead rely on the current users branch.
Comment 28 Martin Renvoize 2020-10-09 10:30:41 UTC
Created attachment 111391 [details] [review]
Bug 23695: (QA follow-up) Revert 'Orginating library' option

At my request, we added an 'Originating library' option to the manual
transfers form.

However, we did not properly pass this through in the controller and I
have since discovered that my request was ill conceived/a
misunderstanding of requirements.

This patch removes the option from the template and drops the references
to it in the controller.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2020-10-09 10:32:03 UTC
Followup patch added as discussed.. I've asked Nick to take a quick look as a final sanity check before pushing.
Comment 30 Nick Clemens 2020-10-09 10:44:20 UTC
Created attachment 111394 [details] [review]
Bug 23695: (QA follow-up) Revert 'Orginating library' option

At my request, we added an 'Originating library' option to the manual
transfers form.

However, we did not properly pass this through in the controller and I
have since discovered that my request was ill conceived/a
misunderstanding of requirements.

This patch removes the option from the template and drops the references
to it in the controller.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 31 Martin Renvoize 2020-10-09 10:57:01 UTC
Shifted back to the PQA queue for the final followup.. Thanks for verifying Nick.
Comment 32 Jonathan Druart 2020-10-09 11:02:24 UTC
Pushed to master for 20.11, thanks to everybody involved!