Hey Hans, (and all OFBiz committers in general) I would like to stress the importance to address reviews on commits (especially if they come from committers, like me, because we have "veto" on commits) asap by improving the code or reverting it.
Actually the etiquette when a committer complains about a commit and motivates the concerns (as I did) should be: 1) immediately improve the code according to the committer's review 2) or immediately provide additional information if you think that the code is good and that, with the additional details, the reviewer will be satisfied; if the discussion takes longer then the commit should be reverted until the agreement is found 3) or immediately revert it The fact that we use a "commit then review" approach doesn't mean that every committers has the power to commit what he wants ignoring the others... the opposite is actually true: every time we do a commit we should be ready to receive feedback and improve the code; only at that point the code will become "official". I hope this is the last time I have to state this fundamental rule, Jacopo On Feb 29, 2012, at 10:40 AM, Jacopo Cappellato wrote: > Hi Hans, > > I don't like the assumption about the string "control": this is a > configurable value (in web.xml) and we should not have it hardcoded in our > code. > I know there are already few examples of this bad pattern and in fact we > should work and fix them as well. > > Jacopo > > > On Feb 29, 2012, at 10:23 AM, hans...@apache.org wrote: > >> Author: hansbak >> Date: Wed Feb 29 09:23:36 2012 >> New Revision: 1295029 >> >> URL: http://svn.apache.org/viewvc?rev=1295029&view=rev >> Log: >> correct url generation for seo friendly url's >> >> Modified: >> >> ofbiz/trunk/applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java >> ofbiz/trunk/specialpurpose/ecommerce/widget/EmailOrderScreens.xml >> >> Modified: >> ofbiz/trunk/applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java?rev=1295029&r1=1295028&r2=1295029&view=diff >> ============================================================================== >> --- >> ofbiz/trunk/applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java >> (original) >> +++ >> ofbiz/trunk/applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java >> Wed Feb 29 09:23:36 2012 >> @@ -102,6 +102,7 @@ public class OfbizCatalogAltUrlTransform >> String productCategoryId = getStringArg(args, >> "productCategoryId"); >> String productId = getStringArg(args, "productId"); >> String url = ""; >> + String CONTROL_MOUNT_POINT = "control"; >> >> Object prefix = env.getVariable("urlPrefix"); >> String viewSize = getStringArg(args, "viewSize"); >> @@ -127,14 +128,19 @@ public class OfbizCatalogAltUrlTransform >> Delegator delegator = >> FreeMarkerWorker.getWrappedObject("delegator", env); >> LocalDispatcher dispatcher = >> FreeMarkerWorker.getWrappedObject("dispatcher", env); >> Locale locale = (Locale) args.get("locale"); >> + String prefixUrl = prefix.toString(); >> + // remove control path from the prefix URL >> + if(prefixUrl.contains(CONTROL_MOUNT_POINT)){ >> + prefixUrl = >> prefixUrl.replaceAll("/"+CONTROL_MOUNT_POINT, ""); >> + } >> if (UtilValidate.isNotEmpty(productId)) { >> GenericValue product = >> delegator.findOne("Product", UtilMisc.toMap("productId", productId), false); >> ProductContentWrapper wrapper = new >> ProductContentWrapper(dispatcher, product, locale, "text/html"); >> - url = >> CatalogUrlFilter.makeProductUrl(delegator, wrapper, null, ((StringModel) >> prefix).getAsString(), previousCategoryId, productCategoryId, productId); >> + url = >> CatalogUrlFilter.makeProductUrl(delegator, wrapper, null, prefixUrl, >> previousCategoryId, productCategoryId, productId); >> } else { >> GenericValue productCategory = >> delegator.findOne("ProductCategory", UtilMisc.toMap("productCategoryId", >> productCategoryId), false); >> CategoryContentWrapper wrapper = new >> CategoryContentWrapper(dispatcher, productCategory, locale, "text/html"); >> - url = >> CatalogUrlFilter.makeCategoryUrl(delegator, wrapper, null, ((StringModel) >> prefix).getAsString(), previousCategoryId, productCategoryId, productId, >> viewSize, viewIndex, viewSort, searchString); >> + url = >> CatalogUrlFilter.makeCategoryUrl(delegator, wrapper, null, prefixUrl, >> previousCategoryId, productCategoryId, productId, viewSize, viewIndex, >> viewSort, searchString); >> } >> out.write(url.toString()); >> } else { >> >> Modified: ofbiz/trunk/specialpurpose/ecommerce/widget/EmailOrderScreens.xml >> URL: >> http://svn.apache.org/viewvc/ofbiz/trunk/specialpurpose/ecommerce/widget/EmailOrderScreens.xml?rev=1295029&r1=1295028&r2=1295029&view=diff >> ============================================================================== >> --- ofbiz/trunk/specialpurpose/ecommerce/widget/EmailOrderScreens.xml >> (original) >> +++ ofbiz/trunk/specialpurpose/ecommerce/widget/EmailOrderScreens.xml Wed >> Feb 29 09:23:36 2012 >> @@ -85,7 +85,7 @@ under the License. >> <property-map resource="PartyUiLabels" map-name="uiLabelMap" >> global="true"/> >> <property-map resource="CommonUiLabels" map-name="uiLabelMap" >> global="true"/> >> <set field="title" >> value="${uiLabelMap.PageTitleOrderConfirmationNotice}"/> >> - <set field="baseEcommerceSecureUrl" >> value="${baseSecureUrl}/ecommerce"/> >> + <set field="baseEcommerceSecureUrl" >> value="${baseSecureUrl}/ecommerce/control"/> >> <set field="allowAnonymousView" value="Y"/> <!-- this field >> will instruction OrderStatus.groovy to allow an anonymous order to be viewed >> by anybody, so the email confirmation screen will work --> >> <script >> location="component://ecommerce/webapp/ecommerce/WEB-INF/actions/order/OrderStatus.groovy"/> >> </actions> >> @@ -105,7 +105,7 @@ under the License. >> <property-map resource="PartyUiLabels" map-name="uiLabelMap" >> global="true"/> >> <property-map resource="OrderUiLabels" map-name="uiLabelMap" >> global="true"/> >> <set field="title" >> value="${uiLabelMap.PageTitleOrderCompleteNotice}"/> >> - <set field="baseEcommerceSecureUrl" >> value="${baseSecureUrl}/ecommerce/control/"/> >> + <set field="baseEcommerceSecureUrl" >> value="${baseSecureUrl}/ecommerce/control"/> >> <script >> location="component://ecommerce/webapp/ecommerce/WEB-INF/actions/order/OrderStatus.groovy"/> >> </actions> >> <widgets> >> >> >
