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

Add script to check if all external links are valid #2983

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Nateowami
Copy link
Collaborator

@Nateowami Nateowami commented Jan 28, 2025

The problem

The ExternalUrlService provides links to things outside the application. Essentially it represents the state of some of "the world" outside the app.

  • We have basic unit tests of logic, but they don't verify the state of the world
  • The state of the world outside the app changes even without changes to the app itself

Proposed solution

A script has been added that enumerates all the URLs that could possibly be linked to by the ExternalUrlService. It then:

  • Checks that they return a 200 status
  • If the link is to a specific ID on the page, it verifies that that ID exists on that page

It can also be run against a different copy of the help site, in order to test changes before they go live.


This change is Reviewable

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.04%. Comparing base (fd6844f) to head (1791278).

Files with missing lines Patch % Lines
.../ClientApp/src/xforge-common/external-url-class.ts 87.50% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2983   +/-   ##
=======================================
  Coverage   82.04%   82.04%           
=======================================
  Files         544      545    +1     
  Lines       31697    31699    +2     
  Branches     5155     5149    -6     
=======================================
+ Hits        26005    26007    +2     
- Misses       4925     4937   +12     
+ Partials      767      755   -12     

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

Copy link
Collaborator

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

Excellent work on this!

It's a great way for us to test our external links are working, we may want to consider adding a button/results to the developer diagnostics overlay or a github action that allows us to run this on demand?

It looks like we do not have a Manual in German as I observed failures for both of the QA and Prod environments. Not sure if we should catch that error or add a note so it doesn't alarm us that something is "broken" when it's a known issue.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Nateowami)


scripts/check_external_urls.mts line 15 at r1 (raw file):

//
// Check against a local copy of the help site:
// ./scripts/verify-external-urls.ts http://localhost:8000

A couple of things. the script file is check_external_urls.mts.

I'm guessing this is how you can run the script on a Linux machine. To run from Windows, while in the web-xforge repo, developers will need to run npm i -g deno (if they have not previously done so) and then run deno scripts\check_external_urls.mts {optional param}.

Code quote:

// Check against production:
// ./scripts/verify-external-urls.ts
//
// Check against the Netlify preview build:
// ./scripts/verify-external-urls.ts https://github-action-preview--scriptureforgehelp.netlify.app
//
// Check against a local copy of the help site:
// ./scripts/verify-external-urls.ts http://localhost:8000

scripts/check_external_urls.mts line 25 at r1 (raw file):

  helpUrl = Deno.args[0];
} else if (Deno.args.length > 1) {
  console.error("Usage: verify-external-urls.ts [help URL]");

Change to check_external_urls.mts

Code quote:

verify-external-urls.ts

@kylebuss kylebuss self-assigned this Jan 29, 2025
Copy link
Collaborator Author

@Nateowami Nateowami left a comment

Choose a reason for hiding this comment

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

Sorry I forgot to mention what it shows about German. I was thinking we should add the script, and once we get everything passing, could make it part of automated tests. That said, there's a bit of a danger, because if the help site were wrongly changed, it would cause builds of the app itself to start failing, and vice-versa. Somehow we should have it run as a sanity check whenever we're changing either, and perhaps we should run it after deploying to live. Haven't figured all that out yet.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @kylebuss)


scripts/check_external_urls.mts line 15 at r1 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

A couple of things. the script file is check_external_urls.mts.

I'm guessing this is how you can run the script on a Linux machine. To run from Windows, while in the web-xforge repo, developers will need to run npm i -g deno (if they have not previously done so) and then run deno scripts\check_external_urls.mts {optional param}.

There are a number of ways Deno can be installed: https://docs.deno.com/runtime/getting_started/installation/

[need to go so can't comment on this further]


scripts/check_external_urls.mts line 25 at r1 (raw file):

Previously, kylebuss (Kyle Buss) wrote…

Change to check_external_urls.mts

🤦 Thanks for catching that.

@Nateowami Nateowami force-pushed the feature/simple-external-url-service branch from d2a04ef to 1791278 Compare January 29, 2025 14:59
Copy link
Collaborator

@kylebuss kylebuss left a comment

Choose a reason for hiding this comment

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

:lgtm: Should I write up a wiki page for running deno scripts on Windows?

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

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

Successfully merging this pull request may close these issues.

2 participants