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

2012-03-07 Thread Hans Bakker 

Jacopo,

Ok I reverted this change and we try to meet your request. The name of 
the 'control' servlet is not available where this code is used. Now we 
have clean code but there is an error in the system (not sure what is 
better)


Do you have a suggestion how we could change this patch so it meets your 
requirements?


Regards,
Hans

On 02/29/2012 04:40 PM, 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=1295029view=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=1295029r1=1295028r2=1295029view=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=1295029r1=1295028r2=1295029view=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

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

2012-03-07 Thread Jacopo Cappellato 
Hans,

please provide more details about the error you are seeing, how to recreate it 
and also how the applications are currently using the urlPrefix env variable 
and me or someone else will help you to find a good way to solve the problem.

Thanks,

Jacopo

On Mar 8, 2012, at 4:28 AM, Hans Bakker wrote:

 Jacopo,
 
 Ok I reverted this change and we try to meet your request. The name of the 
 'control' servlet is not available where this code is used. Now we have clean 
 code but there is an error in the system (not sure what is better)
 
 Do you have a suggestion how we could change this patch so it meets your 
 requirements?
 
 Regards,
 Hans
 
 On 02/29/2012 04:40 PM, 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=1295029view=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=1295029r1=1295028r2=1295029view=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=1295029r1=1295028r2=1295029view=diff

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

2012-03-05 Thread Jacopo Cappellato 
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=1295029view=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=1295029r1=1295028r2=1295029view=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,

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

2012-03-05 Thread 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=1295029view=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=1295029r1=1295028r2=1295029view=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 =

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

2012-03-05 Thread Jacopo Cappellato 
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=1295029view=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=1295029r1=1295028r2=1295029view=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);
 -

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

2012-03-05 Thread Jacques Le Roux 

Maybe then sending back a word saying that it's a WIP would help ;o)

Jacques

From: Hans Bakker mailingl...@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, hans...@apache.org wrote:


Author: hansbak
Date: Wed Feb 29 09:23:36 2012
New Revision: 1295029

URL: http://svn.apache.org/viewvc?rev=1295029view=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=1295029r1=1295028r2=1295029view=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,

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

2012-03-05 Thread Jacopo Cappellato 
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

2012-03-05 Thread Jacques Le Roux 

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 jacopo.cappell...@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

2012-02-29 Thread Jacopo Cappellato 
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=1295029view=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=1295029r1=1295028r2=1295029view=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=1295029r1=1295028r2=1295029view=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