Re: svn commit: r912645 - in /ofbiz/trunk/framework/widget/src/org/ofbiz/widget: form/ModelFormAction.java screen/ModelScreenAction.java

[Erwan de FERRIERES]

On 03/03/2010 22:23, Adam Heath wrote:

> Jacques Le Roux wrote:
>> Thanks Erwan,
>>
>> Looks like we are really building something more robust...
>>
>> Jacques
>>
>> From: "Erwan de FERRIERES"<[hidden email]>
>>> Le 03/03/2010 13:36, Jacques Le Roux a écrit :
>>>> Hi Erwan,
>>>>
>>>> Did you get a chance to have a try before I commit?
>>>>
>>>> Thanks
>>>>
>>>> Jacques
>>>>
>>> Hi Jacques,
>>>
>>> just come back from trying r917244 and it's good.
>>>
>>> BTW, I have added some selenium tests to the OFBIZ-3511 jira.
>>> In those, there is one called NavigateAllApplications which connects
>>> and disconnects the admin user from all the OFBiz apps.
>>>
>>> http://issues.apache.org/jira/browse/OFBIZ-3511
>
> Sorry, but not good enough.  We developers need to be able to run such
> tests when we change something, not wait until after we have committed it.
>

maybe not good enough but a start... and better than nothing !

Those tests can be run before committing to the trunk, you just have to
install to your firefox the selenium IDE extension, load the testsuite
and then click on "run all".

What can be done is a better integration as an ant task.

It will be the same thing with htmlunit, you always need to launch an
OFBiz instance and then run the tests.

--
Erwan de FERRIERES
www.nereide.biz

[Adam Heath-2]

Scott Gray wrote:

> On 3/03/2010, at 2:23 PM, Adam Heath wrote:
>
>> Jacques Le Roux wrote:
>>> Thanks Erwan,
>>>
>>> Looks like we are really building something more robust...
>>>
>>> Jacques
>>>
>>> From: "Erwan de FERRIERES" <[hidden email]>
>>>> Le 03/03/2010 13:36, Jacques Le Roux a écrit :
>>>>> Hi Erwan,
>>>>>
>>>>> Did you get a chance to have a try before I commit?
>>>>>
>>>>> Thanks
>>>>>
>>>>> Jacques
>>>>>
>>>> Hi Jacques,
>>>>
>>>> just come back from trying r917244 and it's good.
>>>>
>>>> BTW, I have added some selenium tests to the OFBIZ-3511 jira.
>>>> In those, there is one called NavigateAllApplications which connects
>>>> and disconnects the admin user from all the OFBiz apps.
>>>>
>>>> http://issues.apache.org/jira/browse/OFBIZ-3511
>> Sorry, but not good enough.  We developers need to be able to run such
>> tests when we change something, not wait until after we have committed it.
>>
>> I've been thinking about the ap/ar issue, and using httpunit to test
>> it(this particular problem doesn't require actually running from a
>> browser).  I've just been too busy the last few days to implement
>> anything.
>>
>
> Discussion is king, at some point someone decided that selenium was the way to go and now that is what we have in the project.  If we want something else to go in, lets discuss it before we throw in any more tools.

Ooooh!  I didn't know we had a king.  I thought we were an autonomous
collective.

If it is in, is there some ant target I can run?

[Scott Gray-2]

On 3/03/2010, at 4:35 PM, Adam Heath wrote:

> Scott Gray wrote:
>> On 3/03/2010, at 2:23 PM, Adam Heath wrote:
>>
>>> Jacques Le Roux wrote:
>>>> Thanks Erwan,
>>>>
>>>> Looks like we are really building something more robust...
>>>>
>>>> Jacques
>>>>
>>>> From: "Erwan de FERRIERES" <[hidden email]>
>>>>> Le 03/03/2010 13:36, Jacques Le Roux a écrit :
>>>>>> Hi Erwan,
>>>>>>
>>>>>> Did you get a chance to have a try before I commit?
>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>> Hi Jacques,
>>>>>
>>>>> just come back from trying r917244 and it's good.
>>>>>
>>>>> BTW, I have added some selenium tests to the OFBIZ-3511 jira.
>>>>> In those, there is one called NavigateAllApplications which connects
>>>>> and disconnects the admin user from all the OFBiz apps.
>>>>>
>>>>> http://issues.apache.org/jira/browse/OFBIZ-3511
>>> Sorry, but not good enough.  We developers need to be able to run such
>>> tests when we change something, not wait until after we have committed it.
>>>
>>> I've been thinking about the ap/ar issue, and using httpunit to test
>>> it(this particular problem doesn't require actually running from a
>>> browser).  I've just been too busy the last few days to implement
>>> anything.
>>>
>>
>> Discussion is king, at some point someone decided that selenium was the way to go and now that is what we have in the project.  If we want something else to go in, lets discuss it before we throw in any more tools.
>
> Ooooh!  I didn't know we had a king.  I thought we were an autonomous
> collective.
>
> If it is in, is there some ant target I can run?

You're fooling yourself.  We're living in a dictatorship.

I have no idea about ant off the top of my head, there is certainly plenty of setup involved and I'm not entirely convinced that we've gone down the best road.

Regards
Scott

[Adam Heath-2]

Jacques Le Roux wrote:

> Done at revision: 918476
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> ---
>>>>>>> ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ModelScreenAction.java
>>>>>>>
>>>>>>>
>>>>>>> (original)
>>>>>>> +++
>>>>>>> ofbiz/trunk/framework/widget/src/org/ofbiz/widget/screen/ModelScreenAction.java
>>>>>>>
>>>>>>>
>>>>>>> Mon Feb 22 16:34:47 2010
>>>>>>> @@ -157,7 +157,7 @@
>>>>>>>                        newValue =
>>>>>>> getInMemoryPersistedFromField(session, context);
>>>>>>>                        if (Debug.verboseOn())
>>>>>>> Debug.logVerbose("In user
>>>>>>> getting value for field from [" + this.fromField.getOriginalName() +
>>>>>>> "]: " + newValue, module);
>>>>>>>                    } else if (!this.valueExdr.isEmpty()) {
>>>>>>> -                    newValue =
>>>>>>> this.valueExdr.expandString(context);
>>>>>>> +                    newValue = this.valueExdr.expand(context);
>>>>>>>                    }
>>>>>>>                } else if (this.fromScope != null&&
>>>>>>> this.fromScope.equals("application")) {
>>>>>>>                    if (!this.fromField.isEmpty()) {
>>>>>>> @@ -165,20 +165,20 @@
>>>>>>>                        newValue =
>>>>>>> getInMemoryPersistedFromField(servletContext, context);
>>>>>>>                        if (Debug.verboseOn()) Debug.logVerbose("In
>>>>>>> application getting value for field from [" +
>>>>>>> this.fromField.getOriginalName() + "]: " + newValue, module);
>>>>>>>                    } else if (!this.valueExdr.isEmpty()) {
>>>>>>> -                    newValue =
>>>>>>> this.valueExdr.expandString(context);
>>>>>>> +                    newValue = this.valueExdr.expand(context);
>>>>>>>                    }
>>>>>>>                } else {
>>>>>>>                    if (!this.fromField.isEmpty()) {
>>>>>>>                        newValue = this.fromField.get(context);
>>>>>>>                        if (Debug.verboseOn()) Debug.logVerbose("In
>>>>>>> screen getting value for field from [" +
>>>>>>> this.fromField.getOriginalName() + "]: " + newValue, module);
>>>>>>>                    } else if (!this.valueExdr.isEmpty()) {
>>>>>>> -                    newValue =
>>>>>>> this.valueExdr.expandString(context);
>>>>>>> +                    newValue = this.valueExdr.expand(context);
>>>>>>>                    }
>>>>>>>                }
>>>>>>>
>>>>>>>                // If newValue is still empty, use the default value
>>>>>>>                if (ObjectType.isEmpty(newValue)&&
>>>>>>> !this.defaultExdr.isEmpty()) {
>>>>>>> -                newValue = this.defaultExdr.expandString(context);
>>>>>>> +                newValue = this.defaultExdr.expand(context);
>>>>>>>                }
>>>>>>>
>>>>>>>                if (UtilValidate.isNotEmpty(this.type)) {

When I originally did this change, it broke ap/ar webapps.  So, I
reverted it, 'cuz I didn't have time to look into it.

Then, when I fixed simpleTypeConvert, ap/ar broke *again*.  This was a
clue, to tell me that the apps themselves were broken, and not the
underlying code.

The exception in both cases was that organizationPartyId wasn't being
passed into a service.  Both this change and the simpleTypeConvert one
ended up changing an empty string to null.  So, what was happening,
was that an empty string was being passed to the service, which was
allowed.  Null was not, so that's why it failed.

This also tells me that whoever wrote these apps(ap/ar) didn't
actually test that organizationPartyId and defaultOrganizationPartyId
weren't being set correctly.

In summary, I'm happy that this change was finally able to go in.