Bug 28824 - Errant variable declaration in C4::Letters
Summary: Errant variable declaration in C4::Letters
Status: RESOLVED DUPLICATE of bug 31587
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 26935
Blocks:
  Show dependency treegraph
 
Reported: 2021-08-06 13:49 UTC by Martin Renvoize
Modified: 2022-10-17 14:36 UTC (History)
2 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 28824: Unit test (2.89 KB, patch)
2021-08-06 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28824: Fix for variable declaration scope (724 bytes, patch)
2021-08-06 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-08-06 13:49:22 UTC
The fix for bug 26935 actually broke the feature introduced with bug 21898... The fix is simple, drop the 'my'.
Comment 1 Martin Renvoize 2021-08-06 14:08:48 UTC
Created attachment 123579 [details] [review]
Bug 28824: Unit test
Comment 2 Martin Renvoize 2021-08-06 14:08:51 UTC
Created attachment 123580 [details] [review]
Bug 28824: Fix for variable declaration scope
Comment 3 Martin Renvoize 2021-08-06 14:10:17 UTC
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.
Comment 4 Marcel de Rooy 2021-08-10 11:16:58 UTC
QA: Looking here now
Comment 5 Marcel de Rooy 2021-08-10 11:29:44 UTC
(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 ;)
Comment 6 Marcel de Rooy 2021-08-10 11:46:31 UTC
And this one might change the game under Plack:

unless ($sth = C4::Context->dbh->prepare($query)) {
Comment 7 Marcel de Rooy 2021-08-10 12:02:53 UTC
Even when I do:

-                'aqbasket'      => $basketno,
+            #    'aqbasket'      => $basketno,

I still have the basket name in the test output?
Comment 8 Katrin Fischer 2021-08-10 12:08:41 UTC
(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...?
Comment 9 Katrin Fischer 2021-08-10 12:10:28 UTC
Sorry, probably totally off with the last comment - time for a break.
Comment 10 Marcel de Rooy 2021-08-10 12:16:43 UTC
(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
Comment 11 Jonathan Druart 2021-08-25 13:31:13 UTC
(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"?
Comment 12 Martin Renvoize 2022-10-11 11:53:24 UTC
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.
Comment 13 Marcel de Rooy 2022-10-13 07:08:28 UTC
(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
Comment 14 Katrin Fischer 2022-10-13 20:41:29 UTC
Could this be a duplicate to the just fixed bug 31587?
Comment 15 Jonathan Druart 2022-10-17 14:36:57 UTC

*** This bug has been marked as a duplicate of bug 31587 ***