Missing Method Implementations in OrderChangeHelper

[Kyra Pritzel-Hentley]

Hello OFBiz Enthusiasts,

I am working to rid OFBiz of Bugs using the help of a code analysis tool called FindBugs. With this, I find all kinds of peculiar code in the OFBiz project. This JIRA ticket describes further details about this effort: https://issues.apache.org/jira/browse/OFBIZ-9450 

FindBugs pointed out to me that inside PayPalEvents#payPalIPN the method OrderChangeHelper.releaseInitialOrderHold is called. But looking at the implementation of the method, it only returns true  even though the parameters LocalDispatcher dispatcher and String orderId are handed over to the method. The same problem arises with OrderChangeHelper#abortOrderProcessing.

There doesn’t seem to be a ticket working on this issue. I am fairly certain, looking at the names of the methods, that something more should happen other than just returning true. Does somebody have an idea what is supposed to happen with these methods?

Thank you,
Kyra Pritzel-Hentley

ecomify GmbH
Bielefeld, Germany

[Deepak Nigam]

Hi Kyra,

It seems that these changes are intentional, you can also check the comment
inside the code itself:

"NOTE DEJ20080609 commenting out this code because the old OFBiz Workflow
Engine is being deprecated and this was only for that get the delegator
from the dispatcher"

For more information, please have a look at the following:

https://svn.apache.org/viewvc?view=revision&revision=665981

Thanks & Regards
--
Deepak Nigam
Technical Consultant/Team Lead
HotWax Systems
www.hotwaxsystems.com

On Thu, Aug 3, 2017 at 2:08 PM, Kyra Pritzel-Hentley <
[hidden email]> wrote:

> Hello OFBiz Enthusiasts,
>
> I am working to rid OFBiz of Bugs using the help of a code analysis tool
> called FindBugs. With this, I find all kinds of peculiar code in the OFBiz
> project. This JIRA ticket describes further details about this effort:
> https://issues.apache.org/jira/browse/OFBIZ-9450
>
> FindBugs pointed out to me that inside PayPalEvents#payPalIPN the method
> OrderChangeHelper.releaseInitialOrderHold is called. But looking at the
> implementation of the method, it only returns true  even though the
> parameters LocalDispatcher dispatcher and String orderId are handed over to
> the method. The same problem arises with OrderChangeHelper#
> abortOrderProcessing.
>
> There doesn’t seem to be a ticket working on this issue. I am fairly
> certain, looking at the names of the methods, that something more should
> happen other than just returning true. Does somebody have an idea what is
> supposed to happen with these methods?
>
> Thank you,
> Kyra Pritzel-Hentley
>
> ecomify GmbH
> Bielefeld, Germany

[Kyra Pritzel-Hentley]

Hello Deepak,

thank you for this information. I understand now that there used to be an implementation that is now deprecated. Would it not make sense to comment on these methods so that who ever else may stumble upon them knows that nothing is wrong? In the newest trunk, nothing indicates that there used to be code there and the methods are used throughout the project as though they are still implemented.

Thank you and best regards,
Kyra Pritzel-Hentley

ecomify GmbH
Bielefeld, Germany

> Am 03.08.2017 um 13:39 schrieb Deepak Nigam <[hidden email]>:
>
> Hi Kyra,
>
> It seems that these changes are intentional, you can also check the comment
> inside the code itself:
>
> "NOTE DEJ20080609 commenting out this code because the old OFBiz Workflow
> Engine is being deprecated and this was only for that get the delegator
> from the dispatcher"
>
> For more information, please have a look at the following:
>
> https://svn.apache.org/viewvc?view=revision&revision=665981
>
> Thanks & Regards
> --
> Deepak Nigam
> Technical Consultant/Team Lead
> HotWax Systems
> www.hotwaxsystems.com
>
> On Thu, Aug 3, 2017 at 2:08 PM, Kyra Pritzel-Hentley <
> [hidden email]> wrote:
>
>> Hello OFBiz Enthusiasts,
>>
>> I am working to rid OFBiz of Bugs using the help of a code analysis tool
>> called FindBugs. With this, I find all kinds of peculiar code in the OFBiz
>> project. This JIRA ticket describes further details about this effort:
>> https://issues.apache.org/jira/browse/OFBIZ-9450
>>
>> FindBugs pointed out to me that inside PayPalEvents#payPalIPN the method
>> OrderChangeHelper.releaseInitialOrderHold is called. But looking at the
>> implementation of the method, it only returns true  even though the
>> parameters LocalDispatcher dispatcher and String orderId are handed over to
>> the method. The same problem arises with OrderChangeHelper#
>> abortOrderProcessing.
>>
>> There doesn’t seem to be a ticket working on this issue. I am fairly
>> certain, looking at the names of the methods, that something more should
>> happen other than just returning true. Does somebody have an idea what is
>> supposed to happen with these methods?
>>
>> Thank you,
>> Kyra Pritzel-Hentley
>>
>> ecomify GmbH
>> Bielefeld, Germany

[Jacques Le Roux]

Hi Kyra,

Actually it was an error to remove this comment (thanks Deepak!) at r1758006 https://s.apache.org/neiF. I wonder if other alike comments were not
wrongly removed...

But anyway I think it's clear those 2 methods (releaseInitialOrderHold and abortOrderProcessing) are no longer needed and a refactoring is needed. For
instance this block of code in OrderServices.setOrderStatus() is only dust and useless:

         // release the inital hold if we are cancelled or approved
         if ("ORDER_CANCELLED".equals(statusId) || "ORDER_APPROVED".equals(statusId)) {
OrderChangeHelper.releaseInitialOrderHold(ctx.getDispatcher(), orderId);

             // cancel any order processing if we are cancelled
             if ("ORDER_CANCELLED".equals(statusId)) {
OrderChangeHelper.abortOrderProcessing(ctx.getDispatcher(), orderId);
             }
         }

We no longer use a workflow engine, and will never use one now that we rely on ECA (ie the transition is finished).

So please create a Jira to remove such old deprecated pieces of code, not only those related with no use of a workflow engine.
A way to do that could be a review of OFBIZ-7536, and would be greatly appreciated, ouch? ;)

Thanks for your effort on this.

Jacques


Le 04/08/2017 à 10:19, Kyra Pritzel-Hentley a écrit :

> Hello Deepak,
>
> thank you for this information. I understand now that there used to be an implementation that is now deprecated. Would it not make sense to comment on these methods so that who ever else may stumble upon them knows that nothing is wrong? In the newest trunk, nothing indicates that there used to be code there and the methods are used throughout the project as though they are still implemented.
>
> Thank you and best regards,
> Kyra Pritzel-Hentley
>
> ecomify GmbH
> Bielefeld, Germany
>
>> Am 03.08.2017 um 13:39 schrieb Deepak Nigam <[hidden email]>:
>>
>> Hi Kyra,
>>
>> It seems that these changes are intentional, you can also check the comment
>> inside the code itself:
>>
>> "NOTE DEJ20080609 commenting out this code because the old OFBiz Workflow
>> Engine is being deprecated and this was only for that get the delegator
>> from the dispatcher"
>>
>> For more information, please have a look at the following:
>>
>> https://svn.apache.org/viewvc?view=revision&revision=665981
>>
>> Thanks & Regards
>> --
>> Deepak Nigam
>> Technical Consultant/Team Lead
>> HotWax Systems
>> www.hotwaxsystems.com
>>
>> On Thu, Aug 3, 2017 at 2:08 PM, Kyra Pritzel-Hentley <
>> [hidden email]> wrote:
>>
>>> Hello OFBiz Enthusiasts,
>>>
>>> I am working to rid OFBiz of Bugs using the help of a code analysis tool
>>> called FindBugs. With this, I find all kinds of peculiar code in the OFBiz
>>> project. This JIRA ticket describes further details about this effort:
>>> https://issues.apache.org/jira/browse/OFBIZ-9450
>>>
>>> FindBugs pointed out to me that inside PayPalEvents#payPalIPN the method
>>> OrderChangeHelper.releaseInitialOrderHold is called. But looking at the
>>> implementation of the method, it only returns true  even though the
>>> parameters LocalDispatcher dispatcher and String orderId are handed over to
>>> the method. The same problem arises with OrderChangeHelper#
>>> abortOrderProcessing.
>>>
>>> There doesn’t seem to be a ticket working on this issue. I am fairly
>>> certain, looking at the names of the methods, that something more should
>>> happen other than just returning true. Does somebody have an idea what is
>>> supposed to happen with these methods?
>>>
>>> Thank you,
>>> Kyra Pritzel-Hentley
>>>
>>> ecomify GmbH
>>> Bielefeld, Germany
>

[Kyra Pritzel-Hentley]

Hello Jacques,

thank you for your reply. I have created a new JIRA-ticket dedicated to removing deprecated code since the one you referred me to seemed to be specifically for code that is commented out.

The ticket can be found here: https://issues.apache.org/jira/browse/OFBIZ-9569 <https://issues.apache.org/jira/browse/OFBIZ-9569>
and any help on refactoring this kind of code (especially if it is not flagged as deprecated) would be greatly appreciated. The first subtask for OrderChangeHelper is already on the roll.

Thank you and best regards,

Kyra Pritzel-Hentley
ecomify GmbH
www.ecomify.de