BAH-2180|sms-alerts-appointments-booking #89
Conversation
shilpa-iplit
requested review from
angshu,
binduak,
gsluthra,
N0-man and
swatigogia2020
| import java.util.Date; | ||
|
|
||
| public interface SmsSender { | ||
| void send(String recipientMobileNumber, String patientName, String smsType, String serviceName, Date startDateTime, String teleLink); |
There was a problem hiding this comment.
I was hoping the interface is Channel vocabulary based and not Appointment vocabulary based. Something like:
MessageSender.send(String phoneNumber, String fullMessage).
Then SMS Sender, Whatsapp Sender, etc can easily implement this interface. They don't care if the message is goign to patient, doctor, customer, relative, etc. They just send a message over the appropriate channel to the number. Its the caller of these classes that constructs the message and delegates the "sending" to the channel implementation.
So.. Appointment ---delegates to ---> AppointmentMessage class ---delegates to--> SMSSender class ---> sends SMS
Similarly,
Appointment ---delegates to ---> AppointmentMessage class ---delegates to (based on config)--> WhatsappSender class ---> sends SMS
Then we write unit tests for AppointmentMessage class which checks if the construction of message is correct based on passed parameters like PatientName, Service Name, Date, Link.
With this logic, the MessageSender interface and its implementations are reusable in the context of different messages (Appointments, Registration, Teleconsult, Doctor-Doctor consult, etc).
For this JIRA ticket, we would only implement SMS (and not whatzapp classes).
There was a problem hiding this comment.
Building of the message text requires sms template identifier. The message template format is available in SMSSender class (DefaultSmsSender) which is needed as outgoing SMS has to be conforming to this format.
Either message template is available in config file or in global properties in openmrs.
Please advise
There was a problem hiding this comment.
Maybe we don't need different templates for SMS or Whatzapp. We just create a template called: appointmentMessageTemplate and reuse the same template for SMS or Whatzapp etc... Or, depending on the current configured settings (SMS, Whatsapp, etc) -- the loaded properties file by AppointmentMessage class is: load appointmentMessageTemplate-sms.properties and constructs the string message, and calls the SMSSender.
| String dateApptPattern = String.format("MMMM '%s', yyyy", dateSuffixText); | ||
| SimpleDateFormat fmtDateAppt = new SimpleDateFormat(dateApptPattern); | ||
|
|
||
| String requestUrl = smsSessionProperties.getProperty("sms.url"); |
There was a problem hiding this comment.
A lot of logic for string concatenation. Needs tests. See the previous comment on how this could possibly be restructured into an AppointmentMessage class, which constructs the message string.
| } | ||
| } | ||
|
|
||
| private Properties getSession() { |
There was a problem hiding this comment.
why is this method called getSession()? all it does it load properties.
maybe call it loadProperties() and not return anything
| if (StringUtils.isNotBlank(smsSessionProperties.getProperty("sms.key.template.identifier")) && "WalkIn".equalsIgnoreCase(smsType)) { | ||
| templateId = "&" + smsSessionProperties.getProperty("sms.key.template.identifier") + "=" + | ||
| URLEncoder.encode(smsSessionProperties.getProperty("sms.appointment.walkin.template.identifier"), "UTF-8"); | ||
| // Dear %s,\n Your appointment is booked for %s on %s at %s by Bahmni Hospital (powered by IPLit) |
| if (StringUtils.isNotBlank(smsSessionProperties.getProperty("sms.key.template.identifier")) && "Virtual".equalsIgnoreCase(smsType)) { | ||
| templateId = "&" + smsSessionProperties.getProperty("sms.key.template.identifier") + "=" + | ||
| URLEncoder.encode(smsSessionProperties.getProperty("sms.appointment.teleconsultation.template.identifier"), "UTF-8"); | ||
| // Dear %s,\nTele-Consultation booked for %s %s\nVideo link: %s Bahmni Hospital(IPLit) |
| if (StringUtils.isNotBlank(smsSessionProperties.getProperty("sms.params.addons"))) { | ||
| addOnParams = "&" + smsSessionProperties.getProperty("sms.params.addons"); | ||
| } | ||
| String message = "&" + smsSessionProperties.getProperty("sms.key.message") + "=" + URLEncoder.encode(msgText, "UTF-8"); |
There was a problem hiding this comment.
use a StringJoiner or a StringBuilder
| addOnParams = "&" + smsSessionProperties.getProperty("sms.params.addons"); | ||
| } | ||
| String message = "&" + smsSessionProperties.getProperty("sms.key.message") + "=" + URLEncoder.encode(msgText, "UTF-8"); | ||
| requestUrl += apiKey + message + sender + number + password + entityId + templateId + addOnParams; |
There was a problem hiding this comment.
sending the msg itself through the URL is not a standard way of passing the sms. Can you check if you can pass through the body/payload ..
Also, its probably better to use a closeable HTTP Client, and you can use try with resources block
|
|
||
| URL url = new URL(requestUrl); | ||
| uc = (HttpURLConnection)url.openConnection(); | ||
| String smsResponse = uc.getResponseMessage(); |
There was a problem hiding this comment.
if you aren't logging the response, why assign to a variable?
| return null; | ||
| } | ||
|
|
||
| private String getDateWithSuffix(int date) { |
There was a problem hiding this comment.
do we really need to hardcode st, nd etc .. why not just convert to string representation of date .. like 30 June 2022. its also not using the locale and using just english. You can leverage a date formatter
There was a problem hiding this comment.
sure. Will incorporate all the comments.
Sample contents for sms-config.properties file to be placed in openmrs data directory
sms.key.username=username
sms.key.password=password
sms.key.phonenumber=destination
sms.key.message=message
sms.key.sender=source
sms.key.entity.identifier=entityid
sms.key.template.identifier=tempid
sms.phonenumber.length=10
sms.url=http://103.16.101.52:8080/bulksms/bulksms?
sms.username=
sms.password=
sms.sender=IPLits
sms.entity.identifier=1201159240485243492
sms.params.addons=type=0&dlr=1
sms.appointment.walkin.template.identifier=1207161751603402657
sms.appointment.teleconsultation.template.identifier=1207162489034032086
sms.appointment.walkin.message.template=Dear %s,\n Your appointment is booked for %s on %s at %s by Bahmni Hospital (powered by IPLit)
sms.appointment.teleconsultation.message.template=Dear %s,\nTele-Consultation booked for %s %s\nVideo link: %s Bahmni Hospital(IPLit)