Re: svn commit: r572172 - in /ofbiz/trunk/framework/common: src/org/ofbiz/common/CommonEvents.java webcommon/includes/listLocales.ftl

[David E Jones-2]

This patch breaks stuff. It shouldn't necessarily be reverted, but the change is incomplete so if the rest of the effort is not finished soon then yes, this patch should be reverted.

The pattern to find this is fairly simple: change something lots of stuff might use and only change one place that uses it, and chances are you have introduced a bug.

In this case checking it was easy. Just search the whole project for "setSessionLocale" and make sure every one that represents a use of it now uses "newLocale" instead of "locale".

Doing this search I found this has NOT been done and there ARE resources referring to the old name.

When reviewing patches like this there are various acceptable things to do:

1. reject the patch describing the problem (more or less as above), and as the contributor to fix it

2. do the review and fix problems and commit it

The alternative of committing without testing and doing some sanity checking is not very helpful for the project in general. Yeah, there is such a thing as CTR (Commit-Then-Review), but that is for group review of concepts and to catch things missed, it is NOT meant to be a replacement for testing and impact review to see if it breaks other stuff.

Thanks for everyone's effort on general contribution review and commit, and thanks to contributors for contributing. Let's all just keep in mind that our lives in general will be a lot easier if we "First Do No Harm" as described on the committers page:

http://docs.ofbiz.org/display/OFBADMIN/OFBiz+Committers+Roles+and+Responsibilities

That page may seem to have a lot of useless crap on it, but please keep in mind that everything I wrote there was in reaction to a real world problem that has come up!

Thanks,
-David


[hidden email] wrote:

> Author: jleroux
> Date: Sun Sep  2 14:46:01 2007
> New Revision: 572172
>
> URL: http://svn.apache.org/viewvc?rev=572172&view=rev
> Log:
> A patch from Adrian Crum for an issue from Bilgin Ibryam"Changing the language in party manager is broken" (https://issues.apache.org/jira/browse/OFBIZ-1223)
>
> Modified:
>     ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java
>     ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl
>
> Modified: ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java?rev=572172&r1=572171&r2=572172&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java (original)
> +++ ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java Sun Sep  2 14:46:01 2007
> @@ -166,7 +166,7 @@
>  
>      /** Simple event to set the users per-session locale setting */
>      public static String setSessionLocale(HttpServletRequest request, HttpServletResponse response) {
> -        String localeString = request.getParameter("locale");
> +        String localeString = request.getParameter("newLocale");
>          if (UtilValidate.isNotEmpty(localeString)) {
>              UtilHttp.setLocale(request, localeString);
>  
> @@ -243,6 +243,7 @@
>          return "success";
>      }
>  }
> +
>  
>  
>  
>
> Modified: ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl
> URL: http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl?rev=572172&r1=572171&r2=572172&view=diff
> ==============================================================================
> --- ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl (original)
> +++ ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl Sun Sep  2 14:46:01 2007
> @@ -50,7 +50,7 @@
>          </#if>
>          <tr <#if altRow>class="alternate-row"</#if>>
>              <td lang="${langAttr}" dir="${langDir}">
> -                <a href="<@ofbizUrl>setSessionLocale</@ofbizUrl>?locale=${availableLocale.toString()}">${availableLocale.getDisplayName(availableLocale)}</a>
> +                <a href="<@ofbizUrl>setSessionLocale</@ofbizUrl>?newLocale=${availableLocale.toString()}">${availableLocale.getDisplayName(availableLocale)}</a>
>              </td>
>          </tr>
>      </#list>
>
>

[Jacques Le Roux]

Thanks for comment David,

I must admit I only tested the use case and dit not thought about all other implications (setSessionLocale is often called as an
event for instance)
I think it's better to revert it and to find another solution : there are too much implications. We could create a new method
setSessionNewLocale to fix the problem Adrian intended to fix and use it in
ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl Unless someone has a better idea ?

I will revert now and reopen the associated Jira issue with a patch with my suggested idea...

Jacques


De : "David E Jones" <[hidden email]>
>
> This patch breaks stuff. It shouldn't necessarily be reverted, but the change is incomplete so if the rest of the effort is not
finished soon then yes, this patch should be reverted.
>
> The pattern to find this is fairly simple: change something lots of stuff might use and only change one place that uses it, and
chances are you have introduced a bug.
>
> In this case checking it was easy. Just search the whole project for "setSessionLocale" and make sure every one that represents a
use of it now uses "newLocale" instead of "locale".

>
> Doing this search I found this has NOT been done and there ARE resources referring to the old name.
>
> When reviewing patches like this there are various acceptable things to do:
>
> 1. reject the patch describing the problem (more or less as above), and as the contributor to fix it
>
> 2. do the review and fix problems and commit it
>
> The alternative of committing without testing and doing some sanity checking is not very helpful for the project in general. Yeah,

there is such a thing as CTR (Commit-Then-Review), but that is for group review of concepts and to catch things missed, it is NOT
meant to be a replacement for testing and impact review to see if it breaks other stuff.
>
> Thanks for everyone's effort on general contribution review and commit, and thanks to contributors for contributing. Let's all
just keep in mind that our lives in general will be a lot easier if we "First Do No Harm" as described on the committers page:
>
> http://docs.ofbiz.org/display/OFBADMIN/OFBiz+Committers+Roles+and+Responsibilities
>
> That page may seem to have a lot of useless crap on it, but please keep in mind that everything I wrote there was in reaction to a
real world problem that has come up!

>
> Thanks,
> -David
>
>
> [hidden email] wrote:
> > Author: jleroux
> > Date: Sun Sep  2 14:46:01 2007
> > New Revision: 572172
> >
> > URL: http://svn.apache.org/viewvc?rev=572172&view=rev
> > Log:
> > A patch from Adrian Crum for an issue from Bilgin Ibryam"Changing the language in party manager is broken"

(https://issues.apache.org/jira/browse/OFBIZ-1223)
> >
> > Modified:
> >     ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java
> >     ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl
> >
> > Modified: ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java
> > URL:
http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java?rev=572172&r1=572171&r2=572172&view=diff

> > ==============================================================================
> > --- ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java (original)
> > +++ ofbiz/trunk/framework/common/src/org/ofbiz/common/CommonEvents.java Sun Sep  2 14:46:01 2007
> > @@ -166,7 +166,7 @@
> >
> >      /** Simple event to set the users per-session locale setting */
> >      public static String setSessionLocale(HttpServletRequest request, HttpServletResponse response) {
> > -        String localeString = request.getParameter("locale");
> > +        String localeString = request.getParameter("newLocale");
> >          if (UtilValidate.isNotEmpty(localeString)) {
> >              UtilHttp.setLocale(request, localeString);
> >
> > @@ -243,6 +243,7 @@
> >          return "success";
> >      }
> >  }
> > +
> >
> >
> >
> >
> > Modified: ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl
> > URL:

http://svn.apache.org/viewvc/ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl?rev=572172&r1=572171&r2=572172&view=diff
> > ==============================================================================
> > --- ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl (original)
> > +++ ofbiz/trunk/framework/common/webcommon/includes/listLocales.ftl Sun Sep  2 14:46:01 2007
> > @@ -50,7 +50,7 @@
> >          </#if>
> >          <tr <#if altRow>class="alternate-row"</#if>>
> >              <td lang="${langAttr}" dir="${langDir}">
> > -                <a
href="<@ofbizUrl>setSessionLocale</@ofbizUrl>?locale=${availableLocale.toString()}">${availableLocale.getDisplayName(availableLocale
)}</a>
> > +                <a
href="<@ofbizUrl>setSessionLocale</@ofbizUrl>?newLocale=${availableLocale.toString()}">${availableLocale.getDisplayName(availableLoc
ale)}</a>
> >              </td>
> >          </tr>
> >      </#list>
> >
> >