Bug 24389

Summary: Claiming an order can display an invalid successful message
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: AcquisitionsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: 1joynelson, hayleypelham, lucas, martin.renvoize, nick
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.04, 19.05.09, 18.11.15
Attachments: Bug 24389: Better error handle when claiming late orders
Bug 24389: Better error handle when claiming late orders
Bug 24389: Better error handle when claiming late orders

Description Jonathan Druart 2020-01-09 17:01:25 UTC
If the vendor does not have a valid contact (like no email), a successful message is displayed ("Email has been sent.") whereas nothing was done!
Comment 1 Jonathan Druart 2020-01-09 17:06:38 UTC
Created attachment 97126 [details] [review]
Bug 24389: Better error handle when claiming late orders

A successful message says that the email has been sent and the order has
been claimed, but nothing happened.

Test plan:
0. Do not apply this patch
1. Create a new vendor, does not create a contact
2. Create some orders, close the basket
3. Go to the late orders page and claims some orders
=> You get a successful message, but the claim has not been done!
4. Apply this patch
5. Claim the orders
=> You get an error
6. Set a contact for the vendor
7. Claim the orders
=> Success

In the docker container, you may need to:
% apt install postfix
% postfix start
Edit /etc/hosts
127.0.0.1       localhost localhost.localdomain
Comment 2 Martin Renvoize 2020-01-30 16:16:22 UTC
Created attachment 98178 [details] [review]
Bug 24389: Better error handle when claiming late orders

A successful message says that the email has been sent and the order has
been claimed, but nothing happened.

Test plan:
0. Do not apply this patch
1. Create a new vendor, does not create a contact
2. Create some orders, close the basket
3. Go to the late orders page and claims some orders
=> You get a successful message, but the claim has not been done!
4. Apply this patch
5. Claim the orders
=> You get an error
6. Set a contact for the vendor
7. Claim the orders
=> Success

In the docker container, you may need to:
% apt install postfix
% postfix start
Edit /etc/hosts
127.0.0.1       localhost localhost.localdomain

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 3 Martin Renvoize 2020-01-30 16:16:44 UTC
Good improvement, Signing off.
Comment 4 Nick Clemens 2020-02-14 17:37:06 UTC
I never get a successful message when vendor has no contact
Comment 5 Jonathan Druart 2020-02-17 09:14:19 UTC
(In reply to Nick Clemens from comment #4)
> I never get a successful message when vendor has no contact

Ok, actually I get the problem when using reset_all: aqbooksellers.bookselleremail is set to an invalid email address. My assumption was incorrect, I thought it was because no contact was associated to the vendor.
Comment 6 Joonas Kylmälä 2020-02-17 11:46:03 UTC
Created attachment 99081 [details] [review]
Bug 24389: Better error handle when claiming late orders

A successful message says that the email has been sent and the order has
been claimed, but nothing happened.

Test plan:
0. Do not apply this patch
1. Create a new vendor, does not create a contact
2. Create some orders, close the basket
3. Go to the late orders page and claims some orders
=> You get a successful message, but the claim has not been done!
4. Apply this patch
5. Claim the orders
=> You get an error
6. Set a contact for the vendor
7. Claim the orders
=> Success

In the docker container, you may need to:
% apt install postfix
% postfix start
Edit /etc/hosts
127.0.0.1       localhost localhost.localdomain

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 7 Martin Renvoize 2020-02-17 13:48:44 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 8 Joy Nelson 2020-03-05 01:44:52 UTC
Pushed to 19.11.x for 19.11.04
Comment 9 Lucas Gass 2020-03-10 22:24:17 UTC
backported to 19.05.x for 19.05.09
Comment 10 Hayley Pelham 2020-03-16 02:54:03 UTC
When I follow the test plan backporting for 18.11.x, I get to step 3 (without having applied the patch) and the claim does not go through and I get a warning 'This vendor has no email'.

If I've missed something let me know, otherwise not backporting this one to 18.11.x
Comment 11 Jonathan Druart 2020-03-17 08:29:48 UTC
(In reply to Hayley Mapley from comment #10)
> When I follow the test plan backporting for 18.11.x, I get to step 3
> (without having applied the patch) and the claim does not go through and I
> get a warning 'This vendor has no email'.
> 
> If I've missed something let me know, otherwise not backporting this one to
> 18.11.x

Indeed, you must fill an invalid aqbooksellers.bookselleremail value, which is not used anywhere. I remember I got that error with the data generated by reset_all, maybe it does not appear in real life.
There is a case that is not taken into account anyway, and I feel like it's safer to have the patch. Do not backport it if you are not confident however.
Comment 12 Hayley Pelham 2020-03-17 20:38:49 UTC
Hi Jonathan,

Thank you for clarifying, it would be useful if this information had been updated in the test plan but as I'm the last contact it hardly matters now.

I will test again with that value inputted.
Comment 13 Hayley Pelham 2020-03-17 21:47:48 UTC
Backported to 18.11.x for 18.11.15