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

RESTWS-739:Reposting metadata should not persist changes #414

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

RESTWS-739:Reposting metadata should not persist changes #414

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
14 changes: 13 additions & 1 deletion .../module/webservices/rest/web/v1_0/controller/openmrs1_10/OrderTypeController1_10Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public void shouldCreateANewOrderType() throws Exception {
assertEquals(orderType.get("description"), Util.getByPath(newOrder, "description"));
}

@Test
@Test(expected = RuntimeException.class)
Copy link
Member

@mozzy11 mozzy11 Nov 26, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@gitcliff after your changes , now these tests return Exceptions ?? Thats wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mozzy11 these tests were run against the update method in DelegatingCrudResource which was allowing reposting of changes metadata but the ticket description says this should not happen any more with metadata hence notifying the client with an exception ,,,thats why the tests that were permitiing the reposting of metadata data initially are now throwing exceptional errors

Copy link
Member

Choose a reason for hiding this comment

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

@gitcliff ,the tickect talks about forbidding re-posting metadata, not updating metadata.
So idealy updating meta-data shouldnt have any problem

Copy link
Member

Choose a reason for hiding this comment

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

Yes, @mozzy11 and @gitcliff I haven't looked deeply at the code but looking at the ticket, the idea is, for example, if you post to the Encounter endpoint, although you can change the encounter type of that encounter, you shouldn't be able to inadvertantly change any of the underlying properties (name, etc) of that encounter type. However, you should be able to post directly to the EncounterType endpoint and change the name, etc.

The idea is that you shouldn't be able to by mistake change a piece of metadata when updating some data that refers to that metadata. Hope that makes sense!

Copy link
Member

Choose a reason for hiding this comment

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

makes sense @mogoodrich

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mozzy11 u can also have a look a this , it concurs with what @mogoodrich was explaining above

public void shouldEditAnOrderType() throws Exception {
final String newName = "Updated name";
SimpleObject conceptMapTypeType = new SimpleObject();
Expand All @@ -176,6 +176,18 @@ public void shouldEditAnOrderType() throws Exception {
assertEquals(newName, service.getOrderTypeByUuid(getUuid()).getName());
}

@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAnAnOrderType() throws Exception {
final String newName = "updated name";
SimpleObject orderType = new SimpleObject();
Copy link

Choose a reason for hiding this comment

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

Suggested change
SimpleObject orderType = new SimpleObject();
final SimpleObject orderType = new SimpleObject();

orderType.add("name", newName);
String json = new ObjectMapper().writeValueAsString(orderType);
Copy link

Choose a reason for hiding this comment

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

new ObjectMapper() is resource consuming. Therefore, create one instance for all the relevant unit tests and re-use that instance. 💭

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);

}

@Test
public void shouldRetireAnOrderType() throws Exception {
assertEquals(false, service.getOrderTypeByUuid(getUuid()).isRetired());
Expand Down
15 changes: 13 additions & 2 deletions ...s/module/webservices/rest/web/v1_0/controller/openmrs1_12/OrderSetController1_12Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
*/
package org.openmrs.module.webservices.rest.web.v1_0.controller.openmrs1_12;

import static org.junit.Assert.assertEquals;

import org.apache.commons.beanutils.PropertyUtils;
import org.codehaus.jackson.map.ObjectMapper;
import org.junit.Assert;
Expand All @@ -23,7 +25,6 @@
import org.openmrs.module.webservices.rest.web.v1_0.controller.MainResourceControllerTest;
import org.springframework.mock.web.MockHttpServletRequest;
import org.springframework.web.bind.annotation.RequestMethod;
import static org.junit.Assert.assertEquals;

/**
* Tests functionality of OrderSet CRUD by MainResourceController
Expand Down Expand Up @@ -127,7 +128,7 @@ public void shouldCreateAnOrderSetWithSomeOrderSetMembers() throws Exception {
Assert.assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditAnOrderSet() throws Exception {

final String editedName = "OrderSet Edited";
Expand All @@ -141,6 +142,16 @@ public void shouldEditAnOrderSet() throws Exception {
Assert.assertNotNull(editedOrderSet);
Assert.assertEquals(editedName, editedOrderSet.getName());
}

@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAnAnOrderSet() throws Exception {

final String editedName = "OrderSet Edited";
String json = "{ \"name\":\"" + editedName + "\" }";
Copy link

Choose a reason for hiding this comment

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

I would rename the json to something meaning like editNameJson

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldRetireAnOrderSet() throws Exception {
Expand Down
17 changes: 16 additions & 1 deletion ...odule/webservices/rest/web/v1_0/controller/openmrs1_8/ConceptSourceController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
*/
package org.openmrs.module.webservices.rest.web.v1_0.controller.openmrs1_8;

import static org.junit.Assert.assertNull;

import java.util.Date;
import org.apache.commons.beanutils.PropertyUtils;
import org.codehaus.jackson.map.ObjectMapper;
Expand Down Expand Up @@ -114,7 +116,7 @@ public void shouldCreateAConceptSource() throws Exception {
Assert.assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditAConceptSource() throws Exception {
final String newName = "updated name";
SimpleObject conceptSource = new SimpleObject();
Expand All @@ -128,6 +130,19 @@ public void shouldEditAConceptSource() throws Exception {
Assert.assertEquals(newName, service.getConceptSourceByUuid(getUuid()).getName());
}

@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAConceptSource() throws Exception {
final String newName = "updated name";
SimpleObject conceptSource = new SimpleObject();
Copy link

Choose a reason for hiding this comment

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

Suggested change
SimpleObject conceptSource = new SimpleObject();
final SimpleObject conceptSource = new SimpleObject();

conceptSource.add("name", newName);
Copy link

Choose a reason for hiding this comment

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

Can't you use a builder for SimpleObject?


String json = new ObjectMapper().writeValueAsString(conceptSource);
Copy link

Choose a reason for hiding this comment

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

Suggested change
String json = new ObjectMapper().writeValueAsString(conceptSource);
final String json = new ObjectMapper().writeValueAsString(conceptSource);

Copy link

Choose a reason for hiding this comment

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

try to re-use an ObjectMapper()


MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldRetireAConceptSource() throws Exception {
Assert.assertEquals(false, service.getConceptSourceByUuid(getUuid()).isRetired());
Expand Down
13 changes: 12 additions & 1 deletion ...odule/webservices/rest/web/v1_0/controller/openmrs1_8/EncounterTypeController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public void shouldCreateAEncounterType() throws Exception {
assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditingAnEncounterType() throws Exception {
final String newName = "updated name";
SimpleObject encounterType = new SimpleObject();
Expand All @@ -123,6 +123,17 @@ public void shouldEditingAnEncounterType() throws Exception {
assertEquals(newName, service.getEncounterTypeByUuid(getUuid()).getName());
}

@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAnEncounterTypeProperty() throws Exception {
Copy link

Choose a reason for hiding this comment

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

shouldThrowAnExceptionWhenEditingAnEncounterTypeProperty, by design there is no way to edit an EncounterType properties? am I correct? Or do you think you need to include the exact criteria when this happens to the unit test name? 😉

final String newName = "updated name";
SimpleObject encounterType = new SimpleObject();
encounterType.add("name", newName);
String json = new ObjectMapper().writeValueAsString(encounterType);
MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldRetireAEncounterType() throws Exception {
assertEquals(false, service.getEncounterTypeByUuid(getUuid()).isRetired());
Expand Down
12 changes: 11 additions & 1 deletion .../module/webservices/rest/web/v1_0/controller/openmrs1_8/LocationTagController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public void shouldListAll() throws Exception {

}

@Test
@Test(expected = RuntimeException.class)
public void shouldUpdateLocationTag() throws Exception {

final String editedName = "Location Tag edited";
Expand All @@ -122,6 +122,16 @@ public void shouldUpdateLocationTag() throws Exception {

}

@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingALocationTag() throws Exception {
Copy link

Choose a reason for hiding this comment

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

same question like the previous test case name applies to this as well.


final String editedName = "Location Tag edited";
String json = "{ \"name\":\"" + editedName + "\" }";
MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldPurgeLocationTag() throws Exception {

Expand Down
14 changes: 13 additions & 1 deletion ...bservices/rest/web/v1_0/controller/openmrs1_8/PatientIdentifierTypeController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public void shouldCreateAPatientIdentifierType() throws Exception {
assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditingAPatientIdentifierType() throws Exception {
final String newName = "updated name";
SimpleObject patientIdentifierType = new SimpleObject();
Expand All @@ -122,6 +122,18 @@ public void shouldEditingAPatientIdentifierType() throws Exception {
assertEquals(newName, service.getPatientIdentifierTypeByUuid(getUuid()).getName());
}

@Test
public void shouldThrowAnExceptionWhenEditingAPatientIdentifierType() throws Exception {
final String newName = "updated name";
SimpleObject patientIdentifierType = new SimpleObject();
patientIdentifierType.add("name", newName);

String json = new ObjectMapper().writeValueAsString(patientIdentifierType);

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
}

@Test
public void shouldRetireAPatientIdentifierType() throws Exception {
assertEquals(false, service.getPatientIdentifierTypeByUuid(getUuid()).isRetired());
Expand Down
18 changes: 17 additions & 1 deletion ...webservices/rest/web/v1_0/controller/openmrs1_8/PersonAttributeTypeController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ public void getPersonAttributeType_shouldGetAFullRepresentationOfAPersonAttribut
* @throws Exception
* @verifies change a property on a person
*/
@Test
@Test(expected = RuntimeException.class)
public void updatePersonAttributeType_shouldChangeAPropertyOnAPersonAttributeType() throws Exception {

final String newDescription = "Updated description";
Expand All @@ -129,6 +129,22 @@ public void updatePersonAttributeType_shouldChangeAPropertyOnAPersonAttributeTyp
Util.log("Edited PersonAttributeType Description: ", editedAttr.getDescription());
}

@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAPersonAttributeType() throws Exception {

final String newDescription = "Updated description";

PersonAttributeType obj = service.getPersonAttributeTypeByUuid(getUuid());
Assert.assertNotNull(obj);
Assert.assertFalse(newDescription.equals(obj.getDescription()));
Util.log("Old PersonAttributeType Description: ", obj.getDescription());
Copy link

Choose a reason for hiding this comment

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

This log statement will not be useful when we are running the test. Because we will expect an Exception to be thrown. So, in this situation why do you think this information is useful to see?


String json = "{\"description\":\"Updated description\"}";
MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

/**
* @see PersonAttributeTypeController#retirePersonAttributeType(PersonAttributeType,String,WebRequest)
* @throws Exception
Expand Down
16 changes: 15 additions & 1 deletion ...rs/module/webservices/rest/web/v1_0/controller/openmrs1_8/PrivilegeController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public void shouldCreateAPrivilege() throws Exception {
assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditingAPrivilege() throws Exception {
final String newDescription = "updated descr";
SimpleObject privilege = new SimpleObject();
Expand All @@ -120,6 +120,20 @@ public void shouldEditingAPrivilege() throws Exception {
assertEquals(newDescription, service.getPrivilegeByUuid(getUuid()).getDescription());
}

@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAPrivilage() throws Exception {
final String newDescription = "updated descr";
SimpleObject privilege = new SimpleObject();
assertEquals(false, newDescription.equals(service.getPrivilegeByUuid(getUuid()).getName()));
privilege.add("description", newDescription);

String json = new ObjectMapper().writeValueAsString(privilege);

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldPurgeAPrivilege() throws Exception {
assertNotNull(service.getPrivilegeByUuid(getUuid()));
Expand Down
12 changes: 11 additions & 1 deletion ...openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_8/RoleController1_8Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ public void getRole_shouldGetAFullRepresentationOfARole() throws Exception {
* HttpServletResponse)
* @verifies change a property on a Role
*/
@Test
@Test(expected = RuntimeException.class)
public void updateRole_shouldChangeAPropertyOnARole() throws Exception {

final String editedDescription = "Role description edited";
Expand All @@ -141,6 +141,16 @@ public void updateRole_shouldChangeAPropertyOnARole() throws Exception {

}

@Test(expected = RuntimeException.class)
public void updateRole_shouldThrowAnExceptionWhenEditingAPrivilage() throws Exception {

final String editedDescription = "Role description edited";
String json = "{ \"description\":\"" + editedDescription + "\" }";
MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

/**
* @see RoleController#delete(String, String, javax.servlet.http.HttpServletRequest,
* HttpServletResponse)
Expand Down
14 changes: 13 additions & 1 deletion ...dule/webservices/rest/web/v1_0/controller/openmrs1_9/ConceptMapTypeController1_9Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ public void shouldCreateAConceptMapType() throws Exception {
assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditingAConceptMapType() throws Exception {
final String newName = "updated name";
SimpleObject conceptMapTypeType = new SimpleObject();
Expand All @@ -118,6 +118,18 @@ public void shouldEditingAConceptMapType() throws Exception {
handle(req);
assertEquals(newName, service.getConceptMapTypeByUuid(getUuid()).getName());
}
@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAConceptMapType() throws Exception {
final String newName = "updated name";
Copy link

Choose a reason for hiding this comment

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

Suggested change
final String newName = "updated name";
final String newConceptName = "updated name";

SimpleObject conceptMapTypeType = new SimpleObject();
conceptMapTypeType.add("name", newName);

String json = new ObjectMapper().writeValueAsString(conceptMapTypeType);

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldRetireAConceptMapType() throws Exception {
Expand Down
14 changes: 13 additions & 1 deletion ...ebservices/rest/web/v1_0/controller/openmrs1_9/ConceptReferenceTermController1_9Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ public void shouldCreateAConceptReferenceTerm() throws Exception {
assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditingAConceptReferenceTerm() throws Exception {
final String newCode = "updated code";
SimpleObject conceptReferenceTermType = new SimpleObject();
Expand All @@ -123,6 +123,18 @@ public void shouldEditingAConceptReferenceTerm() throws Exception {
handle(req);
assertEquals(newCode, service.getConceptReferenceTermByUuid(getUuid()).getCode());
}
@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAConceptReferenceTerm() throws Exception {
final String newCode = "updated code";
Copy link

Choose a reason for hiding this comment

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

Suggested change
final String newCode = "updated code";
final String newConceptCode = "updated code";

SimpleObject conceptReferenceTermType = new SimpleObject();
conceptReferenceTermType.add("code", newCode);

String json = new ObjectMapper().writeValueAsString(conceptReferenceTermType);

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldRetireAConceptReferenceTerm() throws Exception {
Expand Down
11 changes: 10 additions & 1 deletion ...openmrs/module/webservices/rest/web/v1_0/controller/openmrs1_9/DrugController1_9Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public void shouldCreateADrug() throws Exception {

}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditADrug() throws Exception {

final String editedName = "Aspirin Edited";
Expand All @@ -123,6 +123,15 @@ public void shouldEditADrug() throws Exception {
Assert.assertEquals(editedName, editedDrug.getName());

}
@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingADrug() throws Exception {

final String editedName = "Aspirin Edited";
String json = "{ \"name\":\"" + editedName + "\" }";
MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldRetireADrug() throws Exception {
Expand Down
14 changes: 13 additions & 1 deletion ...odule/webservices/rest/web/v1_0/controller/openmrs1_9/EncounterRoleController1_9Test.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ public void shouldCreateAEncounterRole() throws Exception {
assertEquals(originalCount + 1, getAllCount());
}

@Test
@Test(expected = RuntimeException.class)
public void shouldEditingAnEncounterRole() throws Exception {
final String newName = "updated name";
SimpleObject encounterRole = new SimpleObject();
Expand All @@ -106,6 +106,18 @@ public void shouldEditingAnEncounterRole() throws Exception {
handle(req);
assertEquals(newName, service.getEncounterRoleByUuid(getUuid()).getName());
}
@Test(expected = RuntimeException.class)
public void shouldThrowAnExceptionWhenEditingAnEncounterRole() throws Exception {
final String newName = "updated name";
SimpleObject encounterRole = new SimpleObject();
encounterRole.add("name", newName);

String json = new ObjectMapper().writeValueAsString(encounterRole);

MockHttpServletRequest req = request(RequestMethod.POST, getURI() + "/" + getUuid());
req.setContent(json.getBytes());
handle(req);
}

@Test
public void shouldRetireAEncounterRole() throws Exception {
Expand Down
Loading