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>
>>>> 
>>>> 
> 

Reply via email to