-
Notifications
You must be signed in to change notification settings - Fork 0
LAMP-49: Add scheduler to complete programs #5
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
Conversation
|
Asking review from @corneliouzbett since I'm not the best person to review this backend code. |
corneliouzbett
left a comment
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.
Left some comments
| import java.util.List; | ||
| import static org.junit.Assert.assertNotNull; | ||
| import org.mockito.Mockito; | ||
| import static org.mockito.Mockito.*; |
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.
Avoid wildcards imports
| <dependency> | ||
| <groupId>org.openmrs.api</groupId> | ||
| <artifactId>openmrs-api</artifactId> | ||
| <version>${openmrsPlatformVersion}</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.
Looks like you are using a lower version of OpenMRS platform v2.4.0. You should be targetting 2.7.x
| import java.util.Date; | ||
|
|
||
| import org.junit.Before; | ||
| import org.junit.Test; |
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.
I would recommend writing tests using JUnit 5
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.
Cannot use JUnit5 because PowerMock is not officially supported by JUnit 5
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.
You need to get rid of PowerMock in favor of mockStatic from Mockito library
| <comment>Inserting CompletePrograms Task into 'schedule_task_config' table</comment> | ||
| <insert tableName="scheduler_task_config"> | ||
| <column name="name" value="Complete LAMP Program Task" /> | ||
| <column name="description" value="Completes open Programs in Ozone LAMP" /> |
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.
Can this module be used outside Ozone Lamp distro? If yes, let's remove 'Ozone Lamp' in the description.
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.
Not really. This module is tied to Ozone LAMP programs.
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 class shouldn't be a Spring bean but rather just a constants Class.
| <changeSet id="create-complete-program-task-2025-11-04" author="siddharth"> | ||
| <preConditions onFail="MARK_RAN"> | ||
| <sqlCheck expectedResult="0"> | ||
| SELECT COUNT(*) FROM scheduler_task_config | ||
| WHERE schedulable_class = 'org.openmrs.module.lamp.scheduler.CompleteProgramsTask' | ||
| And name = 'Complete LAMP Program Task' | ||
| </sqlCheck> | ||
| </preConditions> | ||
| <comment>Inserting CompletePrograms Task into 'schedule_task_config' table</comment> | ||
| <insert tableName="scheduler_task_config"> |
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.
At some point, I would like us to migrate from using this approach (built-in scheduled task engine). This is because if the module is removed for some reason, there will be always scheduled task configuration hanging around, which throws errors due to classes not being found
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.
@VaishSiddharth can you revert (last commit) to using JUnit 4. Let's have a follow up task to migrate to JUnit 5.
abe9f5d to
0dcf8e4
Compare
JIRA: https://mekomsolutions.atlassian.net/browse/LAMP-49