Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 14 additions & 3 deletions ...ava/org/openmrs/module/radiology/report/template/DefaultMrrtReportTemplateFileParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
import java.io.ByteArrayInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.HashSet;
import java.util.List;
import java.util.Set;
Expand Down Expand Up @@ -78,7 +80,12 @@ public MrrtReportTemplate parse(String mrrtTemplate) throws IOException {

final Document doc = Jsoup.parse(mrrtTemplate, "");
final MrrtReportTemplate result = new MrrtReportTemplate();
initializeTemplate(result, doc);
try {
initializeTemplate(result, doc);
}
catch (ParseException e) {
throw new APIException("radiology.report.template.parser.error", null, e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

}
try {
addTermsToTemplate(result, doc.getElementsByTag("script")
.get(0)
Expand All @@ -90,7 +97,7 @@ public MrrtReportTemplate parse(String mrrtTemplate) throws IOException {
return result;
}

private final void initializeTemplate(MrrtReportTemplate template, Document doc) {
private final void initializeTemplate(MrrtReportTemplate template, Document doc) throws ParseException {
final Elements metaTags = doc.getElementsByTag("meta");

template.setPath(doc.baseUri());
Expand Down Expand Up @@ -125,7 +132,7 @@ private final void initializeTemplate(MrrtReportTemplate template, Document doc)
template.setDcTermsLicense(content);
break;
case DCTERMS_DATE:
template.setDcTermsDate(content);
parseDcTermsDate(template, content);
break;
case DCTERMS_CREATOR:
template.setDcTermsCreator(content);
Expand Down Expand Up @@ -180,4 +187,8 @@ private final ConceptSource getConceptSourceByName(String name, ConceptService c
}
return null;
}

private final void parseDcTermsDate(MrrtReportTemplate template, String content) throws ParseException {
template.setDcTermsDate(new SimpleDateFormat(MrrtReportTemplateConstants.DATE_FORMAT).parse(content));
}
}
29 changes: 27 additions & 2 deletions api/src/main/java/org/openmrs/module/radiology/report/template/MetaTagsValidationEngine.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,16 @@
*/
package org.openmrs.module.radiology.report.template;

import org.jsoup.select.Elements;

import java.text.ParseException;
import java.text.SimpleDateFormat;
import java.util.ArrayList;
import java.util.List;

import org.apache.commons.logging.Log;
import org.apache.commons.logging.LogFactory;
import org.jsoup.nodes.Element;
import org.jsoup.select.Elements;

/**
* Validates <meta> tags of an Mrrt Report Template.
*/
Expand All @@ -24,6 +29,8 @@ class MetaTagsValidationEngine implements ValidationEngine<Elements> {

static String SELECTOR_QUERY_META_ATTRIBUTE_NAME = "meta[name]";

static String SELECTOR_QUERY_META_ATTRIBUTE_DATE = "meta[name=dcterms.date]";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivange94 please address.


private List<ValidationRule<Elements>> rules;

public MetaTagsValidationEngine() {
Expand All @@ -35,6 +42,9 @@ public MetaTagsValidationEngine() {
new ElementsExpressionValidationRule("At least one 'meta' element encoding dublin core attributes expected",
"radiology.MrrtReportTemplate.validation.error.meta.dublinCore.missing",
SELECTOR_QUERY_META_ATTRIBUTE_NAME, subject -> subject.isEmpty()));
rules.add(new ElementsExpressionValidationRule("dcterms.date element should be a valid date",
"radiology.MrrtReportTemplate.validation.error.date.invalid", SELECTOR_QUERY_META_ATTRIBUTE_DATE,
subject -> isDateInvalid(subject)));
}

/**
Expand All @@ -53,4 +63,19 @@ public ValidationResult run(Elements subject) {
}
return validationResult;
}

private final boolean isDateInvalid(Elements elements) {
boolean result = false;
if (elements.isEmpty()) {
return result;
}
Element dcTermsDateElement = elements.get(0);
try {
new SimpleDateFormat(MrrtReportTemplateConstants.DATE_FORMAT).parse(dcTermsDateElement.attr("content"));
}
catch (ParseException ex) {
result = true;
}
return result;
}
}
17 changes: 13 additions & 4 deletions api/src/main/java/org/openmrs/module/radiology/report/template/MrrtReportTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
*/
package org.openmrs.module.radiology.report.template;

import java.util.Date;
import java.util.Set;

import org.openmrs.BaseOpenmrsData;
Expand Down Expand Up @@ -44,7 +45,7 @@ public class MrrtReportTemplate extends BaseOpenmrsData {

private String dcTermsLicense;

private String dcTermsDate;
private Date dcTermsDate;

private String dcTermsCreator;

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

public String getDcTermsDate() {
return dcTermsDate;
/**
* Get dcterms date property of an MRRT report template.
*
* @return Date date property of the MRRT report template or null if no date was set
*/
public Date getDcTermsDate() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ivange94 please address

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

if (dcTermsDate == null) {
return dcTermsDate;
}
return new Date(dcTermsDate.getTime());
}

public void setDcTermsDate(String dcTermsDate) {
public void setDcTermsDate(Date dcTermsDate) {
this.dcTermsDate = dcTermsDate;
}

Expand Down
30 changes: 30 additions & 0 deletions ...c/main/java/org/openmrs/module/radiology/report/template/MrrtReportTemplateConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
/**
* This Source Code Form is subject to the terms of the Mozilla Public License,
* v. 2.0. If a copy of the MPL was not distributed with this file, You can
* obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under
* the terms of the Healthcare Disclaimer located at http://openmrs.org/license.
*
* Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS
* graphic logo is a trademark of OpenMRS Inc.
*/
package org.openmrs.module.radiology.report.template;

/**
* Utility class that contains constants which are used within this module.
*/
public final class MrrtReportTemplateConstants {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed



/**
* Constant for dcterms.date format for an {@code MrrtReportTemplate}.
*
* See table "Table 4.105.4.1.2-1: HTTP Query Parameters" on page 27 of
* the MRRT Standards document.
*/
public static final String DATE_FORMAT = "yyyy-MM-dd";

private MrrtReportTemplateConstants() {
// Utility class not meant to be instantiated.
}

}
1 change: 1 addition & 0 deletions ...c/main/java/org/openmrs/module/radiology/report/template/MrrtReportTemplateValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ public interface MrrtReportTemplateValidator {
* @should throw api exception if html element does not have a body element
* @should throw api exception if html element has more than one body element
* @should catch all violation errors and throw an mrrt report template exception
* @should throw an mrrt report template exception if dcterms date of template file is not valid
*/
public void validate(String mrrtTemplate) throws IOException;
}
2 changes: 1 addition & 1 deletion api/src/main/resources/MrrtReportTemplate.hbm.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
<property name="dcTermsPublisher" column="dcterms_publisher" type="java.lang.String"/>
<property name="dcTermsRights" column="dcterms_rights" type="java.lang.String"/>
<property name="dcTermsLicense" column="dcterms_license" type="java.lang.String"/>
<property name="dcTermsDate" column="dcterms_date" type="java.lang.String"/>
<property name="dcTermsDate" column="dcterms_date" type="java.util.Date"/>
<property name="dcTermsCreator" column="dcterms_creator" type="java.lang.String"/>

<!-- bi-directional many-to-many association to ConceptReferenceTerm -->
Expand Down
4 changes: 4 additions & 0 deletions api/src/main/resources/liquibase.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -568,4 +568,8 @@
<column name="uuid" value="d9015276-b7b1-45f1-ad72-323896e75a52" />
</insert>
</changeSet>
<changeSet id="radiology-46" author="ivange94">
<comment>Change the datatype of dcterms_date from varchar(32) to date</comment>
<modifyDataType tableName="radiology_report_template" columnName="dcterms_date" newDataType="datetime"/>
</changeSet>
</databaseChangeLog>
5 changes: 4 additions & 1 deletion ...g/openmrs/module/radiology/report/template/MrrtReportTemplateFileParserComponentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.InputStream;
import java.text.SimpleDateFormat;
import java.util.Date;

import org.apache.commons.io.IOUtils;
import org.junit.Before;
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ivange94 ivange94 Dec 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


assertNotNull(template);
assertThat(template.getCharset(), is(CHARSET));
Expand All @@ -132,7 +135,7 @@ public void parse_shouldReturnAnMrrtTemplateObjectIfFileIsValid() throws Excepti
assertThat(template.getDcTermsPublisher(), is(TEST_DCTERMS_PUBLISHER));
assertThat(template.getDcTermsRights(), is(TEST_DCTERMS_RIGHTS));
assertThat(template.getDcTermsLicense(), is(TEST_DCTERMS_LICENSE));
assertThat(template.getDcTermsDate(), is(TEST_DCTERMS_DATE));
assertThat(template.getDcTermsDate(), is(dcTermsDate));
assertThat(template.getDcTermsCreator(), is(TEST_DCTERMS_CREATOR));
}

Expand Down
13 changes: 13 additions & 0 deletions ...rg/openmrs/module/radiology/report/template/MrrtReportTemplateValidatorComponentTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,4 +407,17 @@ public void validate_catchAllViolationErrorsAndThrowAnMrrtReportTemplateExceptio
is(4));
}
}

/**
* @see MrrtReportTemplateValidator#validate(String)
* @verifies throw an mrrt report template exception if dcterms date of template file is not valid
*/
@Test
public void validate_shouldThrowAnMrrtReportTemplateExceptionIfDctermsDateOfTemplateFileIsNotValid() throws Exception {

String templateContent = getFileContent(
"mrrttemplates/ihe/connectathon/2015/invalidMrrtReportTemplate-dcTermsDateIsNotAValidDateObject.html");
expectedException.expect(MrrtReportTemplateValidationException.class);
validator.validate(templateContent);
}
}
41 changes: 41 additions & 0 deletions ...tes/ihe/connectathon/2015/invalidMrrtReportTemplate-dcTermsDateIsNotAValidDateObject.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<!DOCTYPE html>
<html>
<head>
<title>CT Chest-Abdomen</title>
<meta charset="UTF-8"/>
<meta name="dcterms.title" content="CT Chest-Abdomen"/>
<meta name="dcterms.identifier" content="1.3.6.1.4.1.21367.13.199.1015"/>
<meta name="dcterms.description" content="CT Chest-Abdomen"/>
<meta name="dcterms.type" content="IMAGE_REPORT_TEMPLATE"/>
<meta name="dcterms.publisher" content="IHE CAT Publisher"/>
<meta name="dcterms.rights" content="IHE Connectathon Rights"/>
<meta name="dcterms.license" content="IHE Connectathon License"/>
<meta name="dcterms.date" content="2013"/>
<meta name="dcterms.creator" content="Creator James, et al."/>
<meta name="dcterms.language" content="en"/>
<link rel="stylesheet" type="text/css" href="IHE_Template_Style.css" />
<script type="text/xml">
<template_attributes>
<top-level-flag>true</top-level-flag>
<status>ACTIVE</status>
<coding_schemes> <coding_scheme name="RADLEX" designator="2.16.840.1.113883.6.256" /> </coding_schemes>
<term type="modality">
<code meaning="computed tomography" value="RID10321" scheme="RADLEX" />
</term>
<term type="body part">
<code meaning="abdomen" value="RID56" scheme="RADLEX" />
</term>
<term type="body part">
<code meaning="thorax" value="RID1243" scheme="RADLEX" />
</term>
<coded_content> </coded_content>
</template_attributes>
</script>
</head>
<body>
<section data-section-name="The Only Section">
<header class="level1">Section Header</header>
<p> This is the CT Chest-Abdomen report template</p>
</section>
</body>
</html>
Expand Down
1 change: 1 addition & 0 deletions omod/src/main/resources/messages.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 needs to be of format yyyy-MM-dd like for example 2016-01-24

@MODULE_ID@.patientId=Patient Id
@MODULE_ID@.patientFullName=Patient Full Name
Expand Down