Bug 24389 - Claiming an order can display an invalid successful message
Summary: Claiming an order can display an invalid successful message
Status: Pushed to oldoldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-01-09 17:01 UTC by Jonathan Druart
Modified: 2020-03-17 21:47 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
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 (1.78 KB, patch)
2020-01-09 17:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24389: Better error handle when claiming late orders (1.85 KB, patch)
2020-01-30 16:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24389: Better error handle when claiming late orders (1.99 KB, patch)
2020-02-17 11:46 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 Mapley 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 Mapley 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 Mapley 2020-03-17 21:47:48 UTC
Backported to 18.11.x for 18.11.15