OFBiz
  1. OFBiz
  2. OFBIZ-659

Refactoring Create Order process

    Details

    • Type: Improvement Improvement
    • Status: Open
    • Priority: Major Major
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: order
    • Labels:
      None

      Description

      Focus of this Jira Issue is Refactoring Create Order Process, Implement a parallel code as needed so that the current Code remains intact.

      From David's email on Mailing list.
      Just some quick thoughts...

      Following the pattern of the anonymous and other checkout processes
      in the ecommerce app would be a great way to go.

      In the order manager there are various paths through the checkout
      process so we might have, for example, 2 different sets of request-
      map definitions and two different "progress bars", one for sales
      orders and one for purchase orders. These two different sets of high-
      level artifacts can point to the same views, services/events,
      screens, data prep scripts, etc wherever the two processes overlap.

      -David

      My Initial comments
      The anonymous checkout process in Ecommerce component provides some high level guiding principals. Few things that I can think of are
      1) moving some code that's embedded in Java classes into small simple methods.
      2) Moving process control logic from event handlers to Controller file.

      1. OrderChekOutProcessRefac.patch
        36 kB
        Anil K Patel
      2. OrderProcessSimpleMethods.patch
        3 kB
        Anil K Patel
      3. OrderChekOutProcessRefac.patch
        45 kB
        Anil K Patel
      4. OrderChekOutProcessRefac.patch
        90 kB
        Anil K Patel
      5. OrderChekOutProcessRefac.patch
        107 kB
        Anil K Patel
      6. OrderCheckoutProcess.patch
        115 kB
        Anil K Patel
      7. OrderCheckoutProcess.patch
        117 kB
        Anil K Patel
      8. OrderCheckoutProcess.patch
        117 kB
        Anil K Patel
      9. OrderCheckoutProcess.patch
        130 kB
        Anil K Patel
      10. OrderCheckoutProcess.patch
        49 kB
        Mridul Pathak

        Activity

        Hide
        Mridul Pathak added a comment -

        Anil and All
        I am working on the refactoring of Order Checkout Process in the Ordermgr for creating 2 different sets of request-map definitions and two different "progress bars", one for sales orders and one for purchase orders as par the description given in this issue.
        Here is the first patch from my side.
        The refactoring is not complete yet. This is just a starting patch. Here is some info of modifications or additions done in this new patch.
        1) Created two different paths for Sales Order and Purchase Order on the application menu.
        2) Only Sales Order Path is functional and not the Purchase Order path.
        3) Implemented initializeSalesOrder and setOrderCurrencyAgreementShipDates simple methods instead of using Java.
        4) Implemented screens to initialize sales order and purchase order.
        5) Added new request definitions.
        The Sales Order checkout process is functional and can be tested. It doesn't affect the current Checkout Process except a small change in "additem" request-map, which will be removed when I'll submit the next updated patch.
        Right now I am wroking to split the ShowCart section for Sales and Purchase Order. I'll Submit the updated patch as soon as I finish it.

        Show
        Mridul Pathak added a comment - Anil and All I am working on the refactoring of Order Checkout Process in the Ordermgr for creating 2 different sets of request-map definitions and two different "progress bars", one for sales orders and one for purchase orders as par the description given in this issue. Here is the first patch from my side. The refactoring is not complete yet. This is just a starting patch. Here is some info of modifications or additions done in this new patch. 1) Created two different paths for Sales Order and Purchase Order on the application menu. 2) Only Sales Order Path is functional and not the Purchase Order path. 3) Implemented initializeSalesOrder and setOrderCurrencyAgreementShipDates simple methods instead of using Java. 4) Implemented screens to initialize sales order and purchase order. 5) Added new request definitions. The Sales Order checkout process is functional and can be tested. It doesn't affect the current Checkout Process except a small change in "additem" request-map, which will be removed when I'll submit the next updated patch. Right now I am wroking to split the ShowCart section for Sales and Purchase Order. I'll Submit the updated patch as soon as I finish it.
        Hide
        Si Chen added a comment -

        Anil,

        Now that we have the release branch I think this is actually something which we should be doing in the main SVN trunk. The old order checkout code could certainly use a lot of enhancing, so if you have the time and interest, this would be a great contribution.

        One suggestion that I have is that the ShoppingCart should be more object-oriented. For example, ShoppingCartItem should be sub-classing into FinishedGoodCartItem, DigitalGoodCartItem, RentalCartItem, FinancialAccountCartItem, etc. etc. They should share common methods in ShoppingCartItem but type-specific code such as that for reserving room dates moved into the specific cart item.

        Good luck, and thanks for all the contributions!

        Si

        Show
        Si Chen added a comment - Anil, Now that we have the release branch I think this is actually something which we should be doing in the main SVN trunk. The old order checkout code could certainly use a lot of enhancing, so if you have the time and interest, this would be a great contribution. One suggestion that I have is that the ShoppingCart should be more object-oriented. For example, ShoppingCartItem should be sub-classing into FinishedGoodCartItem, DigitalGoodCartItem, RentalCartItem, FinancialAccountCartItem, etc. etc. They should share common methods in ShoppingCartItem but type-specific code such as that for reserving room dates moved into the specific cart item. Good luck, and thanks for all the contributions! Si
        Hide
        Anil K Patel added a comment -

        Small inprovements.

        Show
        Anil K Patel added a comment - Small inprovements.
        Hide
        Anil K Patel added a comment -

        With some more improvement made this morning.

        Show
        Anil K Patel added a comment - With some more improvement made this morning.
        Hide
        Anil K Patel added a comment -

        Updated with few more lines of better code.

        Show
        Anil K Patel added a comment - Updated with few more lines of better code.
        Hide
        Anil K Patel added a comment -

        Added few methods for add payment to shoppingcart.

        Show
        Anil K Patel added a comment - Added few methods for add payment to shoppingcart.
        Hide
        Anil K Patel added a comment -

        Working on Payment Options screen. I am planning to have a list of Payment Methods and one line item for gift card information, one for EftAccount. Each line Item will have Text box for amount to use from the payment method and a update button to add it to shopping cart.

        Show
        Anil K Patel added a comment - Working on Payment Options screen. I am planning to have a list of Payment Methods and one line item for gift card information, one for EftAccount. Each line Item will have Text box for amount to use from the payment method and a update button to add it to shopping cart.
        Hide
        Anil K Patel added a comment -

        Work in progress code. Please find time to review this work as its moving forward. Thanks.

        Show
        Anil K Patel added a comment - Work in progress code. Please find time to review this work as its moving forward. Thanks.
        Hide
        Anil K Patel added a comment -

        Jacopo, Thanks for actively helping me in this process.

        OrderEntryCartScreens#ShowCart screen has several ftl files included that are related to Promotions. In case of Purchase Order, I don't see their use. For some reasons (I'll document when I post patch) I have copied this screen and created two more screens, ShowSalesOrderCart, ShowPurchaseOrderCart . I am thinking we can drop including those promotions specific ftl's (promoCodes.ftl, manualPromotions.ftl, promotionsApplied.ftl ) from ShowPurchaseOrderCart. Is this Ok to do or I am missing some functional requirements.
        Feedback is much appreciated.

        Show
        Anil K Patel added a comment - Jacopo, Thanks for actively helping me in this process. OrderEntryCartScreens#ShowCart screen has several ftl files included that are related to Promotions. In case of Purchase Order, I don't see their use. For some reasons (I'll document when I post patch) I have copied this screen and created two more screens, ShowSalesOrderCart, ShowPurchaseOrderCart . I am thinking we can drop including those promotions specific ftl's (promoCodes.ftl, manualPromotions.ftl, promotionsApplied.ftl ) from ShowPurchaseOrderCart. Is this Ok to do or I am missing some functional requirements. Feedback is much appreciated.
        Hide
        Jacopo Cappellato added a comment -

        Anil,

        about the supplier drop down in the ship settings page: this is used to assign to a ship group a supplier; this means that the items in that group will be drop shipped (i.e. a purchase order is created for the supplier for these items where the ship to address is the customer's address and not the company's warehouse).

        Note: from that screen you can also create new ship groups (using the "create new ship group" link); if in the cart there are more than one ship groups, then a new checkout screen will appear, where you can move the items from one ship group to the other

        Show
        Jacopo Cappellato added a comment - Anil, about the supplier drop down in the ship settings page: this is used to assign to a ship group a supplier; this means that the items in that group will be drop shipped (i.e. a purchase order is created for the supplier for these items where the ship to address is the customer's address and not the company's warehouse). Note: from that screen you can also create new ship groups (using the "create new ship group" link); if in the cart there are more than one ship groups, then a new checkout screen will appear, where you can move the items from one ship group to the other
        Hide
        Anil K Patel added a comment -

        Si,
        I went back and looked at the modification I have made. I have created few Event handlers, most of the code in them is from the same file. Also while working on this code, I didn't notice any code that shouldn't be in that file. Still I'll appreciate your comments on this code.

        Show
        Anil K Patel added a comment - Si, I went back and looked at the modification I have made. I have created few Event handlers, most of the code in them is from the same file. Also while working on this code, I didn't notice any code that shouldn't be in that file. Still I'll appreciate your comments on this code.
        Hide
        Anil K Patel added a comment -

        Si,
        Can you please give little more specific details on problem you may not noticed. I try my best to keep code organized.
        Your comments will help to make it easy for final review.

        Show
        Anil K Patel added a comment - Si, Can you please give little more specific details on problem you may not noticed. I try my best to keep code organized. Your comments will help to make it easy for final review.
        Hide
        Si Chen added a comment -

        Whatever you're doing, please make sure that there's still the separation of Events/Helper layers. This is generally good coding practice and would allow somebody else to write a custom checkout routine reusing the existing code.

        Show
        Si Chen added a comment - Whatever you're doing, please make sure that there's still the separation of Events/Helper layers. This is generally good coding practice and would allow somebody else to write a custom checkout routine reusing the existing code.
        Hide
        Anil K Patel added a comment -

        Made some progress. If anybody interested, Please review.
        In the current checkout process, In ShipSettings screen, For Sales Order we have a Dropdown for Supplier, I haven't gone into its details yet but I'll appreciate input on things that I should be aware of while working on this screen.

        Show
        Anil K Patel added a comment - Made some progress. If anybody interested, Please review. In the current checkout process, In ShipSettings screen, For Sales Order we have a Dropdown for Supplier, I haven't gone into its details yet but I'll appreciate input on things that I should be aware of while working on this screen.
        Hide
        Anil K Patel added a comment -

        Jacopo,
        Thanks for your comments.

        Show
        Anil K Patel added a comment - Jacopo, Thanks for your comments.
        Hide
        Jacopo Cappellato added a comment -

        Anil,

        about your last question about agreement screens:

        • I think that the agreement selection should be done as soon as possible; the reason is that the selected agreement is optionally used to determine the currency for the cart and (for sales agreement) also the prices of the products sold to a customer
        • about the terms screen: I think that it is more natural to have it in the checkout screens, if you really think we should move it in the order init screens for me it's ok, but leave this as a configurable option (I mean that playing with the controller entries should allow us to change the behaviour); also there are plans to make the terms screen also available to sales orders (if an agreement is selected), right now it is only shown for purchase orders

        Thanks for your work on this!

        Show
        Jacopo Cappellato added a comment - Anil, about your last question about agreement screens: I think that the agreement selection should be done as soon as possible; the reason is that the selected agreement is optionally used to determine the currency for the cart and (for sales agreement) also the prices of the products sold to a customer about the terms screen: I think that it is more natural to have it in the checkout screens, if you really think we should move it in the order init screens for me it's ok, but leave this as a configurable option (I mean that playing with the controller entries should allow us to change the behaviour); also there are plans to make the terms screen also available to sales orders (if an agreement is selected), right now it is only shown for purchase orders Thanks for your work on this!
        Hide
        Anil K Patel added a comment -

        I'll like input on a Question I have,
        If we select Agreement while creating an Order, Later after we have added Items to Order and we go on to Finalize the order, Order Terms screen is presented where we can make changes to Terms.

        The Question is, The Order Terms step is dependent on selection in Agreement screen, Can we combine these two steps into One Screen or at least have Order Term screen follow immediately the Agreement Screen.

        Input will be helpful in refactoring process.

        Show
        Anil K Patel added a comment - I'll like input on a Question I have, If we select Agreement while creating an Order, Later after we have added Items to Order and we go on to Finalize the order, Order Terms screen is presented where we can make changes to Terms. The Question is, The Order Terms step is dependent on selection in Agreement screen, Can we combine these two steps into One Screen or at least have Order Term screen follow immediately the Agreement Screen. Input will be helpful in refactoring process.
        Hide
        Jonathon Wong added a comment -

        I'm for keeping what's working. But if someone can refactor all those Java codes into Minilang codes without error and without changing the original functionalities, then I'd say go ahead.

        As for Java vs Minilang, are we certain that all the Java codes necessary in the Create Order process can even be translated into Minilang? Last I saw, Minilang has many limitations, particularly error-catching.

        Show
        Jonathon Wong added a comment - I'm for keeping what's working. But if someone can refactor all those Java codes into Minilang codes without error and without changing the original functionalities, then I'd say go ahead. As for Java vs Minilang, are we certain that all the Java codes necessary in the Create Order process can even be translated into Minilang? Last I saw, Minilang has many limitations, particularly error-catching.
        Hide
        Tim Ruppert added a comment -

        Anil, my vote would be to start putting all of this discussion and implementation in http://issues.apache.org/jira/browse/OFBIZ-499 - and close this one down.

        Then discussions of Java vs mini-lang is one that we can discuss, but let's put it in one place

        Show
        Tim Ruppert added a comment - Anil, my vote would be to start putting all of this discussion and implementation in http://issues.apache.org/jira/browse/OFBIZ-499 - and close this one down. Then discussions of Java vs mini-lang is one that we can discuss, but let's put it in one place
        Hide
        Anil K Patel added a comment -

        Si,
        I'll try to review most of the files involved in The Order CheckOut Process, and then Post here my findings on what I think should be in Java and what should be implemented in minilang, May be then we can discuss with more specific details and evaluate pros and cons of doing it.
        Do you think this will be a good approch?

        Show
        Anil K Patel added a comment - Si, I'll try to review most of the files involved in The Order CheckOut Process, and then Post here my findings on what I think should be in Java and what should be implemented in minilang, May be then we can discuss with more specific details and evaluate pros and cons of doing it. Do you think this will be a good approch?
        Hide
        Si Chen added a comment -

        I don't disagree that the implementation of the checkout process can be better, but I disagree with using minilang for it. You may have seen from the other JIRA issue that other people also did not want a minilang implementation of the checkout process either.

        Show
        Si Chen added a comment - I don't disagree that the implementation of the checkout process can be better, but I disagree with using minilang for it. You may have seen from the other JIRA issue that other people also did not want a minilang implementation of the checkout process either.
        Hide
        Anil K Patel added a comment -

        ShoppingCartEvents::setOrderCurrencyAgreementShipDates looks good.

        Show
        Anil K Patel added a comment - ShoppingCartEvents::setOrderCurrencyAgreementShipDates looks good.
        Hide
        Anil K Patel added a comment -

        The current implementation of Checkout process is way too complex and rigid. Like If we look at the ShoppingCartEvents::initializeOrderEntry tries to do too many things
        1) Permission check
        this should be moved out into service collection of its own.
        2) Code to initialize Sales Order and Purchase Order should be moved into methods of their own small methods.

        3) This method does not have but other event handlers on checkout process also do the work that should be handed over to controller.

        Given the rigid nature of current code, I'll like to do contribute a simplified version of this process. While I am doing this might as well do it in minilang.

        Show
        Anil K Patel added a comment - The current implementation of Checkout process is way too complex and rigid. Like If we look at the ShoppingCartEvents::initializeOrderEntry tries to do too many things 1) Permission check this should be moved out into service collection of its own. 2) Code to initialize Sales Order and Purchase Order should be moved into methods of their own small methods. 3) This method does not have but other event handlers on checkout process also do the work that should be handed over to controller. Given the rigid nature of current code, I'll like to do contribute a simplified version of this process. While I am doing this might as well do it in minilang.
        Hide
        Si Chen added a comment -

        Also, Anil, can you post your comments here, instead of replying to the JIRA posts on the dev list?

        Show
        Si Chen added a comment - Also, Anil, can you post your comments here, instead of replying to the JIRA posts on the dev list?
        Hide
        Si Chen added a comment -

        See related discussion about reimplementing checkout in minilang:
        http://issues.apache.org/jira/browse/OFBIZ-499

        Show
        Si Chen added a comment - See related discussion about reimplementing checkout in minilang: http://issues.apache.org/jira/browse/OFBIZ-499
        Hide
        Si Chen added a comment -

        Is the plan actually to re-write the checkout process in minilang?

        Show
        Si Chen added a comment - Is the plan actually to re-write the checkout process in minilang?
        Hide
        Anil K Patel added a comment -

        Initial activity on Implementing OrderCheckOutProcess in Minilang.

        Show
        Anil K Patel added a comment - Initial activity on Implementing OrderCheckOutProcess in Minilang.
        Hide
        Daniel Kunkel added a comment -

        I'd like to propose what I think is an improvement to the order entry process even though it may add an extra step.

        The way the ordering was working for us with a complicated order that takes adjustments is:

        Enter the order without adjustments.
        "Create the Order" which sends an e-mail with-out any adjustments.
        Adjust the order
        "Approve or fulfull the order" which I think sends another e-mail.

        Could the application of adjustments step be moved to a point before the e-mail is sent?

        Thanks

        Daniel

        Show
        Daniel Kunkel added a comment - I'd like to propose what I think is an improvement to the order entry process even though it may add an extra step. The way the ordering was working for us with a complicated order that takes adjustments is: Enter the order without adjustments. "Create the Order" which sends an e-mail with-out any adjustments. Adjust the order "Approve or fulfull the order" which I think sends another e-mail. Could the application of adjustments step be moved to a point before the e-mail is sent? Thanks Daniel
        Hide
        Anil K Patel added a comment -

        This patch is in "Work In progress" status. I am posting here to get feedback from community.

        Plan is to
        1) Break process into unit action processes.
        2) Put together a Skelaton code and get the process working.
        3) Redo code into mini-lang where possible.
        4) Clean up templates.
        5) Work on all rough edges to make it easy on User.

        Regards
        Anil Patel

        Show
        Anil K Patel added a comment - This patch is in "Work In progress" status. I am posting here to get feedback from community. Plan is to 1) Break process into unit action processes. 2) Put together a Skelaton code and get the process working. 3) Redo code into mini-lang where possible. 4) Clean up templates. 5) Work on all rough edges to make it easy on User. Regards Anil Patel

          People

          • Assignee:
            Anil K Patel
            Reporter:
            Anil K Patel
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:

              Development