[Bug 18732] New: Upgraded systems may not have sms_send_config in koha_conf.xml
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Bug ID: 18732 Summary: Upgraded systems may not have sms_send_config in koha_conf.xml Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Test Suite Assignee: chris@bigballofwax.co.nz Reporter: mtompset@hotmail.com QA Contact: gmcharlt@gmail.com 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor Assignee|chris@bigballofwax.co.nz |mtompset@hotmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Upgraded systems may not |Noisy t/SMS.t triggered by |have sms_send_config in |koha_conf.xml without |koha_conf.xml |sms_send_config -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 --- Comment #1 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 64015 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64015&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64015|0 |1 is obsolete| | --- Comment #2 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 64016 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64016&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |m.de.rooy@rijksmuseum.nl Status|Signed Off |Failed QA --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 --- Comment #4 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 64120 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64120&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64016|0 |1 is obsolete| | Attachment #64120|0 |1 is obsolete| | --- Comment #6 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 64121 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64121&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Lee Jamison <ldjamison@marywood.edu> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64121|0 |1 is obsolete| | --- Comment #7 from Lee Jamison <ldjamison@marywood.edu> --- Created attachment 64167 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64167&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #8 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I do not think the behaviour is correct, C4::SMS::send_sms should just do nothing if the config is missing. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #64167|0 |1 is obsolete| | --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 64285 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=64285&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|gmcharlt@gmail.com |m.de.rooy@rijksmuseum.nl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 --- Comment #10 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |ASSIGNED --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |In Discussion --- Comment #13 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ldjamison@marywood.edu -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Passed QA --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Indeed, you are right. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 17.11, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Fridolin SOMERS <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |Pushed to Stable CC| |fridolin.somers@biblibre.co | |m --- Comment #16 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- Pushed to 17.05.x, will be in 17.05.01 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #17 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Patch doesn't apply on 16.11.x - please rebase if you want to have this included. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18732 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Stable |RESOLVED --- Comment #18 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Test is not noisy in 16.11.x. Closing. -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org