Skip to content

Add GitHub Actions workflow for monthly MCI fetch and parse - #14

Open
epence12 wants to merge 4 commits into
mcverter:masterfrom
epence12:epence/scheduled-fetch-parse
Open

Add GitHub Actions workflow for monthly MCI fetch and parse#14
epence12 wants to merge 4 commits into
mcverter:masterfrom
epence12:epence/scheduled-fetch-parse

Conversation

@epence12

Copy link
Copy Markdown
Collaborator

Summary

  • Add a GitHub Actions workflow (.github/workflows/fetch-and-parse.yml) that runs fetch_reports.py and parse_reports.py on the 5th of each month, with manual dispatch support.
  • Un-gitignore mci_output.csv and processed_reports.log so the workflow can persist state between runs.
  • Add a pytest gate in the workflow so regressions block the auto-commit step.
  • Update README to reflect current project structure, scheduling, and outstanding TODOs.
  • Include 3 newly fetched reports (Nov 2025, Dec 2025, Jan 2026).

Code Review Findings

A rigorous review of the full codebase surfaced the following. These are not addressed in this PR but are documented here as next steps.

Critical

ID File Lines Issue
C1 parse_reports.py 129, 133 Double-close of PDF — with context manager already closes it, then pdf.close() is called again
C2 parse_reports.py 21 CSV file opened at module import time as a side effect; never flushed between writes; crash loses buffered data
C3 parse_reports.py 200-203 Work line at end-of-page writes and discards entry prematurely — if the entry continues on the next page, continuation work items are lost or duplicated
C4 parse_reports.py 233-241 No CSV value quoting/escaping — commas in street addresses or work item names corrupt the output. Should use Python's csv module
C5 regexes.py 24 street_address_line_regex (\d+.*) matches any line starting with a digit — page numbers, costs, dates all match. Works only because the elif chain checks it first

Moderate

ID File Lines Issue
M1 regexes.py 21 Work item regex [A-Z]\D* rejects digits in names (e.g., "TYPE 2 ELEVATOR" would not match)
M2 regexes.py 17 Close code hard-coded to exactly 2 characters
M3 parse_reports.py multiple Fragile mutable global state (CSV_OUTPUT_FILE, paths, logger) mutated by tests
M5 fetch_reports.py 85-90 Partial download left on disk if connection drops; next run treats it as valid
M6 parse_reports.py 117-119 One corrupt PDF halts processing of all remaining files
M7 fetch-and-parse.yml 36-49 Tests validate the fixture, not the actual output being committed
M9 parse_reports.py 8-9 Bare imports (from regexes import ...) rely on sys.path; potential double-loading when imported by tests vs run as __main__
M10 parse_reports.py 243-244 CSV headers only written in __main__ block, not by process_directory

Minor

  • Dead docstrings placed after imports in parse_reports.py, classes.py, regexes.py
  • Mixed pathlib/os.path usage in parse_reports.py
  • re.match(compiled_pattern, line) instead of idiomatic compiled_pattern.match(line)
  • No return type annotations on several functions
  • Exact version pins in requirements.txt with no Dependabot or update automation
  • Transitive deps (urllib3, certifi, idna) not pinned

Testing Plan

Existing coverage

  • May 2024 fixture regression test
  • Duplicate-skip / manifest behavior test

Needed before merging follow-up work

  • Add September 2025 fixture (referenced in old README but never created)
  • Unit tests for derive_report_month — valid months, uppercase, non-matching filenames, unknown month names
  • Unit tests for fetch_reports.pyslugify, extract_report_links, ReportLink.filename are purely functional and highly testable
  • Test for write_to_csv edge casesNone address, empty work items, commas in field values
  • Test for multi-page entries — verify a PropertyMci spanning a page boundary is correctly accumulated
  • Test for malformed PDF inputpage.extract_text() returns None or empty string
  • Smoke test for zero-PDF directoryprocess_directory with no PDFs should be a no-op

How to validate this PR specifically

  1. Trigger the workflow manually via workflow_dispatch after merging
  2. Confirm it fetches, parses, passes tests, and commits without error
  3. Spot-check a few rows of the committed CSV against the source PDFs
  4. Verify processed_reports.log includes all expected filenames

Next Steps

1. Fix critical parsing bugs (C1-C5)

Address the double-close, premature end-of-page write, CSV escaping, and overly broad street address regex. These are correctness issues that affect output quality today.

2. Robust parsing strategy for image-based PDFs

The current pipeline only handles machine-readable (text-layer) PDFs. FOIL request data from DHCR arrived as scanned image PDFs that pdfplumber cannot extract text from. A robust strategy should handle both:

Proposed approach:

  • Detection step: Before parsing, check if page.extract_text() returns meaningful content. If it returns None or empty/whitespace, flag the page as image-based.
  • OCR fallback: For image-based pages, use an OCR library (e.g., pytesseract + Tesseract, or easyocr) to extract text, then feed the result into the existing regex-based parser.
  • Confidence scoring: OCR output is noisy. Log a confidence score per page and flag low-confidence extractions for manual review rather than silently committing bad data.
  • Preprocessing: Scanned PDFs often benefit from deskewing, binarization, and DPI normalization before OCR. Libraries like pdf2image + Pillow can handle this.
  • Column layout handling: The FOIL PDFs use a multi-column layout (per Mitchell's Slack note). The parser assumes single-column line-by-line reading. OCR output for multi-column pages will need layout analysis -- either via Tesseract's --psm 1 (auto page segmentation) or a dedicated layout detection step.
  • Separate pipeline entry point: Consider a parse_foil_reports.py or a --format flag, since the FOIL files differ in naming, date range coverage, and layout from the monthly published reports.

3. Harden fetch_reports.py

  • Write partial downloads to a temp file, rename on success (atomic download)
  • Validate Content-Type and/or PDF magic bytes before saving

4. Improve test coverage

Address the test gaps listed above, especially multi-page entries and CSV escaping edge cases.

5. Add Dependabot or Renovate

Automate dependency updates, especially for cryptography and requests.


Note: Maxwell has offered to add this to JustFix's k8s cron infrastructure as an alternative/complement to this GH Actions workflow. Both can coexist.

Schedule fetch_reports.py and parse_reports.py to run on the 5th of
each month via cron, with manual dispatch support. Un-gitignore
mci_output.csv and processed_reports.log so the workflow can persist
state between runs. Include 3 newly fetched reports (Nov 2025, Dec
2025, Jan 2026).
Run the test suite after parsing so that regressions in the parser
prevent bad CSV data from being committed automatically.
- Add project structure section listing all source modules
- Replace forward-looking scheduling notes with actual GH Actions workflow
- Fix testing section (remove nonexistent Sept 2025 fixture reference)
- Consolidate remaining TODOs into their own section
@epence12

Copy link
Copy Markdown
Collaborator Author

Spot-check finding: C4 confirmed in production data

While manually vetting the January 2026 report against the CSV output, I found that row 1421 has a work item named SHED, SCAFFOLD — the comma in the name corrupts the CSV row (14 columns instead of 13). Any CSV reader will misparse this row.

This confirms code review finding C4 (no CSV value quoting/escaping). I'm adding a fix in this PR to use Python's csv module for proper quoting.

Use Python's csv module instead of raw f-string concatenation for
CSV output. This properly quotes fields containing commas (e.g.,
"SHED, SCAFFOLD") so rows always have the correct number of columns.

Confirmed via spot-check of January 2026 report: all 6,666 rows
now have exactly 14 fields. Regenerated mci_output.csv from scratch.
@epence12
epence12 force-pushed the epence/scheduled-fetch-parse branch from 2c46399 to 43d9016 Compare February 17, 2026 01:38
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.

1 participant