Fixes issue 36675: Transaction Cost not created for closing inventory lines
authorMark <markmm82@gmail.com>
Wed, 23 Aug 2017 14:16:09 -0400
changeset 32603 5cd05d20d9cf
parent 32602 90a1381fae84
child 32604 1dc42ad30d5c
Fixes issue 36675: Transaction Cost not created for closing inventory lines

After cost was assigned to closing inventory lines transactions,
when calling to insertTrxCosts() from calculateCosts() method (second call),
CostingUtilsData.countTrxCosts(conn) was getting 0 in countTrx.
It was caused because closing inventory lines cost calculation
was done by OBDal connection, and methods invoked in CostingUtilsData are done
using different connection provided by SQLC, and the transactions are not available
at this moment. See: http://wiki.openbravo.com/wiki/Data_Access_Layer#Transaction_and_Session

To find the best solution for this issue and don't affect the performance (see issue 35959),
different solutions were tested to always use DAL and make process use the same connection.

1.- Doing a single insert-select query with limits is not supported in DAL. We tried it in two different ways (see attached TestCosting_v1.diff):
- Use setMaxResults(). It only works for selects but not for inserts/updates.
- Use setMaxResults() in select query and append it to insert query using getQueryString(). It appends the select without any limit.

2.- Another option could be to refactor insertTrxCosts() method to do a multiple insert query:
We can create the select query with limits using setMaxResults(), iterate it using an scroll
and create and save in each iteration a new TransactionCost. Flush won't be done in every iteration.
When flush is done, it raises every insert in multiple single-row inserts instead of only one multiple-row insert.
Single-row inserts performs worst than multiple-row insert, at least with not many rows (see attached TestCosting_v2.diff).

3.- Finally, we have refactor the process to avoid insertTrxCosts() method:
Our solution makes an insertion in M_Transaction_Cost table each
time we make the updation of related transaction cost in M_Transaction table.
We will do single-row inserts in two methods: updateTrxLegacyCosts() and calculateCosts(),
and multiple-row insert in one method: updateWithZeroCostRemainingTrx().
Thus, we split the number of TransactionCost records to be created in three different steps.
src/org/openbravo/costing/CostingMigrationProcess.java
src/org/openbravo/costing/CostingUtils_data.xsql
--- a/src/org/openbravo/costing/CostingMigrationProcess.java	Tue Aug 29 16:10:25 2017 +0200
+++ b/src/org/openbravo/costing/CostingMigrationProcess.java	Wed Aug 23 14:16:09 2017 -0400
@@ -47,7 +47,6 @@
 import org.openbravo.dal.service.OBCriteria;
 import org.openbravo.dal.service.OBDal;
 import org.openbravo.dal.service.OBQuery;
-import org.openbravo.database.ConnectionProvider;
 import org.openbravo.erpCommon.ad_forms.ProductInfo;
 import org.openbravo.erpCommon.utility.OBDateUtils;
 import org.openbravo.erpCommon.utility.OBError;
@@ -76,6 +75,8 @@
 import org.openbravo.model.materialmgmt.transaction.InventoryCount;
 import org.openbravo.model.materialmgmt.transaction.InventoryCountLine;
 import org.openbravo.model.materialmgmt.transaction.MaterialTransaction;
+import org.openbravo.model.materialmgmt.transaction.ShipmentInOut;
+import org.openbravo.model.materialmgmt.transaction.ShipmentInOutLine;
 import org.openbravo.scheduling.Process;
 import org.openbravo.scheduling.ProcessBundle;
 import org.openbravo.scheduling.ProcessLogger;
@@ -84,7 +85,6 @@
 
 public class CostingMigrationProcess implements Process {
   private ProcessLogger logger;
-  private ConnectionProvider conn;
   private static final Logger log4j = Logger.getLogger(CostingMigrationProcess.class);
   private static CostingAlgorithm averageAlgorithm = null;
   private static final String alertRuleName = "Products with transactions without available cost on date.";
@@ -97,7 +97,6 @@
   private static final String valuedLegacy = "800088";
   private static final String dimensionalLegacy = "800205";
   private static final String processEntity = org.openbravo.model.ad.ui.Process.ENTITY_NAME;
-  private static final int maxTrx = 10000;
 
   @Override
   public void execute(ProcessBundle bundle) throws Exception {
@@ -106,7 +105,6 @@
 
     logger = bundle.getLogger();
     OBError msg = new OBError();
-    conn = bundle.getConnection();
     msg.setType("Success");
     msg.setTitle(OBMessageUtils.messageBD("Success"));
     try {
@@ -431,7 +429,6 @@
     }
 
     updateWithZeroCostRemainingTrx();
-    insertTrxCosts();
     insertStandardCosts();
 
     long end = System.currentTimeMillis();
@@ -597,6 +594,18 @@
         trx.setCostingStatus("CC");
         trx.setProcessed(true);
         OBDal.getInstance().save(trx);
+
+        TransactionCost tc = OBProvider.getInstance().get(TransactionCost.class);
+        tc.setClient(trx.getClient());
+        tc.setOrganization(trx.getOrganization());
+        tc.setInventoryTransaction(trx);
+        tc.setCost(trx.getTransactionCost());
+        tc.setCostDate(trx.getTransactionProcessDate());
+        tc.setCurrency(trx.getCurrency());
+        tc.setAccountingDate(trx.getGoodsShipmentLine() != null ? trx.getGoodsShipmentLine()
+            .getShipmentReceipt().getAccountingDate() : trx.getMovementDate());
+        OBDal.getInstance().save(tc);
+
         Currency legalEntityCur = FinancialUtils.getLegalEntityCurrency(trx.getOrganization());
         BigDecimal cost = BigDecimal.ZERO;
         if (BigDecimal.ZERO.compareTo(trx.getMovementQuantity()) != 0) {
@@ -616,19 +625,17 @@
         // MovementQty is already negative so add to totalStock to decrease it.
         totalStock = totalStock.add(trx.getMovementQuantity());
 
-        if ((i % 100) == 0) {
+        if (++i % 100 == 0) {
           OBDal.getInstance().flush();
           OBDal.getInstance().getSession().clear();
           cur = OBDal.getInstance().get(Currency.class, curId);
         }
-        i++;
       }
     } finally {
       icls.close();
     }
 
     OBDal.getInstance().flush();
-    insertTrxCosts();
 
     long end = System.currentTimeMillis();
     log4j.debug("Ending calculateCosts() at: " + new Date() + ". Duration: " + (end - start)
@@ -782,11 +789,11 @@
     try {
       while (trxs.next()) {
         MaterialTransaction trx = (MaterialTransaction) trxs.get(0);
+        Date accountingDate = trx.getGoodsShipmentLine() != null ? trx.getGoodsShipmentLine()
+            .getShipmentReceipt().getAccountingDate() : trx.getMovementDate();
         log4j.debug("********** UpdateTrxLegacyCosts process trx:" + trx.getIdentifier());
 
-        if (trx.getGoodsShipmentLine() != null
-            && trx.getGoodsShipmentLine().getShipmentReceipt().getAccountingDate()
-                .compareTo(trx.getMovementDate()) != 0) {
+        if (accountingDate.compareTo(trx.getMovementDate()) != 0) {
           // Shipments with accounting date different than the movement date gets the cost valid on
           // the accounting date.
           BigDecimal unitCost = new BigDecimal(new ProductInfo(cost.getProduct().getId(),
@@ -795,24 +802,32 @@
               new DalConnectionProvider(false), OBDal.getInstance().getConnection()));
           BigDecimal trxCost = unitCost.multiply(trx.getMovementQuantity().abs()).setScale(
               standardPrecision, BigDecimal.ROUND_HALF_UP);
-
           trx.setTransactionCost(trxCost);
         } else {
           trx.setTransactionCost(cost.getCost().multiply(trx.getMovementQuantity().abs())
               .setScale(standardPrecision, BigDecimal.ROUND_HALF_UP));
         }
-
         trx.setCurrency(cost.getCurrency());
         trx.setCostCalculated(true);
         trx.setCostingStatus("CC");
         trx.setProcessed(true);
+        OBDal.getInstance().save(trx);
 
-        if ((i % 100) == 0) {
+        TransactionCost tc = OBProvider.getInstance().get(TransactionCost.class);
+        tc.setClient(trx.getClient());
+        tc.setOrganization(trx.getOrganization());
+        tc.setInventoryTransaction(trx);
+        tc.setCost(trx.getTransactionCost());
+        tc.setCostDate(trx.getTransactionProcessDate());
+        tc.setCurrency(trx.getCurrency());
+        tc.setAccountingDate(accountingDate);
+        OBDal.getInstance().save(tc);
+
+        if (++i % 100 == 0) {
           OBDal.getInstance().flush();
           OBDal.getInstance().getSession().clear();
           cost = OBDal.getInstance().get(Costing.class, cost.getId());
         }
-        i++;
       }
     } finally {
       trxs.close();
@@ -843,6 +858,47 @@
         for (Organization org : osp.getLegalEntitiesList()) {
           final Set<String> childOrgs = osp.getChildTree(org.getId(), true);
 
+          StringBuffer insert = new StringBuffer();
+          insert.append(" insert into " + TransactionCost.ENTITY_NAME);
+          insert.append(" (" + TransactionCost.PROPERTY_ID);
+          insert.append(", " + TransactionCost.PROPERTY_CLIENT);
+          insert.append(", " + TransactionCost.PROPERTY_ORGANIZATION);
+          insert.append(", " + TransactionCost.PROPERTY_CREATIONDATE);
+          insert.append(", " + TransactionCost.PROPERTY_CREATEDBY);
+          insert.append(", " + TransactionCost.PROPERTY_UPDATED);
+          insert.append(", " + TransactionCost.PROPERTY_UPDATEDBY);
+          insert.append(", " + TransactionCost.PROPERTY_ACTIVE);
+          insert.append(", " + TransactionCost.PROPERTY_INVENTORYTRANSACTION);
+          insert.append(", " + TransactionCost.PROPERTY_COST);
+          insert.append(", " + TransactionCost.PROPERTY_COSTDATE);
+          insert.append(", " + TransactionCost.PROPERTY_CURRENCY);
+          insert.append(", " + TransactionCost.PROPERTY_ACCOUNTINGDATE);
+          insert.append(")");
+          insert.append(" select get_uuid()");
+          insert.append(", t." + MaterialTransaction.PROPERTY_CLIENT);
+          insert.append(", t." + MaterialTransaction.PROPERTY_ORGANIZATION);
+          insert.append(", now()");
+          insert.append(", t." + MaterialTransaction.PROPERTY_CREATEDBY);
+          insert.append(", now()");
+          insert.append(", t." + MaterialTransaction.PROPERTY_UPDATEDBY);
+          insert.append(", t." + MaterialTransaction.PROPERTY_ACTIVE);
+          insert.append(", t");
+          insert.append(", cast(0 as big_decimal)");
+          insert.append(", t." + MaterialTransaction.PROPERTY_TRANSACTIONPROCESSDATE);
+          insert.append(", t." + MaterialTransaction.PROPERTY_CLIENT + "."
+              + Client.PROPERTY_CURRENCY);
+          insert.append(", coalesce(io." + ShipmentInOut.PROPERTY_ACCOUNTINGDATE + ", t."
+              + MaterialTransaction.PROPERTY_MOVEMENTDATE + ")");
+          insert.append(" from " + MaterialTransaction.ENTITY_NAME + " as t");
+          insert.append(" left join t." + MaterialTransaction.PROPERTY_GOODSSHIPMENTLINE
+              + " as iol");
+          insert.append(" left join iol." + ShipmentInOutLine.PROPERTY_SHIPMENTRECEIPT + " as io");
+          insert.append(" where " + MaterialTransaction.PROPERTY_TRANSACTIONCOST + " is null");
+          insert.append(" and t." + MaterialTransaction.PROPERTY_ORGANIZATION + ".id in (:orgs)");
+          Query insertQry = OBDal.getInstance().getSession().createQuery(insert.toString());
+          insertQry.setParameterList("orgs", childOrgs);
+          insertQry.executeUpdate();
+
           StringBuffer update = new StringBuffer();
           update.append(" update " + MaterialTransaction.ENTITY_NAME);
           update.append(" set " + MaterialTransaction.PROPERTY_ISCOSTCALCULATED + " = true");
@@ -853,12 +909,11 @@
           update.append(", " + MaterialTransaction.PROPERTY_ISPROCESSED + " = true");
           update.append(" where " + MaterialTransaction.PROPERTY_TRANSACTIONCOST + " is null");
           update.append(" and " + MaterialTransaction.PROPERTY_ORGANIZATION + ".id in (:orgs)");
-
           Query updateQry = OBDal.getInstance().getSession().createQuery(update.toString());
-          updateQry.setParameter("currency", org.getCurrency() != null ? org.getCurrency() : org
-              .getClient().getCurrency());
+          updateQry.setParameter("currency", org.getClient().getCurrency());
           updateQry.setParameterList("orgs", childOrgs);
           n = updateQry.executeUpdate();
+
         }
       }
 
@@ -910,36 +965,6 @@
     log4j.debug("** inserted alert recipients: " + inserted);
   }
 
-  private void insertTrxCosts() {
-    long start = System.currentTimeMillis();
-    log4j.debug("Starting insertTrxCosts() at: " + new Date());
-
-    TriggerHandler.getInstance().disable();
-    try {
-      long countTrx = Long.valueOf(CostingUtilsData.countTrxCosts(conn)).longValue();
-      long iters = (countTrx % maxTrx == 0) ? (countTrx / maxTrx) : (countTrx / maxTrx) + 1;
-      String pgLimit = null, oraLimit = null;
-      if (StringUtils.equalsIgnoreCase(conn.getRDBMS(), "ORACLE")) {
-        oraLimit = String.valueOf(maxTrx);
-      } else {
-        pgLimit = String.valueOf(maxTrx);
-      }
-      for (int i = 0; i < iters; i++) {
-        CostingUtilsData.insertTrxCosts(conn, pgLimit, oraLimit);
-        OBDal.getInstance().flush();
-        OBDal.getInstance().getSession().clear();
-      }
-    } catch (Exception e) {
-      log4j.error(e.getMessage());
-    } finally {
-      TriggerHandler.getInstance().enable();
-
-      long end = System.currentTimeMillis();
-      log4j.debug("Ending insertTrxCosts() at: " + new Date() + ". Duration: " + (end - start)
-          + " ms.");
-    }
-  }
-
   private void insertStandardCosts() {
     long start = System.currentTimeMillis();
     log4j.debug("Starting insertStandardCosts() at: " + new Date());
--- a/src/org/openbravo/costing/CostingUtils_data.xsql	Tue Aug 29 16:10:25 2017 +0200
+++ b/src/org/openbravo/costing/CostingUtils_data.xsql	Wed Aug 23 14:16:09 2017 -0400
@@ -99,67 +99,5 @@
       <Parameter name="EndDateAcct"/>
       <Parameter name="AD_Client_ID"/>
       <Parameter name="DocumentType"/>
-   </SqlMethod>  
-   <SqlMethod name="insertTrxCosts" type="preparedStatement" return="rowCount">
-      <SqlMethodComment></SqlMethodComment>
-      <Sql>
-      <![CDATA[
-        insert into M_Transaction_Cost (
-                M_Transaction_Cost_ID, 
-                Isactive, 
-                AD_Client_ID, 
-                AD_Org_ID, 
-                Created, 
-                Createdby, 
-                Updated, 
-                Updatedby, 
-                M_Transaction_ID, 
-                Cost, 
-                CostDate, 
-                C_Currency_ID, 
-                DateAcct)
-        (select 
-                get_uuid(), 
-                t.IsActive, 
-                t.AD_Client_ID, 
-                t.AD_Org_ID,
-                now(), 
-                t.CreatedBy, 
-                now(), 
-                t.UpdatedBy, 
-                t.M_Transaction_ID, 
-                t.TransactionCost, 
-                t.TrxProcessDate, 
-                t.C_Currency_ID, 
-                coalesce(io.DateAcct, t.MovementDate) 
-        from 
-                M_Transaction t 
-                left join M_Transaction_Cost tc on t.M_Transaction_ID=tc.M_Transaction_ID
-                left join M_InOutLine iol on t.M_InOutLine_ID=iol.M_InOutLine_ID
-                left join M_InOut io on iol.M_InOut_ID=io.M_InOut_ID
-        where 
-                (t.TransactionCost is not null) 
-                and (tc.M_Transaction_Cost_ID is null)
-        )
-      ]]>
-      </Sql>
-      <Parameter name="pgLimit" type="argument" optional="true" after="and (tc.M_Transaction_Cost_ID is null)"><![CDATA[LIMIT ]]></Parameter>
-      <Parameter name="oraLimit" type="argument" optional="true" after="and (tc.M_Transaction_Cost_ID is null)"><![CDATA[AND ROWNUM <= ]]></Parameter>
-   </SqlMethod>
-   <SqlMethod name="countTrxCosts" type="preparedStatement" return="String">
-      <SqlMethodComment></SqlMethodComment>
-      <Sql>
-      <![CDATA[
-        select count(1)
-        from 
-            M_Transaction t 
-            left join M_Transaction_Cost tc on t.M_Transaction_ID=tc.M_Transaction_ID 
-            left join M_InOutLine iol on t.M_InOutLine_ID=iol.M_InOutLine_ID 
-            left join M_InOut io on iol.M_InOut_ID=io.M_InOut_ID 
-        where 
-            (t.TransactionCost is not null) 
-            and (tc.M_Transaction_Cost_ID is null)
-      ]]>
-      </Sql>
    </SqlMethod>   
   </SqlClass>
\ No newline at end of file