[MSHOP-46] Migrate code to the latest Magnolia API Created: 22/Oct/12  Updated: 18/Mar/13  Resolved: 14/Mar/13

Status: Closed
Project: Magnolia Shop (closed)
Component/s: None
Affects Version/s: 1.1
Fix Version/s: 1.1.1

Type: Improvement Priority: Neutral
Reporter: Teresa Miyar Assignee: Roman Kovařík
Resolution: Fixed Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: File patch.diff    
Issue Links:
causality
caused by MSHOP-40 getBreadcrumb is deprecated Closed
relation
is related to MSHOP-31 Prices are not properly formatted Closed
is related to MSHOP-37 Dependency Issue Poms Closed
is related to MSHOP-41 NewBar is deprecated Closed
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Comments   
Comment by Jan Haderka [ 22/Jan/13 ]
  • ProductTeaserModel in case of RepositoryException you will wrap null which will end up w/ NPE sooner or later
                     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  	     }
    
  • exception handling in ShoppingCartItem is wrong (in more then just one place quoted below).
                                 } catch (RepositoryException e1) {
    141	 	                 e1.printStackTrace();
    142	 	             }
    
  • 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.
    182    	     public void setProductPrice(Content productPrice) {	     public void setProductPrice(Node productPrice) throws ValueFormatException, RepositoryException {
    
  • exception handling in ShoppingCartParagraphModel is non existent.
    } catch (Exception e) {
    99	             // TODO
    100	       }
    
  • 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???
     * shoppingcart control.
    66	  * @author tmiyar
    67	  *	
    68	  */	 
    69	 public class ShopSingletonParagraphTemplateModel extends STKPageModel<STKPage> {
    
  • ShopTagCloudParagraph - search syntax doesn't look like jcr-sql2 and will most likely fail or return nothing.
      templatingFunctions.search("data", "select * from category", "JCR_SQL2", "")
    
  • ShopProductSearchResultParagraphModel and ShopKeywordSearchResultParagraphModel exception handling is not correct.
           } catch (Exception e) {
    72	 	             //TODO
    73	 	         }
    
  • also keeping around commented out code is unnecessary
     /*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	 	     }*/
    
  • 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
              } catch (RepositoryException e) {
    136	 	                 e.printStackTrace();
    137	 	             }
    
  • ShopParagraphModel exception handling is not correct (in more then just one place quoted below).
     } catch (RepositoryException e) {
    156	 	             // TODO Auto-generated catch block
    157	 	             e.printStackTrace();
    158	 	         }
    

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.
Comment by Jaroslav Simak [ 11/Mar/13 ]

I have to reopen this issue.

  • nodeIterator = QueryUtil.search("data", "select * from category", "JCR-SQL2", "");
    returns empty iterator because returnItemType is just empty string. Also catalina.out is then full of error messages like these:
    2013-03-11 13:17:56,177 ERROR info.magnolia.jcr.predicate.NodeTypePredicate     : Failed to read type of node node /categorization/Politics
    

    You should specify item type to be nt:base or better category. Then it will return all category nodes.

  • After fixing previous one, freemarker error occures on pages with tag cloud because you return List<Node> from getTagCloud(). However in /shop/paragraphs/extras/tagCloud.ftl template you use item.@name signature which is only available in ContentMap, not in JCR Node.
Comment by Jaroslav Simak [ 12/Mar/13 ]

Patch attached.

Generated at Mon Feb 12 07:08:45 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.