The variable aqorders.listprice shows aqbooksellers.listprice value instead.
Created attachment 29382 [details] [review] Bug 12505 - Variable aqorders.listprice from 'Tools' - 'Notices and Slips' Does not 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>>.
*** Bug 10618 has been marked as a duplicate of this bug. ***
Well in that case the whole join is unnecessary and should be removed.
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.
Created attachment 31034 [details] [review] This patch removes the unnecessary join
It would be great if someone could test this!
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.
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
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.
Created attachment 33527 [details] [review] Bug 12505 - Variable aqorders.listprice from 'Tools' - 'Notices and Slips'
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
(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.
Created attachment 33562 [details] [review] Bug 12505 31034: This patch removes the unnecessary join I've applied the patch against master 3.17.00.054 head 11634 IMO the second patch "Variable aqorders.listprice from 'Tools' - 'Notices and Slips'" is not needed. I pass the patch "This patch removes the unnecessary join" to "Signed Off" status.
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
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.
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.
Simith, I reworded the commit messages to make the intention more clear - I hope my changes are ok for you.
Ok for me. Thanks
Patch pushed to master. Thanks Simith!
I did not see this patch, sorry. Is it still possible to select info from the aqbooksellers table in the claim notice?
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.
(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.
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.
Or maybe someone else can get to it before?
I tested again and have definitely fields from aqbooksellers showing up in my notice. I tested with postal address and phone.
<<aqbooksellers.name>> <<aqbooksellers.postal>> <<aqbooksellers.phone>> <order> Ordernumber <<aqorders.ordernumber>> (<<biblio.title>>) (<<aqorders.quantity>> ordered) ($<<aqorders.listprice>> each) has not been received. </order>