#663 canonical url fix #1344

rsonth · 2021-01-15T13:43:36Z

Q	A
Fixed Issues?	` Canonical url fix
Patch: Bug Fix?	#663
Minor: New Feature?	Yes
Major: Breaking Change?	No
Tests Added + Pass?	Yes
Documentation Provided	Yes
Any Dependency Changes?	No
License	Apache License, Version 2.0

codecov · 2021-01-15T13:57:07Z

Codecov Report

Merging #1344 (e72cd82) into development (4c8d80e) will decrease coverage by 0.01%.
The diff coverage is 77.77%.

@@                Coverage Diff                @@
##             development    #1344      +/-   ##
=================================================
- Coverage          84.92%   84.90%   -0.02%     
- Complexity          1802     1805       +3     
=================================================
  Files                169      169              
  Lines               5080     5089       +9     
  Branches             801      803       +2     
=================================================
+ Hits                4314     4321       +7     
  Misses               304      304              
- Partials             462      464       +2

Impacted Files	Coverage Δ	Complexity Δ
...m/core/components/internal/models/v2/PageImpl.java	`82.43% <75.00%> (-0.91%)`	`34.00 <2.00> (+2.00)`	⬇️
.../com/adobe/cq/wcm/core/components/models/Page.java	`100.00% <100.00%> (ø)`	`22.00 <1.00> (+1.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 b26a0e7...4c2ef0a. Read the comment docs.

vladbailescu

This seems like a property that should be inherited and not authored on each page. I'm wondering if we shouldn't use a context-aware configuration for storing it. @gabrielwalt , WDYT?

vladbailescu · 2021-01-15T14:46:38Z

content/src/content/jcr_root/apps/core/wcm/components/page/v2/page/tabs/advanced/.content.xml

+                                jcr:primaryType="nt:unstructured"
+                                cq-msm-lockable="sling:alias"/>
+                        </alias>
+                        <canonical


Instead of copy-pasting the entire tab we could just append the field (using ResourceMerger)

This changes has been taken care as suggested using resource merge. Please review the same

@vladbailescu Did you get a chance to review the changes? These changes taken care upon your review comments?

vladbailescu · 2021-01-15T15:08:47Z

bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/models/v2/PageImpl.java

+		if (currentPage != null) {
+			String authoredCanonicalURL = pageProperties.get(CANONICAL_URL, String.class);
+			PageManager pageManager = currentPage.getPageManager();
+			if (pageManager != null && pageManager.getPage(authoredCanonicalURL)!=null) {			


Please add proper spacing on this line and the line bellow

This has been taken care

@vladbailescu Did you get a chance to review the changes? These changes taken care upon your review comments?

vladbailescu · 2021-01-15T16:02:02Z

bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/models/v2/PageImpl.java

@@ -148,6 +150,8 @@
    private String[] clientLibCategoriesJsHead;

    private List<HtmlPageItem> htmlPageItems;
+
+    public static final String CANONICAL_URL = "canonical";


Constants should be defined in the model interface and follow the name convention there.

This has been taken care

@vladbailescu Did you get a chance to review the changes? These changes taken care upon your review comments?

ky940819 · 2021-01-15T17:23:49Z

bundles/core/src/main/java/com/adobe/cq/wcm/core/components/internal/models/v2/PageImpl.java

+		if (currentPage != null) {
+			String authoredCanonicalURL = pageProperties.get(CANONICAL_URL, String.class);
+			PageManager pageManager = currentPage.getPageManager();
+			if (pageManager != null && pageManager.getPage(authoredCanonicalURL)!=null) {			


I'm pretty sure Page#getPageManager() cannot return null,
Also, the canonical page is resolved twice via PageManager#getPage().
Once to assert the page exists and perform the null check, a second time to actually use it to craft the URL.

Although we can be pretty certain that the second call won't return null, there is no guarantee.

An alternative would be something like

@Override public String getCanonicalURL() { if (currentPage != null) { String authoredCanonicalURL = pageProperties.get(CANONICAL_URL, String.class) PageManager pageManager = currentPage.getPageManager(); return Optional.ofNullable(authoredCanonicalURL) .map(pageManager::getPage) .map(p -> Utils.getURL(request, pageManager, p)) .orElse(authoredCanonicalURL); } return null }

Made changes as suggested.

@vladbailescu @ky940819 Did you get a chance to review the changes? These changes taken care upon your review comments?

ky940819 · 2021-01-15T17:24:49Z

...les/core/src/test/java/com/adobe/cq/wcm/core/components/internal/models/v2/PageImplTest.java

@@ -161,4 +161,18 @@ void testHtmlPageItemsConfig() {
        assertNotNull(page.getHtmlPageItems());
        assertEquals(3, page.getHtmlPageItems().size(), "Unexpected number of HTML page items");
    }
+
+    @Test
+    void testgetCanonicalURL_External() {


The not null assertion isn't enough to validate that it is functioning properly.
Need to ensure that the value returned is the value expected.

Required changes have been taken care.

@vladbailescu @ky940819 Did you get a chance to review the changes? These changes taken care upon your review comments?

Development

sonarcloud · 2021-01-20T09:32:06Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
2.3% Duplication

rsonth

@vladbailescu @ky940819 Did you get a chance to review the changes?

adobe#663 canonical url fix

e72cd82

vladbailescu requested changes Jan 15, 2021

View reviewed changes

ky940819 suggested changes Jan 15, 2021

View reviewed changes

rsonth and others added 2 commits January 20, 2021 14:43

Merge pull request #1 from adobe/development

236e536

Development

adobe#663 Canonical review comment changes

4c2ef0a

rsonth requested a review from vladbailescu January 20, 2021 09:49

rsonth commented Jan 27, 2021

View reviewed changes

gabrielwalt added this to the 2.15.0 milestone Feb 3, 2021

vladbailescu modified the milestones: 2.15.0, 2.16.0 Feb 23, 2021

vladbailescu modified the milestones: 2.16.0, Minor Apr 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#663 canonical url fix #1344

#663 canonical url fix #1344

rsonth commented Jan 15, 2021

codecov bot commented Jan 15, 2021 •

edited

vladbailescu left a comment

vladbailescu Jan 15, 2021 •

edited

rsonth Jan 20, 2021

rsonth Jan 27, 2021

vladbailescu Jan 15, 2021

rsonth Jan 20, 2021

rsonth Jan 27, 2021

vladbailescu Jan 15, 2021

rsonth Jan 20, 2021

rsonth Jan 27, 2021

ky940819 Jan 15, 2021

rsonth Jan 20, 2021

rsonth Jan 27, 2021 •

edited

ky940819 Jan 15, 2021

rsonth Jan 20, 2021

rsonth Jan 27, 2021 •

edited

sonarcloud bot commented Jan 20, 2021

rsonth left a comment

#663 canonical url fix #1344

Are you sure you want to change the base?

#663 canonical url fix #1344

Conversation

rsonth commented Jan 15, 2021

codecov bot commented Jan 15, 2021 • edited

Codecov Report

vladbailescu left a comment

Choose a reason for hiding this comment

vladbailescu Jan 15, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsonth Jan 27, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsonth Jan 27, 2021 • edited

Choose a reason for hiding this comment

sonarcloud bot commented Jan 20, 2021

rsonth left a comment

Choose a reason for hiding this comment

codecov bot commented Jan 15, 2021 •

edited

vladbailescu Jan 15, 2021 •

edited

rsonth Jan 27, 2021 •

edited

rsonth Jan 27, 2021 •

edited