Related to issue 30788: code review improvements
authorVíctor Martínez Romanos <victor.martinez@openbravo.com>
Tue, 15 Sep 2015 14:02:13 +0200
changeset 27847 488b5488e994
parent 27846 1f83e987ac49
child 27848 a7ba1d39303c
Related to issue 30788: code review improvements

Minor modifications:
+ Run query in admin mode
+ Usage of StringUtils.isBlank to control whitespace
+ Usage of StringBuffer to avoid String concatenation
+ Control any possible exception and return null to avoid breaking the report
+ Avoid unnecessary indentation (removed else)
+ Avoid the need of getting the Client object and using directly the id instead
+ Exists clause removed from subquery related to accounts from/to filter because it's useless and affects performance
+ Set the right param to djConfig.searchIds.push
src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal.html
src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal.java
src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal_data.xsql
--- a/src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal.html	Mon Sep 14 13:53:36 2015 +0200
+++ b/src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal.html	Tue Sep 15 14:02:13 2015 +0200
@@ -596,7 +596,7 @@
             </tr>
             <tr id=advancedFiltersrow2>
                <td class="TitleCell"><span class="LabelText">Document No.</span></td>
-               <td class="TextBox_ContentCell"><input dojoType="openbravo:Textbox"  class="dojoValidateValid TextBox_OneCell_width" type="text" name="inpDocumentNo" id="paramDocumentNo" size="10" maxlength="10" value=""></input><script>djConfig.searchIds.push("paramPageNo");</script></td>
+               <td class="TextBox_ContentCell"><input dojoType="openbravo:Textbox"  class="dojoValidateValid TextBox_OneCell_width" type="text" name="inpDocumentNo" id="paramDocumentNo" size="10" maxlength="10" value=""></input><script>djConfig.searchIds.push("paramDocumentNo");</script></td>
             </tr>
              <tr id=advancedFiltersrow3>
               <td class="TitleCell"><span class="LabelText">Initial Entry Number</span></td>
--- a/src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal.java	Mon Sep 14 13:53:36 2015 +0200
+++ b/src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal.java	Tue Sep 15 14:02:13 2015 +0200
@@ -58,7 +58,6 @@
 import org.openbravo.erpCommon.utility.ToolBar;
 import org.openbravo.erpCommon.utility.Utility;
 import org.openbravo.model.ad.datamodel.Table;
-import org.openbravo.model.ad.system.Client;
 import org.openbravo.model.common.enterprise.DocumentType;
 import org.openbravo.model.financialmgmt.accounting.coa.AcctSchema;
 import org.openbravo.model.financialmgmt.accounting.coa.AcctSchemaTable;
@@ -910,32 +909,46 @@
 
   }
 
+  /**
+   * Builds dynamic SQL to filter by document No
+   */
   private String getDocumentNo(String strClient, String strDocument, String strDocumentNo) {
-    if (StringUtils.isEmpty(strDocument) || StringUtils.isEmpty(strDocumentNo)) {
+    if (StringUtils.isBlank(strDocument) || StringUtils.isBlank(strDocumentNo)) {
       return null;
-    } else {
+    }
+
+    try {
+      OBContext.setAdminMode();
       String documentNo = StringEscapeUtils.escapeSql(strDocumentNo);
       documentNo = documentNo.replaceAll(";", "");
-      String string = "( SELECT 1 FROM ";
 
       StringBuffer where = new StringBuffer();
       where.append(" select t." + Table.PROPERTY_DBTABLENAME);
       where.append(" from " + DocumentType.ENTITY_NAME + " as d");
       where.append(" join d." + DocumentType.PROPERTY_TABLE + " as t");
       where.append(" where d." + DocumentType.PROPERTY_DOCUMENTCATEGORY + " = :document");
-      where.append(" and d." + DocumentType.PROPERTY_CLIENT + " = :client");
+      where.append(" and d." + DocumentType.PROPERTY_CLIENT + ".id = :client");
       where.append(" group by d." + DocumentType.PROPERTY_DOCUMENTCATEGORY);
       where.append(" , t." + Table.PROPERTY_DBTABLENAME);
       Query qry = OBDal.getInstance().getSession().createQuery(where.toString());
       qry.setMaxResults(1);
       qry.setParameter("document", strDocument);
-      qry.setParameter("client", OBDal.getInstance().get(Client.class, strClient));
+      qry.setParameter("client", strClient);
       String tablename = (String) qry.uniqueResult();
 
-      string += tablename;
-      string += " dt WHERE record_id = dt." + tablename + "_id";
-      string += " AND dt.documentno = '" + documentNo + "' )";
-      return string;
+      if (StringUtils.isBlank(tablename)) {
+        return null;
+      }
+
+      StringBuffer existsSubQuery = new StringBuffer("( SELECT 1 FROM ");
+      existsSubQuery.append(tablename);
+      existsSubQuery.append(" dt WHERE f.record_id = dt.").append(tablename).append("_id");
+      existsSubQuery.append(" AND dt.documentno = '").append(documentNo).append("' )");
+      return existsSubQuery.toString();
+    } catch (Exception ignore) {
+      return null;
+    } finally {
+      OBContext.restorePreviousMode();
     }
   }
 
--- a/src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal_data.xsql	Mon Sep 14 13:53:36 2015 +0200
+++ b/src/org/openbravo/erpCommon/ad_reports/ReportGeneralLedgerJournal_data.xsql	Tue Sep 15 14:02:13 2015 +0200
@@ -82,7 +82,6 @@
     <Parameter name="parDateFrom" optional="true" after="AND 3=3"><![CDATA[ AND dateacct >= TO_DATE(?)]]></Parameter>
     <Parameter name="parDateTo" optional="true" after="AND 3=3"><![CDATA[ AND dateacct < TO_DATE(?)]]></Parameter>
     <Parameter name="docbasetype" optional="true" after="AND 3=3"><![CDATA[ AND DOCBASETYPE = ?]]></Parameter>
-    <Parameter name="documentNo" optional="true" after="AND 3=3" type="argument"><![CDATA[ AND EXISTS ]]></Parameter>
     <Parameter name="acctschema" optional="true" after="AND 3=3"><![CDATA[ AND C_ACCTSCHEMA_ID = ?]]></Parameter>
     <Parameter name="orgFamily" type="replace" optional="true" after="AND AD_ORG_ID IN(" text="'4'"/>
     <Parameter name="checks" type="replace" optional="true" after="AND FactAcctType IN (" text="'C','N','O','R','D'"/>
@@ -151,7 +150,6 @@
     <Parameter name="parDateFrom" optional="true" after="AND 3=3"><![CDATA[ AND dateacct >= TO_DATE(?)]]></Parameter>
     <Parameter name="parDateTo" optional="true" after="AND 3=3"><![CDATA[ AND dateacct < TO_DATE(?)]]></Parameter>
     <Parameter name="docbasetype" optional="true" after="AND 3=3"><![CDATA[ AND DOCBASETYPE = ?]]></Parameter>
-    <Parameter name="documentNo" optional="true" after="AND 3=3" type="argument"><![CDATA[ AND EXISTS ]]></Parameter>
     <Parameter name="acctschema" optional="true" after="AND 3=3"><![CDATA[ AND C_ACCTSCHEMA_ID = ?]]></Parameter>
     <Parameter name="orgFamily" type="replace" optional="true" after="AND AD_ORG_ID IN(" text="'4'"/>
     <Parameter name="checks" type="replace" optional="true" after="AND FactAcctType IN (" text="'C','N','O','R','D'"/>
@@ -209,7 +207,6 @@
     <Parameter name="parDateFrom" optional="true" after="AND 3=3"><![CDATA[ AND dateacct >= TO_DATE(?)]]></Parameter>
     <Parameter name="parDateTo" optional="true" after="AND 3=3"><![CDATA[ AND dateacct < TO_DATE(?)]]></Parameter>
     <Parameter name="docbasetype" optional="true" after="AND 3=3"><![CDATA[ AND DOCBASETYPE = ?]]></Parameter>
-    <Parameter name="documentNo" optional="true" after="AND 3=3" type="argument"><![CDATA[ AND EXISTS ]]></Parameter>
     <Parameter name="acctschema" optional="true" after="AND 3=3"><![CDATA[ AND C_ACCTSCHEMA_ID = ?]]></Parameter>
     <Parameter name="orgFamily" type="replace" optional="true" after="AND AD_ORG_ID IN(" text="'4'"/>
     <Parameter name="checks" type="replace" optional="true" after="AND FactAcctType IN (" text="'C','N','O','R','D'"/>