Bug 22908 - Modsuggestion will generate a notice even if the modification failed
Summary: Modsuggestion will generate a notice even if the modification failed
Status: RESOLVED FIXED
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: 21241
Blocks: 23038 22907
  Show dependency treegraph
 
Reported: 2019-05-14 17:19 UTC by Jonathan Druart
Modified: 2019-10-14 20:02 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
19.05.00, 18.11.06


Attachments
Bug 22908: Add tests (1.42 KB, patch)
2019-05-14 17:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened (1.37 KB, patch)
2019-05-14 17:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 22908: Add tests (1.47 KB, patch)
2019-05-15 17:52 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened (1.41 KB, patch)
2019-05-15 17:52 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22908: Add tests (1.56 KB, patch)
2019-05-27 09:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 22908: Prevent ModSuggestion to send a notice if something wrong happened (1.50 KB, patch)
2019-05-27 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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