Trunk-5411:Add doseLimitUnits field to Drug #2964

gitcliff · 2019-06-14T12:23:14Z

link:https://issues.openmrs.org/browse/TRUNK-5411?src=confmacro
Trunk-5411:Add doseLimitUnits field to Drug

coveralls · 2019-06-14T12:37:43Z

Coverage increased (+0.007%) to 59.85% when pulling b0b5b6f on gitcliff:Trunk-5411 into 7755cec on openmrs:master.

dkayiwa · 2019-06-18T10:46:18Z

You would need to also add a hibernate mapping file in addition to a liquibase changeset.

gitcliff · 2019-06-18T13:28:55Z

thanks @dkayiwa,, going to work on it

gitcliff · 2019-06-19T19:53:42Z

@dkayiwa i have added the changes you requested in the hibernate mappings and liquibase changeSet

dkayiwa · 2019-06-19T19:56:41Z

pom.xml

@@ -531,7 +531,7 @@
 				<plugin>
 					<groupId>org.apache.maven.plugins</groupId>
 					<artifactId>maven-surefire-plugin</artifactId>
-					<version>2.18.1</version>
+					<version>3.0.0-M3</version>


Is this change required as part of the ticket?

@dkayiwa initially i was getting test failures in the surefire, failing to execute the maven goal in the plugin untill i decided to upgrade it to the latest version as seen here
https://mvnrepository.com/artifact/org.apache.maven.plugins/maven-surefire-plugin/3.0.0-M3

This change is not related to the ticket and hence should not be in this pull request.

api/src/main/resources/liquibase-update-to-latest.xml

gitcliff · 2019-06-20T11:28:22Z

No description provided.

gitcliff · 2019-06-20T13:16:25Z

api/src/main/resources/liquibase-update-to-latest.xml

+            </column>
+        </addColumn>
+    </changeSet>
+


@dkayiwa are these changes enough to answer your column names naming conventions?

Just list a few column names here together with yours.

column name="cause_of_death_non_coded" type="varchar(255)"
column name="speciality_id" type="int"

column name="dose_Limit_Unit" type="int"
@dkayiwa i have listed the first two and then mine last

ooh thanks @dkayiwa,, i have seen the difference here because of using capital letters

@dkayiwa i have updated the changeSet column name

dkayiwa · 2019-06-20T19:33:21Z

api/src/main/java/org/openmrs/Drug.java

@@ -276,4 +278,18 @@ public void addDrugReferenceMap(DrugReferenceMap drugReferenceMap) {
 			getDrugReferenceMaps().add(drugReferenceMap);
 		}
 	}
+
+	/** Returns the doseLimitUnits of a particular drug */


Can you do proper method javadoc?

dkayiwa · 2019-06-20T19:33:38Z

api/src/main/java/org/openmrs/Drug.java

+	}
+
+	/**
+	 * @param doseLimitUnits is the doseLimitUnits to be stored in the drug


Can you do proper method javadoc?

api/src/main/resources/liquibase-update-to-2.3.xml

dkayiwa · 2019-06-20T19:34:04Z

api/src/main/resources/liquibase-schema-only.xml

@@ -3436,7 +3436,7 @@
 	<changeSet id="20120529-2231" author="mvorobey">
        <addForeignKeyConstraint constraintName="privilege_which_can_edit_encounter_type" baseTableName="encounter_type" baseColumnNames="edit_privilege" referencedTableName="privilege" referencedColumnNames="privilege"/>
 	</changeSet>
-


Can you discard these formatting changes?

api/src/main/resources/org/openmrs/api/db/hibernate/Drug.hbm.xml

dkayiwa · 2019-06-20T19:36:52Z

api/src/main/resources/liquibase-update-to-latest.xml

+				<columnExists tableName="drug" columnName="dose_limit_unit"/>				
+			</not>
+		</preConditions>
+		<comment>Adding column dose limit unit in table drug</comment>


Shouldn't you also add a foreign key constraint?

yes @dkayiwa am giong to add it

dkayiwa · 2019-06-20T19:37:11Z

api/src/main/resources/liquibase-update-to-latest.xml

+		<comment>Adding column dose limit unit in table drug</comment>
+        <addColumn tableName="drug">
+            <column name="dose_limit_unit" type="int">
+            	<constraints nullable="false"/>


Are you sure that this column should not allow nulls?

@dkayiwa my thinking on this is that since this column is going to will represent the units of the existing fields of maximumDailyDose and minimumDailyDose of a given drug and a do believe every drug must these fields catered for which necessitates the doselimitunit not to all nulls

api/src/main/java/org/openmrs/Drug.java

dkayiwa · 2019-06-25T07:42:36Z

api/src/main/resources/liquibase-update-to-latest.xml

@@ -20,5 +20,23 @@
    <include file="liquibase-update-to-2.1.xml"/>
    <include file="liquibase-update-to-2.2.xml"/>
    <include file="liquibase-update-to-2.3.xml"/>
+    <changeSet id="Trunk-5411:Add dose limit unit field to Drug" author="gitacliff">


Can you make your changeset id be of the same format like the others?

The changeset should instead be in the liquibase-update-to-2.3.xml file

dkayiwa · 2019-07-24T16:59:25Z

Did you see the travis build failure?

gitcliff · 2019-07-25T12:40:01Z

@dkayiwa yes i have seen it , am getting build success with "mvn clean install" and when i make changes in the travis.yml file to use openjdk8 instead of oraclejdk8 i get successful travis CI build, the changes in the travis .yml seem to be correcting the travis failure yet the file is not part of the ticket...
Some guidance on how to go about this

dkayiwa · 2019-07-25T13:19:23Z

The travis.yml changes are necessary. But not the others as per my inline comments.

gitcliff · 2019-07-25T13:42:51Z

@dkayiwa thank you for the clarification

api/src/main/java/org/openmrs/Drug.java

dkayiwa · 2019-07-25T19:18:12Z

api/src/main/java/org/openmrs/Drug.java

+	}
+
+	/**
+	 * Sets the doseLimitUnits to this drug


Shouldn't this match the description that is in the ticket?

gitcliff · 2019-07-25T22:38:20Z

@dkayiwa i have matched the description to that which is in the ticket,,hope its okay to be merged

dkayiwa · 2019-07-26T07:55:43Z

Did you commit? I do not see any changes.

gitcliff · 2019-07-26T08:35:23Z

yes @dkayiwa i did commit the changes
"Gets the doseLimitUnits field of drug"
"Sets the doseLimitUnits field to drug"

dkayiwa · 2019-07-26T10:19:12Z

Is that how the ticket defines that field?

gitcliff · 2019-07-26T11:54:20Z

@dkayiwa not really,, i have made some changes and requesting for your review.Thanks

dkayiwa · 2019-07-26T12:10:53Z

api/src/main/java/org/openmrs/Drug.java

@@ -276,4 +278,26 @@ public void addDrugReferenceMap(DrugReferenceMap drugReferenceMap) {
 			getDrugReferenceMaps().add(drugReferenceMap);
 		}
 	}
+
+	/**
+	 * Gets doseLimitUnits field to Drug, which will represent the units of the existing fields of


Given that this is already the Dug class, what value does Drug add to the description?

gitcliff · 2019-07-26T12:33:56Z

@dkayiwa i think the value that Drug adds to the description is doseLimitsUnits

dkayiwa · 2019-07-26T12:36:40Z

Can you read your description again?

gitcliff · 2019-07-26T13:45:59Z

@dkayiwa i have read the description and made changes again,kindly review

dkayiwa · 2019-07-26T13:48:22Z

What you have done is not put the description that is in the ticket. Take a step back and ask yourself that, If you were the user of this API, what kind of description would you like to see to understand what this field is used for.

gitcliff · 2019-07-26T14:38:16Z

@dkayiwa is this new description that i have edited okay?,,,,kindly review

gitcliff force-pushed the Trunk-5411 branch from 30c2cd3 to 010a90b Compare June 16, 2019 08:37

gitcliff force-pushed the Trunk-5411 branch from 010a90b to 280627e Compare June 19, 2019 19:39

dkayiwa reviewed Jun 19, 2019

View reviewed changes

api/src/main/resources/liquibase-update-to-latest.xml Outdated Show resolved Hide resolved

gitcliff force-pushed the Trunk-5411 branch from 280627e to 33be62d Compare June 20, 2019 08:57

gitcliff force-pushed the Trunk-5411 branch from 33be62d to 780a93a Compare June 20, 2019 12:53

gitcliff commented Jun 20, 2019

View reviewed changes

gitcliff force-pushed the Trunk-5411 branch 2 times, most recently from c72e85e to 14bc7df Compare June 20, 2019 19:29

dkayiwa reviewed Jun 20, 2019

View reviewed changes

api/src/main/resources/liquibase-update-to-2.3.xml Outdated Show resolved Hide resolved

dkayiwa reviewed Jun 20, 2019

View reviewed changes

api/src/main/resources/org/openmrs/api/db/hibernate/Drug.hbm.xml Show resolved Hide resolved

dkayiwa reviewed Jun 20, 2019

View reviewed changes

gitcliff force-pushed the Trunk-5411 branch 3 times, most recently from eecfb65 to c10f040 Compare June 25, 2019 07:08

dkayiwa reviewed Jun 25, 2019

View reviewed changes

api/src/main/java/org/openmrs/Drug.java Show resolved Hide resolved

dkayiwa reviewed Jun 25, 2019

View reviewed changes

api/src/main/java/org/openmrs/Drug.java Show resolved Hide resolved

dkayiwa reviewed Jun 25, 2019

View reviewed changes

gitcliff force-pushed the Trunk-5411 branch 3 times, most recently from 2efb5f7 to 5d8ee9d Compare June 25, 2019 13:48

gitcliff force-pushed the Trunk-5411 branch 3 times, most recently from 81d4193 to c0d81c7 Compare July 24, 2019 14:51

gitcliff force-pushed the Trunk-5411 branch 3 times, most recently from bddd950 to 8ff6787 Compare July 25, 2019 12:14

gitcliff force-pushed the Trunk-5411 branch from 8ff6787 to 41a5215 Compare July 25, 2019 14:14

dkayiwa reviewed Jul 25, 2019

View reviewed changes

api/src/main/java/org/openmrs/Drug.java Outdated Show resolved Hide resolved

dkayiwa reviewed Jul 25, 2019

View reviewed changes

gitcliff force-pushed the Trunk-5411 branch from 41a5215 to a8bb978 Compare July 25, 2019 22:12

gitcliff force-pushed the Trunk-5411 branch from a8bb978 to 523c741 Compare July 26, 2019 11:39

dkayiwa reviewed Jul 26, 2019

View reviewed changes

gitcliff force-pushed the Trunk-5411 branch from 523c741 to c0e8f9b Compare July 26, 2019 13:36

TRUNK-5411:Add doselimitsunits field to Drug

b0b5b6f

gitcliff force-pushed the Trunk-5411 branch from c0e8f9b to b0b5b6f Compare July 26, 2019 14:34

dkayiwa merged commit 487bcf1 into openmrs:master Jul 26, 2019

Trunk-5411:Add doseLimitUnits field to Drug #2964

Trunk-5411:Add doseLimitUnits field to Drug #2964

Conversation

gitcliff commented Jun 14, 2019 • edited

coveralls commented Jun 14, 2019 • edited

dkayiwa commented Jun 18, 2019

gitcliff commented Jun 18, 2019

gitcliff commented Jun 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitcliff commented Jun 20, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitcliff Jun 20, 2019 • 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

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

dkayiwa commented Jul 24, 2019

gitcliff commented Jul 25, 2019 • edited

dkayiwa commented Jul 25, 2019

gitcliff commented Jul 25, 2019

Choose a reason for hiding this comment

gitcliff commented Jul 25, 2019

dkayiwa commented Jul 26, 2019

gitcliff commented Jul 26, 2019

dkayiwa commented Jul 26, 2019

gitcliff commented Jul 26, 2019 • edited

dkayiwa Jul 26, 2019 • edited

Choose a reason for hiding this comment

gitcliff commented Jul 26, 2019

dkayiwa commented Jul 26, 2019

gitcliff commented Jul 26, 2019

dkayiwa commented Jul 26, 2019

gitcliff commented Jul 26, 2019

gitcliff commented Jun 14, 2019 •

edited

coveralls commented Jun 14, 2019 •

edited

gitcliff Jun 20, 2019 •

edited

gitcliff commented Jul 25, 2019 •

edited

gitcliff commented Jul 26, 2019 •

edited

dkayiwa Jul 26, 2019 •

edited