IdStory IdM modules » sms

This feature will be implement as new standalone module.

I implement first version of new SMS module with one implementation sms 'driver' with GET method.

Commit: https://git.bcvsolutions.eu/modules/sms/commit/4411482fb900cd3f0221b42d926874bc659ecb53
Documentation:
https://wiki.czechidm.com/devel/documentation/modules_sms
https://wiki.czechidm.com/tutorial/adm/modules_sms

Default configurations:

# Enable property for SMS GET script driver
idm.sec.sms.script.get.enabled
#
# Login that will be used for composes URL with gateway
idm.sec.sms.script.get.login
#
# Password that will be used for composes URL with gatewat
idm.sec.sms.script.get.password
#
# Script that be used for composes URL
idm.sec.sms.script.get.scriptCode
#
# Timeout for comunication with gatewat
idm.sec.sms.script.get.timeout
#
# Number that will be used for overriding all recipient number (for testing or?)
idm.sec.sms.script.get.overrideNumber

Tests: https://git.bcvsolutions.eu/modules/sms/commit/7668b552e241da7c1263ae3ccc09441d620ebe9d

I made two new implementation for sms gateway:

• DefaultSmsBranaSmsSender
• DefaultHtmlSmsSender

tests are included + documentation was updated

You must choose which sender will be used: https://wiki.czechidm.com/devel/documentation/application_configuration/backend#notification_senders

Please Petr could you test this snapshot version? Thank you very much :)

I tested this wonderful feature.
Good news is that I was able to send sms throw your defaultHtmlSmsSender!

• Topic for generated password does not contains module prefix.
• Topic sends messages to the administrator instead of person who needs the new passwords. (of course, I like this feature, but its probably dangerous security bug).
• Message sending does not work if the user has a national prefix in phone number. But if I send messages from the terminal, then prefix works.
• I was a bit confused with the sender property settings (correct property name), please modify this section of the documentation.

Thank you for feedback,

• Topic for generated password does not contains module prefix.

• This is probably bug for password reset module, this isn't related to sms gateway module, what topic name do you mean? You probably can report this into password reset module.

• Topic sends messages to the administrator instead of person who needs the new passwords. (of course, I like this feature, but its probably dangerous security bug),

• what topic do you mean? Sms script sender doesn't contains topics, with what topic is the problem?

• Message sending does not work if the user has a national prefix in phone number. But if I send messages from the terminal, then prefix works.

• please could you send me your script for composed url with smsm gateway? You need probably encoded character '+',

• I was a bit confused with the sender property settings (correct property name), please modify this section of the documentation.

• with what configuration property did you have a problem? Configuration property

# sender implementation
idm.sec.<module>.notification-sender.<notificationType>.impl=<beanName>

Could you please test send notification with encoded character '+' if it works?

Radek could you made a review? Project can be found on gitlab: https://git.bcvsolutions.eu/modules/sms

documentation:
https://wiki.czechidm.com/devel/documentation/modules_sms
https://wiki.czechidm.com/tutorial/adm/modules_sms

Thank you :)

Pls Peter do us a favor and make a review, thx.

I did review. New sender is really cool, I found only a few minor things

• Fix doc grammar
• AbstractScriptSmsNotificationSender#getSmsNumber TODO - create ticket for it, do not let it be forgotten
• AbstractScriptSmsNotificationSender#isEnabledSendSms is unused. Maybe you wanted to use it in sendSms on line 152?
• AbstractScriptSmsNotificationSender#sendSms is a little long to my taste, i would break it down a little bit
• TODO in createFailedLog. Is it really TODO? what should be changed?
• Why is AbstractScriptSmsNotificationSender abstract and then DefaultScriptSmsSender is just empty class with overridden constructor and component annotation? Why does not DefaultScriptSmsSender contain the base implementation from AbstractScriptSmsNotificationSender and other senders (eg. DefaultSmsBranaSender) just extend DefaultScriptSmsSender. I am not saying it is wrong, i just need explanation on why did you decide to do it like this, because it now looks strange to me.
• Invalid link to SmsGetResponseExtractor in javadoc in SmsGetConfiguration#isResponseDebug. You must use whole className including package (if class is not imported)

I also added screenshot from sonar analysis. Some of the issues are nosense but some are quite helpful, so take a look at it and decide if anything is worth fixing.

Thanks for feedback,

• i'm not friend with english, all english documentation that we write is written according to our best conscience, please if you found some grammar mistake feel free to edit, I can spent some days and read all documentation that I write into wiki.czechidm.com, but for this exists so many better persons than me,
• create new ticket for TODO's is good idea, thank you :) ,
• I removed method isEnabledSendSms, thanks for hint,
• I agree, long methods is way to hell, but 60 rows with some spaces and LOG calls, isn't so bad, in this case I iterate over recipient and fill result list and skip some recipient (like missing sms number), so I try add some new private method, please check new behavior, (I removed all spaces and empty lines + create two new private methods)
• TODO in createFailedLog is right TODO, because when you set only smsLogDto.setState(NotificationState.NOT) without smsLogDto.setSent(null) on FE is this message marked as successful sent. I haven't time for debug this problem. I remove this todo, but for next we must not forget this 'bug?',
• javadoc in SmsGetConfiguration was fixed, this is propably most dangerous bug in this module :D.

• hardcoded password, this ins't bug for me, this is constant, but If you really want, I can change this constant to something else, but for me isn't this importatnt,
• refactor method from 16 to 15, i'm little bit confused what exactly is wrong with this method?
• add format for string into method getSmsGatewayUrl,
• constats PARAM_LOGIN, PARAM_PASSWORD, PARAM_NUMBER, PARAM_MESSAGE have underscore, but we use this for constant, see application code, are you sure with fix this with only [a-z][a-zA-Z0-9]?
• slf4j LOG is already final and static,
• I removed unused import,
• I removed generic types from constructor in DefaultSmsBranaSmsSender,
• constans in SmsGetConfiguration is correct,we use constants in interfaces, see core, acc and etc.

Thanks for big feedback I didn't expect so many problems, Vitek already created new project on our sonar. Thank you and please could you made a second code review? Thank you.

Fixes are OK. You can close the ticket now.

Thanks for merge. I close the ticket.