-
Notifications
You must be signed in to change notification settings - Fork 85
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BAH 2962 | SMS functionality of appointment booking and reminder moved to backend #117
Conversation
# Conflicts: # api/src/main/java/org/openmrs/module/appointments/service/impl/SMSService.java
# Conflicts: # api/src/main/java/org/openmrs/module/appointments/service/impl/AppointmentsServiceImpl.java # api/src/main/java/org/openmrs/module/appointments/service/impl/SMSService.java # api/src/main/resources/messages.properties # api/src/test/java/org/openmrs/module/appointments/service/impl/AppointmentVisitLocationTest.java
api/src/main/java/org/openmrs/module/appointments/service/AppointmentsService.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/appointments/service/impl/SMSService.java
Outdated
Show resolved
Hide resolved
api/src/test/java/org/openmrs/module/appointments/service/impl/SMSServiceTest.java
Outdated
Show resolved
Hide resolved
omod/src/main/java/org/openmrs/module/appointments/web/controller/AppointmentController.java
Outdated
Show resolved
Hide resolved
# Conflicts: # api/pom.xml # omod/pom.xml
# Conflicts: # api/src/main/java/org/openmrs/module/appointments/scheduler/tasks/ReminderForAppointment.java
api/src/main/java/org/openmrs/module/appointments/constants/PrivilegeConstants.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/appointments/events/AppointmentBookingEvent.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/appointments/events/AppointmentEvent.java
Outdated
Show resolved
Hide resolved
BAHMNI_RECURRING_APPOINTMENT_UPDATED("bahmni-recurring-appointment"); | ||
|
||
private final String topic; | ||
AppointmentEventType(String topic) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we shouldn't be mixing event with the topic here.
if any mapping needs to be done regarding which topic it must be sent to, then the listener to the spring application event (e.g. bahmni events module) - should use some mapping to identify the topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@binduak ^
api/src/main/java/org/openmrs/module/appointments/events/RecurringAppointmentEvent.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/openmrs/module/appointments/scheduler/tasks/ReminderForAppointment.java
Show resolved
Hide resolved
import org.springframework.util.StringUtils; | ||
|
||
|
||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace with single line imports .. no util.* import
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not replaced .. please replace util.* with single line imports
api/src/main/java/org/openmrs/module/appointments/service/impl/AppointmentVisitLocation.java
Outdated
Show resolved
Hide resolved
...n/java/org/openmrs/module/appointments/events/eventListener/AppointmentSMSEventListener.java
Show resolved
Hide resolved
String message = messageBuilderService.getAppointmentBookingMessage(appointmentArgumentsMapper.createArgumentsMapForRecurringAppointmentBooking(appointment), providersName); | ||
communicationService.sendSMS(phoneNumber, message); | ||
} | ||
private boolean checkGlobalCondition() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
may a method name "shouldSendEmailForBookingAppointment()" is more appropriate than "checkGlobalCondition"?
import org.springframework.util.StringUtils; | ||
|
||
|
||
import java.util.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is not replaced .. please replace util.* with single line imports
omod/src/main/resources/config.xml
Outdated
<defaultValue>false</defaultValue> | ||
</globalProperty> | ||
|
||
<globalProperty> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not be setting property like this. This attribute should be associated with location. Also where is this used? if not used, please remove.
omod/src/main/resources/config.xml
Outdated
</globalProperty> | ||
|
||
<globalProperty> | ||
<property>bahmni.sms.timezone</property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this prefixed with bahmni.sms, where other properties are sms.*?
omod/src/main/resources/config.xml
Outdated
</globalProperty> | ||
|
||
<globalProperty> | ||
<property>bahmni.sms.dateformat</property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same question as above?
@@ -50,6 +50,7 @@ | |||
<javaxMail.version>1.6.2</javaxMail.version> | |||
<servletApi.version>3.0.1</servletApi.version> | |||
<webServices.version>2.29.0</webServices.version> | |||
<communications.version>1.2.0-SNAPSHOT</communications.version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NOTE: remember to merge changes onto communication omod and ensure that latest snapshot is released before you merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure
SMS functionality of appointment booking and reminder moved to backend