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
[EMAIL PROTECTED] 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>
