Bug 8004 - Items on hold lose transfer after being scanned twice
Summary: Items on hold lose transfer after being scanned twice
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on: 10626
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-24 15:43 UTC by Kyle M Hall
Modified: 2014-05-26 21:04 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8004 - Items on Hold Lose Transfer After Being Scanned Twice (5.37 KB, patch)
2012-04-24 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8004 - Followup - Rename KohaBranchName filter to BranchName (2.68 KB, patch)
2012-11-19 20:15 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8004 - Followup - Rename KohaBranchName filter to BranchName (2.86 KB, patch)
2012-11-19 20:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8004 - Followup - Rename KohaBranchName filter to BranchName (3.07 KB, patch)
2013-05-02 18:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8004 - Followup - Rename KohaBranchName filter to BranchName (3.17 KB, patch)
2013-08-23 14:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 8004 - Followup [follow-up] Rename KohaBranchName filter to BranchName (13.94 KB, patch)
2013-08-23 14:58 UTC, Owen Leonard
Details | Diff | Splinter Review
Signed off patch (16.19 KB, patch)
2013-08-28 03:12 UTC, Campbell Reid-Tait
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2012-04-24 15:43:06 UTC
When a hold is placed on an item where the pickup location is different than the holding library, Koha initiates a branch transfer for that item to fill that hold when items is run through the returns system.

If the item is then run through returns a second time, the system is supposed to close that transfer as a 'wrongtransfer', and open a new transfer with the same from and to branches as the original.

The problem is that the original transfer is closed, but the new transfer is not created. This is because at some point, someone replaced the template variable WrongTransfer, which had previously contained the branchcode for the library to transfer to, with the full name of the library instead ( I assume to make a look nicer ).

The consequence of this, is the the name of the library is passed to updateWrongTransfer instead of the branchcode, causing the failure.
Comment 1 Kyle M Hall 2012-04-24 15:55:54 UTC Comment hidden (obsolete)
Comment 2 Melissa Geist 2012-06-05 16:25:28 UTC
Test with a sandbox. Works as advertised.
Comment 3 Paul Poulain 2012-06-09 17:13:35 UTC
QA comment:

 * Kyle, what is fun is the answer to your question:
> This is because at some point, someone replaced the template
> variable WrongTransfer, which had previously contained the branchcode for
> the library to transfer to, with the full name of the library instead ( I
> assume to make a look nicer ).
git blame circ/returns.pl, says:
74f6e35c (Kyle M Hall          2010-04-01 15:45:25 +0000 340)     $messages->{'WrongTransfer'} = GetBranchName( $messa...

:D :D (and, incenditally, it's 2 years old !)

However, this is a tiny patch, that adds a new plugin. After the presentation chris made today about T::T plugins/views/..., i'm wondering if creating a plugin for that is the best option we have. Shouldn't it be a view ?

Plus, if I push this patch, it means we should get rid of any branchname display everywhere, remove LEFT JOIN branches USING(branchname) in many SQL queries, cache GetBranchName to avoid useless SQL queries (branches are not updated frequently, they are a good candidate for caching).
I'm not against this idea (in fact, I like it quite a lot), but we must advertise it as much as possible, and if you can promize you'll take care of patches cleaning branchname dependancies, that would be a good + ;-)

I won't push the patch immediately, I suggest we speak of it tomorrow with other hackers, and chris_c first, as he is experienced on T::T
Comment 4 Kyle M Hall 2012-06-11 07:30:42 UTC
I guess I need to yell at myself from two years ago ; )

I don't think this is a good case for a view, considering the simplicity of the action. We can definitely discuss it though. I do think we could indeed get rid of perl code that gets the branchname and replace it with this filter, it will make code simpler and more understandable. It seems more correct to me, considering it is a matter of presentation, which is the job of the templates.

(In reply to comment #3)
> QA comment:
> 
>  * Kyle, what is fun is the answer to your question:
> > This is because at some point, someone replaced the template
> > variable WrongTransfer, which had previously contained the branchcode for
> > the library to transfer to, with the full name of the library instead ( I
> > assume to make a look nicer ).
> git blame circ/returns.pl, says:
> 74f6e35c (Kyle M Hall          2010-04-01 15:45:25 +0000 340)    
> $messages->{'WrongTransfer'} = GetBranchName( $messa...
> 
> :D :D (and, incenditally, it's 2 years old !)
> 
> However, this is a tiny patch, that adds a new plugin. After the
> presentation chris made today about T::T plugins/views/..., i'm wondering if
> creating a plugin for that is the best option we have. Shouldn't it be a
> view ?
> 
> Plus, if I push this patch, it means we should get rid of any branchname
> display everywhere, remove LEFT JOIN branches USING(branchname) in many SQL
> queries, cache GetBranchName to avoid useless SQL queries (branches are not
> updated frequently, they are a good candidate for caching).
> I'm not against this idea (in fact, I like it quite a lot), but we must
> advertise it as much as possible, and if you can promize you'll take care of
> patches cleaning branchname dependancies, that would be a good + ;-)
> 
> I won't push the patch immediately, I suggest we speak of it tomorrow with
> other hackers, and chris_c first, as he is experienced on T::T
Comment 5 Kyle M Hall 2012-06-11 10:48:22 UTC
Spoke to Chris about his, he also feels that the filter is the right way to go.
Comment 6 Paul Poulain 2012-06-11 14:04:10 UTC
Patch pushed.

thinking of it Kyle, I think naming it KohaBranchName.pm could be improved to Branchname.pm, because it's all about Koha, so... if you want to change the name in the follow-ups you'll do...
Comment 7 Kyle M Hall 2012-11-19 20:15:32 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2012-11-19 20:23:19 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2013-03-19 17:06:05 UTC
Please add a commit message with a test plan.
Comment 10 Kyle M Hall 2013-05-02 18:39:23 UTC Comment hidden (obsolete)
Comment 11 Owen Leonard 2013-08-23 14:58:33 UTC Comment hidden (obsolete)
Comment 12 Owen Leonard 2013-08-23 14:58:46 UTC Comment hidden (obsolete)
Comment 13 Campbell Reid-Tait 2013-08-28 03:12:05 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2013-09-22 20:50:29 UTC
Kyle, I am afraid this conflicts with 10626 - could you check please?
Comment 15 Kyle M Hall 2013-10-24 15:33:45 UTC
I'm a but confused about the state of this bug. The initial patch is in master. Do we still need these followups?
Comment 16 Kyle M Hall 2013-10-25 15:27:46 UTC
Initial patch was pushed to master. These followups are covered by bug 10626, and thus are not needed.