Responding to review comments for: Setting order numbers in the api

dkayiwa · dkayiwa · commit 683e3bfcb51b · 2013-12-23T13:49:33.000+03:00
needs to be safe in a clustered environment - TRUNK-4183
diff --git a/api/src/main/java/org/openmrs/api/OrderService.java b/api/src/main/java/org/openmrs/api/OrderService.java
@@ -180,5 +180,5 @@ public <Ord extends Order> List<Ord> getOrders(Class<Ord> orderClassType, List<P
 	 * @return the order number seed
 	 */
 	@Authorized(PrivilegeConstants.ADD_ORDERS)
-	public Long getNextOrderNumberSeed();
+	public Long getNextOrderNumberSeedSequenceValue();
 }
diff --git a/api/src/main/java/org/openmrs/api/db/OrderDAO.java b/api/src/main/java/org/openmrs/api/db/OrderDAO.java
@@ -80,5 +80,5 @@ public <Ord extends Order> List<Ord> getOrders(Class<Ord> orderClassType, List<P
 	 * 
 	 * @return the order number seed
 	 */
-	public Long getNextOrderNumberSeed();
+	public Long getNextOrderNumberSeedSequenceValue();
 }
diff --git a/api/src/main/java/org/openmrs/api/db/hibernate/HibernateOrderDAO.java b/api/src/main/java/org/openmrs/api/db/hibernate/HibernateOrderDAO.java
@@ -48,8 +48,6 @@ public class HibernateOrderDAO implements OrderDAO {
 	
 	protected static final Log log = LogFactory.getLog(HibernateOrderDAO.class);
 	
-	private static final String ORDER_NUMBER_START_VALUE = "1";
-	
 	/**
 	 * Hibernate session factory
 	 */
@@ -152,18 +150,18 @@ public Order getOrderByOrderNumber(String orderNumber) {
 	}
 	
 	/**
-	 * @see org.openmrs.api.db.OrderDAO#getNextOrderNumberSeed()
+	 * @see org.openmrs.api.db.OrderDAO#getNextOrderNumberSeedSequenceValue()
 	 */
 	@Override
-	public Long getNextOrderNumberSeed() {
+	public Long getNextOrderNumberSeedSequenceValue() {
 		Criteria searchCriteria = sessionFactory.getCurrentSession().createCriteria(GlobalProperty.class);
 		searchCriteria.add(Restrictions.eq("property", OpenmrsConstants.GLOBAL_PROPERTY_NEXT_ORDER_NUMBER_SEED));
 		searchCriteria.setLockMode(LockMode.PESSIMISTIC_WRITE);
 		
 		GlobalProperty globalProperty = (GlobalProperty) searchCriteria.uniqueResult();
 		if (globalProperty == null) {
-			globalProperty = new GlobalProperty(OpenmrsConstants.GLOBAL_PROPERTY_NEXT_ORDER_NUMBER_SEED,
-			        ORDER_NUMBER_START_VALUE, "The next order number available for assignment");
+			throw new APIException("Missing global property named: "
+			        + OpenmrsConstants.GLOBAL_PROPERTY_NEXT_ORDER_NUMBER_SEED);
 		}
 		
 		String gpTextValue = globalProperty.getPropertyValue();
diff --git a/api/src/main/java/org/openmrs/api/impl/OrderServiceImpl.java b/api/src/main/java/org/openmrs/api/impl/OrderServiceImpl.java
@@ -175,7 +175,7 @@ public Order getOrderByUuid(String uuid) throws APIException {
 	 */
 	@Override
 	public synchronized String getNewOrderNumber() {
-		return ORDER_NUMBER_PREFIX + Context.getOrderService().getNextOrderNumberSeed();
+		return ORDER_NUMBER_PREFIX + Context.getOrderService().getNextOrderNumberSeedSequenceValue();
 	}
 	
 	/**
@@ -205,10 +205,10 @@ public List<Order> getOrderHistoryByConcept(Patient patient, Concept concept) {
 	}
 	
 	/**
-	 * @see org.openmrs.api.OrderService#getNextOrderNumberSeed()
+	 * @see org.openmrs.api.OrderService#getNextOrderNumberSeedSequenceValue()
 	 */
 	@Override
-	public Long getNextOrderNumberSeed() {
-		return dao.getNextOrderNumberSeed();
+	public Long getNextOrderNumberSeedSequenceValue() {
+		return dao.getNextOrderNumberSeedSequenceValue();
 	}
 }
diff --git a/api/src/test/java/org/openmrs/api/OrderServiceTest.java b/api/src/test/java/org/openmrs/api/OrderServiceTest.java
@@ -125,6 +125,9 @@ public void purgeOrder_shouldDeleteObsThatReference() throws Exception {
 	@Verifies(value = "should always return unique orderNumbers when called multiple times without saving orders", method = "getNewOrderNumber()")
 	public void getNewOrderNumber_shouldAlwaysReturnUniqueOrderNumbersWhenCalledMultipleTimesWithoutSavingOrders()
 	        throws Exception {
+		
+		executeDataSet("org/openmrs/api/include/OrderServiceTest-globalProperties.xml");
+		
 		int N = 50;
 		final Set<String> uniqueOrderNumbers = new HashSet<String>(50);
 		List<Thread> threads = new ArrayList<Thread>();
diff --git a/api/src/test/resources/org/openmrs/api/include/OrderServiceTest-globalProperties.xml b/api/src/test/resources/org/openmrs/api/include/OrderServiceTest-globalProperties.xml
@@ -0,0 +1,4 @@
+<?xml version='1.0' encoding='UTF-8'?>
+<dataset>
+	<global_property property="order.nextOrderNumberSeed" property_value="1" uuid="1d55827e-26fe-102b-80cb-0017a47879y5"/>
+</dataset>

Original file line number	Diff line number	Diff line change
`@@ -180,5 +180,5 @@ public <Ord extends Order> List<Ord> getOrders(Class<Ord> orderClassType, List<P`
`180`	`180`	`* @return the order number seed`
`181`	`181`	`*/`
`182`	`182`	`@Authorized(PrivilegeConstants.ADD_ORDERS)`
`183`		`- public Long getNextOrderNumberSeed();`
	`183`	`+ public Long getNextOrderNumberSeedSequenceValue();`
`184`	`184`	`}`
Original file line number	Diff line number	Diff line change
`@@ -80,5 +80,5 @@ public <Ord extends Order> List<Ord> getOrders(Class<Ord> orderClassType, List<P`
`80`	`80`	`*`
`81`	`81`	`* @return the order number seed`
`82`	`82`	`*/`
`83`		`- public Long getNextOrderNumberSeed();`
	`83`	`+ public Long getNextOrderNumberSeedSequenceValue();`
`84`	`84`	`}`
Original file line number	Diff line number	Diff line change
`@@ -175,7 +175,7 @@ public Order getOrderByUuid(String uuid) throws APIException {`
`175`	`175`	`*/`
`176`	`176`	`@Override`
`177`	`177`	`public synchronized String getNewOrderNumber() {`
`178`		`- return ORDER_NUMBER_PREFIX + Context.getOrderService().getNextOrderNumberSeed();`
	`178`	`+ return ORDER_NUMBER_PREFIX + Context.getOrderService().getNextOrderNumberSeedSequenceValue();`
`179`	`179`	`}`
`180`	`180`
`181`	`181`	`/**`
`@@ -205,10 +205,10 @@ public List<Order> getOrderHistoryByConcept(Patient patient, Concept concept) {`
`205`	`205`	`}`
`206`	`206`
`207`	`207`	`/**`
`208`		`- * @see org.openmrs.api.OrderService#getNextOrderNumberSeed()`
	`208`	`+ * @see org.openmrs.api.OrderService#getNextOrderNumberSeedSequenceValue()`
`209`	`209`	`*/`
`210`	`210`	`@Override`
`211`		`- public Long getNextOrderNumberSeed() {`
`212`		`- return dao.getNextOrderNumberSeed();`
	`211`	`+ public Long getNextOrderNumberSeedSequenceValue() {`
	`212`	`+ return dao.getNextOrderNumberSeedSequenceValue();`
`213`	`213`	`}`
`214`	`214`	`}`