Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java specialpurpose/ecommerce/widget/EmailOrderScreens.xml

[Hans Bakker]

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>