[ http://jira.magnolia-cms.com/browse/MSHOP-46?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Jan Haderka reopened MSHOP-46: ------------------------------ - {{ProductTeaserModel}} in case of {{RepositoryException}} you will wrap null which will end up w/ NPE sooner or later {code} Node product = null; 90 if (StringUtils.isNotBlank(productUUID)) { 91 try { 92 product = NodeUtil.getNodeByIdentifier("data", productUUID); 93 } catch (RepositoryException e) { 94 log.error("Can't find Product with UUID "+productUUID); 95 } 96 } 97 return new I18nNodeWrapper(product); 98 } {code} - exception handling in {{ShoppingCartItem}} is wrong (in more then just one place quoted below). {code} } catch (RepositoryException e1) { 141 e1.printStackTrace(); 142 } {code} - {{ShoppingCartItem}} - you can't change method signature for public class in minor version like this. Actually more in general many of the API changes introduced in this commit like generics require to make this major rather then minor release. {code} 182 public void setProductPrice(Content productPrice) { public void setProductPrice(Node productPrice) throws ValueFormatException, RepositoryException { {code} - exception handling in {{ShoppingCartParagraphModel}} is non existent. {code} } catch (Exception e) { 99 // TODO 100 } {code} - what are dependencies on asm and cglib necessary for? Since commit comment doesn't explain it, there should be comment in the pom itself explaining what it is for and in the issue as well. - javadoc for {{ShopSingletonParagraphModel}} seems to be wrong. It claims that model is control??? {code} * shoppingcart control. 66 * @author tmiyar 67 * 68 */ 69 public class ShopSingletonParagraphTemplateModel extends STKPageModel<STKPage> { {code} - {{ShopTagCloudParagraph}} - search syntax doesn't look like jcr-sql2 and will most likely fail or return nothing. {code} templatingFunctions.search("data", "select * from category", "JCR_SQL2", "") {code} - {{ShopProductSearchResultParagraphModel}} and {{ShopKeywordSearchResultParagraphModel}} exception handling is not correct. {code} } catch (Exception e) { 72 //TODO 73 } {code} - also keeping around commented out code is unnecessary {code} /*protected String generateSimpleQuery(String searchString) { 79 //escape single quote 80 searchString = searchString.replace("'", "''"); 81 return MessageFormat.format(SEARCH_QUERY_PATTERN, new String[]{ShopUtil.getShopName(), searchString}); 82 }*/ {code} - {{productList.ftl}} before change {{pager}} was not mandatory, now it is. Are you sure this is correct change? It should be explained in the commit comment or at the very least in the issue itself. - {{DefaultShoppingCart}} exception handling is not correct {code} } catch (RepositoryException e) { 136 e.printStackTrace(); 137 } {code} - {{ShopParagraphModel}} exception handling is not correct (in more then just one place quoted below). {code} } catch (RepositoryException e) { 156 // TODO Auto-generated catch block 157 e.printStackTrace(); 158 } {code} General comments - the fact that this issue links to many others and combines all of those changes in one commit makes proper review very complicated and cumbersome. It is not possible to review and assert each of the changes individually - many of the changes here - API changes require major version release. - many of the java code changes require tests - exception handling of the whole module seems to be nearly non existent and needs to be improved. > Migrate code to the latest Magnolia API > --------------------------------------- > > Key: MSHOP-46 > URL: http://jira.magnolia-cms.com/browse/MSHOP-46 > Project: Magnolia Shop > Issue Type: Improvement > Affects Versions: 1.1 > Reporter: Teresa Miyar > Assignee: Teresa Miyar > Fix For: 1.1.1 > > -- This message is automatically generated by JIRA. - If you think it was sent incorrectly contact one of the administrators: http://jira.magnolia-cms.com/secure/Administrators.jspa - For more information on JIRA, see: http://www.atlassian.com/software/jira ---------------------------------------------------------------- For list details, see: http://www.magnolia-cms.com/community/mailing-lists.html Alternatively, use our forums: http://forum.magnolia-cms.com/ To unsubscribe, E-mail to: <dev-list-unsubscr...@magnolia-cms.com> ----------------------------------------------------------------