Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/o
Locked
Re: svn commit: r1295029 - in /ofbiz/trunk: applications/product/src/org/o
 
|
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, [hidden email] 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/o
|
|
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, [hidden email] 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/o
|
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, [hidden email] 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/o
|
|
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, [hidden email] 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/o
Administrator
|
In reply to this post by hans_bakker
Maybe then sending back a word saying that it's a WIP would help ;o)
Jacques From: "Hans Bakker" <[hidden email]> > 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, [hidden email] 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/o
|
|
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/o
|
Administrator
|
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" <[hidden email]> > 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/o
|
|
In reply to this post by Jacopo Cappellato-4
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, [hidden email] 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/o
|
|
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, [hidden email] 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/o
|
Administrator
|
I stumbled upon this while working on https://issues.apache.org/jira/browse/OFBIZ-5338
Quoting Jacopo: >>> 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 I suggest to consider, review and hopefully commit https://issues.apache.org/jira/browse/OFBIZ-5312 Jacques Jacopo Cappellato wrote: > 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, [hidden email] 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> |
«
Return to OFBiz - Dev
|
1 view|%1 views
| Free forum by Nabble | Edit this page |