TRUNK-4917 - Add Creatable and Changeable interfaces #2245

wluyima · 2017-09-22T21:01:52Z

Description of what I changed

I added the new Creatable and Changeable interfaces, deprecated the changedBy and dateChanged setters and getters in the relevant super classes

This pull request is just a partial commit so that I can get some feedback from other devs before I can go ahead to do the major refactoring of updating the domain objects to extend BaseChangeableOpenmrsData or BaseChangeableOpenmrsMetadata

Issue I worked on

see https://issues.openmrs.org/browse/TRUNK-4917

wluyima · 2017-09-22T21:02:39Z

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

-	 */
-	public void setDateChanged(Date dateChanged);
-
+public interface Auditable extends Creatable, Changeable {


Should we actually get rid of this interface? Or should we keep it around?

Unless we can scan core code and popular modules and ensure nobody is referencing it, I would keep it around for now.

Definitely it's referenced in core, but in the follow up commit I was going to replace its usage. I wasn't going to remove it in this version, we would mark it as deprecated now and eventually remove it may be 2 versions later.

wluyima · 2017-09-22T21:05:10Z

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

+ * @since 2.2
+ */
+public abstract class BaseChangeableOpenmrsData extends BaseOpenmrsData {
+


I added these overrides so that these setters/getters don't appear deprecated as an effect from the superclass since eventually the methods will still exist on this class.

Make sense.

public interface OpenmrsData extends OpenmrsObject, Auditable, Voidable {}
Which more methods will be added in OpenmrsData interface?
Or OpenmrsData interface will implement the methods of Creatable interface which contain getCreator, getDateCreated methods and Changeable interface contain getChangedBy, getDateChanged methods?

OpenmrsData for now will stay the way it is, in future it will implement Creatable instead of Auditable because Data should be immutable by default.

wluyima · 2017-09-22T21:10:29Z

@djazayeri and @bmamlin does this seem in line with option D from this design call?

coveralls · 2017-09-22T21:11:13Z

Coverage decreased (-0.06%) to 56.502% when pulling abb327c on wluyima:TRUNK-4917-2 into 42b4339 on openmrs:master.

coveralls · 2017-09-29T18:17:07Z

Coverage increased (+0.05%) to 56.608% when pulling fa57c91 on wluyima:TRUNK-4917-2 into 42b4339 on openmrs:master.

bmamlin

Looks good. I think the summary for TRUNK-4917 needs to be updated to reflect what you've done here (i.e., "Changeable" vs. "Mutable"). Also, would like to know how big of an effect this has on core and popular modules.

bmamlin · 2017-10-04T15:36:38Z

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

- * @see OpenmrsData
- * @see OpenmrsMetadata
+ * @see MutableOpenmrsData
+ * @see MutableOpenmrsMetadata


Do MutableOpenmrsData and MutableOpenmrsMetadata classes exist outside of this PR? Or should these be references Changeable, BaseChangeableOpenmrsData, and BaseChangeableOpenmrsMetadata classes?

Good catch! I had them and removed them and should have updated these javadocs

bmamlin · 2017-10-04T15:40:45Z

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

-	 */
-	public void setDateChanged(Date dateChanged);
-
+public interface Auditable extends Creatable, Changeable {


Unless we can scan core code and popular modules and ensure nobody is referencing it, I would keep it around for now.

bmamlin · 2017-10-04T15:41:01Z

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

+ * @since 2.2
+ */
+public abstract class BaseChangeableOpenmrsData extends BaseOpenmrsData {
+


Make sense.

bmamlin · 2017-10-04T15:41:49Z

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

 	 */
 	@Override
 	public void setDateCreated(Date dateCreated) {
 		this.dateCreated = dateCreated;
 	}

 	/**
-	 * @see org.openmrs.Auditable#getChangedBy()
+	 * @deprecated


I assume @deprecated in javadoc would ideally include "as of version x.y".

Yes, I will add it

wluyima · 2017-10-05T18:07:54Z

In core we are going to change subclasses of BaseOpenmrsData to extend BaseChangeableOpenmrsData instead, likewise subclasses of BaseOpenmrsMetadata will extend BaseChangeableOpenmrsMetadata. The same will apply for all modules, although it doesn't have to be immediate but will have to once we remove the deprecated code.

@SInCE

…escriptions for the @SInCE annotations TRUNK-4917 - Removed non-existent type references

coveralls · 2017-10-19T22:03:12Z

Coverage decreased (-0.02%) to 56.535% when pulling cc711fe on wluyima:TRUNK-4917-2 into 42b4339 on openmrs:master.

wluyima · 2017-10-23T19:55:05Z

@bmamlin I did address the review comments, I think we can merge this along with the change of all subclasses of BaseOpenmrsData and BaseOpenmrsMetadata to extend the new base classes.

wluyima · 2017-11-06T22:09:13Z

@dkayiwa , @rkorytkowski do you mind merging this PR after you review it? I need to do the next step of changing domain objects to extend BaseChangeableOpenmrsData or BaseChangeableOpenmrsMetadata instead.

dkayiwa · 2017-11-07T14:58:07Z

@wluyima will it take you much time to squash these commits into one? I find it easier to review one commit than multiple, especially when they are followup commits to address earlier review comments.

wluyima · 2017-11-07T18:51:07Z

@dkayiwa you shouldn't be looking at individual commits, the PR shows them as one

wluyima · 2017-11-07T18:52:34Z

Aren't you viewing all the changes as one in Files changed tab on the PR?

wluyima · 2017-11-09T21:14:23Z

Still waiting on you @dkayiwa

dkayiwa · 2017-11-10T13:56:27Z

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

 	 */
 	@Override
 	public void setDateCreated(Date dateCreated) {
 		this.dateCreated = dateCreated;
 	}

 	/**
-	 * @see org.openmrs.Auditable#getChangedBy()
+	 * @see org.openmrs.OpenmrsMetadata#getChangedBy()
+	 * @deprecated as of version 2.2


Our convention has been to include what they should alternatively use. https://wiki.openmrs.org/display/docs/Java+Conventions#JavaConventions-Deprecation

The methods are actually going away with no replacement, I think I just have to add more details as to why it was necessary to remove the method.

And to mention that we introduced a new interface that one would need to implement in case they want to have a mutable class

Actually the method javadocs have the @see the superclass where information as to why it's deprecated is included

@SInCE

* TRUNK-4917 - Add Creatable and Changeable interfaces * TRUNK-4917 - Renamed usaged of Mutable to Changeable * TRUNK-4917 - Removed unnecessary deprecation comments * TRUNK-4917 - Removed non-existent type references and added missing descriptions for the @SInCE annotations TRUNK-4917 - Removed non-existent type references

wluyima added 2 commits September 22, 2017 16:03

TRUNK-4917 - Add Creatable and Changeable interfaces

f9cef64

TRUNK-4917 - Renamed usaged of Mutable to Changeable

abb327c

wluyima commented Sep 22, 2017

View reviewed changes

TRUNK-4917 - Removed unnecessary deprecation comments

fa57c91

bmamlin reviewed Oct 4, 2017

View reviewed changes

TRUNK-4917 - Removed non-existent type references and added missing d…

cc711fe

…escriptions for the @SInCE annotations TRUNK-4917 - Removed non-existent type references

openmrs deleted a comment Oct 19, 2017

dkayiwa reviewed Nov 10, 2017

View reviewed changes

dkayiwa merged commit 0d8bf59 into openmrs:master Nov 10, 2017

wluyima deleted the TRUNK-4917-2 branch November 14, 2017 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRUNK-4917 - Add Creatable and Changeable interfaces #2245

TRUNK-4917 - Add Creatable and Changeable interfaces #2245

wluyima commented Sep 22, 2017

wluyima Sep 22, 2017

bmamlin Oct 4, 2017

wluyima Oct 5, 2017

wluyima Sep 22, 2017

bmamlin Oct 4, 2017

ravirajmane Nov 11, 2017

wluyima Nov 13, 2017 •

edited

wluyima commented Sep 22, 2017 •

edited

coveralls commented Sep 22, 2017

coveralls commented Sep 29, 2017

bmamlin left a comment

bmamlin Oct 4, 2017

wluyima Oct 5, 2017 •

edited

bmamlin Oct 4, 2017

bmamlin Oct 4, 2017

bmamlin Oct 4, 2017

wluyima Oct 5, 2017

wluyima commented Oct 5, 2017

coveralls commented Oct 19, 2017

wluyima commented Oct 23, 2017

wluyima commented Nov 6, 2017 •

edited

dkayiwa commented Nov 7, 2017

wluyima commented Nov 7, 2017

wluyima commented Nov 7, 2017 •

edited

wluyima commented Nov 9, 2017

dkayiwa Nov 10, 2017

wluyima Nov 10, 2017

wluyima Nov 10, 2017 •

edited

wluyima Nov 10, 2017

TRUNK-4917 - Add Creatable and Changeable interfaces #2245

TRUNK-4917 - Add Creatable and Changeable interfaces #2245

Conversation

wluyima commented Sep 22, 2017

Description of what I changed

Issue I worked on

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

wluyima Nov 13, 2017 • edited

Choose a reason for hiding this comment

wluyima commented Sep 22, 2017 • edited

coveralls commented Sep 22, 2017

coveralls commented Sep 29, 2017

bmamlin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wluyima Oct 5, 2017 • 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

wluyima commented Oct 5, 2017

coveralls commented Oct 19, 2017

wluyima commented Oct 23, 2017

wluyima commented Nov 6, 2017 • edited

dkayiwa commented Nov 7, 2017

wluyima commented Nov 7, 2017

wluyima commented Nov 7, 2017 • edited

wluyima commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wluyima Nov 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wluyima Nov 13, 2017 •

edited

wluyima commented Sep 22, 2017 •

edited

wluyima Oct 5, 2017 •

edited

wluyima commented Nov 6, 2017 •

edited

wluyima commented Nov 7, 2017 •

edited

wluyima Nov 10, 2017 •

edited