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

feat/addBrokenLinksReporter #1810

Merged
merged 2 commits into from
Feb 22, 2024
Merged

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Feb 22, 2024

Problem

Adding broken link checker to front end

Solution

Adds in the main UI for the broken link checkers. This is not design approved, the alignment with @sehyunidaaa at the moment is that we will use this to experiment how quickly users are able to repair their sites on their own before polishing the UI and removing the feature flag as a whole.

Note: the introduction of the feature flag is done in a downstream pr.

Main user page:

Screen.Recording.2024-02-22.at.12.08.16.PM.mov

Only entry point:

Screen.Recording.2024-02-22.at.12.10.43.PM.mov

Breaking Changes

  • Yes - this PR contains breaking changes
    • Details ...
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Tests

  • Log in to staging via github
  • Access the page /sites/<site-name>/linkCheckerReport for any site here
  • Verify that the content is loading, and you are able to access the page in staging + you are able to go into a valid edit page
  • Verify that when you click on the breadcrumbs on the side, it leads to the correct table (ie what was done in the video above)

Deploy Notes

Need to clone site-link checker into both prod and staging

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@kishore03109 kishore03109 mentioned this pull request Feb 22, 2024
4 tasks
@kishore03109 kishore03109 marked this pull request as ready for review February 22, 2024 04:13
@kishore03109 kishore03109 requested a review from a team February 22, 2024 04:13
siteName
)

const viewableLinkInStaging = stagingUrl + viewablePageInStaging.slice(1) // rm the leading `/`
Copy link
Contributor

Choose a reason for hiding this comment

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

Is stagingUrl always guaranteed to have a trailing /?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hey you are right, caught this in testing + commit is here

tldr this is fixed upstream

Comment on lines +61 to +63

const { data: brokenLinks } = useGetBrokenLinks(siteName)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm by default, does a site have no errors if the error checker isn't run prior? e.g. for new sites

Copy link
Contributor Author

@kishore03109 kishore03109 Feb 22, 2024

Choose a reason for hiding this comment

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

hey you are right on this, there is no auto link checker, since it triggered manually
Not doing this for this experiment since the ones that we are experimenting with are a select, hardcoded list which we can control.
in the future if doesnt exist, be should create!
even if we forget, this pr already has a recovery path as shown here:

Screen.Recording.2024-02-22.at.2.18.37.PM.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alexanderleegs wah you caught me tripping off alarms.

2578d9a
Fixing this issue upstream so that fe dont not ping be in the event of non feature flagged

Comment on lines 34 to 53
const getBreadcrumb = (viewablePageInCms: string): string => {
/**
* There are four main types of pages
* 1. /folders/parentFolder/subfolders/childFolder/editPage/page.md -> parentFolder/childFolder/page
* 2. /folders/parentFolder/editPage/page.md -> parentFolder/page
* 3. /editPage/page.md -> Feedback Form
* 4. /resourceRoom/resourceRmName/resourceCategory/resourceCatName/editPage/page.md -> resourceRmName/resourceCatName/page
*/
const paths = viewablePageInCms.split("/")
let breadcrumb = paths
.filter((_, index) => index % 2 === 0)
.slice(2)
.join(" / ")
.replace(/-/g, " ")
if (breadcrumb.endsWith(".md")) {
breadcrumb = breadcrumb.slice(0, -3)
}

return breadcrumb
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm does the broken link checker also support homepage/navbar/contact us? The breadcrumb wouldn't work properly for those right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

arh shucks you are right, missed these out. 8837e07 as a todo.

navbar is not affected since its a yml, currently the scanner doesnt scan.
bug exists for homepage + contact us, will fix upstream

Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

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

Approving with followups to do since this is feature flagged

Copy link
Contributor Author

kishore03109 commented Feb 22, 2024

Merge activity

@kishore03109 kishore03109 merged commit 1620e1c into develop Feb 22, 2024
9 checks passed
@mergify mergify bot deleted the 02-22-feat/addBrokenLinksReporter branch February 22, 2024 08:03
@alexanderleegs alexanderleegs mentioned this pull request Feb 22, 2024
10 tasks
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.

2 participants