Thank Hans, from now on please revert the commit until you find time to commit a better version.
Regards, Jacopo On Mar 5, 2012, at 12:01 PM, Hans Bakker wrote: > Jacopo, > > in general i honor all comments although it can take some time, also this one > it is comming.... > > Regards, > Hans > > On 03/05/2012 05:57 PM, Jacopo Cappellato wrote: >> 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> >>>> >>>> >
