-
Notifications
You must be signed in to change notification settings - Fork 57
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.
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
Updating to Platform 2.0.0 #66
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,7 +81,15 @@ | |
</exclusion> | ||
</exclusions> | ||
</dependency> | ||
|
||
|
||
<dependency> | ||
<groupId>org.simpleframework</groupId> | ||
<artifactId>simple-xml</artifactId> | ||
<version>2.7.1</version> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is the scope for this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added this to the test scope |
||
<scope>test</scope> | ||
</dependency> | ||
|
||
|
||
</dependencies> | ||
|
||
</project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,6 +21,7 @@ | |
|
||
import org.junit.Test; | ||
import org.openmrs.Concept; | ||
import org.openmrs.ConceptDescription; | ||
import org.openmrs.ConceptName; | ||
import org.openmrs.GlobalProperty; | ||
import org.openmrs.api.context.Context; | ||
|
@@ -42,7 +43,8 @@ public List<?> prepareExportServer() throws Exception { | |
Concept c = new Concept(); | ||
ConceptName conceptName = new ConceptName("1234567890-1234567890", Locale.US); | ||
c.addName(conceptName); | ||
|
||
c.addDescription(new ConceptDescription("Description", Locale.ENGLISH)); | ||
|
||
Context.getConceptService().saveConcept(c); | ||
|
||
// check to make sure the child server uses this one | ||
|
@@ -75,13 +77,17 @@ public void shouldNotPreserveConceptIdAcrossServersIfImportingServerDoesntReques | |
runShareTest(new ShareTestHelper() { | ||
|
||
Integer newConceptId = null; | ||
|
||
Concept concept; | ||
|
||
@Override | ||
public List<?> prepareExportServer() throws Exception { | ||
|
||
Concept c = new Concept(); | ||
c.addName(new ConceptName("Test", Locale.US)); | ||
|
||
c.addDescription(new ConceptDescription("Description", Locale.ENGLISH)); | ||
concept = c; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the above line? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In platform 2.0, the concept must have a description otherwise it gives the following error when saving.
|
||
|
||
Context.getConceptService().saveConcept(c); | ||
|
||
// check to make sure the child server uses this one | ||
|
@@ -104,8 +110,8 @@ public void prepareImportServer() throws Exception { | |
@Override | ||
public void runOnImportServerAfterImport() throws Exception { | ||
Concept c = Context.getConceptService().getConcept(newConceptId); | ||
Assert.assertNull("The concept was found with the same id, wha happened??", c); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you mind explaining why you changed this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In platform 2.0 |
||
|
||
Assert.assertEquals(concept, c); | ||
} | ||
}); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this does not have any runtime side effects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we run this in AutoFlushMode we get the following error: https://gist.github.com/wikumChamith/1a1c66da05937e3883d0f7c922871218
This might be caused because we have added Transactional here: https://github.com/openmrs/openmrs-core/blob/889ccc1cf4d13b4e09ae9ff3ea6713cb685b3cef/api/src/main/java/org/openmrs/api/db/hibernate/HibernateContextDAO.java#L319-L322
I tested this and didn't notice any problems. I think there is a chance this could slightly affect the performance.