TRUNK-4730 Verify that all relevant domain objects have changed_by and date_changed fields #1880

manuela-grindei · 2016-12-01T02:49:21Z

Description

Added new attributes to classes, added new mappings to Hibernate files, added corresponding Liquibase patches

Related Issue

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

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-12-01T02:49:22Z

@manuela-grindei, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bmamlin, @WolfSchlegel and @ivange94 to be potential reviewers.

coveralls · 2016-12-01T02:54:07Z

Coverage increased (+0.02%) to 55.747% when pulling 5cd7729 on manuela-grindei:master into 4dc0e2c on openmrs:master.

dkayiwa · 2016-12-03T23:25:23Z

@manuela-grindei thanks again for working on this! 👍
Could you fix the integration test failure that happen with these changes?
To see the failures, just run: mvn clean install -Pintegration-test

dkayiwa · 2016-12-04T00:39:05Z

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

+	<changeSet id="TRUNK-4730-20161114-1000" author="grindei" dbms="mysql">
+		<preConditions onFail="MARK_RAN">
+			<not>
+				<columnExists tableName="concept_class" columnName="date_changed"/>


Since the fix version is 2.1, can you move these changesets to liquibase-update-to-2.1.xml

manuela-grindei · 2016-12-04T02:56:51Z

Fixed integration test failures.

coveralls · 2016-12-04T03:02:38Z

Coverage increased (+0.04%) to 55.825% when pulling 78a6ad6 on manuela-grindei:master into 76aa48d on openmrs:master.

dkayiwa · 2016-12-04T08:39:42Z

@manuela-grindei can you squash these commits into one as advised at https://wiki.openmrs.org/display/docs/Pull+Request+Tips

… and liquibase patches

manuela-grindei · 2016-12-04T21:55:34Z

Squashed commits

dkayiwa · 2016-12-04T21:57:14Z

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

@@ -83,7 +83,6 @@ public void addAttribute(A attribute) {
 		if (getAttributes() == null) {
 			setAttributes(new LinkedHashSet<A>());
 		}
-		// TODO validate


Are these three TODO removals also part of this ticket?

I had to squash everything, including a merge of upstream master, which gave those TODO removals.

There used to be 3 commits, one of which was a master upstream merge

dkayiwa · 2016-12-04T21:58:39Z

api/src/test/resources/apiCacheConfig.properties

+subscription.maxElementsInMemory=500
+subscription.eternal=false
+subscription.timeToIdleSeconds=300
+subscription.timeToLiveSeconds=300


Is this file also part of this ticket?

dkayiwa · 2016-12-04T21:58:50Z

api/src/test/java/org/openmrs/api/cache/OpenmrsCacheManagerFactoryBeanTest.java

+ * 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.
+ *


Is this part of this ticket?

No, this comes from the upstream merge

dkayiwa · 2016-12-04T21:59:25Z

@manuela-grindei i see so many changes that have nothing to do with this ticket. Can you fix that?

manuela-grindei · 2016-12-04T21:59:54Z

I'm afraid I don't know how...

dkayiwa · 2016-12-04T22:01:00Z

Did you get a chance to read this https://wiki.openmrs.org/display/docs/Pull+Request+Tips and https://wiki.openmrs.org/display/docs/Pull+Request+Tips

manuela-grindei · 2016-12-04T22:01:24Z

Absolutely, I read it

dkayiwa · 2016-12-04T22:04:17Z

As a last resort, if you do not find it easy to separate your changes from other commits that have already been merged, you may just create a new branch and put back your changes one by one before making a new pull request.

coveralls · 2016-12-04T22:05:02Z

Coverage increased (+0.04%) to 55.825% when pulling 338f1f2 on manuela-grindei:master into 76aa48d on openmrs:master.

manuela-grindei · 2016-12-04T22:12:18Z

This looks very strange, I think I will need to delete the repo again and recreate everything using patches... However, we need to merge quickly next time, so that we do not need to squash anything again. Please let me know if you are fine with the actual changes, as we need the final version in the next PR to be able to quickly merge it into master. We could choose a time of very low activity in the repo, maybe during Christmas, to prevent upstream master changes from happening.

dkayiwa · 2016-12-04T22:21:28Z

@manuela-grindei there will always be a possibility of commits before your pull request is merged. Even if a pull request is open and reviewed in 30 minutes, there will always be a chance of commits that happen within for instance 15 minutes before the merging. This is the nature of distributed development. :)
You could choose a time of low activity when the activity is too low to even have anyone to review and merge your changes. :)
So the ultimate solution is frequently pulling upstream changes into your local ones and getting to master how to squash commits.
In the ideal world, we would merge pull requests immediately, but this is not always possible because of lots of other pull requests we need to test and review, amidst other activities.
Even after a review which may look like the final version, trying to test out your changes by running the web application may prove otherwise. :)

manuela-grindei · 2016-12-04T22:52:17Z

You're totally right in what you say.
I don't know why squashing commits doesn't work properly on my machine, I'll need to investigate and fix it, otherwise I'll be unable to contribute to openmrs.

manuela-grindei · 2016-12-04T22:54:20Z

In the meantime, I can provide a patch with the changes to you or someone else who can squash without issues, so we can still finish the story and merge the code in the repo.

dkayiwa · 2016-12-04T22:57:37Z

@manuela-grindei if you fail to get it right, you can post on talk, detailing the exact steps or commands you executed and the results. Then you will get answers from other contributors who have been there before. For i have seem many of them successfully squash their commits. So be sure that you will get help from others' experiences! :)

dkayiwa · 2016-12-04T23:01:37Z

@manuela-grindei my advice is that you do not miss out on the chance to learn this. As long as you plan to have a software development career, there are things you will have no option but patiently learn, regardless of the project you will chose to work on! :)
So this is not just about merging your contributions, but also giving you back an opportunity to learn from others! I never knew any of the things am sharing with you now, until when i decided to get my feet wet in an open source community like this. :)

dkayiwa reviewed Dec 4, 2016

View reviewed changes

TRUNK-4730 Added changedBy and dateChanged to Java classes, hbm files…

338f1f2

… and liquibase patches

manuela-grindei force-pushed the master branch from 78a6ad6 to 338f1f2 Compare December 4, 2016 21:54

dkayiwa reviewed Dec 4, 2016

View reviewed changes

manuela-grindei closed this Dec 4, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TRUNK-4730 Verify that all relevant domain objects have changed_by and date_changed fields #1880

TRUNK-4730 Verify that all relevant domain objects have changed_by and date_changed fields #1880

manuela-grindei commented Dec 1, 2016 •

edited

mention-bot commented Dec 1, 2016

coveralls commented Dec 1, 2016

dkayiwa commented Dec 3, 2016

dkayiwa Dec 4, 2016

manuela-grindei Dec 4, 2016

manuela-grindei commented Dec 4, 2016

coveralls commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa Dec 4, 2016 •

edited

manuela-grindei Dec 4, 2016

manuela-grindei Dec 4, 2016

dkayiwa Dec 4, 2016

dkayiwa Dec 4, 2016

manuela-grindei Dec 4, 2016

dkayiwa commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

coveralls commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016 •

edited

manuela-grindei commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

dkayiwa commented Dec 4, 2016 •

edited

TRUNK-4730 Verify that all relevant domain objects have changed_by and date_changed fields #1880

TRUNK-4730 Verify that all relevant domain objects have changed_by and date_changed fields #1880

Conversation

manuela-grindei commented Dec 1, 2016 • edited

Description

Related Issue

Checklist:

mention-bot commented Dec 1, 2016

coveralls commented Dec 1, 2016

dkayiwa commented Dec 3, 2016

dkayiwa Dec 4, 2016

Choose a reason for hiding this comment

manuela-grindei Dec 4, 2016

Choose a reason for hiding this comment

manuela-grindei commented Dec 4, 2016

coveralls commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa Dec 4, 2016 • edited

Choose a reason for hiding this comment

manuela-grindei Dec 4, 2016

Choose a reason for hiding this comment

manuela-grindei Dec 4, 2016

Choose a reason for hiding this comment

dkayiwa Dec 4, 2016

Choose a reason for hiding this comment

dkayiwa Dec 4, 2016

Choose a reason for hiding this comment

manuela-grindei Dec 4, 2016

Choose a reason for hiding this comment

dkayiwa commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

coveralls commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016 • edited

manuela-grindei commented Dec 4, 2016

manuela-grindei commented Dec 4, 2016

dkayiwa commented Dec 4, 2016

dkayiwa commented Dec 4, 2016 • edited

manuela-grindei commented Dec 1, 2016 •

edited

dkayiwa Dec 4, 2016 •

edited

dkayiwa commented Dec 4, 2016 •

edited

dkayiwa commented Dec 4, 2016 •

edited