Bug 18732

Summary: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
Product: Koha Reporter: Mark Tompsett <mtompset>
Component: Test SuiteAssignee: Mark Tompsett <mtompset>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: minor    
Priority: P5 - low CC: fridolin.somers, jonathan.druart, katrin.fischer, ldjamison, m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
[SIGNED-OFF] Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Description Mark Tompsett 2017-06-05 22:02:47 UTC
As a result:
prove t/SMS.t
generates noise:
Use of uninitialized value in subroutine entry at /home/mtompset/kohaclone/C4/SMS.pm line 98.
Comment 1 Mark Tompsett 2017-06-05 22:23:00 UTC
Created attachment 64015 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This tweaks the tests to compensate and change for missing case.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone, test added
run koha qa test tools.
Comment 2 Josef Moravec 2017-06-06 04:11:29 UTC
Created attachment 64016 [details] [review]
[SIGNED-OFF] Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This tweaks the tests to compensate and change for missing case.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone, test added
run koha qa test tools.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 3 Marcel de Rooy 2017-06-08 15:50:58 UTC
What about this line in C4/SMS:
if ( -f $conf_file ) {

If the file does not exist, it does not seem to hurt. You just do not have additional arguments.
But this patch makes it exit.

Please explain.
Comment 4 Mark Tompsett 2017-06-09 04:35:21 UTC
(In reply to Marcel de Rooy from comment #3)
> What about this line in C4/SMS:
> if ( -f $conf_file ) {
> 
> If the file does not exist, it does not seem to hurt. You just do not have
> additional arguments.
> But this patch makes it exit.
> 
> Please explain.

Oh... right, probably not correct action.
Comment 5 Mark Tompsett 2017-06-09 04:46:29 UTC
Created attachment 64120 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This quietly bypasses that.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.
Comment 6 Mark Tompsett 2017-06-09 04:48:08 UTC
Created attachment 64121 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This silently bypasses the problem.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.
Comment 7 Lee Jamison 2017-06-09 17:42:16 UTC
Created attachment 64167 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This silently bypasses the problem.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Works correctly as indicated by the testing plan.
No "uninitialized" noise after patch is applied.
Comment 8 Jonathan Druart 2017-06-13 15:02:28 UTC
I do not think the behaviour is correct, C4::SMS::send_sms should just do nothing if the config is missing.
Comment 9 Marcel de Rooy 2017-06-14 14:30:56 UTC
Created attachment 64285 [details] [review]
Bug 18732: Noisy t/SMS.t triggered by koha_conf.xml without sms_send_config

Upgraded systems may be lacking sms_send_config which makes t/SMS.t noisy.
This silently bypasses the problem.

Remove sms_send_config from your koha-conf.xml file
prove t/SMS.t
-- it will be noisy, but pass.
apply patch
prove t/SMS.t
-- noise gone.
run koha qa test tools.

Signed-off-by: Lee Jamison <ldjamison@marywood.edu>
Works correctly as indicated by the testing plan.
No "uninitialized" noise after patch is applied.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Slightly amended: turned the iif around.
Comment 10 Marcel de Rooy 2017-06-14 14:32:35 UTC
(In reply to Jonathan Druart from comment #8)
> I do not think the behaviour is correct, C4::SMS::send_sms should just do
> nothing if the config is missing.

Could be discussed on another report and perhaps solved. At first glance, the config seems to be something additional not required.
This just deals with eliminating a uninit warn.
Comment 11 Jonathan Druart 2017-06-14 14:38:42 UTC
(In reply to Marcel de Rooy from comment #10)
> (In reply to Jonathan Druart from comment #8)
> > I do not think the behaviour is correct, C4::SMS::send_sms should just do
> > nothing if the config is missing.
> 
> Could be discussed on another report and perhaps solved. At first glance,
> the config seems to be something additional not required.
> This just deals with eliminating a uninit warn.

Nope, please fix it here.
Comment 12 Marcel de Rooy 2017-06-14 14:39:43 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Marcel de Rooy from comment #10)
> > (In reply to Jonathan Druart from comment #8)
> > > I do not think the behaviour is correct, C4::SMS::send_sms should just do
> > > nothing if the config is missing.
> > 
> > Could be discussed on another report and perhaps solved. At first glance,
> > the config seems to be something additional not required.
> > This just deals with eliminating a uninit warn.
> 
> Nope, please fix it here.

I respectfully disagree. This can be pushed imo.
Comment 13 Mark Tompsett 2017-06-14 18:09:55 UTC
(In reply to Jonathan Druart from comment #11)
> (In reply to Marcel de Rooy from comment #10)
> > (In reply to Jonathan Druart from comment #8)
> > > I do not think the behaviour is correct, C4::SMS::send_sms should just do
> > > nothing if the config is missing.
> > 
> > Could be discussed on another report and perhaps solved. At first glance,
> > the config seems to be something additional not required.
> > This just deals with eliminating a uninit warn.
> 
> Nope, please fix it here.

"Any parameters with a leading underscore are considered private driver-specific options and will be passed through without alteration.

Any other parameters without a leading underscore will be silently stripped out and not passed through to the driver." (SMS::Send - http://search.cpan.org/~adamk/SMS-Send-1.06/lib/SMS/Send.pm)

The configuration file is used to set extraneous parameters based on the driver. It has nothing to do whether sending a message should be attempted or not.

In the case of SMS::Send::Test, it won't matter because it is a test driver. 

In the case of driver where it matters, they may not take extra parameters, so the configuration file is pointless and should be sent.

In the case where it matters, and the extra parameters are needed, the sending will fail and the warn $@; return; will be triggered.

This patch is correct.
Comment 14 Jonathan Druart 2017-06-14 18:59:43 UTC
Indeed, you are right.
Comment 15 Jonathan Druart 2017-06-15 19:09:32 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 16 Fridolin Somers 2017-06-20 08:27:55 UTC
Pushed to 17.05.x, will be in 17.05.01
Comment 17 Katrin Fischer 2017-06-21 06:27:14 UTC
Patch doesn't apply on 16.11.x - please rebase if you want to have this included.
Comment 18 Katrin Fischer 2017-07-16 12:56:39 UTC
Test is not noisy in 16.11.x. Closing.