RAD-385 MrrtReportTemplate.dcTermsDate should be a Date object not a string #499

ivange94 · 2016-10-22T13:56:07Z

RAD-385 MrrtReportTemplate.dcTermsDate should be a Date object not a string

Description

Changed type of DcTermsDate from String to Date
Implemented validation for dcterms.date. And make it to fail if the date could not be parsed from string to a java Date object
Added unit test for the validate method

Related Issue

see https://issues.openmrs.org/browse/RAD-385

Checklist:

My pull request only contains one single commit.
My pull request is based on the latest master branch
git pull --rebase upstream master.
I ran mvn clean package right before creating this pull request and
added all formatting changes to my commit.
My code follows the code style of this project.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

mention-bot · 2016-10-22T13:56:09Z

@ivange94, thanks for your PR! By analyzing the history of the files in this pull request, we identified @teleivo to be a potential reviewer.

coveralls · 2016-10-22T14:01:05Z

Coverage decreased (-0.06%) to 94.079% when pulling 31d1810 on ivange94:RAD-385 into bb941ce on openmrs:master.

dkayiwa · 2016-10-28T14:18:01Z

@teleivo what do you think of this? :)

dkayiwa · 2016-11-15T13:18:29Z

@teleivo do you think we should merge this? :)

teleivo · 2016-11-15T13:42:37Z

@dkayiwa sorry but was covered in work, will test the PRs next week

teleivo · 2016-11-22T09:41:43Z

api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java

    static String SELECTOR_QUERY_META_ATTRIBUTE_CHARSET = "meta[charset]";

    static String SELECTOR_QUERY_META_ATTRIBUTE_NAME = "meta[name]";

+    static String SELECTOR_QUERY_META_ATTRIBUTE_DATE = "meta[name=dcterms.date]";


@ivange94 please address.

teleivo · 2016-11-22T10:11:51Z

api/src/main/java/org/openmrs/module/radiology/report/template/MrrtReportTemplate.java

@@ -148,11 +149,11 @@ public void setDcTermsLicense(String dcTermsLicense) {
        this.dcTermsLicense = dcTermsLicense;
    }

-    public String getDcTermsDate() {
+    public Date getDcTermsDate() {
        return dcTermsDate;


return new Date(dcTermsDate);

dcTermsDate is already a date object. Why the need for the constructor again?

Date is mutable. If you return it one can mutate the date bypassing our setter. Of course now the setter just sets the date, that's another point we might need to adapt.

makes perfect sense. Am glad I asked. Let update now

Hmm but there is no Date(Date) constructor though

teleivo · 2016-11-22T10:12:14Z

...n/java/org/openmrs/module/radiology/report/template/DefaultMrrtReportTemplateFileParser.java

@@ -62,6 +64,8 @@

    private static final String DCTERMS_CREATOR = "dcterms.creator";

+    private static final String DATE_FORMAT = "yyyy-MM-dd";


are you sure thats the only format we have to support? just asking :)

Page 27 of the MRRT document says "This value shall be encoded in the XMLprimitive date format. Multiple instances of this parameter are not permitted."

A quick google search of "XMLprimitive date format" leads me here http://www.w3schools.com/xml/schema_dtypes_date.asp. Do you think I should modify it?

teleivo · 2016-11-22T10:15:56Z

api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java

+        if (!elements.isEmpty()) {
+            Element dcTermsDateElement = elements.get(0);
+            try {
+                new SimpleDateFormat("yyyy-MM-dd").parse(dcTermsDateElement.attr("content"));


you created a constant for the date format but you arent using it here. dangerous.

either extract the mrrt constants that are used in multiple places into a central class so it can be shared without introducing dependencies between classes which shouldnt have dependencies on each other or find a way to extract your rule of datevalidation so one could pass a dateformat to this rule. also make sure that the error message contains the dateformat so the user knows what format it needs to have. so the dateformat should be passed to the exception and used in the message with something like "date's need to be of format {0}"

teleivo · 2016-11-22T10:16:20Z

api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java

@@ -53,4 +65,19 @@ public ValidationResult run(Elements subject) {
        }
        return validationResult;
    }
+
+    private final boolean validateDate(Elements elements) {
+        boolean dateIsNotValid = false;


stick to calling the return variables "result" which we generally do

teleivo · 2016-11-22T10:17:55Z

api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java

+            }
+            catch (ParseException ex) {
+                dateIsNotValid = true;
+                log.debug(ex.getMessage(), ex);


wouldnt log that. make sure the violation is shown to the user with a proper message. thats the goal of this rule based system. give the user good information about whats wrong so he can correct his template. here I would say you just fill the log with info thats not worth to store there as the user can fix it easily.

teleivo · 2016-11-22T10:18:22Z

.../org/openmrs/module/radiology/report/template/MrrtReportTemplateFileParserComponentTest.java

@@ -120,6 +122,7 @@ public void parse_shouldReturnAnMrrtTemplateObjectIfFileIsValid() throws Excepti
        String templateContent = getFileContent("mrrttemplates/ihe/connectathon/2015/CTChestAbdomen.html");

        MrrtReportTemplate template = parser.parse(templateContent);
+        Date dcTermsDate = new SimpleDateFormat("yyyy-MM-dd").parse(TEST_DCTERMS_DATE);


dont hard code the format of the date

teleivo · 2016-11-22T10:20:05Z

omod/src/main/resources/messages.properties

@@ -181,6 +181,7 @@
 @MODULE_ID@.MrrtReportTemplate.not.imported.empty=Failed to import report template because it was empty
 @MODULE_ID@.MrrtReportTemplate.validation.error.meta.charset.occurence=Template file should have exactly one 'meta' element with attribute 'charset'
 @MODULE_ID@.MrrtReportTemplate.validation.error.meta.dublinCore.missing=Template file should have at least one 'meta' element encoding dublin core attributes
+@MODULE_ID@.MrrtReportTemplate.validation.error.date.invalid=dcterms.date of Template file should be a valid date object


think about the user. would you know what to do with this message? or would you think "whats a valid date object"?
think: I am a radiologist importing a template. if I get a message like "date's need to be of format yyyy-MM-dd like for example 2016-01-24" I would know what to do

ok. Will fix that

teleivo · 2016-11-22T10:20:23Z

@ivange94 made some comments, please check them out

teleivo · 2016-11-27T11:15:15Z

api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java

@@ -53,4 +64,18 @@ public ValidationResult run(Elements subject) {
        }
        return validationResult;
    }
+
+    private final boolean validateDate(Elements elements) {


rename to isDateInvalid ; easier to read

coveralls · 2016-11-27T18:07:06Z

Coverage decreased (-0.1%) to 94.054% when pulling 59ec9d5 on ivange94:RAD-385 into f072080 on openmrs:master.

coveralls · 2016-11-27T18:38:37Z

Coverage decreased (-0.1%) to 94.054% when pulling 62eab8e on ivange94:RAD-385 into f072080 on openmrs:master.

teleivo · 2016-12-05T08:50:14Z

...n/java/org/openmrs/module/radiology/report/template/DefaultMrrtReportTemplateFileParser.java

@@ -125,7 +132,7 @@ private final void initializeTemplate(MrrtReportTemplate template, Document doc)
                    template.setDcTermsLicense(content);
                    break;
                case DCTERMS_DATE:
-                    template.setDcTermsDate(content);
+                    template.setDcTermsDate(new SimpleDateFormat(MrrtReportTemplateConstants.DATE_FORMAT).parse(content));


can you extract

new SimpleDateFormat(MrrtReportTemplateConstants.DATE_FORMAT).parse(content)

into a private method so its easier to read. For example

parseDcTermsDate(String content)

teleivo · 2016-12-05T08:50:59Z

api/src/main/java/org/openmrs/module/radiology/report/template/MrrtReportTemplate.java

@@ -148,11 +149,14 @@ public void setDcTermsLicense(String dcTermsLicense) {
        this.dcTermsLicense = dcTermsLicense;
    }

-    public String getDcTermsDate() {
-        return dcTermsDate;
+    public Date getDcTermsDate() {


@ivange94 please address

teleivo · 2016-12-05T08:54:04Z

api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java

+
+    private final boolean isDateInvalid(Elements elements) {
+        boolean result = false;
+        if (!elements.isEmpty()) {


@ivange94 can you turn this into a positive check/guard clause. so

if (elements.isEmpty()) { return false; }

this removes one level of nesting and makes it easier to read, since the reader see's immediately that if the elements is empty we return.

teleivo · 2016-12-05T10:34:08Z

api/src/main/java/org/openmrs/module/radiology/report/template/MrrtReportTemplateConstants.java

+ */
+package org.openmrs.module.radiology.report.template;
+
+public final class MrrtReportTemplateConstants {


@ivange94 please also add a private constructor like so

https://github.com/openmrs/openmrs-module-radiology/blob/master/api/src/main/java/org/openmrs/module/radiology/RadiologyConstants.java#L102-L104

as this class should only serve as a container for constants.

also add javadocs to the class and the constant (refer to the section in the IHE standard where you got the format from).

teleivo · 2016-12-05T10:37:25Z

.../org/openmrs/module/radiology/report/template/MrrtReportTemplateFileParserComponentTest.java

@@ -120,6 +122,7 @@ public void parse_shouldReturnAnMrrtTemplateObjectIfFileIsValid() throws Excepti
        String templateContent = getFileContent("mrrttemplates/ihe/connectathon/2015/CTChestAbdomen.html");

        MrrtReportTemplate template = parser.parse(templateContent);
+        Date dcTermsDate = new SimpleDateFormat(MrrtReportTemplateConstants.DATE_FORMAT).parse(TEST_DCTERMS_DATE);


@ivange94 this seems bit lost here. simply change the type of TEST_DCTERMS_DATE from String to Date.

there is no Date constructor that can construct a Date instance from a String. So even if I change type of TEST_DCTERMS_DATE, i still need to put the logic(which I am doing here by using the SimpleDateFormat.parse()) that will convert the string to a date. So I'll end up with something like this

private static final Date TEST_DCTERMS_DATE = new SimpleDateFormat(format).parse("2013-06-01");

And since the parse method throws and exception I will now have to declare it the class to throw that exception. I thought it would be better to throw the exception at a method level since all our tests throw exceptions anyways.

There is actually a couple constructors I can call without using the formatter but they are all deprecated.

teleivo · 2016-12-05T10:41:34Z

...n/java/org/openmrs/module/radiology/report/template/DefaultMrrtReportTemplateFileParser.java

+            initializeTemplate(result, doc);
+        }
+        catch (ParseException e) {
+            throw new APIException("radiology.report.template.parser.error", null, e);


@ivange94 shouldnt this exception be the same as in the validation?
since the exception comes from your change which is the parsing of the date.

I'm not sure I understand this point. The code that validates the template is different from the code that parses and they both throw this exception. Did you mean I should not have caught it here too?

teleivo · 2016-12-05T10:47:50Z

api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java

@@ -24,6 +29,10 @@

    static String SELECTOR_QUERY_META_ATTRIBUTE_NAME = "meta[name]";

+    static String SELECTOR_QUERY_META_ATTRIBUTE_DATE = "meta[name=dcterms.date]";
+
+    private static final Log log = LogFactory.getLog(MetaTagsValidationEngine.class);


@ivange94 remove log, not used

…string

coveralls · 2016-12-11T13:43:06Z

Coverage decreased (-0.2%) to 94.033% when pulling d259ca6 on ivange94:RAD-385 into 0fef48b on openmrs:master.

teleivo reviewed Nov 22, 2016

View reviewed changes

ivange94 force-pushed the RAD-385 branch from 31d1810 to ac55bff Compare November 27, 2016 10:11

teleivo reviewed Nov 27, 2016

View reviewed changes

ivange94 force-pushed the RAD-385 branch 2 times, most recently from 1aa0f8d to 59ec9d5 Compare November 27, 2016 18:01

ivange94 force-pushed the RAD-385 branch from 59ec9d5 to 62eab8e Compare November 27, 2016 18:33

teleivo reviewed Dec 5, 2016

View reviewed changes

RAD-385 MrrtReportTemplate.dcTermsDate should be a Date object not a …

d259ca6

…string

ivange94 force-pushed the RAD-385 branch from 62eab8e to d259ca6 Compare December 11, 2016 13:38

ivange94 closed this Dec 23, 2016

		@@ -62,6 +64,8 @@

		private static final String DCTERMS_CREATOR = "dcterms.creator";

		private static final String DATE_FORMAT = "yyyy-MM-dd";

RAD-385 MrrtReportTemplate.dcTermsDate should be a Date object not a string #499

RAD-385 MrrtReportTemplate.dcTermsDate should be a Date object not a string #499

Conversation

ivange94 commented Oct 22, 2016

Description

Related Issue

Checklist:

mention-bot commented Oct 22, 2016

coveralls commented Oct 22, 2016

dkayiwa commented Oct 28, 2016

dkayiwa commented Nov 15, 2016

teleivo commented Nov 15, 2016

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

teleivo commented Nov 22, 2016

Choose a reason for hiding this comment

coveralls commented Nov 27, 2016

coveralls commented Nov 27, 2016

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

ivange94 Dec 11, 2016 • edited

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 Dec 11, 2016

ivange94 Dec 11, 2016 •

edited