qualogy-solutions/qualogy-scraper

🗄️ Closed elsewhere (merged/declined outside this tool) (2)

Fix/scraper diagnostics en documentatie

#2 by Altaaf Doekhie · fix/scraper-diagnostics-en-documentatie → main · Open in Bitbucket →

high

Blast radius

This PR touches nearly every core module of the scraper platform (config, mailer, runner, routers, scrapers, seen_store, webhook, schemas) plus all individual portal scrapers, alongside a large volume of new documentation. It removes the N8N webhook integration entirely, changes mail-sending/dedup logic (seen_store, mailer, aanvraag normalization), alters save/output behavior for failed runs, and modifies concurrency/locking behavior in the runner and API trigger endpoint — any regression here could cause duplicate or missed emails to the Salesforce mailbox, silent data loss, or broken scraper scheduling in production.

Very large, sprawling diff (4016+/440- across 44 files) touching almost every core app module at once
Changes to core business logic: mailer.py, seen_store.py (dedup), aanvraag/normalize.py (data parsing), runner.py (concurrency/locking), webhook.py removal
Modifies save/failure semantics in scrapers (e.g., APG Magnit no longer overwrites output on failed runs) which changes data integrity guarantees
Touches API endpoints (app/routers/api.py, app/routers/dashboard.py) and trigger/locking behavior which affects public-facing behavior and concurrency safety
Removes an integration (N8N webhook) and .env.example entries, which is a behavior/contract change for any downstream consumers
Security-relevant history mentioned in added AUDIT.md (leaked credentials in git history, missing auth on /api/trigger) indicating high sensitivity of the surrounding system even though this PR itself doesn't fix those
Changes span 8+ independent scraper integrations plus shared common/ modules used by all of them, so a subtle bug could cascade across multiple production scrapers

Draft AI review

This PR is primarily documentation (new AUDIT.md, MISLUKTE_RUNS_LOG.md, per-scraper READMEs) plus a couple of small, well-targeted bug fixes (APG Magnit no longer clobbers previous successful output on a failed run; StaffingNow now uses the cross-platform kill_process_tree instead of Windows-only taskkill). The diff is large but low-risk — no correctness or security issues found in the reviewable portions. Note: the diff was truncated before I could see the full contents of several files (app/config.py, app/runner.py, app/mailer.py, app/scrapers.py, common/normalize.py, Stedin/Striive/Flextender scraper changes, tests), so this review only covers what was visible.

APG Magnit/APG Magnit Scraper.py:464 · info

Good fix: previously a failed login would overwrite the previous successful run's output with an empty list; now save_result() refuses to overwrite existing output on failure. Logic in main() looks correct (save_result(False, ...) before the early return, save_result(True, ...) only when jobs is non-empty).

StaffingNow/StaffingNow scraper.py:1071 · info

Replacing the Windows-only `taskkill` call with the shared cross-platform `kill_process_tree()` is a solid fix for the Linux/Azure container case. Note the surrounding `except Exception: pass` still silently swallows failures from kill_process_tree, consistent with prior behavior, so no functional regression.

Feature/aanvraag mail module

#1 by Deborah · feature/aanvraag-mail-module → main · Open in Bitbucket →

high

Blast radius

This PR is a massive, sprawling change (147 files, +8120/-91933 lines) that overhauls the entire application architecture: replacing the old main.py dashboard server with a new FastAPI app, adding a new mailer module that sends live emails to Salesforce, rewriting Docker deployment config (Dockerfile, docker-compose, non-root user, healthcheck, port change from 8765 to 8000), deleting the committed .env with live production credentials, and modifying every scraper module's shared driver/setup code. A regression in the mailer, scheduler, or Docker/deploy config could break scraper execution, mis-deliver or leak sales lead emails, or take down the deployed service entirely.

Very large, sprawling diff touching 147 files across nearly every module in the repo simultaneously
New mailer module (app/mailer.py) sends real emails to a business-critical Salesforce mailbox — a bug could spam, leak, or drop sales leads
Infrastructure/deployment changes: Dockerfile rewritten (non-root user, new CMD/entrypoint, port 8765->8000, healthcheck), docker-compose.yml modified
Secrets handling changed: committed .env with live credentials deleted and replaced by .env.example — risk of breaking deployed configs if not migrated correctly
Architectural replacement of the main runner/dashboard (main.py) with a new FastAPI app (app/main.py, routers, runner, scheduler) — significant behavior change to a running service
Shared refactor of scraper base code (common/scraper_base.py) used by all scraper modules (Magnit, Mercell, Bergler, APG Magnit, etc.), so a bug affects multiple independent scraping pipelines at once
Diff includes binary/output artifacts and downloaded customer documents committed to the repo, indicating messy scope and potential data handling concerns

Draft AI review

Large PR bundling a big architectural refactor (Flask/CLI dashboard → FastAPI app, new common/ shared scraper helpers, mail module, tests) together with committing/removing hundreds of unrelated binary artifacts (scraped output, downloaded .docx/.xlsx/.pdf files, .pyc files, node_modules-style n8n workflow json). Given the diff truncation I could only review a subset in detail (Dockerfile, .env handling, scrapers refactor, README). No blocking correctness bugs found in what was reviewed, but there are real repo-hygiene and security concerns that should be addressed before merge.

.env:1 · critical

This diff deletes the checked-in .env file, but the fact that a .env containing real plaintext credentials (MERCELL_PASSWORD, MAGNIT_PASSWORD, APG_MAGNIT_PASSWORD, STRIIVE_PASSWORD, FLEXTENDER_PASSWORD, STEDIN_PASSWORD, STAFFINGNOW_PASSWORD, BERGLER_PASSWORD, and an n8n webhook URL) was ever committed to the repository means these secrets are permanently present in git history and must be treated as compromised. Rotate every one of these credentials immediately regardless of merging this PR, and consider scrubbing history (BFG/git-filter-repo) if the repo can be rewritten.

Dockerfile:45 · warning

The image now runs as non-root `appuser`, which is good, but `RUN python -c "...ChromeDriverManager().install()"` happens before `USER appuser` is set while `WDM_LOCAL=1`/`ENV` are already set — this caches the driver under `/app/.wdm` while still running as root during build, then `chown -R appuser:appuser /app` afterwards. Verify the ordering actually gives appuser write access to `/app/.wdm` at runtime (e.g. if scrapers ever try to re-download to that path) and that no other root-owned files remain under /app after the chown that the app needs to write to besides `output/`.

Mercell/Mercell Scraper.py:587 · warning

wait_for_downloads_to_finish only considers files already present in `before` as pre-existing; if two tender downloads race (e.g. leftover slow download from a previous tender iteration completes during the current tender's wait window) a document could be misattributed to the wrong tender_id since attribution is purely based on directory diff timing, not filename/content correlation. Given tenders are processed sequentially with a driver.get() reload in between this is a low-probability edge case, but worth a comment/guard given documents are moved (not copied) into tender-specific folders based on this diff alone.

APG Magnit/APG Magnit Scraper.py:66 · info

Env var name for the max-per-run setting was changed from APG_MAGNIT_MAX_OPDRACHTEN_PER_RUN to APG_MAGNIT_MAX_AANVRAGEN_PER_RUN to match .env.example, which is fine, but double check any external deployment configs (Azure app settings, CI secrets) that reference the old variable name are updated too, otherwise the limit silently reverts to unlimited (default 0) in production.