Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CIF-1321 - Empty Commerce tab in page properties #256

Merged
merged 6 commits into from
Apr 30, 2020
Merged

CIF-1321 - Empty Commerce tab in page properties #256

merged 6 commits into from
Apr 30, 2020

Conversation

LSantha
Copy link
Collaborator

@LSantha LSantha commented Apr 22, 2020

Added commerce tab in page properties with appropriate render conditions for custom PDPs and PLPs.

Description

Empty commerce tab (no fields only a text "Magento Store Configuration") is displayed in the page properties of storefront pages: "us", "en", "search".

The solution moves the 'Commerce' tab from the archetype to the aem-core-cif-components project and adds render conditions to the tab to be displayed only when it's needed.

Related PR: adobe/aem-cif-project-archetype#103

Related Issue

CIF-1321

How Has This Been Tested?

Manually.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes and the overall coverage did not decrease.
  • All unit tests pass on CircleCi.
  • I ran all tests locally and they pass.

 * added commerce tab in page properties with appropriate render conditions for custom PDPs and PLPs
selectionId="id">
<granite:rendercondition jcr:primaryType="nt:unstructured"
sling:resourceType="core/cif/components/renderconditions/pagetemplate"
templatePath="/conf/atest/settings/wcm/templates/category-page"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is /conf/atest?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we need the templatePath here, which we don't have, should we move the portions or the entire dialog to the archetype?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dplaton , good catch!
@mhaack , I'm looking for an alternative since this dialog would normally belong to this project.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've eliminated the page template from the render conditions.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM now

<granite:rendercondition
jcr:primaryType="nt:unstructured"
sling:resourceType="core/cif/components/renderconditions/pagetemplate"
templatePath="/conf/atest/settings/wcm/templates/product-page"/>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above.

 * added new render condtion to avoid using page templates in render conditions for page properties of custom PDPs and PLPs
 * deprecated old render condition
@codecov
Copy link

codecov bot commented Apr 22, 2020

Codecov Report

Merging #256 into master will increase coverage by 0.04%.
The diff coverage is 68.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #256      +/-   ##
============================================
+ Coverage     62.28%   62.32%   +0.04%     
- Complexity      724      728       +4     
============================================
  Files           171      171              
  Lines          5236     5240       +4     
  Branches        817      820       +3     
============================================
+ Hits           3261     3266       +5     
+ Misses         1864     1862       -2     
- Partials        111      112       +1     
Flag Coverage Δ Complexity Δ
#jest 40.21% <ø> (ø) 0.00 <ø> (ø)
#karma 94.88% <ø> (ø) 0.00 <ø> (ø)
#unittests 84.82% <68.00%> (+0.07%) 728.00 <9.00> (+4.00)
Impacted Files Coverage Δ Complexity Δ
...ernal/servlets/PageTypeRenderConditionServlet.java 68.00% <68.00%> (ø) 9.00 <9.00> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 157b877...ff0c7c0. Read the comment docs.

@@ -34,6 +34,8 @@
/**
* This servlet handles <code>granite:rendercondition</code> requests from the CIF page component,
* in order to decide if the product and category pickers should be displayed in the page properties dialog.
*
* @deprecated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why no remove this Servlet? This is "internal" used, nothing customers directly dependent on. And if you replace it we don't need it. Customers which don't upgrade can still use the older version of the bundle.

Copy link
Collaborator Author

@LSantha LSantha Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In principle this servlet is public API because it's exposed publicly via the resource type core/cif/components/renderconditions/pagetemplate. If a customer is using it somewhere in a similar way as it was used in the archetype then that code will break.
Strictly speaking the new servlet is not a drop-in replacement for this because it's doing different kind of checks and it's not using templates, it's part of a different approach to solve the issue in the page properties dialog.

Nevertheless, if we decide to remove it, I'm also fine with that because the likelihood of somebody using it elsewhere may be quite small. I just thought that deprecation is the safer way.

Copy link
Contributor

@mhaack mhaack Apr 24, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevertheless, if we decide to remove it, I'm also fine with that because the likelihood of somebody using it elsewhere may be quite small. I just thought that deprecation is the safer way.

+1 for remove, the number of customers using this might be overseeable. Most might be even on CIF versions before it was introduced.

@LSantha LSantha added the bug Something isn't working label Apr 27, 2020
@cjelger cjelger merged commit d248b34 into master Apr 30, 2020
@cjelger cjelger deleted the CIF-1321 branch April 30, 2020 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants