Add functionality to review web pages - #4
Conversation
There was a problem hiding this comment.
Pull request overview
Adds new tooling and reporting to support reviewing WordPress-exported content
and Confluence pages for sensitive terms and publish/archive decisions for the
public-facing e3sm.org site.
Changes:
- Add new
e3sm-comms-exported-xml-reviewerCLI that parses WordPress export XML
and generates multiple review reports (sensitive terms, navigation issues, and
internal link validity). - Expand
e3sm-comms-e3sm-org-reviewerto use WordPress XML exports plus
Confluence-derived inputs to generate Markdown path/terms/action-item reports. - Improve website reviewer output by recording Confluence page created dates in
the sensitive-terms output and making Confluence traversal more fault-tolerant.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Updates command documentation and inputs/outputs for the new workflows. |
| pyproject.toml | Adds a new console script entry point for the exported XML reviewer. |
| examples/review_xml.bash | Adds a runnable example workflow for exported XML review. |
| examples/review_terms.bash | Adds a runnable example workflow for terms/path review. |
| e3sm_comms/website_reviewer/main.py | Adjusts default Confluence top-level input selection. |
| e3sm_comms/page_reviewer/utils_website_reviewer.py | Adds created-date context to sensitive-term output (but currently contains a syntax error). |
| e3sm_comms/page_reviewer/utils_base.py | Adds created_date on pages and introduces get_e3sm_url_status. |
| e3sm_comms/page_reviewer/confluence_page_reviewer.py | Improves resilience and logging during Confluence traversal and newsletter processing. |
| e3sm_comms/exported_xml_reviewer/main.py | Implements the new WordPress-exported XML reviewer and report generation. |
| e3sm_comms/exported_xml_reviewer/init.py | Declares the new package module. |
| e3sm_comms/e3sm_org_reviewer/main.py | Replaces the older reviewer with an XML-export-driven analyzer producing multiple Markdown reports. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
29b6b25 to
42d6888
Compare
|
The latest commit (8512e8c) ports over remaining functionality in Remaining action items:
|
8512e8c to
07bcdb7
Compare
There was a problem hiding this comment.
🟡 Changes recommended
It contains at least one correctness issue in report categorization/counting and a security hardening gap in XML parsing that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
e3sm_comms/utils.py:2
- Using the stdlib xml.etree.ElementTree parser leaves this tool vulnerable to XML entity-expansion / related denial-of-service issues if someone accidentally (or maliciously) supplies a non-WordPress XML file. Since this code already anticipates defusedxml, prefer it when available (with a safe fallback) so the default behavior is hardened.
import re
import xml.etree.ElementTree as ET
- Files reviewed: 17/18 changed files
- Comments generated: 2
- Review effort level: Lite
| if redirect_target and normalize_url(redirect_target) in actual_urls: | ||
| redirected_links.add(linked_norm) | ||
| valid_links.add(linked_norm) | ||
| elif timed_out: |
| except requests.exceptions.HTTPError as e: | ||
| status_code = e.response.status_code if e.response is not None else None | ||
| if status_code == 503 and ( | ||
| e.response.url if e.response is not None else "" | ||
| ).startswith("https://e3sm.org"): | ||
| return "link not whitelisted" | ||
| return "link raises RequestException" | ||
| except requests.exceptions.RequestException: | ||
| return "link raises RequestException" |
forsyth2
left a comment
There was a problem hiding this comment.
I've done a very high-level visual inspection to go along with Copilot's review.
The functionality here was mostly ad-hoc for the website review, so it's not overly important that we have "productized" code (i.e., if we need a different report later, that's fine).
| print(f"Wrote report to {OUTPUT_TERMS_REPORT}") | ||
| print(f"Wrote hierarchical outline to {OUTPUT_HIERARCHICAL_OUTLINE}") | ||
| print(f"Wrote navigation issues report to {OUTPUT_NAVIGATION_ISSUES_REPORT}") | ||
| print( | ||
| f"Wrote invalid internal links report to {OUTPUT_INVALID_INTERNAL_LINKS_REPORT}" | ||
| ) | ||
| print(f"Wrote published pages link report to {OUTPUT_PUBLISHED_PAGES_LINK_REPORT}") | ||
| print(f"Wrote external links report to {OUTPUT_EXTERNAL_LINKS_REPORT}") |
There was a problem hiding this comment.
Consider reordering the reports to be alphabetical or in any case a more coherent order (e.g., internal links should be listed paired with external links)
| echo "1. ${IO_DIR}/output/exported_xml_reviewer/wordpress_sensitive_terms_report.md" | ||
| echo "2. ${IO_DIR}/output/exported_xml_reviewer/wordpress_hierarchical_outline.txt" | ||
| echo "3. ${IO_DIR}/output/exported_xml_reviewer/wordpress_navigation_issues_report.md" | ||
| echo "4. ${IO_DIR}/output/exported_xml_reviewer/wordpress_invalid_internal_links_report.md" | ||
| echo "5. ${IO_DIR}/output/exported_xml_reviewer/wordpress_published_pages_link_report.md" | ||
| echo "6. ${IO_DIR}/output/exported_xml_reviewer/wordpress_invalid_external_links_report.md" | ||
| echo "Optional (pass --check-non-published-access to also get):" |
There was a problem hiding this comment.
Reorder these as well.
| - From WordPress under Tools > Export: xml file of WordPress pages, xml file of WordPress posts. NOTE: Only ever use the XML files downloaded directly from WordPress; the source must be trusted. | ||
| - From output of `e3sm-comms-website-reviewer`: txt file of hierarchical outline of Confluence pages | ||
| - Other: txt file of sensitive terms, txt file of whitelisted e3sm.org pages, txt file of links known to be inaccessible for automated review | ||
| - output: 6 files under `output/exported_xml_reviewer/`: (1) `wordpress_sensitive_terms_report.md`, (2) `wordpress_hierarchical_outline.txt`, (3) `wordpress_navigation_issues_report.md`, (4) `wordpress_invalid_internal_links_report.md`, (5) `wordpress_published_pages_link_report.md`, (6) `wordpress_invalid_external_links_report.md` |
There was a problem hiding this comment.
Reorder these as well
| f.write(f"## 5. Requested links ({len(sorted_requested_link_records)})\n\n") | ||
| if sorted_requested_link_records: | ||
| f.write( | ||
| "| e3sm.org link | Included later on this page? | Current status | Currently whitelisted? | Requesting URLs |\n" |
There was a problem hiding this comment.
Is "Included later on this page?" a holdover from a previous ordering? This is the last section on the report currently.
Add functionality to review web pages for inclusion/exclusion on the public-facing website.