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.
Created attachment 9289 [details] [review] Bug 8004 - Items on Hold Lose Transfer After Being Scanned Twice 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 ). Solved by removing the line that was changing the variable contents from the branchcode to the branch name, and adding a new TT filter, KohaBranchName that functions in the same way as KohaDates, except it takes a branchcode and returns the branch name for the given branchcode. The consequence of this, is the the name of the library is passed to updateWrongTransfer instead of the branchcode, causing the failure.
Test with a sandbox. Works as advertised.
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
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
Spoke to Chris about his, he also feels that the filter is the right way to go.
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...
Created attachment 13536 [details] [review] Bug 8004 - Followup - Rename KohaBranchName filter to BranchName
Created attachment 13537 [details] [review] Bug 8004 - Followup - Rename KohaBranchName filter to BranchName
Please add a commit message with a test plan.
Created attachment 17911 [details] [review] Bug 8004 - Followup - Rename KohaBranchName filter to BranchName This patch renames the TT filter KohaBranchName to just BranchName, as the Koha is superfluous. Test Plan: 1) Apply patch 2) Attempt to transfer an item to the incorrect branch, you should see the name of the branch in the "please return to" message.
Created attachment 20582 [details] [review] [SIGNED-OFF] Bug 8004 - Followup - Rename KohaBranchName filter to BranchName This patch renames the TT filter KohaBranchName to just BranchName, as the Koha is superfluous. Test Plan: 1) Apply patch 2) Attempt to transfer an item to the incorrect branch, you should see the name of the branch in the "please return to" message. Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 20583 [details] [review] Bug 8004 - Followup [follow-up] Rename KohaBranchName filter to BranchName Since the original follow-up to 8004 was submitted the KohaBranchName filter was put to use in several other templates and a test was added. This patch corrects the filter name in the templates and changes the name of, and in, the test. To test: - Enable IndependentBranches and log in to Koha as a non-superlibrarian. Try to check out an item which belongs to another library. The library name should appear correctly in the warning message. - In Acquisitions, view the list of basket groups for any vendor. The "billing place" and "delivery place" columns should show library names correctly. - View the detail page for a subscription (serials/subscription-detail.pl?subscriptionid=X). The library name should appear correctly. - In suggestions management, view the details for a suggestion (/suggestion/suggestion.pl?suggestionid=X&op=show). Library names should appear correctly in the "suggestions management" table and in the "acquisition information" block. - Test the Koha_template_plugin_BranchName.t test: In a test database that has the sample branchesloaded, run prove -v t/db_dependent/Koha_template_plugin_BranchName.t
Created attachment 20682 [details] [review] Signed off patch
Kyle, I am afraid this conflicts with 10626 - could you check please?
I'm a but confused about the state of this bug. The initial patch is in master. Do we still need these followups?
Initial patch was pushed to master. These followups are covered by bug 10626, and thus are not needed.