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

Fix external redirect link not shown to author #2882

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ky940819
Copy link
Contributor

@ky940819 ky940819 commented Oct 7, 2024

Q                       A
Fixed Issues? Fixes #2812
Patch: Bug Fix? Yes
Minor: New Feature? No
Major: Breaking Change? No
Tests Added + Pass? Yes
Documentation Provided Yes (code comments)
Any Dependency Changes? No
License Apache License, Version 2.0

Fixes an issue where a page configured as a redirect and having a target of anything other than a Page (e.g. Asset, or external URL) does not display the link target to the author viewing the page in edit mode.

This issue occurred because:

  1. v1/RedirectItemImpl resolved the Page for the redirect target before handing that page to the LinkManager.
  2. LinkManager#get(Page) requires Page to be not null; however it would be passed a null unless the redirect target could be resolved to a Page.
  3. In the event that LinkManager#get(null) is called (because the redirect target does not point to a Page), a Link is returned where the URL for the link is null.

Additionally, even if a valid Link were returned, it would not be presented to the author because of a typo in page/v3/redirect.html:

<a [...]>${redirectTarget.page.title || redirectTarget.linkURL}</a>

The problem here is that redirectTarget.linkURL does not exist because there is no method NavigationItem#getLinkURL().
This is probably a simple typo that should have been redirectTarget.link.URL

Solution
The solution included in this PR resolves the issue by:

  1. Correcting the redirect.html.
  2. Changing v1/RedirectItemImpl to call LinkManager#get(Resource) with a Resource that has the redirect target path as a property, instead of forcing it to be a page and calling LinkManager#get(Page).

This solution allows the link to be anything: Page, Asset, external, etc.
The path/URL will be shown to the author in all cases except where the redirect target is a page, in which case the page title will continue to be shown.

Fixes an issue where a page configured as a redirect and havign a
target of anything other than a Page (e.g. Asset, or external URL)
does not display the link target to the author viewing the page in
edit mode.
Copy link

codecov bot commented Oct 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.18%. Comparing base (a517a37) to head (a0aaedf).

Additional details and impacted files
@@             Coverage Diff              @@
##               main    #2882      +/-   ##
============================================
+ Coverage     87.15%   87.18%   +0.02%     
- Complexity     2692     2697       +5     
============================================
  Files           235      235              
  Lines          7188     7196       +8     
  Branches       1100     1099       -1     
============================================
+ Hits           6265     6274       +9     
  Misses          365      365              
+ Partials        558      557       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Page page = getPageUnderTest(REDIRECT_PAGE_EXTERNAL);
NavigationItem redirectTarget = page.getRedirectTarget();
assertNotNull(redirectTarget);
assertNull(redirectTarget.getPage());

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
NavigationItem.getPage
should be avoided because it has been deprecated.
NavigationItem redirectTarget = page.getRedirectTarget();
assertNotNull(redirectTarget);
assertNull(redirectTarget.getPage());
assertEquals("https://www.adobe.com/", redirectTarget.getURL());

Check notice

Code scanning / CodeQL

Deprecated method or constructor invocation Note test

Invoking
ListItem.getURL
should be avoided because it has been deprecated.
Copy link

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

External redirect link fails to display to page author
2 participants