-
Notifications
You must be signed in to change notification settings - Fork 211
[MWPW-185403] Preflight Link check error message incorrectly assumes VPN issue for all connection failures #5259
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
base: LinkCheckerFix
Are you sure you want to change the base?
Conversation
|
This PR is being Merged into |
libs/blocks/preflight/checks/seo.js
Outdated
| } catch (e) { | ||
| window.lana.log(`There was a problem connecting to the link check API ${url}. ${e}`, { tags: 'preflight', errorType: 'i' }); | ||
| return false; | ||
| return { success: false, isVpnError: false }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the logic here is a bit "made up" and has assumptions
"if the request goes through and we get a repsonse = its either a 200 which is great, or 401, 403 status which we assume to be vpn problem"
"if we get no response from the request that we sent = server has issues"
|
This PR has not been updated recently and will be closed in 7 days if no action is taken. Please ensure all checks are passing, https://github.com/orgs/adobecom/discussions/997 provides instructions. If the PR is ready to be merged, please mark it with the "Ready for Stage" label. |
|
Closing this PR due to inactivity. |
|
reopening the PR because it was closed due to incativity during the winter break |
overmyheadandbody
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall, but I do have a couple of small notes:
- the string could be consolidated between the LANA log and the description, since they describe the same issue
- it's likely personal preference, but
connectionError(false)makes me believe there isn't a connection error. A slight method renaming or having some named arguments might make things clearer
When the Spidy link check service fails to connect, the preflight panel always displays "A VPN connection is required to use the link check service. Please turn on VPN and refresh the page." This message is misleading because the connection failure could be caused by reasons other than VPN.
Resolves: MWPW-185403
Test URLs:
Before: https://main--milo--adobecom.aem.page/?martech=off
After: https://VPN-error-handling--milo--adobecom.aem.page/?martech=off
Before: https://main--dc--adobecom.aem.page/acrobat?milolibs=LinkCheckerFix--milo--adobecom
After: https://main--dc--adobecom.aem.page/acrobat?milolibs=VPN-error-handling--milo--adobecom