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

Bug fix on sitemap adds localFileScan, move report directory up one level #364

Merged
merged 15 commits into from
Jun 19, 2024

Conversation

angyonghaseyo
Copy link
Contributor

@angyonghaseyo angyonghaseyo commented Jun 16, 2024

  • Allow sitemap to be able to recurse all the files (Previously it was just getting the last childSitemap files, instead of all of the files)

  • Allow pdfScanFunc to run both filePath and url (it uses fs for file path and got for url)

  • Prevent sitemap from recursing infinitely

  • Do checking if its a file path or url else skip it, without stopping the scan

  • Added crawlLocalFile (-c 5)

  • Added results and log folder name based on the local file name given

  • Added try catch to all the json parsing for sitemap

  • Fix bug of able to scan files with dot separator within the file name

  • Allow all types of files to be scanned (verapdf for pdf files, axescript for non pdf files)

  • Added typing for crawLocalFile.js

  • Move reports directory to parent directory

  • I've kept this PR as small as possible (~500 lines) by splitting it into PRs with manageable chunks of code

  • I've requested reviews from 1 reviewer

  • I've tested existing features (website scan, sitemap, custom flow) in both node index and cli

  • I've synced this fork with GovTechSG repo

  • I've added/updated unit tests

  • I've added/updated any necessary dependencies in package[-lock].json npm audit, portable installation on GitHub Actions

@younglim younglim self-requested a review June 19, 2024 05:33
@younglim
Copy link
Collaborator

younglim commented Jun 19, 2024

@angyonghaseyo

Local file scan:

Ambiguous help text

npm run cli -- -c 5 -p 100 -u "https://www.tech.gov.sg" -k "..." -a none -h no

Output

 Provided URL or filepath is not a sitemap.

Rename it to Provided URL file path is not a local file or sitemap.

Unhandled exception

npm run cli -- -c 5 -p 100 -u "/Users/young/purple-a11y/results/some_report.html" -k "..." -a none -h no

Output

{"timestamp":"2024-06-19 13:33:18","level":"error","message":"uncaughtException: Invalid URL\nTypeError: Invalid URL\n    at new URL (node:internal/url:797:36)\n    at combineRun (file:///Users/young/purple-a11y/dist/combine.js:89:13)\n    at async scanInit (file:///Users/young/purple-a11y/dist/cli.js:264:5)"}

Copy link
Collaborator

@younglim younglim left a comment

Choose a reason for hiding this comment

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

See comments

Copy link
Collaborator

@younglim younglim left a comment

Choose a reason for hiding this comment

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

Please set file:// as the finalUrl much earlier in the call stack and not check if local file scan superfluously. You need to think about code maintainability / readability

src/combine.ts Outdated Show resolved Hide resolved
src/constants/common.ts Outdated Show resolved Hide resolved
src/constants/common.ts Outdated Show resolved Hide resolved
@younglim younglim reopened this Jun 19, 2024
@younglim younglim changed the title Fix on sitemap and Added localFileScan Bug fix on sitemap adds localFileScan, move report directory up one level Jun 19, 2024
@younglim younglim merged commit 3a7abd1 into master Jun 19, 2024
1 check passed
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