You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Jacopo Cappellato <ja...@hotwaxmedia.com> on 2012/03/05 11:57:27 UTC
Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java specialpurpose/ecommerce/widget/EmailOrderScreens.xml
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, hansbak@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>
>>
>>
>
Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java specialpurpose/ecommerce/widget/EmailOrderScreens.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
I agree about reverting when someone specially ask about it, moreover it's easy to do, easier/quicker than fixing which can be done
later. So I see no reasons to not do it (anyway Hans just did it :o)
I wanted also to emphasize that often miscommunication is the root of problems. It's just about respecting each other... we can dot
it, I'm sure ...
Jacques
From: "Jacopo Cappellato" <ja...@hotwaxmedia.com>
> Yes,
>
> *reverting* and then sending a word back that an improved version is in progress would also be fine.
>
> Jacopo
>
> On Mar 5, 2012, at 12:10 PM, Jacques Le Roux wrote:
>
>> Maybe then sending back a word saying that it's a WIP would help ;o)
>>
>> Jacques
>
>
Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java specialpurpose/ecommerce/widget/EmailOrderScreens.xml
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
Yes,
*reverting* and then sending a word back that an improved version is in progress would also be fine.
Jacopo
On Mar 5, 2012, at 12:10 PM, Jacques Le Roux wrote:
> Maybe then sending back a word saying that it's a WIP would help ;o)
>
> Jacques
Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java specialpurpose/ecommerce/widget/EmailOrderScreens.xml
Posted by Jacques Le Roux <ja...@les7arts.com>.
Maybe then sending back a word saying that it's a WIP would help ;o)
Jacques
From: "Hans Bakker" <ma...@antwebsystems.com>
> 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, hansbak@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>
>>>>
>>>>
>
Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java specialpurpose/ecommerce/widget/EmailOrderScreens.xml
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
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, hansbak@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>
>>>>
>>>>
>
Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/ofbiz/product/category/OfbizCatalogAltUrlTransform.java
specialpurpose/ecommerce/widget/EmailOrderScreens.xml
Posted by Hans Bakker <ma...@antwebsystems.com>.
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, hansbak@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>
>>>
>>>