Bug 12505 - Make variable aqorders.listprice in acq claim notice work and improve error handling
Summary: Make variable aqorders.listprice in acq claim notice work and improve error h...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: simith.doliveira
QA Contact: Testopia
URL:
Keywords:
: 10618 (view as bug list)
Depends on:
Blocks:
 
Reported: 2014-06-30 18:04 UTC by simith.doliveira
Modified: 2015-06-04 23:33 UTC (History)
10 users (show)

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


Attachments
Bug 12505 - Variable aqorders.listprice from 'Tools' - 'Notices and Slips' Does not work (1.66 KB, patch)
2014-06-30 18:17 UTC, simith.doliveira
Details | Diff | Splinter Review
This patch removes the unnecessary join (1.97 KB, patch)
2014-08-20 19:40 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12505 - Variable aqorders.listprice from 'Tools' - 'Notices and Slips' (2.26 KB, patch)
2014-11-13 18:26 UTC, simith.doliveira
Details | Diff | Splinter Review
Bug 12505 31034: This patch removes the unnecessary join (2.09 KB, patch)
2014-11-14 10:34 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 12505 - Acq claim: Show error message when no order is selected (2.67 KB, patch)
2014-11-16 10:30 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12505 - Variable aqorders.listprice in acq claim notice doesn't work (2.16 KB, patch)
2014-11-16 10:30 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description simith.doliveira 2014-06-30 18:04:13 UTC
The variable aqorders.listprice shows aqbooksellers.listprice value instead.
Comment 1 simith.doliveira 2014-06-30 18:17:43 UTC Comment hidden (obsolete)
Comment 2 simith.doliveira 2014-07-02 12:58:17 UTC
*** Bug 10618 has been marked as a duplicate of this bug. ***
Comment 3 Srdjan Jankovic 2014-08-18 06:37:53 UTC
Well in that case the whole join is unnecessary and should be removed.
Comment 4 Chris Cormack 2014-08-18 08:16:36 UTC
I agree doing a join where we dont either return any of the data from the table, or use it in any condition doesn't make sense.
Comment 5 simith.doliveira 2014-08-20 19:40:56 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2014-11-04 06:49:12 UTC
It would be great if someone could test this!
Comment 7 Paola Rossi 2014-11-11 15:29:26 UTC
I've applied the patch against master 3.17.00.052 head 13113

I cloned the ACQCLAIM to ACQCLAIM-CPL to test the patch:
ACQCLAIM-CPL: ($<<aqorders.listprice>> -- <<aqbooksellers.listprice>> each) as required
ACQCLAIM: ($<<aqorders.listprice>> each) as by default

MASTER:
ACQCLAIM-CPL answers: ($USD -- USD each) WRONG
ACQCLAIM answers: ($USD each) WRONG

APPLIED:
ACQCLAIM-CPL answers: ($2000.000000 -- USD each) RIGHT as required
ACQCLAIM doesn't send any mail WRONG

So I pass the patch to "Failed QA" status.
Comment 8 simith.doliveira 2014-11-12 17:20:02 UTC
I followed the same test (master 3.17.00.054) as Paola executed: 
ACQCLAIM-CPL: ($<<aqorders.listprice>> -- <<aqbooksellers.listprice>> each) as required
ACQCLAIM: ($<<aqorders.listprice>> each) as by default

MASTER:
ACQCLAIM-CPL answers: ($USD -- USD each) WRONG
ACQCLAIM answers: ($USD each) WRONG

APPLIED:
ACQCLAIM-CPL answers: ($2000.000000 -- USD each) RIGHT as required

But: 

ACQCLAIM: ($2000.000000 each)

So, I pass to Needs Signoff
Comment 9 Paola Rossi 2014-11-13 10:46:57 UTC
Thanks, Simith.

Today I've applied again the patch, against master 3.17.00.054 HEAD 11634.

My tests agreed with the Simith's ones in comment 8. So:
2 notices:
ACQCLAIM-CPL: ($<<aqorders.listprice>> -- <<aqbooksellers.listprice>> each) as required
ACQCLAIM: ($<<aqorders.listprice>> each) as by default

On ACQ, on lateorders.pl:
MASTER:
ACQCLAIM-CPL answers: ($USD -- USD each) WRONG
ACQCLAIM answers: ($USD each) WRONG

APPLIED:
ACQCLAIM-CPL answers: ($2000.000000 -- USD each) RIGHT as required
ACQCLAIM: ($2000.000000 each) RIGHT

When I didn't check any order in late, and select "Claim order", koha answered:

Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054. 

I think this is an error.
So I pass the patch to "Failed QA" status.
Comment 10 simith.doliveira 2014-11-13 18:26:45 UTC Comment hidden (obsolete)
Comment 11 simith.doliveira 2014-11-13 18:37:25 UTC
The problem described in the comment 9:

Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2054. 

I could verify even before applying the patch, but I corrected it in the second patch
Comment 12 Paola Rossi 2014-11-14 10:26:44 UTC
(In reply to simith.doliveira from comment #11)
> The problem described in the comment 9:
> 
> Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm
> line 2054. 
> 
> I could verify even before applying the patch, but I corrected it in the
> second patch

Kind Simith, I'm very sorry, the error is mine.

I should not have considered that behaviour in this bug, I beg your pardon.
Comment 13 Paola Rossi 2014-11-14 10:34:31 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2014-11-16 10:24:14 UTC
Hi Simith and Paola,

thank you for working on this!

The behaviour on clicking "Claim order" without any order selected has been improved to display a proper error message - this is nice and should be reflected in the commit messages - no need to hide enhancements :)

Can't use an undefined value as an ARRAY reference at /usr/lib/perl5/DBI.pm line 2064. 

turned to:

No order selected
Comment 15 Katrin Fischer 2014-11-16 10:30:42 UTC
Created attachment 33602 [details] [review]
[PASSED QA] Bug 12505 - Acq claim: Show error message when no order is selected

If no order is selected on the acq claim page when clicking
'Claim order' an ugly perl error message is displayed.

This patch corrects the behaviour to display a human readable
'No order selected'
instead.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Reworded commit message to reflect what the patch achieves.
Works as described and passes tests.
Comment 16 Katrin Fischer 2014-11-16 10:30:56 UTC
Created attachment 33603 [details] [review]
[PASSED QA] Bug 12505 - Variable aqorders.listprice in acq claim notice doesn't work

Modified:

C4/Letters.pm               - remove aqbooksellers.* from SELECT statement

In Letters - SendAlerts subrotine, is safe to remove aqbooksellers.* from SELECT statement
for type=claimacquisition or claimissues. Aqbooksellers is passed to GetPreparedLetter subrotine in tables variable.

Testing:

I Apply the patch

Select Tools -> Notices and slips;
Edit ACQCLAIM;
Add :
<order>Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered) ($<<aqorders.listprice>>  <<aqbooksellers.listprice>> each) has not been received.</order>
Save modifications;
Create a vendor (Acquisition module);
Create an order (Acquisition module);
Click Acquisitions -> Late orders;
Select the order created;
Click Claim order button;
Valide <<aqorders.listprice>>;
Valide <<aqbooksellers.listprice>>.

Signed-off-by: Paola Rossi <paola.rossi@cineca.it>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Works as described. It's now possible to output the actual price
in the claim notice.
Comment 17 Katrin Fischer 2014-11-16 10:32:14 UTC
Simith, I reworded the commit messages to make the intention more clear - I hope my changes are ok for you.
Comment 18 simith.doliveira 2014-11-17 13:19:33 UTC
Ok for me. Thanks
Comment 19 Tomás Cohen Arazi 2014-11-20 00:50:50 UTC
Patch pushed to master.

Thanks Simith!
Comment 20 Jonathan Druart 2014-11-21 16:32:44 UTC
I did not see this patch, sorry.
Is it still possible to select info from the aqbooksellers table in the claim notice?
Comment 21 Katrin Fischer 2014-11-21 16:56:23 UTC
I checked and the bookseller name was still printed into my notice - I was wondering too. If I didn't test it wrong, it should not cause any harm.
Comment 22 Jonathan Druart 2014-11-24 12:12:14 UTC
(In reply to Katrin Fischer from comment #21)
> I checked and the bookseller name was still printed into my notice - I was
> wondering too. If I didn't test it wrong, it should not cause any harm.

Just reading the code, I suspect a regression:
The fields from the aqbooksellers table won't be correctly replaced.
"name" is replaced because it's a field in the aqcontact table.

I don't have the time to test this.
Comment 23 Katrin Fischer 2014-11-24 12:13:52 UTC
I can try to retest - but it was "Katrin's bookshop" - so the vendor name that appeared in the notice if I remember correctly. So hopefully no regression, but will try to verify later today.
Comment 24 Katrin Fischer 2014-11-24 12:15:02 UTC
Or maybe someone else can get to it before?
Comment 25 Katrin Fischer 2014-11-24 22:02:18 UTC
I tested again and have definitely fields from aqbooksellers showing up in my notice. I tested with postal address and phone.
Comment 26 Katrin Fischer 2014-11-24 22:03:43 UTC
<<aqbooksellers.name>>
<<aqbooksellers.postal>>
<<aqbooksellers.phone>>


<order>
Ordernumber <<aqorders.ordernumber>>
(<<biblio.title>>) 
(<<aqorders.quantity>> ordered) ($<<aqorders.listprice>> each) 
has not been received.
</order>