RESTWS-665 : SystemInfo REST implementation #285

suthagar23 · 2017-06-26T13:55:49Z

Implemented REST part for SystemInfo.
https://issues.openmrs.org/browse/RESTWS-665

coveralls · 2017-06-26T14:08:35Z

Coverage increased (+0.01%) to 46.078% when pulling 6c75542 on suthagar23:RESTWS-665 into 67deaa3 on openmrs:master.

dkayiwa · 2017-06-26T14:40:33Z

Can you squash the commits into one?

coveralls · 2017-06-26T15:24:40Z

Coverage increased (+0.01%) to 46.078% when pulling 54126fc on suthagar23:RESTWS-665 into 67deaa3 on openmrs:master.

coveralls · 2017-06-26T15:29:27Z

Coverage increased (+0.01%) to 46.078% when pulling 54126fc on suthagar23:RESTWS-665 into 67deaa3 on openmrs:master.

suthagar23 · 2017-06-26T15:30:12Z

@dkayiwa I have squashed into One. Can you check it?

dkayiwa · 2017-06-26T16:14:54Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8.java

+		// Initilize rest as a SinmpleObject 
+		SimpleObject rest = new SimpleObject();
+		try {
+            //getAdministrationService().getSystemInformation() will give all System informations


Do we need these comments?

dkayiwa · 2017-06-26T16:15:08Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8.java

+		catch (Exception ex) {
+			System.out.println("SystemInfo Resource is getting Error");
+		}
+		// return the rest object


Do we need this comment?

dkayiwa · 2017-06-26T16:15:22Z

...s/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8Test.java

+
+	@Test
+	public void testGetAll() throws Exception {
+        // Manually retrive the System Information 


Do we need this comment?

dkayiwa · 2017-06-26T16:18:04Z

@suthagar23 i see many comments which do not seem to add value. When the code is self explanatory, it is not advisable to comment the obvious. Do you agree? 😊

dkayiwa · 2017-06-26T16:19:35Z

In reference to the above, you could take a look at this: https://dzone.com/articles/5-best-practices-commenting

suthagar23 · 2017-06-26T16:23:09Z

@dkayiwa I just developed it for the first time, So used those comments 😄 . Anyway, I will manage that and make it better in the future. Thanks for the guide 👍

coveralls · 2017-06-26T16:41:43Z

Coverage increased (+0.01%) to 46.078% when pulling 59c1bee on suthagar23:RESTWS-665 into 67deaa3 on openmrs:master.

dkayiwa · 2017-06-26T16:42:33Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8.java

+	public SimpleObject getAll(RequestContext context) throws ResponseException {
+		SimpleObject rest = new SimpleObject();
+		try {
+			//Push System informations to rest object with Key as SystemInfo


Do we need the above comment?

Removed :-)

dkayiwa · 2017-06-26T16:42:51Z

...s/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8Test.java

+import org.apache.struts.mock.MockHttpServletResponse;
+import org.apache.commons.beanutils.PropertyUtils;
+import org.junit.Assert;
+//import org.junit.Before;


What about these two commented out lines?

Removed :-)

dkayiwa · 2017-06-26T16:43:08Z

...s/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8Test.java

+		SimpleObject rest = new SimpleObject();
+		rest.put("SystemInfo", Context.getAdministrationService().getSystemInformation());
+
+		// Retrive the System Information using HTTP GET Request


Do we need the above comment?

Removed :-)

suthagar23 · 2017-06-30T02:22:01Z

@dkayiwa Can you please review this PR now 😅 ?

coveralls · 2017-06-30T02:32:35Z

Coverage increased (+0.008%) to 46.11% when pulling b2cbf90 on suthagar23:RESTWS-665 into cb6450d on openmrs:master.

dkayiwa · 2017-06-30T14:46:53Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8.java

+			rest.put("SystemInfo", Context.getAdministrationService().getSystemInformation());
+		}
+		catch (Exception ex) {
+			System.out.println(ex.getMessage());


Did you look at our java conventions in regards to the above line? https://wiki.openmrs.org/display/docs/Java+Conventions

dkayiwa · 2017-06-30T14:47:30Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8.java

+			System.out.println(ex.getMessage());
+		}
+		finally {
+			return rest;


What is the use of the finally block here? Are you trying to clean up resources or something?

dkayiwa · 2017-06-30T14:52:41Z

...s/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8Test.java

+	public void testGetAll() throws Exception {
+		SimpleObject rest = new SimpleObject();
+		rest.put("SystemInfo", Context.getAdministrationService().getSystemInformation());
+		MockHttpServletResponse response = new MockHttpServletResponse();


Can you do a test like those in SystemSettingController1_9Test that do a real GET request?

@suthagar23 are you still working on this?

@dkayiwa I am working with test cases, I couldn't implement those type of test cases here, Give me one more day to finalize

@suthagar23 how did this go? 😊

@suthagar23 people cannot test the system info app locally because this pull request is not yet done. Can you prioritize it please?

@dkayiwa I completed REST Resource part, but struggling with TEST code :-(
I will complete that within today :-)

Thanks for the update! I anxiously look forward to it. 😊

coveralls · 2017-07-24T19:27:50Z

Coverage increased (+0.04%) to 46.147% when pulling eb0565b on suthagar23:RESTWS-665 into cb6450d on openmrs:master.

suthagar23 · 2017-07-24T19:34:34Z

@dkayiwa Can you please review this PR now, I have updated my works

dkayiwa · 2017-07-25T14:39:26Z

@suthagar23 can you squash these commits into one as advised at https://wiki.openmrs.org/display/docs/Pull+Request+Tips?

suthagar23 · 2017-07-25T15:54:23Z

@dkayiwa Completed, Now there is one commit for this PR

coveralls · 2017-07-25T16:06:10Z

Coverage increased (+0.02%) to 46.147% when pulling 4f6f6c2 on suthagar23:RESTWS-665 into 12efaa5 on openmrs:master.

dkayiwa · 2017-07-25T17:51:06Z

...enmrs/module/webservices/rest/web/v1_0/resource/openmrs1_8/SystemInformationResource1_8.java

+	@Override
+	public SimpleObject getAll(RequestContext context) throws ResponseException {
+		SimpleObject rest = new SimpleObject();
+		rest.put("SystemInfo", Context.getAdministrationService().getSystemInformation());


Our convention is to start with lowercase for keys or properties.

updated 👍

Delete SystemInfomationResource1_8.java SystemInfo REST Implementation REST Implementation for SystemInfo Delete SystemInfomationResource1_8.java SystemInfo REST - Comments removal REST SystemInfo - Comments Reviewed SystemInformation REST updates SysInfo REST Implemenation & Convention updated

coveralls · 2017-07-25T18:22:45Z

Coverage increased (+0.02%) to 46.147% when pulling 3fbdfe3 on suthagar23:RESTWS-665 into 12efaa5 on openmrs:master.

suthagar23 force-pushed the RESTWS-665 branch 2 times, most recently from 740d232 to 54126fc Compare June 26, 2017 15:17

dkayiwa reviewed Jun 26, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-665 branch from 54126fc to 59c1bee Compare June 26, 2017 16:29

dkayiwa reviewed Jun 26, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-665 branch from 59c1bee to b2cbf90 Compare June 30, 2017 02:20

dkayiwa reviewed Jun 30, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-665 branch from eb0565b to 4f6f6c2 Compare July 25, 2017 15:52

dkayiwa reviewed Jul 25, 2017

View reviewed changes

suthagar23 force-pushed the RESTWS-665 branch from 4f6f6c2 to 3fbdfe3 Compare July 25, 2017 18:10

dkayiwa merged commit a148f53 into openmrs:master Jul 25, 2017

RESTWS-665 : SystemInfo REST implementation #285

RESTWS-665 : SystemInfo REST implementation #285

Conversation

suthagar23 commented Jun 26, 2017

coveralls commented Jun 26, 2017

dkayiwa commented Jun 26, 2017

coveralls commented Jun 26, 2017

coveralls commented Jun 26, 2017

suthagar23 commented Jun 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkayiwa commented Jun 26, 2017

dkayiwa commented Jun 26, 2017

suthagar23 commented Jun 26, 2017

coveralls commented Jun 26, 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

suthagar23 commented Jun 30, 2017

coveralls commented Jun 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

coveralls commented Jul 24, 2017

suthagar23 commented Jul 24, 2017

dkayiwa commented Jul 25, 2017

suthagar23 commented Jul 25, 2017

coveralls commented Jul 25, 2017

dkayiwa Jul 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 25, 2017

dkayiwa Jul 25, 2017 •

edited

Loading