Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Major Major
    • Resolution: Incomplete
    • Affects Version/s: Trunk
    • Fix Version/s: Trunk
    • Component/s: order
    • Labels:

      Description

      Reports under Order component has following issues -
      1. Following reports are not showing any records due some issue
      a. Last three month sales report.
      b. Sales report.
      c. Sales Order discount code report.
      d. Product demand report.
      2. Inconsistency in opening the pdf report - some reports are opening in new tab window but some are in the same window.

      1. OFBIZ-5297-Trunk.patch
        70 kB
        Parimal Gain
      2. OFBIZ-5297-11.04.patch
        41 kB
        Parimal Gain
      3. Last3MonthSalesReport.PNG
        56 kB
        Parimal Gain

        Issue Links

          Activity

          Hide
          Parimal Gain added a comment -

          Provided patch for both OFBiz-11.04 as well as for OFBiz-Trunk.
          Note: To run the reports all dimension table should have records and for trunk Component Order in specialpurpose/birt/ofbiz-component.xml should be uncommented also.

          Show
          Parimal Gain added a comment - Provided patch for both OFBiz-11.04 as well as for OFBiz-Trunk. Note: To run the reports all dimension table should have records and for trunk Component Order in specialpurpose/birt/ofbiz-component.xml should be uncommented also.
          Hide
          Jacques Le Roux added a comment -

          Parimal,

          I did not get a chance to review nor test your patch (it applies well). Anyway I hope to fix OFBIZ-5267 before. Because then we could get rid of the uncommenting stuff I put there to prevent breaking the contextual help.

          Also Taher wanted to provide some help on the same subject and to create a Jira for that. Maybe not all what he suggested is done with your patch, but I believe a good part.

          Thanks!

          Show
          Jacques Le Roux added a comment - Parimal, I did not get a chance to review nor test your patch (it applies well). Anyway I hope to fix OFBIZ-5267 before. Because then we could get rid of the uncommenting stuff I put there to prevent breaking the contextual help. Also Taher wanted to provide some help on the same subject and to create a Jira for that. Maybe not all what he suggested is done with your patch, but I believe a good part. Thanks!
          Hide
          Taher Alkhateeb added a comment -

          Hi Jacques and Parimal,

          Please note that the patch I'm working on (I did not create a JIRA yet as I want more substantial material to patch) is quite different. I am working on infrastructure improvement including exporting shared functionality to a separate JS library and unified formatting.

          A quick review of this patch shows that my work would probably veer off quite differently. For example, if you look at this snippet:

          @@ -215,7 +215,7 @@
                       <method name="open"><![CDATA[salesOrderItemStarSchemas = null;
           userLogin = null;
           try {
          -    userLogin = delegator.findOne("UserLogin",UtilMisc.toMap("userLoginId","admin"), false);
          +    userLogin = delegator.findByPrimaryKey("UserLogin",UtilMisc.toMap("userLoginId","admin"));
          

          You will see that the fundamental problem of hard-coding userLoginId is still not resolved. In addition, my objective is to actually yank out all code for userLogin into a separate shared library. My work might actually also apply some changes to BirtWorker.java

          With that being said, I will adjust my work on my patch based on this JIRA and try to merge with this patch to be able to upload what I originally had in mind, or perhaps I will wait until you finalize all issues with this JIRA and close it before I start to submit mine. What do you think?

          Show
          Taher Alkhateeb added a comment - Hi Jacques and Parimal, Please note that the patch I'm working on (I did not create a JIRA yet as I want more substantial material to patch) is quite different. I am working on infrastructure improvement including exporting shared functionality to a separate JS library and unified formatting. A quick review of this patch shows that my work would probably veer off quite differently. For example, if you look at this snippet: @@ -215,7 +215,7 @@ <method name= "open" ><![CDATA[salesOrderItemStarSchemas = null ; userLogin = null ; try { - userLogin = delegator.findOne( "UserLogin" ,UtilMisc.toMap( "userLoginId" , "admin" ), false ); + userLogin = delegator.findByPrimaryKey( "UserLogin" ,UtilMisc.toMap( "userLoginId" , "admin" )); You will see that the fundamental problem of hard-coding userLoginId is still not resolved. In addition, my objective is to actually yank out all code for userLogin into a separate shared library. My work might actually also apply some changes to BirtWorker.java With that being said, I will adjust my work on my patch based on this JIRA and try to merge with this patch to be able to upload what I originally had in mind, or perhaps I will wait until you finalize all issues with this JIRA and close it before I start to submit mine. What do you think?
          Hide
          Parimal Gain added a comment -

          Thanks Jacques and Taher. I think both of you have good amount of discussion on birt report and also working on resolving lots of issues apart from the issue I reported. So definitely Jacques will be able to take a call on this. LMK if I can do more on these.

          Show
          Parimal Gain added a comment - Thanks Jacques and Taher. I think both of you have good amount of discussion on birt report and also working on resolving lots of issues apart from the issue I reported. So definitely Jacques will be able to take a call on this. LMK if I can do more on these.
          Hide
          Jacques Le Roux added a comment -

          OFBIZ-5267 ready to test

          Show
          Jacques Le Roux added a comment - OFBIZ-5267 ready to test
          Hide
          Jacques Le Roux added a comment -

          Hi Parimal,

          After loading demo data, applying your patch (OFBIZ-5267 being fixed), I indeed see no longer errors in log like before, but I still don't see any values, just titles, in the PDFs, normal?

          Show
          Jacques Le Roux added a comment - Hi Parimal, After loading demo data, applying your patch ( OFBIZ-5267 being fixed), I indeed see no longer errors in log like before, but I still don't see any values, just titles, in the PDFs, normal?
          Hide
          Jacques Le Roux added a comment -

          Note: I also tried this morning by adding new orders, still no numbers in the PDF.

          Show
          Jacques Le Roux added a comment - Note: I also tried this morning by adding new orders, still no numbers in the PDF.
          Hide
          Parimal Gain added a comment - - edited

          Hi Jacques,

          These patches are working fine at my localhost. I'm currently in following revision in my localhost
          1) OFBiz-11.04 : 1512887
          2) OFBiz-Trunk : 1519364

          Following are the checkpoint -
          1) Order should be Approved
          2) All Dimension table(CurrencyDimension, DateDimension, ProductDimension)should have record.
          3) After approval of order, order record should be created in 'SalesOrderItemFact' table. These is the OLAP table used to display data in reports.

          Description -
          1) If dimension table doesn't have records then following services should execute from service engine -
          a) loadAllProductsInProductDimension
          b) loadCurrencyDimension
          c) loadDateDimension
          2) Calling service to create record in 'SalesOrderItemFact' is updated into patch. On commit operation of service 'changeOrderItemStatus' if order item is approved then 'loadSalesOrderFact' service called through SECAS.

          Please LMK the revision and the branch you are testing if report doesn't display any value even after applying these changes.

          For reference I have attached the screenshot of Last3Month sales report from my localhost that shows sales order detail of July, Aug and Sep(current) month.

          Show
          Parimal Gain added a comment - - edited Hi Jacques, These patches are working fine at my localhost. I'm currently in following revision in my localhost 1) OFBiz-11.04 : 1512887 2) OFBiz-Trunk : 1519364 Following are the checkpoint - 1) Order should be Approved 2) All Dimension table(CurrencyDimension, DateDimension, ProductDimension)should have record. 3) After approval of order, order record should be created in 'SalesOrderItemFact' table. These is the OLAP table used to display data in reports. Description - 1) If dimension table doesn't have records then following services should execute from service engine - a) loadAllProductsInProductDimension b) loadCurrencyDimension c) loadDateDimension 2) Calling service to create record in 'SalesOrderItemFact' is updated into patch. On commit operation of service 'changeOrderItemStatus' if order item is approved then 'loadSalesOrderFact' service called through SECAS. Please LMK the revision and the branch you are testing if report doesn't display any value even after applying these changes. For reference I have attached the screenshot of Last3Month sales report from my localhost that shows sales order detail of July, Aug and Sep(current) month.
          Hide
          Jacques Le Roux added a comment -

          Thanks Parimal,

          I did no thought about BI issues. After your explanations, indeed all the reported as fixed reports are working again. There is now only an issue with NetBeforeOverheadReport.pdf. Maybe it's what Taher is working on...

          Your patch is in trunk at revision: 1519544. I will backport later after OFBIZ-4794

          Show
          Jacques Le Roux added a comment - Thanks Parimal, I did no thought about BI issues. After your explanations, indeed all the reported as fixed reports are working again. There is now only an issue with NetBeforeOverheadReport.pdf. Maybe it's what Taher is working on... Your patch is in trunk at revision: 1519544. I will backport later after OFBIZ-4794
          Hide
          Parimal Gain added a comment -

          Great, It sounds good ... Thanks Jacques for your help.

          Show
          Parimal Gain added a comment - Great, It sounds good ... Thanks Jacques for your help.
          Hide
          Jacques Le Roux added a comment -

          Parimal,

          I thought about it. Since it needs to have BI tables seeded, I believe we should at least let know the user (a sentence in the UI, or popup layer, or something) or even do it OOTB, if one of those reports is called and it's not done yet. What do you think?

          Show
          Jacques Le Roux added a comment - Parimal, I thought about it. Since it needs to have BI tables seeded, I believe we should at least let know the user (a sentence in the UI, or popup layer, or something) or even do it OOTB, if one of those reports is called and it's not done yet. What do you think?
          Hide
          Taher Alkhateeb added a comment -

          Hi gentlemen,

          I am not sure if this might help because I'm not sure what is meant by "not done yet" if it means non-existent report or no data fetched? Anyway I think pretty much birt reports are scripted in three event methods in OFBiz:

          • initialize: you setup the birtParameters passed in here and do other basic stuff
          • open: you execute the query (using Delegator or a service or whatever)
          • fetch: you retrieve one row and populate your data-set.

          The problem occurs on the fetch method. When the first record of your result is empty, fetch immediately returns false and nothing is rendered on the screen, hence you get this blank report, or depending on the report design and how you bind the data to the table, it might render partial results or stuff like that. I did not check this report specifically.

          I think there are some solutions for such surprises, and I list them here for sharing thoughts:

          • you surround your fetch portion with try/catch blocks. Then, if data retrieved is empty, you populate an error message variable which is rendered conditionally in the report (perhaps with dynamic text).
          • you create an if condition before execution perhaps in onPrepare method to check for table existence / count and display an error in the generated report if empty. This takes more coding and processing time, but might be faster on failure.

          In the first solution you might have the shared library (discussed earlier) handle all missing data with one function that populates shared error variables in all BIRT reports in OFBiz. Again I would like to tackle this after changes in this issue are committed so I can build on top of it instead of merging conflicts. If you have other thoughts to guide me it would also be great.

          Show
          Taher Alkhateeb added a comment - Hi gentlemen, I am not sure if this might help because I'm not sure what is meant by "not done yet" if it means non-existent report or no data fetched? Anyway I think pretty much birt reports are scripted in three event methods in OFBiz: initialize: you setup the birtParameters passed in here and do other basic stuff open: you execute the query (using Delegator or a service or whatever) fetch: you retrieve one row and populate your data-set. The problem occurs on the fetch method. When the first record of your result is empty, fetch immediately returns false and nothing is rendered on the screen, hence you get this blank report, or depending on the report design and how you bind the data to the table, it might render partial results or stuff like that. I did not check this report specifically. I think there are some solutions for such surprises, and I list them here for sharing thoughts: you surround your fetch portion with try/catch blocks. Then, if data retrieved is empty, you populate an error message variable which is rendered conditionally in the report (perhaps with dynamic text). you create an if condition before execution perhaps in onPrepare method to check for table existence / count and display an error in the generated report if empty. This takes more coding and processing time, but might be faster on failure. In the first solution you might have the shared library (discussed earlier) handle all missing data with one function that populates shared error variables in all BIRT reports in OFBiz. Again I would like to tackle this after changes in this issue are committed so I can build on top of it instead of merging conflicts. If you have other thoughts to guide me it would also be great.
          Hide
          Jacques Le Roux added a comment -

          Taher,

          "not done yet" meant here that the BI services were not run, this Parimal's comment above:

          1) If dimension table doesn't have records then following services should execute from service engine -
          a) loadAllProductsInProductDimension
          b) loadCurrencyDimension
          c) loadDateDimension

          Changes in this issue are committed in trunk. They will backported to releases after OFBIZ-4794. If you have something to submit, you should not wait for backports, they are totally dependent on trunk.

          You can see Birt reports are working today on trunk demo. Because I ran those services before creating and approving a new order. My knowledge about Birt and BI is limited, but I wonder why we need to run those services prior to have results in reports. I thought that running "Quick Init DataWarehouse" option in BI compoment would be enough but it did not work for me. So I had to run those services by hand. Maybe because I did not create an order just after, I can't remember. I will check that point...

          Show
          Jacques Le Roux added a comment - Taher, "not done yet" meant here that the BI services were not run, this Parimal's comment above: 1) If dimension table doesn't have records then following services should execute from service engine - a) loadAllProductsInProductDimension b) loadCurrencyDimension c) loadDateDimension Changes in this issue are committed in trunk. They will backported to releases after OFBIZ-4794 . If you have something to submit, you should not wait for backports, they are totally dependent on trunk. You can see Birt reports are working today on trunk demo. Because I ran those services before creating and approving a new order. My knowledge about Birt and BI is limited, but I wonder why we need to run those services prior to have results in reports. I thought that running "Quick Init DataWarehouse" option in BI compoment would be enough but it did not work for me. So I had to run those services by hand. Maybe because I did not create an order just after, I can't remember. I will check that point...
          Hide
          Jacques Le Roux added a comment -

          I checked locally with an updated instance. I used the BI button, checked the dimensions tables had values. I then created and approved an order, then checked SalesOrderItemFact had values. Then OrderDiscountCode.pdf has numbers but not checkReportBy nor Last3MonthsSalesReport.pdf. I then ran the loadDateDimension service by hand. Because I noticed it has date parameters. Setting them to a wide range did the trick. So we have maybe to wider the default range (in data or service to be checked) and all should be ok: only 1 button to push . Then a sentence in UI where reports are used should be enough: KISS and YAGNI ways...

          Show
          Jacques Le Roux added a comment - I checked locally with an updated instance. I used the BI button, checked the dimensions tables had values. I then created and approved an order, then checked SalesOrderItemFact had values. Then OrderDiscountCode.pdf has numbers but not checkReportBy nor Last3MonthsSalesReport.pdf. I then ran the loadDateDimension service by hand. Because I noticed it has date parameters. Setting them to a wide range did the trick. So we have maybe to wider the default range (in data or service to be checked) and all should be ok: only 1 button to push . Then a sentence in UI where reports are used should be enough: KISS and YAGNI ways...
          Hide
          Parimal Gain added a comment -

          Hi Jacques/Taher,

          IMO: Having service level check/check for exception is always good to reduce the risk of failure. If this is doable then will be great but the question is why the user should be informed after viewing the report why not prior to that or As user comes to report section is gets aware about all configuration.

          I mean there should be a screen with all these configuration of dimension table with notification of currently configured value. For ex. if DateDimension is configured from 01/01/2013 to 31/12/2013 then there should be message to user that report can be generated for these date range only is report is require for the above date the execute the loadDateDimentin service and the screen contains the button to load the date data. Similarly for currency and product there should be drop-down with available currency and product for report.

          What you guys think?.....

          Show
          Parimal Gain added a comment - Hi Jacques/Taher, IMO: Having service level check/check for exception is always good to reduce the risk of failure. If this is doable then will be great but the question is why the user should be informed after viewing the report why not prior to that or As user comes to report section is gets aware about all configuration. I mean there should be a screen with all these configuration of dimension table with notification of currently configured value. For ex. if DateDimension is configured from 01/01/2013 to 31/12/2013 then there should be message to user that report can be generated for these date range only is report is require for the above date the execute the loadDateDimentin service and the screen contains the button to load the date data. Similarly for currency and product there should be drop-down with available currency and product for report. What you guys think?.....
          Hide
          Jacques Le Roux added a comment -

          Yes I'm more inclined to this kind of solution indeed

          Show
          Jacques Le Roux added a comment - Yes I'm more inclined to this kind of solution indeed
          Hide
          Taher Alkhateeb added a comment - - edited

          Hi Parimal,

          Your suggestion makes sense and feels more intuitive by running validation logic in the screen/form/script. If validation fails, we simply do not call the report .. I really like that!

          This suggestion, however, means that we have two sets of code now: inside the report and outside. How do we make sure they both work when you change something?

          So this brings a new problem in BIRT inside OFBiz: Testing! you can only call JUnit tests (okay, OFBizTestCase) which cannot call the code inside the reports! and it seems BIRT encapsulates exceptions which means I cannot catch them in my unit tests.

          I feel a little stuck, does that mean we should always test manually? can we automate this?

          Show
          Taher Alkhateeb added a comment - - edited Hi Parimal, Your suggestion makes sense and feels more intuitive by running validation logic in the screen/form/script. If validation fails, we simply do not call the report .. I really like that! This suggestion, however, means that we have two sets of code now: inside the report and outside. How do we make sure they both work when you change something? So this brings a new problem in BIRT inside OFBiz: Testing! you can only call JUnit tests (okay, OFBizTestCase) which cannot call the code inside the reports! and it seems BIRT encapsulates exceptions which means I cannot catch them in my unit tests. I feel a little stuck, does that mean we should always test manually? can we automate this?
          Hide
          Jacques Le Roux added a comment -

          I reiterate my KISS and YAGNI solution:

          Currently the quickInitDataWarehouse BI button uses these dates (set in BI main screen)

          <set field="fromDate" value="2008-01-01 00:00:00.0" type="Timestamp"/>
          <set field="thruDate" value="2010-01-01 00:00:00.0" type="Timestamp"/>

          This generates 1000+ records. A simple solution would be to widen the range (say from 2013-09-01 to 2043-09-01). 30 years ahead would generate 10 000+ records which is still reasonnable. Then a message, placed in the UI, in all places where BI dimension reports are used would inform the user that for this type of reports he needs 1st to run the data warehouse init using the BI button. Also these message will clarify the range, to be certain that in 30 years people will still easily know what to change...

          Show
          Jacques Le Roux added a comment - I reiterate my KISS and YAGNI solution: Currently the quickInitDataWarehouse BI button uses these dates (set in BI main screen) <set field="fromDate" value="2008-01-01 00:00:00.0" type="Timestamp"/> <set field="thruDate" value="2010-01-01 00:00:00.0" type="Timestamp"/> This generates 1000+ records. A simple solution would be to widen the range (say from 2013-09-01 to 2043-09-01). 30 years ahead would generate 10 000+ records which is still reasonnable. Then a message, placed in the UI, in all places where BI dimension reports are used would inform the user that for this type of reports he needs 1st to run the data warehouse init using the BI button. Also these message will clarify the range, to be certain that in 30 years people will still easily know what to change...
          Hide
          Jacques Le Roux added a comment -

          Taher,

          Our comments crossed on wire. For tests, I believe it's all about running the quickInitDataWarehouse service during the test phase (with appropriate dates of course).

          Show
          Jacques Le Roux added a comment - Taher, Our comments crossed on wire. For tests, I believe it's all about running the quickInitDataWarehouse service during the test phase (with appropriate dates of course).
          Hide
          Taher Alkhateeb added a comment -

          Hi Jacques,

          Okay, that is KISS indeed. Noted and will work accordingly from my side!

          Show
          Taher Alkhateeb added a comment - Hi Jacques, Okay, that is KISS indeed. Noted and will work accordingly from my side!
          Hide
          Jacques Le Roux added a comment -

          At revision: 1521589 I commited the change I suggested

          I set 10 years ahead (from sept. 2013) in a new bi.properties file used by the BI main screen. So it generates only 3000+ records (and others related in BI DB) which is still reasonable. A message, placed at the top of the Order Reports page, informs users that for 4 Birt reports s/he needs 1st to run the data warehouse init using the BI button. Also this message clarifies the range, to be certain that in 10 years people will still easily know what to change...

          I have also replaced a lot of labels and added English and French translations (by-product: some labels cleaning in CommonUiLabels.xml)

          I did not do the same for the accounting paymentReport and ViewFacilityInventoryHistoryReport facility Birt reports which only need translation.

          I noticed that there are no ways to get to ViewFacilityInventoryHistoryReport from UI though (I fixed a typo in the Birt controller, it's not yet available in trunk demo but the url will be tomorrow morning (EU time) https://demo-trunk.ofbiz.apache.org/facility/control/ViewFacilityInventoryHistoryReport. I will see later how to branch this from Facility Report menu...

          I will maybe backport all this later in releases...

          Show
          Jacques Le Roux added a comment - At revision: 1521589 I commited the change I suggested I set 10 years ahead (from sept. 2013) in a new bi.properties file used by the BI main screen. So it generates only 3000+ records (and others related in BI DB) which is still reasonable. A message, placed at the top of the Order Reports page, informs users that for 4 Birt reports s/he needs 1st to run the data warehouse init using the BI button. Also this message clarifies the range, to be certain that in 10 years people will still easily know what to change... I have also replaced a lot of labels and added English and French translations (by-product: some labels cleaning in CommonUiLabels.xml) I did not do the same for the accounting paymentReport and ViewFacilityInventoryHistoryReport facility Birt reports which only need translation. I noticed that there are no ways to get to ViewFacilityInventoryHistoryReport from UI though (I fixed a typo in the Birt controller, it's not yet available in trunk demo but the url will be tomorrow morning (EU time) https://demo-trunk.ofbiz.apache.org/facility/control/ViewFacilityInventoryHistoryReport . I will see later how to branch this from Facility Report menu... I will maybe backport all this later in releases...
          Hide
          Jacques Le Roux added a comment -

          I forgot to commit a change with r1521589: done at r1521740

          Show
          Jacques Le Roux added a comment - I forgot to commit a change with r1521589: done at r1521740
          Hide
          Jacques Le Roux added a comment -

          About my comment above

          I noticed that there are no ways to get to ViewFacilityInventoryHistoryReport from UI though (I fixed a typo in the Birt controller, it's not yet available in trunk demo but the url will be tomorrow morning (EU time) https://demo-trunk.ofbiz.apache.org/facility/control/ViewFacilityInventoryHistoryReport. I will see later how to branch this from Facility Report menu...

          I was removed by Jacopo when he moved birt to specialpurpose. Because the way it was done there were strong dependencies to the birt component. I will use the same strategy than in order. Later I will check the accounting report also...

          Show
          Jacques Le Roux added a comment - About my comment above I noticed that there are no ways to get to ViewFacilityInventoryHistoryReport from UI though (I fixed a typo in the Birt controller, it's not yet available in trunk demo but the url will be tomorrow morning (EU time) https://demo-trunk.ofbiz.apache.org/facility/control/ViewFacilityInventoryHistoryReport . I will see later how to branch this from Facility Report menu... I was removed by Jacopo when he moved birt to specialpurpose. Because the way it was done there were strong dependencies to the birt component. I will use the same strategy than in order. Later I will check the accounting report also...
          Hide
          Jacques Le Roux added a comment - - edited

          == WRONG commits references ==
          Following Jacopo's message on dev ML, at revision: 1522675+1521740, I reverted r1518777
          I also reverted the change to ReportScreens.xml in order component at r1521589.

          Show
          Jacques Le Roux added a comment - - edited == WRONG commits references == Following Jacopo's message on dev ML , at revision: 1522675+1521740, I reverted r1518777 I also reverted the change to ReportScreens.xml in order component at r1521589.
          Hide
          Jacques Le Roux added a comment - - edited

          I was quite confused when at r1522664 I reverted changes in ReportScreens, I put them back at r1538081

          Show
          Jacques Le Roux added a comment - - edited I was quite confused when at r1522664 I reverted changes in ReportScreens, I put them back at r1538081

            People

            • Assignee:
              Jacques Le Roux
              Reporter:
              Parimal Gain
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development