Bug 8004

Summary: Items on hold lose transfer after being scanned twice
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact:
Severity: normal    
Priority: P5 - low CC: campbellreidtait, gmcharlt, katrin.fischer, kyle, mgeist, paul.poulain
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 10626    
Bug Blocks:    
Attachments: Bug 8004 - Items on Hold Lose Transfer After Being Scanned Twice
Bug 8004 - Followup - Rename KohaBranchName filter to BranchName
Bug 8004 - Followup - Rename KohaBranchName filter to BranchName
Bug 8004 - Followup - Rename KohaBranchName filter to BranchName
[SIGNED-OFF] Bug 8004 - Followup - Rename KohaBranchName filter to BranchName
Bug 8004 - Followup [follow-up] Rename KohaBranchName filter to BranchName
Signed off patch

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.