Skip to content

Conversation

@cjelger
Copy link
Contributor

@cjelger cjelger commented Jan 15, 2020

Description

This PR is a possible solution to fix the editing issue of "specific pages": the idea as detailed by @mhaack in CIF-1045 is to disable the SpecificPageFilter on author (actually except when WCMMode.DISABLED is set) and also have product and category URLs point to the specific pages. This way, on author one will see and be able to edit specific pages and links, while on publish all these "deep links and specific pages" will be hidden.

How Has This Been Tested?

Extended unit tests and manually verified everything on publish.

Screenshots (if appropriate):

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:

  • 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.

- Disable CIF specific page filter on author instance
- Make sure all the product links are properly set on author or publish instances
- Adapted all components and unit tests
- Disable deep links (= hide specific page) on publish instance
@codecov
Copy link

codecov bot commented Jan 15, 2020

Codecov Report

Merging #179 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #179      +/-   ##
============================================
+ Coverage     58.03%   58.04%   +<.01%     
- Complexity      481      482       +1     
============================================
  Files           148      148              
  Lines          4204     4205       +1     
  Branches        723      724       +1     
============================================
+ Hits           2440     2441       +1     
  Misses         1670     1670              
  Partials         94       94
Flag Coverage Δ Complexity Δ
#jest 37.56% <ø> (ø) 0 <ø> (ø) ⬇️
#karma 94.53% <ø> (ø) 0 <ø> (ø) ⬇️
#unittests 82.66% <100%> (+0.01%) 482 <0> (+1) ⬆️
Impacted Files Coverage Δ Complexity Δ
...models/v1/relatedproducts/RelatedProductsImpl.java 85.36% <100%> (+0.36%) 14 <0> (+1) ⬆️

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 d0b722e...d2c26d4. Read the comment docs.

- use Sling HTTP request in SiteNavigation to build product and category URLs
- fix minor code formatting issue
* @param variantSku An optional sku of the variant that will be "selected" on the product page, can be null.
* @return The product page URL.
*/
public String toProductUrl(Page page, String slug, String variantSku) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If SiteNavigation has the request object then all the methods asking for the Page should be refactored to just use the request path from the request object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be too easy, links are not created for and to the current page ;-) For example, on the category page you create links to product pages.

import com.day.cq.wcm.api.PageManager;
import com.day.cq.wcm.api.WCMMode;

public class SiteNavigation {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not 100% related to this research but I think we should have this URL generation an OSGI service. So customers can bring their own implementation to generate PDP / PLP URLs based on their needs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it's a good idea, customers will surely want to customise that.

@herzog31 herzog31 added the enhancement New feature or request label Jan 23, 2020
@herzog31 herzog31 merged commit f792eed into master Jan 23, 2020
@herzog31 herzog31 deleted the CIF-1045 branch January 23, 2020 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request verified

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants