Bug 22908

Summary: Modsuggestion will generate a notice even if the modification failed
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: m.de.rooy, martin.renvoize, nick, wizzyrea
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06
Bug Depends on: 21241    
Bug Blocks: 22907, 23038    
Attachments: Bug 22908: Add tests
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened
Bug 22908: Add tests
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened
Bug 22908: Add tests
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened

Description Jonathan Druart 2019-05-14 17:19:37 UTC
There is a return value $status_update_table that is set to 0, then we send the alert (whatever the value of this variable). We must return earlier if something happened!
Comment 1 Jonathan Druart 2019-05-14 17:24:32 UTC
Created attachment 89765 [details] [review]
Bug 22908: Add tests
Comment 2 Jonathan Druart 2019-05-14 17:24:34 UTC
Created attachment 89766 [details] [review]
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened

There is a return value $status_update_table that is set to 0, then we
send the alert (whatever the value of this variable). We must return
earlier if something happened!

Test plan:
Not sure how to recreate it via the interface, so just run the tests?
Comment 3 Liz Rea 2019-05-15 17:52:25 UTC
Created attachment 89792 [details] [review]
Bug 22908: Add tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Resolved small conflict with number of tests
Comment 4 Liz Rea 2019-05-15 17:52:34 UTC
Created attachment 89793 [details] [review]
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened

There is a return value $status_update_table that is set to 0, then we
send the alert (whatever the value of this variable). We must return
earlier if something happened!

Test plan:
Not sure how to recreate it via the interface, so just run the tests?

Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Comment 5 Marcel de Rooy 2019-05-17 10:20:35 UTC
Works when you run prove on the test, but not with perl [other sql mode]? New test fails then. Shouldnt DBIx always raise a warning if you want to pollute a date field?
Comment 6 Jonathan Druart 2019-05-25 01:57:45 UTC
(In reply to Marcel de Rooy from comment #5)
> Works when you run prove on the test, but not with perl [other sql mode]?
> New test fails then. Shouldnt DBIx always raise a warning if you want to
> pollute a date field?

DBI will not explode and so the notice will be generated.
I have no idea how to fix that, I'd say we should require that tests are ran using prove or with the strict_sql_modes flag in the config.
Comment 7 Marcel de Rooy 2019-05-27 09:52:56 UTC
Created attachment 90113 [details] [review]
Bug 22908: Add tests

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2019-05-27 09:53:00 UTC
Created attachment 90114 [details] [review]
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened

There is a return value $status_update_table that is set to 0, then we
send the alert (whatever the value of this variable). We must return
earlier if something happened!

Test plan:
Not sure how to recreate it via the interface, so just run the tests?

Signed-off-by: Liz Rea <wizzyrea@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Nick Clemens (kidclamp) 2019-05-30 10:38:57 UTC
Awesome work all!

Pushed to master for 19.05
Comment 10 Martin Renvoize 2019-05-30 20:17:37 UTC
Pushed to 18.11.x for 18.11.06