[RESTWS-673] Scheduler REST Implementation #293

suthagar23 · 2017-08-16T17:23:08Z

Created TaskDefinitionResource1_8 for the task REST implementation

dkayiwa · 2017-08-16T17:25:51Z

Can you add some tests?

suthagar23 · 2017-08-16T17:26:52Z

coveralls · 2017-08-16T17:35:11Z

Coverage decreased (-0.3%) to 45.863% when pulling cbd8195 on suthagar23:RESTWS-673 into e12f8c7 on openmrs:master.

gayanW · 2017-08-24T14:26:26Z

I just ran the tests :) Tests in TaskActionController1_8Test.java fail.

coveralls · 2017-08-24T20:04:33Z

Coverage decreased (-0.05%) to 46.131% when pulling 7c082ef on suthagar23:RESTWS-673 into e12f8c7 on openmrs:master.

coveralls · 2017-08-24T20:09:19Z

Coverage decreased (-0.05%) to 46.131% when pulling 7c082ef on suthagar23:RESTWS-673 into e12f8c7 on openmrs:master.

dkayiwa · 2017-08-24T20:16:20Z

.../openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskDefinitionResource1_8.java

+	//	@Override
+	//	public DelegatingResourceDescription getUpdatableProperties() {
+	//		DelegatingResourceDescription description = new DelegatingResourceDescription();
+	//		description.addRequiredProperty("name");


Did you take a look at this? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotcommentoutcode

Removed those Comments

dkayiwa · 2017-08-24T20:16:41Z

.../openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskDefinitionResource1_8.java

+	 */
+	@Override
+	public TaskDefinition save(TaskDefinition taskDefinition) {
+		//Context.getSchedulerService().saveTaskDefinition(taskDefinition);


What is the above line for? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotcommentoutcode

Removed that line

dkayiwa · 2017-08-24T20:18:20Z

omod-common/src/main/java/org/openmrs/module/webservices/helper/TaskFactoryWrapper.java

+				schedulerService.scheduleTask(task);
+			}
+			catch (SchedulerException e) {
+				throw new UnsupportedOperationException();


Does this really mean that the operation is not supported? The same applies elsewhere.

Removed unwanted Catch Exceptions

dkayiwa · 2017-08-24T20:19:44Z

omod-common/src/main/java/org/openmrs/module/webservices/helper/TaskFactoryWrapper.java

+ */
+package org.openmrs.module.webservices.helper;
+
+import org.openmrs.scheduler.*;


Did you take a look at this? https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotusewildcardimports

Refactored :-)

dkayiwa · 2017-08-24T20:21:23Z

.../org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskActionResource1_8.java

+			// ensure they're not trying to modify the REST module
+			for (TaskDefinition taskDef : taskDefinitions) {
+				// if they specified a module that's not loaded, it will show up here as null
+				if (taskDef == null) {


How does taskDefinitions become a collection of nulls?

dkayiwa · 2017-08-24T20:24:46Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+	@Autowired
+	RestService restService;
+
+	private TaskDefinition TestTask = new TaskDefinition(1, "TestTask", "TestTask Description",


Our convention is to use variable names that start with lower case.

dkayiwa · 2017-08-24T20:24:59Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+
+	MockTaskFactoryWrapper mockTaskFactory = new MockTaskFactoryWrapper();
+
+	String TestTaskMockJson = "{\"id\" : 1,\"name\":\"TestTask\",\"description\":\"TestTask Description\","


same as above for variable names.

dkayiwa · 2017-08-24T20:26:22Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+
+	@Test
+	public void shouldStopTestTask() throws Exception {
+		System.out.println("shouldStopTestTask");


Take a look at this: https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Donotprintout

Removed :-)

coveralls · 2017-08-24T20:48:52Z

Coverage decreased (-0.02%) to 46.158% when pulling 177dfba on suthagar23:RESTWS-673 into e12f8c7 on openmrs:master.

suthagar23 · 2017-08-24T20:54:45Z

@dkayiwa I have refactored the code about your comments

dkayiwa · 2017-08-24T20:59:37Z

How about other comments that have nothing to do with refactoring?

dkayiwa · 2017-08-24T21:01:11Z

.../org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskActionResource1_8.java

+
+		ServletContext servletContext = getServletContext(context);
+
+		if (action.getAction().equals("SCHEDULETASK") || action.getAction().equals("RESCHEDULETASK")


Don't you already have constants for SCHEDULETASK, RESCHEDULETASK, etc?

Changed with constants from TaskAction

dkayiwa · 2017-08-24T21:03:07Z

.../org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskActionResource1_8.java

+				taskFactoryWrapper.reScheduleTask(taskDef);
+			}
+			catch (SchedulerException e) {
+				throw new UnsupportedOperationException();


I see so many UnsupportedOperationException. Does this mean if anything goes wrong during these actions, it means that the REST call is not supported?

I couldn't throw the SchedulerException , because method implemented from Creatable Interface doesn't allow to throw the SchedulerException. Else I wanted to change the Creatable Interface. So I decided to throw UnsupportedOperationException

I can't throw ResponseException instead of UnsupportedOperationException. Because I can't instantiate that abstract here

Since you are not doing anything with the thrown exception, just do not catch it.

If I do not catch it, then want to throw it by methods.
If I throw it from here, then want to throw it from Create method.
But I can't throw it because It is a overridden method. So I caught and throw like this.
Are there any perfect methods for this?

@dkayiwa Shall I throw like this,
throw new ResourceDoesNotSupportOperationException("SchedulerException");

@dkayiwa If I throw the exception from here, then I can catch this from the angular REST request to indicate the user as well

Is it good to use like,
throw ObjectNotFoundException("SchedulerException : Ca not execute the action")

Do something along these lines:
throw new APIException("Errors occurred while rescheduling task", e);

Changed! Thanks @dkayiwa :-)

dkayiwa · 2017-08-24T21:04:41Z

omod-1.8/src/test/java/org/openmrs/module/webservices/rest/web/MockTaskFactoryWrapper.java

+		}
+		if (!contains) {
+			scheduledTasks.add(task.getName());
+		}


Did you intentionally decide to silently ignore a call for scheduling a task which does not exist?

The same applies for others like task shutdown, reschedule, etc

Here, I am doing same as ModuleAction Test. I using mock collections to maintain registered and scheduled tasks. When REST call arrives at Resource, that will call these MockTaskFactoryWrapper class methods. Here I am mocking that tasks and assigning to the collections to check the status from Test Class.

dkayiwa · 2017-08-25T07:50:37Z

.../org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskActionResource1_8.java

+				taskFactoryWrapper.unloadTask(taskDef);
+			}
+			catch (SchedulerException e) {
+				throw new APIException("Errors occurred while shutdowning all tasks", e);


Is this exception message correct?

Ow sorry, Missed :-(
Changed to "Errors occurred while deleting task".

coveralls · 2017-08-25T07:55:47Z

Coverage decreased (-0.01%) to 46.168% when pulling 447299e on suthagar23:RESTWS-673 into e12f8c7 on openmrs:master.

coveralls · 2017-08-30T10:00:30Z

Coverage increased (+0.3%) to 40.328% when pulling aaf0df0 on suthagar23:RESTWS-673 into 4f5eb1a on openmrs:master.

suthagar23 · 2017-08-30T10:06:14Z

@dkayiwa Completed 😊 Can you please take a look to merge!

dkayiwa · 2017-08-30T10:09:04Z

.../org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskActionResource1_8.java

+		}
+		if (!action.isAllTasks()) {
+			for (TaskDefinition taskDef : taskDefinitions) {
+				if (taskDef == null) {


Can a collection contain a null value?

I have removed this Condition, because the wrong TaskDefinition will be caught by the setConvertedProperties() and If there are any issues, then It will throw the error.

dkayiwa · 2017-08-30T10:10:42Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+	}
+
+	@Test
+	public void shouldScheduleTestTask() throws Exception {


No need to add "Test" to any of these test methods. For instance, shouldScheduleTask is enough instead of shouldScheduleTestTask

dkayiwa · 2017-08-30T10:11:18Z

.../module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskDefinitionController1_8Test.java

+	}
+
+	@Test
+	public void shouldGetModuleByUuid() throws Exception {


Should get what?

dkayiwa · 2017-08-30T10:11:55Z

.../module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskDefinitionController1_8Test.java

+
+	@Test
+	@Override
+	public void shouldGetDefaultByUuid() throws Exception {


When you say shouldGet, add what exactly you are getting.

Here I have @OverRide the method from the super class. that will express the Default representation and I can't change the name here

dkayiwa · 2017-08-30T10:12:44Z

omod-common/src/main/java/org/openmrs/module/webservices/helper/TaskServiceWrapper.java

+/**
+ * It will used to retrieve the required data from the Scheduler Service
+ */
+public class TaskServiceWrapper {


What value does this class add?

Wrapping the required task information between related resource and SchedulerService

This wrapping is simply one method call hence rendering it useless.

yah, but in the flow I want this class to create another Mock class for testing.
Otherwise, I can't do the testing.
I am using TaskServiceWrapper for active service works
MockTaskServiceWrapper for mock testing works

You can always mock Context.getSchedulerService().getTask(), etc without writing new mock classes.

Here I followed the same way of the ModuleActionResource and ModuleWrapper and achieve the target 😊
What do you mean by the mocking Services?

Ok, let us go with what you have. 😊

coveralls · 2017-08-30T10:55:06Z

Coverage increased (+0.3%) to 40.311% when pulling e38193c on suthagar23:RESTWS-673 into 4f5eb1a on openmrs:master.

suthagar23 · 2017-08-30T11:16:16Z

@dkayiwa So Finally If you haven't any concerns or comments, Could we move to merge it 😊

dkayiwa · 2017-08-30T11:20:22Z

.../org/openmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskActionResource1_8.java

+
+	@Override
+	public TaskAction getByUniqueId(String uniqueId) {
+		throw new UnsupportedOperationException("TaskAction cannot get by id");


Shouldn't we take this to be the internal id, if a number, or task name?

Oh sorry. Just ignore the above. 😊

Here It is the integer number. Here I have overridden that method in TaskAction.
Anyway, TaskAction is not supported to that function.

dkayiwa · 2017-08-30T11:23:41Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+
+	@Override
+	@Test(expected = Exception.class)
+	public void shouldGetDefaultByUuid() throws Exception {


should get what?

Overridden method. It is also like used to get Default Representation. But not supported to TaskAction.

dkayiwa · 2017-08-30T11:26:34Z

omod-common/src/main/java/org/openmrs/module/webservices/helper/TaskServiceWrapper.java

+	 * @throws SchedulerException - It will throw in case of any SchedulerService exceptions
+	 */
+	public void deleteTask(TaskDefinition task) throws SchedulerException {
+		if (Context.getSchedulerService().getTaskByName(task.getName()).getStarted()) {


What is the use of this logic? Isn't it taken care of by the service layer? Why don't you just Context.getSchedulerService().deleteTask?

Nope, SchedulerService is allowed to delete only the shutdown tasks. Otherwise, It threw the error. (I think, Core doesn't care about the task status)
So I just check about the task status before delete that task

Even then, that is the business of the service layer. Your duty is to simply call Context.getSchedulerService().deleteTask

dkayiwa · 2017-08-30T11:28:24Z

...nmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/TaskDefinitionResource1_8Test.java

+		assertPropEquals("started", getObject().getStarted());
+		assertPropEquals("taskClass", getObject().getTaskClass());
+		assertPropEquals("lastExecutionTime", getObject().getLastExecutionTime());
+		assertPropPresent("startTimePattern");


Shouldn't the above also be assertPropEquals()?

How about the one below it? 😊

That too changed 😊

dkayiwa · 2017-08-30T11:29:37Z

.../module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskDefinitionController1_8Test.java

+		MockHttpServletRequest req = request(RequestMethod.POST, getURI());
+		req.setContent(json.getBytes());
+		deserialize(handle(req));
+		Assert.assertEquals(mockTaskServiceWrapper.getTaskByName("MockTask").getName(), "MockTask");


How sure can i be that this task did not exist before saving?

I will add another test before to this to confirm that

dkayiwa · 2017-08-30T11:33:48Z

.../module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskDefinitionController1_8Test.java

+	}
+
+	@Test
+	public void shouldGetAllRegistered() throws Exception {


What is the difference between shouldGetAll and shouldGetAllRegistered?

Defaultly GetAll method in the resource has been set to return the list of Scheduled tasks. (I can't change the name because it is an Overriden method)
So I am using another separate method to test about the Registered tasks.

Isn't it more natural for GetAll to return all tasks regardless of whether they are scheduled or not?

So I will change the way to retrieve all the registered for the getAll method

dkayiwa · 2017-08-30T11:34:50Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+	}
+
+	@Test
+	public void shouldDoNothingIfTaskAlreadyShutdown() throws Exception {


This should be: shutdown_shouldDoNothingIfTaskAlreadyShutdown

Changed to shutdownTask_shouldDoNothingIfTaskAlreadyShutdown

dkayiwa · 2017-08-30T11:35:11Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+	}
+
+	@Test
+	public void shouldDoNothingIfTaskAlreadyScheduled() throws Exception {


scheduleTask _shouldDoNothingIfTaskAlreadyScheduled

dkayiwa · 2017-08-30T11:36:20Z

...nmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/TaskActionController1_8Test.java

+		mockTaskServiceWrapper.scheduledTasks.add(testTask);
+		deserialize(handle(newPostRequest(getURI(), "{\"action\": \"shutdowntask\", \"tasks\":[\"" + getTestTaskName()
+		        + "\"]}")));
+		assertThat(mockTaskServiceWrapper.scheduledTasks, not(hasItem(testTask)));


How how are you that the task wasn't already shutdown even before the shutdown command?

Here I am checking the Scheduled list before making shout down the request. The scheduled list will contain this task If it is not shut down.
After the request, It will be removed from the Scheduled list (shutdown). Then I am checking for removal.

Oh which line number are you checking that the task is not already shutdown?

Ohh, Sorry really missed that 😢
I will add it quickly

update TaskDefinition and TaskAction Implementation

coveralls · 2017-08-30T13:03:32Z

Coverage increased (+0.3%) to 40.318% when pulling 1bb91de on suthagar23:RESTWS-673 into 4f5eb1a on openmrs:master.

coveralls · 2017-08-30T13:11:08Z

Coverage increased (+0.3%) to 40.318% when pulling 1bb91de on suthagar23:RESTWS-673 into 4f5eb1a on openmrs:master.

suthagar23 · 2017-08-30T13:40:09Z

Thank you so much @dkayiwa for the review and merge 😊

dkayiwa · 2017-08-30T13:41:22Z

Thank you too for responding to all my back and forth review comments! 😊

suthagar23 force-pushed the RESTWS-673 branch from cbd8195 to 87005dd Compare August 21, 2017 12:20

suthagar23 changed the title ~~[RESTWS-673] Initial commit for the TaskDefinition Resource~~ [RESTWS-673] Scheduler REST Implementation Aug 21, 2017

suthagar23 force-pushed the RESTWS-673 branch 2 times, most recently from 58e1557 to 7c082ef Compare August 24, 2017 19:56

dkayiwa reviewed Aug 24, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-673 branch from 7c082ef to 177dfba Compare August 24, 2017 20:33

dkayiwa reviewed Aug 24, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-673 branch from 177dfba to ade87d7 Compare August 25, 2017 07:43

dkayiwa reviewed Aug 25, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-673 branch from ade87d7 to 447299e Compare August 25, 2017 07:55

suthagar23 force-pushed the RESTWS-673 branch from 447299e to 8de847e Compare August 25, 2017 08:00

suthagar23 force-pushed the RESTWS-673 branch 2 times, most recently from 4a068e8 to aaf0df0 Compare August 30, 2017 09:48

dkayiwa reviewed Aug 30, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-673 branch from aaf0df0 to e38193c Compare August 30, 2017 10:42

dkayiwa reviewed Aug 30, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-673 branch 2 times, most recently from 5ff8ce8 to 15140a5 Compare August 30, 2017 12:51

TaskDefinition and TaskAction REST Implementation

1bb91de

update TaskDefinition and TaskAction Implementation

suthagar23 force-pushed the RESTWS-673 branch from 15140a5 to 1bb91de Compare August 30, 2017 12:58

dkayiwa merged commit c165168 into openmrs:master Aug 30, 2017


		MockTaskFactoryWrapper mockTaskFactory = new MockTaskFactoryWrapper();

		String TestTaskMockJson = "{\"id\" : 1,\"name\":\"TestTask\",\"description\":\"TestTask Description\","


		ServletContext servletContext = getServletContext(context);

		if (action.getAction().equals("SCHEDULETASK") \|\| action.getAction().equals("RESCHEDULETASK")

[RESTWS-673] Scheduler REST Implementation #293

[RESTWS-673] Scheduler REST Implementation #293

Conversation

suthagar23 commented Aug 16, 2017

dkayiwa commented Aug 16, 2017

suthagar23 commented Aug 16, 2017

coveralls commented Aug 16, 2017

gayanW commented Aug 24, 2017 • edited Loading

coveralls commented Aug 24, 2017

coveralls commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suthagar23 Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 24, 2017

suthagar23 commented Aug 24, 2017

dkayiwa commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa Aug 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

suthagar23 Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 25, 2017

coveralls commented Aug 30, 2017

suthagar23 commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 30, 2017

suthagar23 commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gayanW commented Aug 24, 2017 •

edited

Loading

suthagar23 Aug 24, 2017 •

edited

Loading

dkayiwa Aug 24, 2017 •

edited

Loading

suthagar23 Aug 25, 2017 •

edited

Loading