The fix for bug 26935 actually broke the feature introduced with bug 21898... The fix is simple, drop the 'my'.
Created attachment 123579 [details] [review] Bug 28824: Unit test
Created attachment 123580 [details] [review] Bug 28824: Fix for variable declaration scope
I'm confused.. I can't seem to get the unit test to fail.. but in real life testing it does fail without the fix.
QA: Looking here now
(In reply to Martin Renvoize from comment #3) > I'm confused.. I can't seem to get the unit test to fail.. but in real life > testing it does fail without the fix. The assignment on L413 should affect the call of GetPreparedLetter on L467. But only when the notice actually uses data from aqbasket. It seems to me that we should assign $basketno = $externalid right at the start of the else block (L366) ? Using one variable for different types of id's appears to be a bad idea ;)
And this one might change the game under Plack: unless ($sth = C4::Context->dbh->prepare($query)) {
Even when I do: - 'aqbasket' => $basketno, + # 'aqbasket' => $basketno, I still have the basket name in the test output?
(In reply to Marcel de Rooy from comment #7) > Even when I do: > > - 'aqbasket' => $basketno, > + # 'aqbasket' => $basketno, > > I still have the basket name in the test output? I had a library claim once the wrong notice would be sent, if the basket name was the same... now I wonder...?
Sorry, probably totally off with the last comment - time for a break.
(In reply to Marcel de Rooy from comment #7) > Even when I do: > > - 'aqbasket' => $basketno, > + # 'aqbasket' => $basketno, > > I still have the basket name in the test output? Debugging Letters :) The info is in the $repeat and is filled in when reaching the repeat no enclosing tags block. my vendor|John Smith|<<aqbasket.basketname>>|<order>Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered)</order>my vendor|J ohn Smith|Named basket|<order>Ordernumber 114 (Silence in the library) (1 ordered)</order> In that block basketname is replaced. And not via $tables ! This essentially means that the second patch does nothing for the orderacquistion notice. The unit test shows that you can fill basketname via $repeat but actually has no real added value. FQA
(In reply to Martin Renvoize from comment #3) > I'm confused.. I can't seem to get the unit test to fail.. but in real life > testing it does fail without the fix. Can you detail how you get the problem "in real life testing"?
Hit by this again at our most recent upgrade... If you want to use aqbasket in the title of the email without this patch you can't.
(In reply to Martin Renvoize from comment #12) > Hit by this again at our most recent upgrade... > > If you want to use aqbasket in the title of the email without this patch you > can't. Martin, status is still FQA. And patch does not apply: Bug 28824 - Errant variable declaration in C4::Letters 123579 - Bug 28824: Unit test 123580 - Bug 28824: Fix for variable declaration scope Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 28824: Unit test Using index info to reconstruct a base tree... M t/db_dependent/Letters.t Falling back to patching base and 3-way merge... Auto-merging t/db_dependent/Letters.t CONFLICT (content): Merge conflict in t/db_dependent/Letters.t error: Failed to merge in the changes. Patch failed at 0001 Bug 28824: Unit test
Could this be a duplicate to the just fixed bug 31587?
*** This bug has been marked as a duplicate of bug 31587 ***