Feat/notion#35
Conversation
…il management and UI updates
…s and structure for better user experience
…ode readability with English translations
…pdate navigation links and styles
…tegration; improve database handling and error reporting
… uv package manager and enhance quick start instructions
…erate config.js from template for GitHub Pages
…hance origin handling for improved flexibility
There was a problem hiding this comment.
Pull request overview
This PR migrates scoring persistence/leaderboard from a local SQLAlchemy DB + auth flow to a Notion-backed workflow, while updating the web UI to support anonymous submissions and adding a new “Tools” page for instance generation and solution verification.
Changes:
- Replace DB-backed submissions/auth with synchronous scoring submission and background Notion upsert + Notion-sourced scoreboard.
- Add scoring utilities for ZIP structure validation and expand test coverage for scoring + Notion normalization.
- Rework GitHub Pages frontend: introduce Tools page, new config templating, and simplified submission/scoreboard UI.
Reviewed changes
Copilot reviewed 36 out of 41 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| uv.lock | Adds notion-client to the lockfile. |
| requirements.txt | Adds notion-client and bumps pytest. |
| pyproject.toml | Declares notion-client dependency. |
| tests/test_scoring_utils.py | Unit tests for ZIP structure helpers in scoring utils. |
| tests/test_scoring_route.py | API tests for /scoring/submit payload formatting and validation. |
| tests/test_score_evaluation.py | Tests for process_full_submission success/failure behaviors. |
| tests/test_notion.py | Unit tests for Notion ranking, upsert behavior, and value extraction. |
| tests/test_api.py | Regression tests for scoreboard payload normalization with Notion date objects. |
| tests/conftest.py | Simplifies FastAPI client fixture; removes DB setup fixtures. |
| start.sh | Removes legacy startup script. |
| pages/tools.html | Adds a new frontend Tools page (generator + verifier). |
| pages/submission.html | Replaces auth-based submission UI with email/name + zip upload flow; adds Tools nav. |
| pages/static/js/tools.js | Implements Tools page tab UI and API calls for generator/verifier. |
| pages/static/js/scoreboard.js | Removes auth UI logic; switches to config-driven API URL and /scoreboard path. |
| pages/static/js/config.template.js | Adds template for runtime API URL injection. |
| pages/static/js/config.js | Removes committed config in favor of generated config. |
| pages/static/js/auth.js | Replaces auth flow with anonymous submission logic + local “known submitters” list. |
| pages/static/css/tools.css | Adds styling for the Tools page. |
| pages/static/css/submission.css | Restyles submission page for the new form + result display. |
| pages/scoreboard.html | Adds Tools nav entry and removes auth nav elements. |
| index.html | Updates navigation to point to the new Tools page; removes logout section. |
| generate_config.sh | Adds helper to generate config.js from .env. |
| backup/database/notion.py | Adds Notion integration: querying, extracting values, upsert + ranking. |
| backup/database/models_db.py | Removes SQLAlchemy models. |
| backup/database/models.py | Adds Pydantic models (not used by FastAPI routes in this PR). |
| backup/database/db.py | Removes SQLAlchemy engine/session helpers. |
| backup/core/scoring/utils.py | Adds ZIP structure discovery/validation and standardized failure result helpers. |
| backup/core/scoring/score_evaluation.py | Refactors scoring to return a dict result directly (no DB writes). |
| backup/core/model/feasibility.py | Translates docstrings/messages to English (logic unchanged). |
| backup/core/auth/auth_logic.py | Removes JWT auth logic. |
| backup/app/schemas.py | Removes auth/history schemas; adjusts scoring/leaderboard schema fields. |
| backup/app/routes/scoring.py | Makes scoring submission return results directly and (optionally) upsert to Notion. |
| backup/app/routes/scoreboard.py | Replaces DB leaderboard with Notion-backed leaderboard. |
| backup/app/routes/auth.py | Removes auth endpoints. |
| backup/app/main.py | Removes DB lifecycle logic and auth router; adjusts CORS origins config. |
| README.md | Updates setup/run/test instructions and adds Docker section. |
| Dockerfile | Switches to Python 3.12 + uv-based install and runs uvicorn directly. |
| .gitignore | Ignores temp dir and generated pages/static/js/config.js. |
| .github/workflows/deploy.yml | Adds GitHub Pages deploy workflow that generates config.js from a secret. |
Comments suppressed due to low confidence (1)
README.md:166
- This
.envexample includesSECRET_KEYandDATABASE_URL, but the current codebase no longer references these env vars after the auth/DB removal. Update the example to document only the env vars that are actually consumed (e.g.,NOTION_*,FRONTEND_*).
Create a `.env` file in the root directory:
```bash
# Example .env
SECRET_KEY=your-secret-key-here
DATABASE_URL=sqlite:///./mpvrp_scoring.db
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #!/bin/bash | ||
| source .env |
There was a problem hiding this comment.
This script assumes .env exists and that API_URL is set; source .env will fail (or silently leave API_URL empty) in many environments. Add strict mode (set -euo pipefail) and an explicit check/error when API_URL is unset, so config generation failures are caught early.
| #!/bin/bash | |
| source .env | |
| #!/bin/bash | |
| set -euo pipefail | |
| if [[ ! -f .env ]]; then | |
| echo "Error: .env file not found in the current directory." >&2 | |
| exit 1 | |
| fi | |
| # Load environment variables | |
| source .env | |
| # Ensure API_URL is set and non-empty | |
| : "${API_URL:?Error: API_URL is not set in .env}" |
| for entry in entries: | ||
| props = entry.get("properties", {}) | ||
| score = _extract_value(props.get("Score", {})) | ||
| date_str = _extract_value(props.get("Submission Date", {})) |
There was a problem hiding this comment.
_compute_rankings reads the submission timestamp from the Notion property "Submission Date", but upsert_submission writes "Submission date" (lowercase d) and the scoreboard route also reads "Submission date". This means the tie-breaker by submission date will never be applied in production. Use a single consistent property name (or fall back to both) across _compute_rankings, upsert_submission, and the scoreboard route.
| date_str = _extract_value(props.get("Submission Date", {})) | |
| # Prend en charge à la fois "Submission date" et "Submission Date" pour compatibilité. | |
| raw_date_prop = props.get("Submission date") or props.get("Submission Date") or {} | |
| date_str = _extract_value(raw_date_prop) |
| with zipfile.ZipFile(zip_path, 'r') as zip_ref: | ||
| zip_ref.extractall(extract_path) | ||
| except zipfile.BadZipFile: |
There was a problem hiding this comment.
Using ZipFile.extractall(...) on user-provided ZIPs is vulnerable to Zip Slip (entries with ../ can write outside extract_path). Add safe extraction that validates each member path stays within extract_path (or use a vetted helper) before writing files.
| let authMode = 'login'; | ||
| const API_URL = window.APP_CONFIG?.API_URL; | ||
| const KNOWN_SUBMITTERS_KEY = 'mpvrp_known_submitter_emails'; | ||
|
|
There was a problem hiding this comment.
If pages/static/js/config.js is not generated/loaded, API_URL will be undefined and requests will go to undefined/.... Add an early guard that checks API_URL is set and shows a clear error (or provide a safe default).
| function ensureApiUrlConfigured() { | |
| if (API_URL) { | |
| return; | |
| } | |
| // Log a clear configuration error for debugging | |
| console.error('Configuration error: window.APP_CONFIG.API_URL is not set. Backend API calls are disabled.'); | |
| // Defer DOM interactions until the document is ready | |
| window.addEventListener('DOMContentLoaded', () => { | |
| // Show a clear message in the UI | |
| try { | |
| if (typeof showMessage === 'function') { | |
| showMessage( | |
| 'Configuration error: The service is temporarily unavailable because API_URL is not set. Please contact the site administrator.', | |
| 'error' | |
| ); | |
| } | |
| } catch (e) { | |
| // Swallow any UI errors to avoid breaking the page completely | |
| console.error('Failed to display API_URL configuration error message:', e); | |
| } | |
| // Disable the upload button to prevent requests to an invalid API URL | |
| try { | |
| const uploadBtn = document.getElementById('upload-btn'); | |
| if (uploadBtn) { | |
| uploadBtn.disabled = true; | |
| uploadBtn.innerText = 'Service unavailable'; | |
| } | |
| } catch (e) { | |
| console.error('Failed to disable upload button after API_URL configuration error:', e); | |
| } | |
| }); | |
| } | |
| ensureApiUrlConfigured(); |
|
|
||
| // Remove null values | ||
| Object.keys(formData).forEach(key => formData[key] === null && delete formData[key]); | ||
|
|
There was a problem hiding this comment.
This fetch uses window.APP_CONFIG.API_URL directly; if config.js wasn’t generated/loaded, it will throw and break the tools page. Guard against missing config and display a clear error to the user.
| // Ensure API configuration is available before making the request | |
| if (!window.APP_CONFIG || !window.APP_CONFIG.API_URL) { | |
| statusDiv.className = 'status-message error'; | |
| statusDiv.innerHTML = 'Configuration error: backend API URL is not available. Please ensure config.js is loaded and try reloading this page.'; | |
| btn.disabled = false; | |
| btn.innerHTML = 'Generate'; | |
| return; | |
| } |
| FRONTEND_DEV_URL = os.getenv("FRONTEND_DEV_URL", "") | ||
| FRONTEND_PROD_URL = os.getenv("FRONTEND_PROD_URL", "") | ||
| FRONTEND_PROD_URL_2 = os.getenv("FRONTEND_PROD_URL_2", "") |
There was a problem hiding this comment.
These frontend URLs default to empty strings, which can leave ALLOWED_ORIGINS containing invalid/empty origins. Filter out falsy values before passing them to CORSMiddleware to avoid confusing CORS behavior.
| def test_process_full_submission_returns_failure_for_missing_zip(tmp_path): | ||
| missing_zip = tmp_path / "missing.zip" | ||
|
|
||
| result = score_evaluation.process_full_submission(str(missing_zip)) |
There was a problem hiding this comment.
This test file uses tab characters for indentation (see the leading tabs on these lines). Tabs can trigger IndentationError or fail formatting/linting checks; convert indentation to spaces consistently (typically 4 spaces).
| const API_URL = window.APP_CONFIG?.API_URL; | ||
|
|
There was a problem hiding this comment.
If pages/static/js/config.js is not generated/loaded, API_URL will be undefined and fetch(${API_URL}/scoreboard) will fail. Add a guard + user-facing message when API_URL is missing so the page fails gracefully.
| cat_report["present"] = False | ||
| report["errors"].append( | ||
| f"Dossier '{category}' absent (recherche insensible à la casse) — " | ||
| f"les 50 instances {category} recevront la pénalité maximale." |
There was a problem hiding this comment.
The error message hardcodes "50 instances" even though the module already has NUMBER_OF_INSTANCES_PER_CATEGORY (and tests monkeypatch it). Use the constant in the message to keep behavior and reporting consistent when the number changes.
| f"les 50 instances {category} recevront la pénalité maximale." | |
| f"les {NUMBER_OF_INSTANCES_PER_CATEGORY} instances {category} recevront la pénalité maximale." |
| """Retourne un résultat d'échec standardisé.""" | ||
| return { | ||
| "ok": False, | ||
| "total_weighted_score": BIG_M * 150, |
There was a problem hiding this comment.
_failed_result hardcodes totals for 150 instances (BIG_M * 150) even though the module has NUMBER_OF_INSTANCES_PER_CATEGORY. Consider computing the total instances from the constant (e.g., 3 * NUMBER_OF_INSTANCES_PER_CATEGORY) so the penalty stays consistent if the constant changes.
| "total_weighted_score": BIG_M * 150, | |
| "total_weighted_score": BIG_M * len(COEFFS) * NUMBER_OF_INSTANCES_PER_CATEGORY, |
No description provided.