OFBiz
  1. OFBiz
  2. OFBIZ-583

Offline payment selection now prevents completion of sales order

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: order
    • Labels:
      None

      Description

      I upgraded to the latest SVN on the weekend and now I find that when I raise a Sales Order in the Order Manager, and select OFFLINE as the payment option, the sale cannot proceed because it complains about no payment method being selected.

      CheckOutEvent.java has undergone some significant changes regarding checkout processing, and I suspect the following snippet:

      methodType = request.getParameter("paymentMethodType");
      if ("offline".equals(methodType))

      { Debug.log("Changing mode from->to: " + mode + "->payment", module); mode = "payment"; }

      If I've selected offline payment, shouldn't the mode be set to "addparty", not "payment"?

      Not sure who contributed the most recent changes, but would appreciate a speedy resolution to this one as I can't do sales orders without it!!!

      Cheers, Iain

      1. CheckOutEvents.patch
        3 kB
        Iain Fogg
      2. CheckOutEvents.patch
        2 kB
        Iain Fogg

        Activity

        Hide
        Si Chen added a comment -

        This is now fixed as of SVN r 502340. Note that the fix is different than the patch so that more general cases could be supported. If you have already applied this patch, you will need to $ svn revert them before $ svn update.

        Show
        Si Chen added a comment - This is now fixed as of SVN r 502340. Note that the fix is different than the patch so that more general cases could be supported. If you have already applied this patch, you will need to $ svn revert them before $ svn update.
        Hide
        Guido Amarilla added a comment -

        It works for me. I am using offline payments only and I couldn´t create an order until I installed this patch.

        Show
        Guido Amarilla added a comment - It works for me. I am using offline payments only and I couldn´t create an order until I installed this patch.
        Hide
        Si Chen added a comment -

        Can anyone else try this patch and let me know if it works?

        Show
        Si Chen added a comment - Can anyone else try this patch and let me know if it works?
        Hide
        David Shere added a comment -

        Thanks Ian. I've installed the patch and it works for me.

        Show
        David Shere added a comment - Thanks Ian. I've installed the patch and it works for me.
        Hide
        Iain Fogg added a comment -

        All,

        Sorry about the previous patch. I mistakenly ran a "diff" instead of an "svn diff". Hopefully people interested will have more luck with the later patch.

        Cheers, Iain

        Show
        Iain Fogg added a comment - All, Sorry about the previous patch. I mistakenly ran a "diff" instead of an "svn diff". Hopefully people interested will have more luck with the later patch. Cheers, Iain
        Hide
        David Shere added a comment -

        Thanks Ian.

        How do I apply this patch? I get these results when I try:

        root@raptor:/ofbiz# patch -p 0 < patches/CheckOutEvents.patch
        can't find file to patch at input line 1
        Perhaps you used the wrong -p or --strip option?
        File to patch: patches/CheckOutEvents.patch
        patching file patches/CheckOutEvents.patch
        Hunk #6 FAILED at 754.
        1 out of 6 hunks FAILED – saving rejects to file patches/CheckOutEvents.patch.rej
        root@raptor:/ofbiz#

        Show
        David Shere added a comment - Thanks Ian. How do I apply this patch? I get these results when I try: root@raptor:/ofbiz# patch -p 0 < patches/CheckOutEvents.patch can't find file to patch at input line 1 Perhaps you used the wrong -p or --strip option? File to patch: patches/CheckOutEvents.patch patching file patches/CheckOutEvents.patch Hunk #6 FAILED at 754. 1 out of 6 hunks FAILED – saving rejects to file patches/CheckOutEvents.patch.rej root@raptor:/ofbiz#
        Hide
        Iain Fogg added a comment -

        All,

        Sorry it's been a while, but I've had a hard time getting spare cycles to do the right thing and generate a patch. My latest update from the repository is r491209, against which I've generated the patch file. I realise this is almost a month old; I'll be trying to upgrade this weekend and can regenerate the patch if it causes problems for other people (i.e., if CheckOutEvents.java has changed significantly in the last few weeks).

        FYI, I've been running this patch for the last month and have had no problems, although you should be aware that I don't do any online payments.

        • Iain
        Show
        Iain Fogg added a comment - All, Sorry it's been a while, but I've had a hard time getting spare cycles to do the right thing and generate a patch. My latest update from the repository is r491209, against which I've generated the patch file. I realise this is almost a month old; I'll be trying to upgrade this weekend and can regenerate the patch if it causes problems for other people (i.e., if CheckOutEvents.java has changed significantly in the last few weeks). FYI, I've been running this patch for the last month and have had no problems, although you should be aware that I don't do any online payments. Iain
        Hide
        David Shere added a comment -

        Anil, can I get some clarification on where to apply your patch? Starting at my line 244:

        public static String checkPaymentMethods(HttpServletRequest request, HttpServletResponse response) {
        ShoppingCart cart = (ShoppingCart) request.getSession().getAttribute("shoppingCart");
        LocalDispatcher dispatcher = (LocalDispatcher) request.getAttribute("dispatcher");
        GenericDelegator delegator = (GenericDelegator) request.getAttribute("delegator");
        CheckOutHelper checkOutHelper = new CheckOutHelper(dispatcher, delegator, cart);
        Map resp = checkOutHelper.validatePaymentMethods();
        if (ServiceUtil.isError(resp))

        { request.setAttribute("_ERROR_MESSAGE_", ServiceUtil.getErrorMessage(resp)); return "error"; }

        // THIS IS LINE 253
        return "success";
        }

        Should your patch be put between the if statement and "return "success";"?

        Show
        David Shere added a comment - Anil, can I get some clarification on where to apply your patch? Starting at my line 244: public static String checkPaymentMethods(HttpServletRequest request, HttpServletResponse response) { ShoppingCart cart = (ShoppingCart) request.getSession().getAttribute("shoppingCart"); LocalDispatcher dispatcher = (LocalDispatcher) request.getAttribute("dispatcher"); GenericDelegator delegator = (GenericDelegator) request.getAttribute("delegator"); CheckOutHelper checkOutHelper = new CheckOutHelper(dispatcher, delegator, cart); Map resp = checkOutHelper.validatePaymentMethods(); if (ServiceUtil.isError(resp)) { request.setAttribute("_ERROR_MESSAGE_", ServiceUtil.getErrorMessage(resp)); return "error"; } // THIS IS LINE 253 return "success"; } Should your patch be put between the if statement and "return "success";"?
        Hide
        Si Chen added a comment -

        I have noticed this problem too and will try to help you with it.

        To everybody on this thread:

        If you are creating a fix please update a patch file so that it can be applied. It is very time consuming to walk through descriptive text like Iain Fogg's above. Also we need to have the contribution be made with the "Grant license..." button checked on as well.

        Show
        Si Chen added a comment - I have noticed this problem too and will try to help you with it. To everybody on this thread: If you are creating a fix please update a patch file so that it can be applied. It is very time consuming to walk through descriptive text like Iain Fogg's above. Also we need to have the contribution be made with the "Grant license..." button checked on as well.
        Hide
        David Shere added a comment -

        This issue also has the side effect of telling you to specify a payment method, but isn't at a screen where you can do so.

        Show
        David Shere added a comment - This issue also has the side effect of telling you to specify a payment method, but isn't at a screen where you can do so.
        Hide
        Iain Fogg added a comment -

        Following Anil's lead, I made the following patches to CheckOutEvents.java:

        Around line 715 (with Anil's patch) replace:

        // check for offline payment type
        // payment option; if offline we skip the payment screen
        methodType = request.getParameter("paymentMethodType");
        if ("offline".equals(methodType))

        { Debug.log("Changing mode from->to: " + mode + "->payment", module); mode = "payment"; }

        with:

        // check for offline payment type
        // payment option; if offline we skip the payment screen
        HttpServletRequestWrapper wrapper = null;
        methodType = request.getParameter("paymentMethodType");
        if ("offline".equals(methodType)) {
        Debug.log("Changing mode from->to: " + mode + "->payment", module);
        mode = "payment";
        wrapper = new HttpServletRequestWrapper((HttpServletRequest)request) {
        public java.lang.String[] getParameterValues(java.lang.String name) {
        if ("checkOutPaymentId".equals(name)) {
        String[] ids = super.getParameterValues(name);
        int len = ids != null ? ids.length + 1 : 1;

        String[] ret = new String[len];

        for (int i = 0; i < len - 1; i++)

        { ret[i] = ids[i]; }

        ret[len - 1] = "EXT_OFFLINE";
        return ret;
        }
        else

        { return super.getParameterValues(name); }

        }
        };
        }

        and a little later (about line 747 or so), replace

        Map selectedPaymentMethods = getSelectedPaymentMethods(request);

        with

        Map selectedPaymentMethods = getSelectedPaymentMethods(wrapper != null ? wrapper : request);

        Since getSelectedPaymentMethods needs a HttpServletRequest param, we need to modify the request params on the way through. Apologies if there is a utility class available to add EXT_OFFLINE to other (possibly) existing checkOutPaymentId values, but I don't know if such exists.

        I can test the patch for OFFLINE as well as BILLING_ACCOUNT payments, but I'm not configured to do online stuff like CC and EFT. I hope this gets us a little closer to a working solution.

        Show
        Iain Fogg added a comment - Following Anil's lead, I made the following patches to CheckOutEvents.java: Around line 715 (with Anil's patch) replace: // check for offline payment type // payment option; if offline we skip the payment screen methodType = request.getParameter("paymentMethodType"); if ("offline".equals(methodType)) { Debug.log("Changing mode from->to: " + mode + "->payment", module); mode = "payment"; } with: // check for offline payment type // payment option; if offline we skip the payment screen HttpServletRequestWrapper wrapper = null; methodType = request.getParameter("paymentMethodType"); if ("offline".equals(methodType)) { Debug.log("Changing mode from->to: " + mode + "->payment", module); mode = "payment"; wrapper = new HttpServletRequestWrapper((HttpServletRequest)request) { public java.lang.String[] getParameterValues(java.lang.String name) { if ("checkOutPaymentId".equals(name)) { String[] ids = super.getParameterValues(name); int len = ids != null ? ids.length + 1 : 1; String[] ret = new String [len] ; for (int i = 0; i < len - 1; i++) { ret[i] = ids[i]; } ret [len - 1] = "EXT_OFFLINE"; return ret; } else { return super.getParameterValues(name); } } }; } and a little later (about line 747 or so), replace Map selectedPaymentMethods = getSelectedPaymentMethods(request); with Map selectedPaymentMethods = getSelectedPaymentMethods(wrapper != null ? wrapper : request); Since getSelectedPaymentMethods needs a HttpServletRequest param, we need to modify the request params on the way through. Apologies if there is a utility class available to add EXT_OFFLINE to other (possibly) existing checkOutPaymentId values, but I don't know if such exists. I can test the patch for OFFLINE as well as BILLING_ACCOUNT payments, but I'm not configured to do online stuff like CC and EFT. I hope this gets us a little closer to a working solution.
        Hide
        Anil K Patel added a comment -

        This problem is reported when new payment method is created. Event for the customer that have payment methods, when we try to create new Payment Method, we get this error.

        Show
        Anil K Patel added a comment - This problem is reported when new payment method is created. Event for the customer that have payment methods, when we try to create new Payment Method, we get this error.
        Hide
        Scott Gray added a comment -

        Hi Jacopo

        You need to use a customer with no payment details previously loaded (like a credit card), try it with admin and you should see the problem.

        Show
        Scott Gray added a comment - Hi Jacopo You need to use a customer with no payment details previously loaded (like a credit card), try it with admin and you should see the problem.
        Hide
        Jacopo Cappellato added a comment -

        I just created a sales order with offline payment in the demo server without any problems:

        https://demo.dejc.com:8443/ordermgr/control/orderview?orderId=WS10001

        How can I test this issue?

        Show
        Jacopo Cappellato added a comment - I just created a sales order with offline payment in the demo server without any problems: https://demo.dejc.com:8443/ordermgr/control/orderview?orderId=WS10001 How can I test this issue?
        Hide
        Iain Fogg added a comment -

        Scott/Anil,

        Thanks for the follow-up.

        You're absolutely right about Tim and co, asking for people to test. I foolishly thought that since all the mods seemed to relate to anonymous checkout, then it wouldn't affect me - oops! Having said that, I've also become a little blase about incorporating patches, as generally I've found patchess to be pretty well tested and normally are backward compatibility (or at least "do no harm"). I think this one just fell through the cracks.

        Please let me know if/when you find the solution - I'll be the first to patch and test!

        Cheers, Iain

        Show
        Iain Fogg added a comment - Scott/Anil, Thanks for the follow-up. You're absolutely right about Tim and co, asking for people to test. I foolishly thought that since all the mods seemed to relate to anonymous checkout, then it wouldn't affect me - oops! Having said that, I've also become a little blase about incorporating patches, as generally I've found patchess to be pretty well tested and normally are backward compatibility (or at least "do no harm"). I think this one just fell through the cracks. Please let me know if/when you find the solution - I'll be the first to patch and test! Cheers, Iain
        Hide
        Scott Gray added a comment -

        It looks like some changes Jacopo made last month, I'll revert a few things and see what happens. Thanks for your help Anil

        Show
        Scott Gray added a comment - It looks like some changes Jacopo made last month, I'll revert a few things and see what happens. Thanks for your help Anil
        Hide
        Anil K Patel added a comment -

        If you paste this following code at line # 253 in CheckOutEvents.java. I know this will fix the CreditCard problem.

        if (paymentMethods == null) {
        String paymentMethodId = (String) request.getAttribute("paymentMethodId");
        if (paymentMethodId != null) {
        String[] paymentMethodsTemp =

        {paymentMethodId}

        ;
        paymentMethods = paymentMethodsTemp;
        }
        }

        Show
        Anil K Patel added a comment - If you paste this following code at line # 253 in CheckOutEvents.java. I know this will fix the CreditCard problem. if (paymentMethods == null) { String paymentMethodId = (String) request.getAttribute("paymentMethodId"); if (paymentMethodId != null) { String[] paymentMethodsTemp = {paymentMethodId} ; paymentMethods = paymentMethodsTemp; } }
        Hide
        Anil K Patel added a comment -

        I did some research, I think problem is in CheckOutEvents.java in getSelectedPaymentMethods. When a new PaymentMethod is created by the createCreditCard service and paymentMethodId is put in request.Attributes.

        I think getSelectedPaymentMethods should be modified to also look for paymentMethodId instead of just looking for checkOutPaymentId. But this may not be rights because I don't understand the checkout process in Ordermanager.

        Show
        Anil K Patel added a comment - I did some research, I think problem is in CheckOutEvents.java in getSelectedPaymentMethods. When a new PaymentMethod is created by the createCreditCard service and paymentMethodId is put in request.Attributes. I think getSelectedPaymentMethods should be modified to also look for paymentMethodId instead of just looking for checkOutPaymentId. But this may not be rights because I don't understand the checkout process in Ordermanager.
        Hide
        Anil K Patel added a comment -

        I see the problem but, this actually does not touch the modifications done in ecommerce checkout process. In the mean time, I am looking at the problem, I post my comments ASAP.

        Show
        Anil K Patel added a comment - I see the problem but, this actually does not touch the modifications done in ecommerce checkout process. In the mean time, I am looking at the problem, I post my comments ASAP.
        Hide
        Scott Gray added a comment -

        I understand your frustration Iain, but it does show the importance of trying out the patches that people submit. Tim and co. did ask for people to test the patch but I must admit I didn't, and if we had this might have been picked up and then fixed straight away prior to being commited.

        Show
        Scott Gray added a comment - I understand your frustration Iain, but it does show the importance of trying out the patches that people submit. Tim and co. did ask for people to test the patch but I must admit I didn't, and if we had this might have been picked up and then fixed straight away prior to being commited.
        Hide
        Iain Fogg added a comment -

        I just wanted to keep this issue near the top of the list - would it be possible to the team who worked recently on the checkout process to take a look at this and at least let us know why offline payments no longer work.

        I guess this raises a question about regression testing of significant new features, but that for another thread

        Cheers, Iain

        Show
        Iain Fogg added a comment - I just wanted to keep this issue near the top of the list - would it be possible to the team who worked recently on the checkout process to take a look at this and at least let us know why offline payments no longer work. I guess this raises a question about regression testing of significant new features, but that for another thread Cheers, Iain
        Hide
        Scott Gray added a comment -

        That's the screen I'm getting to when trying offline payment except its not showing the CC stuff but just the billing address form

        Show
        Scott Gray added a comment - That's the screen I'm getting to when trying offline payment except its not showing the CC stuff but just the billing address form
        Hide
        Iain Fogg added a comment -

        Scott,

        Thanks for taking a look. When I tried it using CC payment, it took me to the next screen (entering CC details), but I didn't go further than that.

        I agree it would be good if the folks who have worked on this recently taking a look to see if the previously working functionality can be reinstated.

        Show
        Iain Fogg added a comment - Scott, Thanks for taking a look. When I tried it using CC payment, it took me to the next screen (entering CC details), but I didn't go further than that. I agree it would be good if the folks who have worked on this recently taking a look to see if the previously working functionality can be reinstated.
        Hide
        Scott Gray added a comment -

        Hi Iain

        I've had at look at this and the problem is not related to the code block you mention above. The problem is that the payment methods are never set in the cart so the system keeps thinking that payment is the next step to be completed. I tried to do it with CC payment to see what it does to get to the next step, but the same problem exists for all payment methods.

        I would rather someone who knows more about the check process fixes this as I'm not sure where and when to set the payment method in the cart.

        Show
        Scott Gray added a comment - Hi Iain I've had at look at this and the problem is not related to the code block you mention above. The problem is that the payment methods are never set in the cart so the system keeps thinking that payment is the next step to be completed. I tried to do it with CC payment to see what it does to get to the next step, but the same problem exists for all payment methods. I would rather someone who knows more about the check process fixes this as I'm not sure where and when to set the payment method in the cart.

          People

          • Assignee:
            Si Chen
            Reporter:
            Iain Fogg
          • Votes:
            3 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development