REPORT-862:Create an HttpReportProcessor by gitcliff · Pull Request #196 · openmrs/openmrs-module-reporting

gitcliff · 2020-04-07T08:30:52Z

link:https://issues.openmrs.org/browse/REPORT-862?src=confmacro

mseaton

Thanks for getting this started @gitcliff . Still some work to do, please see my comments below.

mseaton · 2020-04-07T10:56:38Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

@@ -0,0 +1,64 @@
+package org.openmrs.module.reporting.report.processor;


License Header

i have fixed it

mseaton · 2020-04-07T10:56:55Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+import org.springframework.stereotype.Component;
+
+@Component
+public class HttpReportProcessor implements ReportProcessor {


i have added it

mseaton · 2020-04-07T10:57:37Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+		ret.add("connection url");
+		ret.add("subject");
+		ret.add("Add report");
+		return ret;


These should not contain spaces in the names. Please make these camel case.

Since you refer to these names below, you should also make them into Constants and then refer to the constants in each place.

i have fixed it

@mseaton kindly review ..Thanks

mseaton · 2020-04-07T10:59:48Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+			URL url = new URL(configuration.getProperty("connection url"));
+			HttpURLConnection connection = (HttpURLConnection) url.openConnection();
+			connection.setRequestMethod("POST");
+			connection.setRequestProperty("Content-Type", "text/html; charset=UTF-8");


Why limit this to text/html? Wouldn't it be better to get the report.getOutputContentType(), and set this content type?

mseaton · 2020-04-07T11:02:55Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+			connection.connect();
+
+			if (report.getRenderedOutput() != null && "true".equalsIgnoreCase(configuration.getProperty("Add report"))) {	
+				String addcontent = configuration.getProperty(("Add report") + configuration.getProperty("subject"));


I don't really understand what all of this is doing with the "Add report" and "subject" properties. Is this meant to be like saying "enabled=true/false"?

There is no javadoc, no examples, and no unit tests showing what is expected here. All of that needs to be added.

mseaton · 2020-04-07T11:03:30Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+				String addcontent = configuration.getProperty(("Add report") + configuration.getProperty("subject"));
+				if (report.getOutputContentType().contains("text") || report.getOutputContentType().contains("html")) {
+					addcontent =  new String(report.getRenderedOutput(),"UTF-8");	
+				}	


Per my comment above, there is no reason to limit this to text/html, is there?

mseaton · 2020-04-07T11:04:04Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+					    connection.getOutputStream());
+				writer.write(addcontent);
+				writer.flush();
+				writer.close();


Does this need to be in a finally block?

No because the finally blocks can't be inserted in try-catch blocks

With I/O, typically it's a good idea to close the connection in a finally block. You can have nested try/catch blocks if you need to. In this case, I would declare "HttpURLConnection" outside of the try block, assign it and use it inside the try block, and close it in the finally block:

HttpURLConnection connection; OutputStreamWriter writer; try { connection = (HttpURLConnection) url.openConnection(); ... writer = new OutputStreamWriter(connection.getOutputStream()); ... } catch (Exception e) { throw new RuntimeException("Error occurred while sending report via HTTP POST", e); } finally { IOUtils.closeQuietly(writer); IOUtils.closeQuietly(connection); }

@mseaton the connection variable is as a result of casting the url variable to HttpURLConnection of which if am to declare "HttpURLConnection" outside of the try block then i will need to declare URL outside to, which requires also a try block to catch the MalformedURLException and if this is done,then the scope of the url variable used here url.openConnection() will be limited

mseaton

See additional comments.

mseaton · 2020-04-10T12:06:31Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+			HttpURLConnection connection = (HttpURLConnection) url.openConnection();
+			connection.setRequestMethod("POST");
+			String outPutContentType = report.getOutputContentType();
+			connection.setRequestProperty("Content-Type", "outPutContentType; charset=UTF-8");


Typo in outPutContentType?

@mseaton i don't understand clearly what you mean here..please elaborate

mseaton · 2020-04-10T12:16:58Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+					    connection.getOutputStream());
+				writer.write(addcontent);
+				writer.flush();
+				writer.close();


With I/O, typically it's a good idea to close the connection in a finally block. You can have nested try/catch blocks if you need to. In this case, I would declare "HttpURLConnection" outside of the try block, assign it and use it inside the try block, and close it in the finally block:

HttpURLConnection connection; OutputStreamWriter writer; try { connection = (HttpURLConnection) url.openConnection(); ... writer = new OutputStreamWriter(connection.getOutputStream()); ... } catch (Exception e) { throw new RuntimeException("Error occurred while sending report via HTTP POST", e); } finally { IOUtils.closeQuietly(writer); IOUtils.closeQuietly(connection); }

mseaton · 2020-04-10T12:19:00Z

api/src/main/java/org/openmrs/module/reporting/report/processor/HttpReportProcessor.java

+			connection.setRequestProperty("Content-Type", "outPutContentType; charset=UTF-8");
+			connection.setDoOutput(true);
+			connection.connect();
+			 //when the parameter ADD_REPORT is set to true then the rendered report is added to the url connection 


I still don't quite understand this. What happens if ADD_REPORT is set to false or left empty? Why are you opening a connection above at all in this case if you are not actually writing anything to it? If the intention is to send the subject to that URL but not the report, then this code isn't achieving that. If the intention is to just disable this processor altogether, then put this check at the very top before you do anything with a connection at all...

Sir the intention to send the subject to the report.
Am going to add the ADD_REPORT to the very top because my intention is that when its not set or false then the processor is disabled.
i used connection.connect() because it opens a connection link to a resource referenced by the URL but i will go ahead remove it

mseaton requested changes Apr 7, 2020

View reviewed changes

gitcliff force-pushed the REPORT-862 branch from ecd3ed6 to e13390b Compare April 10, 2020 06:34

mseaton reviewed Apr 10, 2020

View reviewed changes

REPORT-862:Create an HttpReportProcessor

ff5e337

gitcliff force-pushed the REPORT-862 branch from e13390b to ff5e337 Compare April 13, 2020 08:26

		@@ -0,0 +1,64 @@
		package org.openmrs.module.reporting.report.processor;

Conversation

gitcliff commented Apr 7, 2020

Uh oh!

mseaton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseaton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gitcliff Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

gitcliff Apr 10, 2020 •

edited

Loading