From 1cf15dfb04ae714c630c970c34b430647f35496a Mon Sep 17 00:00:00 2001 From: "Matthew M. Emma" Date: Sun, 9 Aug 2026 10:25:49 -0700 Subject: [PATCH] Harden the four remaining gaps Query.get() -> Session.get(), 22 call sites. Legacy in SQLAlchemy 2.0 and removed in 2.1, with the bound at sqlalchemy<3, so Dependabot would have proposed 2.1 and broken CI on code nobody touched. Third instance of that shape here after db.engine.execute and MPXJ's package rename, so there is now a test that fails on the next one. Deprecation warnings dropped from 188 to 29. One admin blueprint instead of two. blueprints/admin.py at /admin and admin/user_management.py at /management both implemented manage_users, create_user and company_settings against the same templates, and redirected into each other. The duplication hid a live bug: the two create_user copies disagreed about the role field -- UserRole(role) keys on the enum value, UserRole[role] on its name -- and the form sends ADMIN, a name, so POST /admin/users/create raised ValueError on every submission. /management/* now 308s to /admin/* so links and bookmarks survive; 308 rather than 302 because a 302 turns a POST into a GET and drops the form. The 32 mutating routes are exercised. They were the untested half, and the half where authorisation lives. A rival-company admin fixture asserts writes across tenants are refused and the victim's rows are unchanged -- this repository has had that bypass before. Walking them found five routes that answered an empty POST with a 500: /auth/register, /azure/configure, /api/projects/quick-task, /projects//tasks/create and /projects//resources/create all wrote before validating, or read request.json on a body that was not JSON. Each validates first now and answers 400. Registration also let a stranger join an existing tenant. It looked the company up by name and, when found, attached the new user to it as a SCHEDULER -- so anyone who guessed a customer's company name received an account inside it. Registration creates a company; joining one is an invitation an administrator issues. The executive dashboard measures. It generated twelve months of revenue from a 2,500,000 base on a 2% curve, a 12% margin, four hardcoded US regions and per-size bands fixed at 15.2% margin and 95.8% complete -- the same for every company, carrying a `simulated` flag a chart is free to ignore. Revenue and cost now come from transactions and invoices, bucketed by month in Python rather than func.strftime, which is SQLite-only and would have worked in development and failed on PostgreSQL. Geography groups by Project.location, sector by the template a project came from, and both document what to add to extend them. Figures nothing supports are None with a reason, never zero. And realised margin is separated from budget consumed. On unfinished work (budget - spend) / budget is money not yet spent; reporting it as margin made the demo project read 86% margin four months into a two-year job. --- PLAN.md | 13 +- README.md | 17 +- admin/user_management.py | 386 +++------------------- app.py | 7 +- azure_ai/predictive_analytics.py | 4 +- blueprints/admin.py | 415 ++++++++++++++++++++---- blueprints/auth.py | 104 ++++-- blueprints/azure_integration.py | 67 +++- blueprints/project_management.py | 20 +- blueprints/project_templates.py | 21 +- blueprints/projects.py | 74 ++++- blueprints/schedule_api.py | 2 +- collaboration/real_time.py | 3 +- reports/executive_dashboard.py | 445 +++++++++++++++++++++----- services/azure_ai.py | 6 +- services/fabric_service.py | 3 +- services/foundry_service.py | 5 +- services/schedule_analysis.py | 8 +- services/schedule_io.py | 2 +- services/schedule_optimizer.py | 3 +- tasks/background_tasks.py | 9 +- templates/admin/audit_logs.html | 6 +- templates/admin/company_settings.html | 2 +- templates/admin/edit_user.html | 8 +- tests/test_all_routes.py | 10 +- tests/test_executive_dashboard.py | 328 +++++++++++++++++++ tests/test_mutating_routes.py | 386 ++++++++++++++++++++++ tests/test_static_integrity.py | 33 ++ 28 files changed, 1793 insertions(+), 594 deletions(-) create mode 100644 tests/test_executive_dashboard.py create mode 100644 tests/test_mutating_routes.py diff --git a/PLAN.md b/PLAN.md index 1140a3c..b4f7eaf 100644 --- a/PLAN.md +++ b/PLAN.md @@ -283,9 +283,16 @@ not exist. `tests/test_templates.py` now asserts every rendered template resolve 6. **Schedule comparison between baselines.** The snapshots are stored; diffing them — what moved, what was added, what logic changed — is where delay analysis starts, and delay analysis is where the money is in construction. -7. **The remaining facade.** `reports/executive_dashboard.py` generates its revenue trends - and geographic breakdown. Those payloads now carry a `simulated` flag so the UI can - label them, but generated figures in an executive dashboard should be built or removed. +7. **The remaining facade — resolved.** `reports/executive_dashboard.py` now measures. + Revenue and cost come from the transaction ledger and invoices, bucketed by month in + Python rather than with `func.strftime`, which is SQLite-only and would have worked in + development and failed on the PostgreSQL this deploys to. Geography groups by + `Project.location`; sector by the construction template a project was created from. + Every figure is `None` with a reason where nothing supports it. + + The subtler fix was separating realised margin from budget consumed. On unfinished work + `(budget - spend) / budget` is not margin, it is money not yet spent — and reporting it + as margin made the least advanced project look like the most profitable. It is now the only one left: the nine missing pages are built, the eleven analytics helpers are implemented, and `admin/system_status.html` no longer reports a hardcoded "245ms average response time, 127 requests per minute, 0.2% error rate" — it measures diff --git a/README.md b/README.md index d082920..5832425 100644 --- a/README.md +++ b/README.md @@ -5,7 +5,7 @@ [![CI](https://github.com/ibuilder/AIHackScheduler/actions/workflows/ci.yml/badge.svg)](https://github.com/ibuilder/AIHackScheduler/actions/workflows/ci.yml) [![Python 3.11+](https://img.shields.io/badge/python-3.11%2B-blue)](https://www.python.org/) [![License: MIT](https://img.shields.io/badge/license-MIT-green)](LICENSE) -[![Tests](https://img.shields.io/badge/tests-327%20passing-brightgreen)](tests/) +[![Tests](https://img.shields.io/badge/tests-359%20passing-brightgreen)](tests/) Most schedule tools assume the schedule they are given is sound. Most are not. BBSchedule computes the critical path properly, then grades the schedule against the @@ -391,9 +391,18 @@ payment recording and invoice balances, equipment utilisation from logged hours. **Working, thin** — project and task management, Gantt/linear/pull-planning views, auth and roles, transaction ledger. -**Still a facade** — `reports/executive_dashboard.py` generates its revenue trends and -geographic breakdown rather than measuring them. Those payloads now carry a `simulated` -flag so the UI can label them, but they should be built or removed. +**No facades left** — `reports/executive_dashboard.py` measured nothing: twelve months of +revenue generated from a 2,500,000 base on a 2% growth curve, a 12% profit margin, four +hardcoded US regions, and per-size bands where small projects were always 15.2% margin. +Identical for every company. It now aggregates transactions and invoices by month, groups +by the location on each project and the template it came from, and returns `None` with a +stated reason where nothing supports a figure — never zero, because zero profit and unknown +profit are different statements. + +It also separates *realised margin* from *budget consumed*. On unfinished work +`(budget - spend) / budget` is money not yet spent, and reporting it as margin made the +least advanced project look like the most profitable: the demo project read "86% margin" +four months into a two-year job. **Not attempted** — card processing. Payments are recorded, not taken: no key management, no webhook reconciliation, no PCI scope. The README previously advertised "Stripe diff --git a/admin/user_management.py b/admin/user_management.py index 2546214..ad47ce0 100644 --- a/admin/user_management.py +++ b/admin/user_management.py @@ -1,359 +1,65 @@ -import logging -from datetime import datetime, timezone +"""Compatibility shim for the retired ``/management`` admin surface. -from flask import ( - Blueprint, - current_app, - flash, - jsonify, - redirect, - render_template, - request, - url_for, -) -from flask_login import current_user, login_required -from werkzeug.security import generate_password_hash +Administration used to be split across two blueprints — ``admin`` at ``/admin`` +and ``user_management`` at ``/management`` — implementing the same three views +against the same templates, and redirecting into each other. They are merged +into :mod:`blueprints.admin`. -from audit.audit_logger import audit_logger -from extensions import db -from models import AuditLog, Company, Project, User, UserRole +This keeps ``/management/*`` resolving so existing links, bookmarks and any +integration that hardcoded a path continue to work. Every route here is a +permanent redirect to its ``/admin`` equivalent and nothing else; the real +implementations live in one place now. -admin_bp = Blueprint("user_management", __name__) +Delete this module once the redirects stop being hit. +""" +from flask import Blueprint, redirect, url_for -@admin_bp.route("/users") -@login_required -def manage_users(): - """User management dashboard""" - if current_user.role.name not in ["ADMIN"]: - flash("Access denied. Admin privileges required.", "error") - return redirect(url_for("main.dashboard")) +user_management_bp = Blueprint("user_management", __name__) - users = User.query.filter_by(company_id=current_user.company_id).all() - return render_template("admin/users.html", users=users) +# Retired path -> the endpoint that replaced it. Kept as data so +# tests/test_admin.py can assert every entry still resolves. +REDIRECTS = { + "/users": "admin.manage_users", + "/users/create": "admin.create_user", + "/company/settings": "admin.company_settings", + "/audit-logs": "admin.audit_logs", + "/system-status": "admin.system_status", +} +REDIRECTS_WITH_USER = { + "/users//edit": "admin.edit_user", + "/users//deactivate": "admin.deactivate_user", + "/users//activate": "admin.activate_user", +} -@admin_bp.route("/users/create", methods=["GET", "POST"]) -@login_required -def create_user(): - """Create new user""" - if current_user.role.name not in ["ADMIN"]: - flash("Access denied. Admin privileges required.", "error") - return redirect(url_for("main.dashboard")) - if request.method == "POST": - try: - # Validate input - username = request.form.get("username") - email = request.form.get("email") - first_name = request.form.get("first_name") - last_name = request.form.get("last_name") - role = request.form.get("role") - password = request.form.get("password") +def _register(): + """Build one redirect view per retired path. - if not all([username, email, first_name, last_name, role, password]): - flash("All fields are required", "error") - return render_template("admin/create_user.html") - - # Check if user already exists - if User.query.filter_by(username=username).first(): - flash("Username already exists", "error") - return render_template("admin/create_user.html") - - if User.query.filter_by(email=email).first(): - flash("Email already exists", "error") - return render_template("admin/create_user.html") - - # Create user - user = User() - user.username = username - user.email = email - user.first_name = first_name - user.last_name = last_name - user.role = UserRole[role] - user.company_id = current_user.company_id - user.password_hash = generate_password_hash(password) - user.is_active = True - - db.session.add(user) - db.session.commit() - - # Log user creation - audit_logger.log_user_management( - "user_created", user.id, {"created_username": username, "role": role} - ) - - flash(f"User {username} created successfully!", "success") - return redirect(url_for("admin.manage_users")) - - except Exception as e: - db.session.rollback() - logging.error(f"User creation error: {str(e)}") - flash("Error creating user. Please try again.", "error") - - return render_template("admin/create_user.html") - - -@admin_bp.route("/users//edit", methods=["GET", "POST"]) -@login_required -def edit_user(user_id): - """Edit user details""" - if current_user.role.name not in ["ADMIN"]: - flash("Access denied. Admin privileges required.", "error") - return redirect(url_for("main.dashboard")) - - user = User.query.get_or_404(user_id) - - # Can only edit users in same company - if user.company_id != current_user.company_id: - flash("Access denied", "error") - return redirect(url_for("admin.manage_users")) - - if request.method == "POST": - try: - original_data = {"role": user.role.value, "is_active": user.is_active} - - # Update user fields - user.first_name = request.form.get("first_name") - user.last_name = request.form.get("last_name") - user.email = request.form.get("email") - user.role = UserRole[request.form.get("role")] - user.is_active = request.form.get("is_active") == "on" - - # Update password if provided - new_password = request.form.get("password") - if new_password: - user.password_hash = generate_password_hash(new_password) - - db.session.commit() - - # Log user modification - changes = {} - if original_data["role"] != user.role.value: - changes["role_changed"] = f"{original_data['role']} -> {user.role.value}" - if original_data["is_active"] != user.is_active: - changes["status_changed"] = ( - f"{'active' if original_data['is_active'] else 'inactive'} -> {'active' if user.is_active else 'inactive'}" - ) - - audit_logger.log_user_management("user_modified", user.id, changes) - - flash("User updated successfully!", "success") - return redirect(url_for("admin.manage_users")) - - except Exception as e: - db.session.rollback() - logging.error(f"User edit error: {str(e)}") - flash("Error updating user. Please try again.", "error") - - return render_template("admin/edit_user.html", user=user) - - -@admin_bp.route("/users//deactivate", methods=["POST"]) -@login_required -def deactivate_user(user_id): - """Deactivate a user""" - if current_user.role.name not in ["ADMIN"]: - return jsonify({"error": "Access denied"}), 403 - - user = User.query.get_or_404(user_id) - - if user.company_id != current_user.company_id: - return jsonify({"error": "Access denied"}), 403 - - if user.id == current_user.id: - return jsonify({"error": "Cannot deactivate yourself"}), 400 - - try: - user.is_active = False - db.session.commit() - - audit_logger.log_user_management("user_deactivated", user.id) - - return jsonify({"success": True, "message": f"User {user.username} deactivated"}) - - except Exception as e: - db.session.rollback() - logging.error(f"User deactivation error: {str(e)}") - return jsonify({"error": "Failed to deactivate user"}), 500 - - -@admin_bp.route("/users//activate", methods=["POST"]) -@login_required -def activate_user(user_id): - """Activate a user""" - if current_user.role.name not in ["ADMIN"]: - return jsonify({"error": "Access denied"}), 403 - - user = User.query.get_or_404(user_id) - - if user.company_id != current_user.company_id: - return jsonify({"error": "Access denied"}), 403 - - try: - user.is_active = True - db.session.commit() - - audit_logger.log_user_management("user_activated", user.id) - - return jsonify({"success": True, "message": f"User {user.username} activated"}) - - except Exception as e: - db.session.rollback() - logging.error(f"User activation error: {str(e)}") - return jsonify({"error": "Failed to activate user"}), 500 - - -@admin_bp.route("/company/settings", methods=["GET", "POST"]) -@login_required -def company_settings(): - """Manage company settings""" - if current_user.role.name not in ["ADMIN"]: - flash("Access denied. Admin privileges required.", "error") - return redirect(url_for("main.dashboard")) - - company = Company.query.get(current_user.company_id) - - if request.method == "POST": - try: - company.name = request.form.get("name") - company.address = request.form.get("address") - company.phone = request.form.get("phone") - company.email = request.form.get("email") - company.azure_tenant_id = request.form.get("azure_tenant_id") - company.fabric_workspace_id = request.form.get("fabric_workspace_id") - - db.session.commit() - - audit_logger.log_action( - "company_settings_updated", resource_type="company", resource_id=company.id - ) - - flash("Company settings updated successfully!", "success") - - except Exception as e: - db.session.rollback() - logging.error(f"Company settings update error: {str(e)}") - flash("Error updating company settings. Please try again.", "error") - - return render_template("admin/company_settings.html", company=company) - - -@admin_bp.route("/audit-logs") -@login_required -def audit_logs(): - """View audit logs""" - if current_user.role.name not in ["ADMIN"]: - flash("Access denied. Admin privileges required.", "error") - return redirect(url_for("main.dashboard")) - - page = request.args.get("page", 1, type=int) - per_page = 50 - - logs = ( - AuditLog.query.filter_by(company_id=current_user.company_id) - .order_by(AuditLog.timestamp.desc()) - .paginate(page=page, per_page=per_page, error_out=False) - ) - - return render_template("admin/audit_logs.html", logs=logs) - - -@admin_bp.route("/system-status") -@login_required -def system_status(): - """System status and health monitoring""" - if current_user.role.name not in ["ADMIN"]: - flash("Access denied. Admin privileges required.", "error") - return redirect(url_for("main.dashboard")) - - return render_template("admin/system_status.html", status=collect_system_status()) - - -def collect_system_status() -> dict: - """Measure what the platform can actually observe about itself. - - This used to return a literal dict: database "healthy", cache "healthy", - average response time "245ms", 127 requests per minute, error rate "0.2%". - None of it was measured. An administrator opening the page to decide - whether the system was in trouble was reading numbers that never changed, - which is worse than showing nothing. - - Everything below is either measured now or reported as unknown. Request - rate and error rate are deliberately absent rather than invented: nothing - in the application records them, and the place to read them is the - ``/health/metrics`` endpoint that Prometheus scrapes. + 308 rather than 302: a permanent redirect preserves the method and body, so + a POST to a retired path still arrives at the new one as a POST. A 302 + would silently turn it into a GET and lose the form. """ - import os - import time - - import psutil - - from monitoring.health_checks import _database_is_reachable + for path, endpoint in REDIRECTS.items(): - status = {"checked_at": datetime.now(timezone.utc).isoformat()} + def view(_endpoint=endpoint): + return redirect(url_for(_endpoint), code=308) - try: - status["database"] = { - "status": "healthy", - "response_time_ms": _database_is_reachable(), - } - except Exception as exc: - logging.error("System status: database unreachable: %s", exc, exc_info=True) - status["database"] = {"status": "unhealthy"} + view.__name__ = f"redirect_{endpoint.split('.')[-1]}" + user_management_bp.add_url_rule(path, view_func=view, methods=["GET", "POST"]) - # The cache is configured at startup; report the backend actually in use - # rather than asserting health of something that may be a no-op. - try: - cache_type = current_app.config.get("CACHE_TYPE", "unknown") - status["cache"] = { - "status": "healthy" if cache_type else "not_configured", - "backend": str(cache_type), - } - except Exception as exc: - logging.error("System status: cache check failed: %s", exc, exc_info=True) - status["cache"] = {"status": "unknown"} + for path, endpoint in REDIRECTS_WITH_USER.items(): - # Background jobs need a broker. Without one, Celery is not running, and - # saying so is more useful than a green tick. - broker = os.environ.get("CELERY_BROKER_URL") or os.environ.get("REDIS_URL") - status["background_jobs"] = { - "status": "configured" if broker else "not_configured", - "broker": "redis" if broker else None, - } + def user_view(user_id, _endpoint=endpoint): + return redirect(url_for(_endpoint, user_id=user_id), code=308) - # An integration is configured when its credentials are present. This is - # the same test services/optional.py applies before enabling a feature. - status["integrations"] = { - "azure_ai": "configured" - if os.environ.get("AZURE_OPENAI_ENDPOINT") and os.environ.get("AZURE_OPENAI_KEY") - else "not_configured", - "fabric": "configured" if os.environ.get("AZURE_FABRIC_ENDPOINT") else "not_configured", - "power_bi": "configured" - if all( - os.environ.get(name) - for name in ("POWERBI_CLIENT_ID", "POWERBI_CLIENT_SECRET", "POWERBI_TENANT_ID") - ) - else "not_configured", - "stripe": "configured" if os.environ.get("STRIPE_SECRET_KEY") else "not_configured", - } + user_view.__name__ = f"redirect_{endpoint.split('.')[-1]}" + user_management_bp.add_url_rule(path, view_func=user_view, methods=["GET", "POST"]) - try: - process = psutil.Process() - status["process"] = { - "pid": process.pid, - "uptime_seconds": round(time.time() - process.create_time()), - "memory_mb": round(process.memory_info().rss / (1024 * 1024), 1), - "cpu_percent": psutil.cpu_percent(interval=None), - "system_memory_percent": psutil.virtual_memory().percent, - } - except Exception as exc: - logging.error("System status: process metrics failed: %s", exc, exc_info=True) - status["process"] = {} - status["records"] = { - "users": User.query.filter_by(company_id=current_user.company_id).count(), - "projects": Project.query.filter_by(company_id=current_user.company_id).count(), - } +_register() - return status +# The old module exported `admin_bp`; keep the name importable so nothing that +# still refers to it breaks on import. +admin_bp = user_management_bp diff --git a/app.py b/app.py index 38d6ac1..0137597 100644 --- a/app.py +++ b/app.py @@ -159,10 +159,11 @@ def create_app(config_class=None): def load_user(user_id): from models import User - return User.query.get(int(user_id)) + return db.session.get(User, int(user_id)) # Register blueprints - from admin.user_management import admin_bp as user_mgmt_bp + # Retired /management surface: redirects only. See admin/user_management.py. + from admin.user_management import user_management_bp from analytics.advanced_analytics import analytics_bp from azure_ai.predictive_analytics import azure_ai_bp from blueprints.admin import admin_bp @@ -189,7 +190,7 @@ def load_user(user_id): app.register_blueprint(reports_bp, url_prefix="/reports") app.register_blueprint(admin_bp, url_prefix="/admin") app.register_blueprint(analytics_bp, url_prefix="/api/analytics") - app.register_blueprint(user_mgmt_bp, url_prefix="/management") + app.register_blueprint(user_management_bp, url_prefix="/management") app.register_blueprint(project_templates_bp, url_prefix="/project-templates") app.register_blueprint(collaboration_bp, url_prefix="/collaboration") app.register_blueprint(executive_bp, url_prefix="/") diff --git a/azure_ai/predictive_analytics.py b/azure_ai/predictive_analytics.py index 65c91ac..37942d5 100644 --- a/azure_ai/predictive_analytics.py +++ b/azure_ai/predictive_analytics.py @@ -929,7 +929,9 @@ def _calculate_cost_impact(self, suggestions: list[dict[str, Any]]) -> dict[str, if not excess: continue resource = ( - Resource.query.get(suggestion["resource_id"]) if suggestion["resource_id"] else None + db.session.get(Resource, suggestion["resource_id"]) + if suggestion["resource_id"] + else None ) if resource and resource.unit_cost: priced += excess * resource.unit_cost diff --git a/blueprints/admin.py b/blueprints/admin.py index fcb9456..27dea20 100644 --- a/blueprints/admin.py +++ b/blueprints/admin.py @@ -1,29 +1,73 @@ -from flask import Blueprint, flash, jsonify, redirect, render_template, request, url_for +"""Administration: users, company settings, integrations, audit and status. + +This was two blueprints. ``blueprints/admin.py`` served ``/admin`` and +``admin/user_management.py`` served ``/management``, and both implemented +``manage_users``, ``create_user`` and ``company_settings`` against the same +templates. Views in one redirected to endpoints in the other, so a user editing +their company details could be bounced between two different user lists. + +The duplication was not merely untidy. The two ``create_user`` implementations +disagreed about how to read the role field: one did ``UserRole(role)``, keyed +on the enum *value*, the other ``UserRole[role]``, keyed on its *name*. The form +sends ``ADMIN``, which is a name, so ``POST /admin/users/create`` raised +``ValueError: 'ADMIN' is not a valid UserRole`` on every submission while the +``/management`` copy worked. Nothing caught it because no test had ever posted +to either. + +One blueprint now, at ``/admin``. ``/management/*`` still resolves — it +redirects, so existing links and bookmarks survive. +""" + +import logging +from datetime import datetime, timezone +from functools import wraps + +from flask import ( + Blueprint, + current_app, + flash, + jsonify, + redirect, + render_template, + request, + url_for, +) from flask_login import current_user, login_required from werkzeug.security import generate_password_hash +from audit.audit_logger import audit_logger from extensions import db -from models import AzureIntegration, Company, Project, User, UserRole +from models import AuditLog, AzureIntegration, Company, Project, User, UserRole admin_bp = Blueprint("admin", __name__) def admin_required(f): + """Refuse anyone who is not an administrator of their own company. + + ``functools.wraps`` rather than assigning ``__name__`` by hand: Flask keys + endpoints on the function name, so the hand-rolled version worked, but it + dropped the docstring and module and would silently collide if two wrapped + views ever shared a name. + """ + + @wraps(f) def decorated_function(*args, **kwargs): if not current_user.is_authenticated or current_user.role != UserRole.ADMIN: flash("Access denied. Administrator privileges required.", "error") return redirect(url_for("main.dashboard")) return f(*args, **kwargs) - decorated_function.__name__ = f.__name__ return decorated_function +# ── overview ───────────────────────────────────────────────────────────── + + @admin_bp.route("/dashboard") @login_required @admin_required def dashboard(): - # Get company statistics total_users = User.query.filter_by(company_id=current_user.company_id).count() active_users = User.query.filter_by(company_id=current_user.company_id, is_active=True).count() @@ -32,14 +76,12 @@ def dashboard(): company_id=current_user.company_id, status="active" ).count() - # Get recent activities recent_projects = ( Project.query.filter_by(company_id=current_user.company_id) .order_by(Project.created_at.desc()) .limit(5) .all() ) - recent_users = ( User.query.filter_by(company_id=current_user.company_id) .order_by(User.created_at.desc()) @@ -58,6 +100,9 @@ def dashboard(): ) +# ── users ──────────────────────────────────────────────────────────────── + + @admin_bp.route("/users") @login_required @admin_required @@ -71,86 +116,328 @@ def manage_users(): @admin_required def create_user(): if request.method == "POST": - username = request.form.get("username") - email = request.form.get("email") - password = request.form.get("password") - first_name = request.form.get("first_name") - last_name = request.form.get("last_name") - role = request.form.get("role") - - # Check if user already exists - if User.query.filter_by(username=username).first(): - flash("Username already exists", "error") - return render_template("admin/create_user.html") - - if User.query.filter_by(email=email).first(): - flash("Email already registered", "error") - return render_template("admin/create_user.html") - - # Create user - user = User( - username=username, - email=email, - password_hash=generate_password_hash(password), - first_name=first_name, - last_name=last_name, - company_id=current_user.company_id, - role=UserRole(role), - ) - - db.session.add(user) - db.session.commit() - - flash("User created successfully!", "success") - return redirect(url_for("admin.manage_users")) + try: + username = request.form.get("username") + email = request.form.get("email") + first_name = request.form.get("first_name") + last_name = request.form.get("last_name") + role = request.form.get("role") + password = request.form.get("password") + + if not all([username, email, first_name, last_name, role, password]): + flash("All fields are required", "error") + return render_template("admin/create_user.html") + + if User.query.filter_by(username=username).first(): + flash("Username already exists", "error") + return render_template("admin/create_user.html") + + if User.query.filter_by(email=email).first(): + flash("Email already exists", "error") + return render_template("admin/create_user.html") + + if role not in UserRole.__members__: + flash("Unknown role", "error") + return render_template("admin/create_user.html") + + user = User() + user.username = username + user.email = email + user.first_name = first_name + user.last_name = last_name + # Keyed on the enum NAME, because templates/admin/create_user.html + # sends "ADMIN". UserRole(role) keys on the value ("admin") and + # raises ValueError for every option the form offers. + user.role = UserRole[role] + user.company_id = current_user.company_id + user.password_hash = generate_password_hash(password) + user.is_active = True + + db.session.add(user) + db.session.commit() + + audit_logger.log_user_management( + "user_created", user.id, {"created_username": username, "role": role} + ) + + flash(f"User {username} created successfully!", "success") + return redirect(url_for("admin.manage_users")) + + except Exception as e: + db.session.rollback() + logging.error("User creation error: %s", e, exc_info=True) + flash("Error creating user. Please try again.", "error") return render_template("admin/create_user.html") -@admin_bp.route("/integrations") +@admin_bp.route("/users//edit", methods=["GET", "POST"]) @login_required @admin_required -def manage_integrations(): - integrations = ( - AzureIntegration.query.join(Project) - .filter(Project.company_id == current_user.company_id) - .all() - ) +def edit_user(user_id): + user = db.session.get(User, user_id) + if user is None: + flash("User not found", "error") + return redirect(url_for("admin.manage_users")) - return render_template("admin/integrations.html", integrations=integrations) + # Tenant isolation: an administrator administers their own company. + if user.company_id != current_user.company_id: + flash("Access denied", "error") + return redirect(url_for("admin.manage_users")) + + if request.method == "POST": + try: + original = {"role": user.role.value, "is_active": user.is_active} + + role = request.form.get("role") + if role not in UserRole.__members__: + flash("Unknown role", "error") + return render_template("admin/edit_user.html", user=user) + + user.first_name = request.form.get("first_name") + user.last_name = request.form.get("last_name") + user.email = request.form.get("email") + user.role = UserRole[role] + user.is_active = request.form.get("is_active") == "on" + + new_password = request.form.get("password") + if new_password: + user.password_hash = generate_password_hash(new_password) + + db.session.commit() + + changes = {} + if original["role"] != user.role.value: + changes["role_changed"] = f"{original['role']} -> {user.role.value}" + if original["is_active"] != user.is_active: + changes["status_changed"] = ( + f"{'active' if original['is_active'] else 'inactive'} -> " + f"{'active' if user.is_active else 'inactive'}" + ) + audit_logger.log_user_management("user_modified", user.id, changes) + + flash("User updated successfully!", "success") + return redirect(url_for("admin.manage_users")) + + except Exception as e: + db.session.rollback() + logging.error("User edit error: %s", e, exc_info=True) + flash("Error updating user. Please try again.", "error") + + return render_template("admin/edit_user.html", user=user) + + +def _set_active(user_id: int, active: bool, action: str, message: str): + """Shared body for activate and deactivate.""" + user = db.session.get(User, user_id) + if user is None or user.company_id != current_user.company_id: + flash("Access denied", "error") + return redirect(url_for("admin.manage_users")) + if not active and user.id == current_user.id: + # Locking yourself out of the only admin account is unrecoverable + # through the interface. + flash("You cannot deactivate your own account.", "error") + return redirect(url_for("admin.edit_user", user_id=user_id)) -@admin_bp.route("/company/settings", methods=["GET", "POST"]) + try: + user.is_active = active + db.session.commit() + audit_logger.log_user_management(action, user.id, {"username": user.username}) + flash(message, "success") + except Exception as e: + db.session.rollback() + logging.error("User %s error: %s", action, e, exc_info=True) + flash("Error updating the account. Please try again.", "error") + + return redirect(url_for("admin.manage_users")) + + +@admin_bp.route("/users//deactivate", methods=["POST"]) @login_required @admin_required -def company_settings(): - company = Company.query.get(current_user.company_id) +def deactivate_user(user_id): + return _set_active(user_id, False, "user_deactivated", "User deactivated.") - if request.method == "POST": - company.name = request.form.get("name") - company.address = request.form.get("address") - company.phone = request.form.get("phone") - company.email = request.form.get("email") - company.azure_tenant_id = request.form.get("azure_tenant_id") - company.fabric_workspace_id = request.form.get("fabric_workspace_id") - db.session.commit() - flash("Company settings updated successfully!", "success") - return redirect(url_for("admin.company_settings")) - - return render_template("admin/company_settings.html", company=company) +@admin_bp.route("/users//activate", methods=["POST"]) +@login_required +@admin_required +def activate_user(user_id): + return _set_active(user_id, True, "user_activated", "User reactivated.") @admin_bp.route("/api/users//toggle-status", methods=["POST"]) @login_required @admin_required def toggle_user_status(user_id): - user = User.query.get_or_404(user_id) - + user = db.session.get(User, user_id) + if user is None: + return jsonify({"error": "Not found"}), 404 if user.company_id != current_user.company_id: return jsonify({"error": "Access denied"}), 403 + if user.id == current_user.id and user.is_active: + return jsonify({"error": "You cannot deactivate your own account"}), 400 user.is_active = not user.is_active db.session.commit() - + audit_logger.log_user_management( + "user_activated" if user.is_active else "user_deactivated", + user.id, + {"username": user.username}, + ) return jsonify({"success": True, "user_id": user_id, "is_active": user.is_active}) + + +# ── company ────────────────────────────────────────────────────────────── + + +@admin_bp.route("/company/settings", methods=["GET", "POST"]) +@login_required +@admin_required +def company_settings(): + company = db.session.get(Company, current_user.company_id) + + if request.method == "POST" and company is not None: + try: + company.name = request.form.get("name") + company.address = request.form.get("address") + company.phone = request.form.get("phone") + company.email = request.form.get("email") + company.azure_tenant_id = request.form.get("azure_tenant_id") + company.fabric_workspace_id = request.form.get("fabric_workspace_id") + + db.session.commit() + audit_logger.log_action( + "company_settings_updated", resource_type="company", resource_id=company.id + ) + flash("Company settings updated successfully!", "success") + return redirect(url_for("admin.company_settings")) + + except Exception as e: + db.session.rollback() + logging.error("Company settings update error: %s", e, exc_info=True) + flash("Error updating company settings. Please try again.", "error") + + return render_template("admin/company_settings.html", company=company) + + +@admin_bp.route("/integrations") +@login_required +@admin_required +def manage_integrations(): + integrations = ( + AzureIntegration.query.join(Project) + .filter(Project.company_id == current_user.company_id) + .all() + ) + return render_template("admin/integrations.html", integrations=integrations) + + +# ── audit and status ───────────────────────────────────────────────────── + + +@admin_bp.route("/audit-logs") +@login_required +@admin_required +def audit_logs(): + page = request.args.get("page", 1, type=int) + logs = ( + AuditLog.query.filter_by(company_id=current_user.company_id) + .order_by(AuditLog.timestamp.desc()) + .paginate(page=page, per_page=50, error_out=False) + ) + return render_template("admin/audit_logs.html", logs=logs) + + +@admin_bp.route("/system-status") +@login_required +@admin_required +def system_status(): + return render_template("admin/system_status.html", status=collect_system_status()) + + +def collect_system_status() -> dict: + """Measure what the platform can actually observe about itself. + + This used to return a literal dict: database "healthy", cache "healthy", + average response time "245ms", 127 requests per minute, error rate "0.2%". + None of it was measured. An administrator opening the page to decide + whether the system was in trouble was reading numbers that never changed, + which is worse than showing nothing. + + Everything below is either measured now or reported as unknown. Request + rate and error rate are deliberately absent rather than invented: nothing + in the application records them, and the place to read them is the + ``/health/metrics`` endpoint that Prometheus scrapes. + """ + import os + import time + + import psutil + + from monitoring.health_checks import _database_is_reachable + + status = {"checked_at": datetime.now(timezone.utc).isoformat()} + + try: + status["database"] = {"status": "healthy", "response_time_ms": _database_is_reachable()} + except Exception as exc: + logging.error("System status: database unreachable: %s", exc, exc_info=True) + status["database"] = {"status": "unhealthy"} + + # The cache is configured at startup; report the backend actually in use + # rather than asserting health of something that may be a no-op. + try: + cache_type = current_app.config.get("CACHE_TYPE", "unknown") + status["cache"] = { + "status": "healthy" if cache_type else "not_configured", + "backend": str(cache_type), + } + except Exception as exc: + logging.error("System status: cache check failed: %s", exc, exc_info=True) + status["cache"] = {"status": "unknown"} + + # Background jobs need a broker. Without one, Celery is not running, and + # saying so is more useful than a green tick. + broker = os.environ.get("CELERY_BROKER_URL") or os.environ.get("REDIS_URL") + status["background_jobs"] = { + "status": "configured" if broker else "not_configured", + "broker": "redis" if broker else None, + } + + # An integration is configured when its credentials are present. This is + # the same test services/optional.py applies before enabling a feature. + status["integrations"] = { + "azure_ai": "configured" + if os.environ.get("AZURE_OPENAI_ENDPOINT") and os.environ.get("AZURE_OPENAI_KEY") + else "not_configured", + "fabric": "configured" if os.environ.get("AZURE_FABRIC_ENDPOINT") else "not_configured", + "power_bi": "configured" + if all( + os.environ.get(name) + for name in ("POWERBI_CLIENT_ID", "POWERBI_CLIENT_SECRET", "POWERBI_TENANT_ID") + ) + else "not_configured", + "stripe": "configured" if os.environ.get("STRIPE_SECRET_KEY") else "not_configured", + } + + try: + process = psutil.Process() + status["process"] = { + "pid": process.pid, + "uptime_seconds": round(time.time() - process.create_time()), + "memory_mb": round(process.memory_info().rss / (1024 * 1024), 1), + "cpu_percent": psutil.cpu_percent(interval=None), + "system_memory_percent": psutil.virtual_memory().percent, + } + except Exception as exc: + logging.error("System status: process metrics failed: %s", exc, exc_info=True) + status["process"] = {} + + status["records"] = { + "users": User.query.filter_by(company_id=current_user.company_id).count(), + "projects": Project.query.filter_by(company_id=current_user.company_id).count(), + } + + return status diff --git a/blueprints/auth.py b/blueprints/auth.py index 3f024e2..727ddc5 100644 --- a/blueprints/auth.py +++ b/blueprints/auth.py @@ -1,4 +1,4 @@ -from flask import Blueprint, flash, redirect, render_template, request, url_for +from flask import Blueprint, current_app, flash, redirect, render_template, request, url_for from flask_login import current_user, login_required, login_user, logout_user from werkzeug.security import check_password_hash, generate_password_hash @@ -46,15 +46,54 @@ def logout(): @auth_bp.route("/register", methods=["GET", "POST"]) def register(): + """Self-registration, which creates a new company. + + Two things were wrong here, both reachable without a session. + + The company was created and flushed before any field was validated, so an + empty POST inserted ``Company(name=None)`` and raised IntegrityError — a + 500 on an unauthenticated endpoint. A request carrying a company name but + no password got as far as creating the company and then bailed on the + password check, leaving an orphan company behind. + + More seriously, a registration naming an *existing* company joined it:: + + company = Company.query.filter_by(name=company_name).first() + ... + user.role = PROJECT_MANAGER if not company.users else SCHEDULER + + So anyone who guessed a customer's company name received a scheduler + account inside that tenant, and with it every project the tenant owns. + Joining an existing company is an invitation, issued by an administrator of + that company from the users page — never something a stranger asserts about + themselves. + """ if request.method == "POST": - username = request.form.get("username") - email = request.form.get("email") - password = request.form.get("password") - first_name = request.form.get("first_name") - last_name = request.form.get("last_name") - company_name = request.form.get("company_name") + username = (request.form.get("username") or "").strip() + email = (request.form.get("email") or "").strip() + password = request.form.get("password") or "" + first_name = (request.form.get("first_name") or "").strip() + last_name = (request.form.get("last_name") or "").strip() + company_name = (request.form.get("company_name") or "").strip() + + # Validate everything before writing anything. + required = { + "username": username, + "email": email, + "password": password, + "first name": first_name, + "last name": last_name, + "company name": company_name, + } + missing = [name for name, value in required.items() if not value] + if missing: + flash(f"Please provide: {', '.join(missing)}.", "error") + return render_template("auth/register.html") + + if len(password) < 8: + flash("Password must be at least 8 characters.", "error") + return render_template("auth/register.html") - # Check if user already exists if User.query.filter_by(username=username).first(): flash("Username already exists", "error") return render_template("auth/register.html") @@ -63,31 +102,40 @@ def register(): flash("Email already registered", "error") return render_template("auth/register.html") - # Create or get company - company = Company.query.filter_by(name=company_name).first() - if not company: + # Registration creates a company. It never joins one. + if Company.query.filter_by(name=company_name).first(): + flash( + "An organisation with that name is already registered. Ask one of its " + "administrators to create an account for you.", + "error", + ) + return render_template("auth/register.html") + + try: company = Company() company.name = company_name db.session.add(company) - db.session.flush() # Get company ID - - # Create user - if not password: - flash("Password is required", "error") + db.session.flush() + + user = User() + user.username = username + user.email = email + user.password_hash = generate_password_hash(password) + user.first_name = first_name + user.last_name = last_name + user.company_id = company.id + # Sole member of a brand new company, so an administrator of it. + user.role = UserRole.ADMIN + user.is_active = True + + db.session.add(user) + db.session.commit() + except Exception: + db.session.rollback() + current_app.logger.exception("Registration failed") + flash("Registration could not be completed. Please try again.", "error") return render_template("auth/register.html") - user = User() - user.username = username - user.email = email - user.password_hash = generate_password_hash(password) - user.first_name = first_name - user.last_name = last_name - user.company_id = company.id - user.role = UserRole.PROJECT_MANAGER if not company.users else UserRole.SCHEDULER - - db.session.add(user) - db.session.commit() - flash("Registration successful! Please log in.", "success") return redirect(url_for("auth.login")) diff --git a/blueprints/azure_integration.py b/blueprints/azure_integration.py index ecce8ef..600f51a 100644 --- a/blueprints/azure_integration.py +++ b/blueprints/azure_integration.py @@ -1,6 +1,15 @@ import json -from flask import Blueprint, flash, jsonify, redirect, render_template, request, url_for +from flask import ( + Blueprint, + current_app, + flash, + jsonify, + redirect, + render_template, + request, + url_for, +) from flask_login import current_user, login_required from extensions import db @@ -9,6 +18,8 @@ from services.fabric_service import FabricService from services.foundry_service import FoundryService +SERVICE_TYPES = {"ai", "fabric", "foundry"} + azure_bp = Blueprint("azure", __name__) @@ -121,22 +132,46 @@ def configure_integration(project_id): return redirect(url_for("projects.list_projects")) if request.method == "POST": - service_type = request.form.get("service_type") + # Validate before writing. service_type is NOT NULL, so a POST without + # it inserted AzureIntegration(service_type=None) and raised + # IntegrityError -- a 500, and a poisoned session for whatever ran next + # in the same request. `configuration` went straight into json.loads, + # so any malformed value raised JSONDecodeError the same way. + service_type = (request.form.get("service_type") or "").strip() + if service_type not in SERVICE_TYPES: + flash(f"Choose a service: {', '.join(sorted(SERVICE_TYPES))}.", "error") + return redirect(url_for("azure.configure_integration", project_id=project_id)) + + raw_configuration = request.form.get("configuration") or "{}" + try: + configuration = json.loads(raw_configuration) + except ValueError: + flash("Configuration must be valid JSON.", "error") + return redirect(url_for("azure.configure_integration", project_id=project_id)) + if not isinstance(configuration, dict): + flash("Configuration must be a JSON object.", "error") + return redirect(url_for("azure.configure_integration", project_id=project_id)) + + try: + integration = AzureIntegration.query.filter_by( + project_id=project_id, service_type=service_type + ).first() + + if not integration: + integration = AzureIntegration(project_id=project_id, service_type=service_type) + db.session.add(integration) + + integration.endpoint_url = request.form.get("endpoint_url") + integration.workspace_id = request.form.get("workspace_id") + integration.configuration = configuration + + db.session.commit() + except Exception: + db.session.rollback() + current_app.logger.exception("Azure integration configuration failed") + flash("Could not save the integration. Please try again.", "error") + return redirect(url_for("azure.configure_integration", project_id=project_id)) - # Update or create integration - integration = AzureIntegration.query.filter_by( - project_id=project_id, service_type=service_type - ).first() - - if not integration: - integration = AzureIntegration(project_id=project_id, service_type=service_type) - db.session.add(integration) - - integration.endpoint_url = request.form.get("endpoint_url") - integration.workspace_id = request.form.get("workspace_id") - integration.configuration = json.loads(request.form.get("configuration", "{}")) - - db.session.commit() flash("Integration configured successfully", "success") return redirect(url_for("azure.dashboard")) diff --git a/blueprints/project_management.py b/blueprints/project_management.py index b2d7c07..550fc88 100644 --- a/blueprints/project_management.py +++ b/blueprints/project_management.py @@ -71,14 +71,26 @@ def create_project(): @login_required def quick_add_task(): """Quick add task via AJAX""" + # `request.json` raises when the body is not JSON, and the bare except + # below turned that into a 500 -- so a client sending the wrong + # content-type got a server error for what is a client mistake. + payload = request.get_json(silent=True) + if not isinstance(payload, dict): + return jsonify({"error": "A JSON object body is required"}), 400 + try: - project_id = request.json.get("project_id") - task_name = request.json.get("name") - duration = request.json.get("duration", 1) + project_id = payload.get("project_id") + task_name = payload.get("name") + duration = payload.get("duration", 1) if not all([project_id, task_name]): return jsonify({"error": "Project ID and task name required"}), 400 + try: + duration = int(duration) + except (TypeError, ValueError): + return jsonify({"error": "Duration must be a whole number of days"}), 400 + # Verify project access project = Project.query.get_or_404(project_id) if project.company_id != current_user.company_id: @@ -88,7 +100,7 @@ def quick_add_task(): task = Task() task.name = task_name task.project_id = project_id - task.duration = int(duration) + task.duration = duration task.start_date = date.today() task.end_date = date.today() # Will be calculated properly later task.status = TaskStatus.NOT_STARTED diff --git a/blueprints/project_templates.py b/blueprints/project_templates.py index 78b6ae2..b3aefae 100644 --- a/blueprints/project_templates.py +++ b/blueprints/project_templates.py @@ -188,11 +188,22 @@ def api_estimate_project(template_id): try: template = ConstructionProjectTemplates.get_template(template_id) - # Get estimation parameters - data = request.get_json() - crew_size_factor = data.get("crew_size_factor", 1.0) - complexity_factor = data.get("complexity_factor", 1.0) - weather_factor = data.get("weather_factor", 1.0) + # request.get_json() returns None for a non-JSON body, and the bare + # except below reported that as a 500 -- a server error for a client + # mistake. A crew size of zero divides by zero for the same reason. + data = request.get_json(silent=True) + if not isinstance(data, dict): + return jsonify({"error": "A JSON object body is required", "success": False}), 400 + + try: + crew_size_factor = float(data.get("crew_size_factor", 1.0)) + complexity_factor = float(data.get("complexity_factor", 1.0)) + weather_factor = float(data.get("weather_factor", 1.0)) + except (TypeError, ValueError): + return jsonify({"error": "Factors must be numbers", "success": False}), 400 + + if crew_size_factor <= 0: + return jsonify({"error": "Crew size factor must be positive", "success": False}), 400 # Calculate adjusted timeline base_duration = sum(task.get("duration", 0) for task in template.get("tasks", [])) diff --git a/blueprints/projects.py b/blueprints/projects.py index 436e951..ae9820b 100644 --- a/blueprints/projects.py +++ b/blueprints/projects.py @@ -9,6 +9,17 @@ projects_bp = Blueprint("projects", __name__) +def _json_body() -> dict: + """The request body as a dict, or an empty one. + + ``request.json`` raises on a body that is not JSON, which surfaced as a 500 + rather than a 400. Returning an empty dict lets the field checks below + report what is actually missing. + """ + payload = request.get_json(silent=True) + return payload if isinstance(payload, dict) else {} + + @projects_bp.route("/") @login_required def list_projects(): @@ -120,18 +131,39 @@ def create_task(project_id): if current_user.company_id != project.company_id: return jsonify({"error": "Access denied"}), 403 + # Validate before constructing. strptime(None) raises TypeError, which + # escaped as a 500 -- a POST with no body crashed the server rather than + # being told what it was missing. + body = _json_body() + name = (body.get("name") or "").strip() + if not name: + return jsonify({"error": "name is required"}), 400 + + dates = {} + for field in ("start_date", "end_date"): + raw = body.get(field) + if not raw: + return jsonify({"error": f"{field} is required (YYYY-MM-DD)"}), 400 + try: + dates[field] = datetime.strptime(raw, "%Y-%m-%d").date() + except (TypeError, ValueError): + return jsonify({"error": f"{field} must be YYYY-MM-DD"}), 400 + + if dates["end_date"] < dates["start_date"]: + return jsonify({"error": "end_date cannot precede start_date"}), 400 + task = Task( - name=request.json.get("name"), - description=request.json.get("description"), + name=name, + description=body.get("description"), project_id=project_id, - start_date=datetime.strptime(request.json.get("start_date"), "%Y-%m-%d").date(), - end_date=datetime.strptime(request.json.get("end_date"), "%Y-%m-%d").date(), - duration=request.json.get("duration", 1), - priority=request.json.get("priority", "medium"), - location=request.json.get("location"), - station_start=request.json.get("station_start"), - station_end=request.json.get("station_end"), - pull_plan_week=request.json.get("pull_plan_week"), + start_date=dates["start_date"], + end_date=dates["end_date"], + duration=body.get("duration", 1), + priority=body.get("priority", "medium"), + location=body.get("location"), + station_start=body.get("station_start"), + station_end=body.get("station_end"), + pull_plan_week=body.get("pull_plan_week"), ) db.session.add(task) @@ -156,15 +188,23 @@ def create_resource(project_id): if current_user.company_id != project.company_id: return jsonify({"error": "Access denied"}), 403 + # name and type are NOT NULL, so an empty body raised IntegrityError and + # left the session needing a rollback for whatever ran next. + body = _json_body() + name = (body.get("name") or "").strip() + kind = (body.get("type") or "").strip() + if not name or not kind: + return jsonify({"error": "name and type are required"}), 400 + resource = Resource( - name=request.json.get("name"), - type=request.json.get("type"), + name=name, + type=kind, project_id=project_id, - unit=request.json.get("unit"), - unit_cost=request.json.get("unit_cost"), - total_quantity=request.json.get("total_quantity"), - available_quantity=request.json.get("available_quantity"), - location=request.json.get("location"), + unit=body.get("unit"), + unit_cost=body.get("unit_cost"), + total_quantity=body.get("total_quantity"), + available_quantity=body.get("available_quantity"), + location=body.get("location"), ) db.session.add(resource) diff --git a/blueprints/schedule_api.py b/blueprints/schedule_api.py index ee8aada..389a77b 100644 --- a/blueprints/schedule_api.py +++ b/blueprints/schedule_api.py @@ -34,7 +34,7 @@ def _authorised_project(project_id): """Fetch a project scoped to the caller's company, or return an error.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: return None, (jsonify({"error": "Project not found"}), 404) if project.company_id != current_user.company_id: diff --git a/collaboration/real_time.py b/collaboration/real_time.py index bcd805c..9921a8b 100644 --- a/collaboration/real_time.py +++ b/collaboration/real_time.py @@ -9,6 +9,7 @@ from flask_login import current_user, login_required from audit.audit_logger import audit_logger +from extensions import db from models import Project, User collaboration_bp = Blueprint("collaboration", __name__) @@ -83,7 +84,7 @@ def get_project_activities(self, project_id): # Format activities for display formatted_activities = [] for activity in activities[-10:]: # Get last 10 - user = User.query.get(activity["user_id"]) + user = db.session.get(User, activity["user_id"]) username = user.username if user else "Unknown User" if activity["type"] == "message": diff --git a/reports/executive_dashboard.py b/reports/executive_dashboard.py index 7b44af5..6452001 100644 --- a/reports/executive_dashboard.py +++ b/reports/executive_dashboard.py @@ -3,12 +3,14 @@ High-level analytics and KPIs for executive decision making """ -from datetime import date, datetime, timedelta +from datetime import date, timedelta from flask import Blueprint, jsonify, render_template, request from flask_login import current_user, login_required +from sqlalchemy import func -from models import Equipment, Project, User +from extensions import db +from models import Equipment, Invoice, Project, Transaction, TransactionType, User executive_bp = Blueprint("executive", __name__) @@ -40,16 +42,19 @@ def get_company_overview(self, company_id, date_range_days=30): sum(p.budget for p in projects if p.budget and p.status == "completed") or 0 ) - # Calculate profit margins (simulated data) - profit_margin = 0.12 # 12% average profit margin - estimated_profit = total_contract_value * profit_margin + # Margin from the ledger, or nothing. This was `profit_margin = 0.12` + # and `budget_variance = 0.05` — two constants presented to an executive + # as measurements. Both are computed now, and both are None until there + # are transactions to compute them from. + ledger = self._ledger_totals(company_id) + profit_margin = ledger["margin_percent"] + estimated_profit = ledger["profit"] + budget_variance = self._budget_variance(company_id, projects) # Resource utilization total_users = User.query.filter_by(company_id=company_id, is_active=True).count() - # Risk indicators overdue_projects = 0 - budget_variance = 0.05 # 5% over budget average for project in projects: if project.end_date and project.end_date < end_date and project.status != "completed": @@ -68,9 +73,21 @@ def get_company_overview(self, company_id, date_range_days=30): "financial": { "total_contract_value": total_contract_value, "completed_value": completed_value, + # None where the ledger holds nothing to measure. A caller + # should render "no data yet", not a zero that reads as a fact. + "recorded_income": ledger["income"], + "recorded_expense": ledger["expense"], "estimated_profit": estimated_profit, - "profit_margin": profit_margin * 100, - "budget_variance": budget_variance * 100, + "profit_margin": profit_margin, + "budget_variance": budget_variance, + "measured": ledger["measured"], + "note": ( + None + if ledger["measured"] + else "No income or expense transactions recorded, so margin cannot be " + "measured. Record transactions against projects and these populate " + "with no further change." + ), }, "resources": { "total_staff": total_users, @@ -88,6 +105,230 @@ def get_company_overview(self, company_id, date_range_days=30): }, } + # Construction templates carry the sector in their id, so a project created + # from one already records what it is. Projects created directly do not. + TEMPLATE_SECTORS = { + "commercial_office": "Commercial", + "retail_center": "Commercial", + "residential_complex": "Residential", + "industrial_warehouse": "Industrial", + "infrastructure_road": "Infrastructure", + "hospital_medical": "Healthcare", + } + + @classmethod + def _by_sector(cls, projects: list, spend: dict[int, float]) -> list[dict]: + """Portfolio by sector, inferred from the template a project came from. + + This was four hardcoded rows — Commercial 15 projects, $18.5M, 11.2% + margin — identical for every company and unmoved by anything anyone did. + + There is no sector column, so the sector is read from ``template_used``, + which a project created from a construction template already records. + Anything else is reported as Unspecified rather than guessed at. + + **Extension path:** add a ``sector`` column to ``Project``, prefer it + here and fall back to the template mapping. Nothing else in this method + changes, and the payload shape stays the same for the UI. + """ + grouped: dict[str, dict] = {} + for project in projects: + sector = cls.TEMPLATE_SECTORS.get(project.template_used or "", "Unspecified") + entry = grouped.setdefault( + sector, + {"sector": sector, "projects": 0, "contract_value": 0.0, "recorded_spend": 0.0}, + ) + entry["projects"] += 1 + entry["contract_value"] += float(project.budget or 0) + entry["recorded_spend"] += spend.get(project.id, 0.0) + + for entry in grouped.values(): + value = entry["contract_value"] + cost = entry["recorded_spend"] + entry["contract_value"] = round(value, 2) + entry["recorded_spend"] = round(cost, 2) + # Deliberately budget consumed, not margin. Margin on unfinished + # work is money not yet spent, and calling it margin makes an + # untouched project look like the most profitable in the book. + entry["budget_consumed_percent"] = ( + round(cost / value * 100, 1) if value and cost else None + ) + + return sorted(grouped.values(), key=lambda e: -e["contract_value"]) + + @staticmethod + def _spend_by_project(company_id) -> dict[int, float]: + """Recorded expense per project, for margin where a budget exists.""" + return { + project_id: float(total or 0) + for project_id, total in db.session.query( + Transaction.project_id, func.sum(Transaction.amount) + ) + .filter( + Transaction.company_id == company_id, + Transaction.transaction_type == TransactionType.EXPENSE, + ) + .group_by(Transaction.project_id) + .all() + } + + @staticmethod + def _band_performance(members: list, spend: dict[int, float]) -> dict: + """Completion rate, mean duration and margin for one size band. + + Every figure is None rather than zero when nothing supports it. A band + with no projects has no completion rate; saying 0% would read as a + portfolio in trouble rather than a portfolio without data. + """ + if not members: + return { + "count": 0, + "avg_margin": None, + "completion_rate": None, + "avg_duration": None, + "measured": False, + } + + completed = [p for p in members if p.status == "completed"] + + durations = [ + (p.end_date - p.start_date).days / 30.44 for p in members if p.start_date and p.end_date + ] + + # Margin is only meaningful once a project is finished. On work still in + # flight, (budget - spend) / budget is budget *remaining*, and reporting + # that as margin flatters a project that has simply not spent its money + # yet — the demo project reads 86% "margin" four months into a two-year + # job. The two are separated here. + finished = [p for p in completed if p.budget and p.id in spend] + finished_budget = sum(p.budget for p in finished) + finished_cost = sum(spend[p.id] for p in finished) + + in_flight = [p for p in members if p.status != "completed" and p.budget and p.id in spend] + in_flight_budget = sum(p.budget for p in in_flight) + in_flight_cost = sum(spend[p.id] for p in in_flight) + + return { + "count": len(members), + "completion_rate": round(len(completed) / len(members) * 100, 1), + "avg_duration": round(sum(durations) / len(durations), 1) if durations else None, + # Realised margin, on finished work only. + "avg_margin": ( + round((finished_budget - finished_cost) / finished_budget * 100, 1) + if finished_budget + else None + ), + "margin_basis": ( + f"{len(finished)} completed project(s) with a budget and recorded spend" + if finished + else "No completed project in this band has both a budget and recorded " + "spend, so realised margin cannot be measured" + ), + # How much of the approved budget in-flight work has consumed. + "budget_consumed_percent": ( + round(in_flight_cost / in_flight_budget * 100, 1) if in_flight_budget else None + ), + "measured": True, + } + + @staticmethod + def _by_location(projects: list, spend: dict[int, float]) -> list[dict]: + """Portfolio grouped by the location recorded on each project. + + **Extension path:** this reads ``Project.location`` as free text, so a + deployment gets a breakdown the moment locations are filled in. To + group by region rather than by exact string, add a ``region`` column to + ``Project`` and change the key below — the rest of the shape holds. + """ + grouped: dict[str, dict] = {} + for project in projects: + key = (project.location or "").strip() or "Unspecified" + entry = grouped.setdefault( + key, {"region": key, "projects": 0, "contract_value": 0.0, "recorded_spend": 0.0} + ) + entry["projects"] += 1 + entry["contract_value"] += float(project.budget or 0) + entry["recorded_spend"] += spend.get(project.id, 0.0) + + for entry in grouped.values(): + entry["contract_value"] = round(entry["contract_value"], 2) + entry["recorded_spend"] = round(entry["recorded_spend"], 2) + + return sorted(grouped.values(), key=lambda e: -e["contract_value"]) + + @staticmethod + def _ledger_totals(company_id) -> dict: + """Income, expense and margin from recorded transactions. + + Returns ``measured: False`` and ``None`` figures when the ledger is + empty, rather than zeros. Zero profit and unknown profit are different + statements, and an executive dashboard must not confuse them. + """ + totals = dict( + db.session.query(Transaction.transaction_type, func.sum(Transaction.amount)) + .filter(Transaction.company_id == company_id) + .group_by(Transaction.transaction_type) + .all() + ) + + income = float(totals.get(TransactionType.INCOME) or 0) + expense = float(totals.get(TransactionType.EXPENSE) or 0) + + # Invoices are revenue too, and a deployment may raise invoices without + # posting income transactions. Take the larger of the two rather than + # double counting. + invoiced = float( + db.session.query(func.coalesce(func.sum(Invoice.total_amount), 0)) + .filter(Invoice.company_id == company_id) + .scalar() + or 0 + ) + revenue = max(income, invoiced) + + if not revenue and not expense: + return { + "measured": False, + "income": None, + "expense": None, + "profit": None, + "margin_percent": None, + } + + profit = revenue - expense + return { + "measured": True, + "income": round(revenue, 2), + "expense": round(expense, 2), + "profit": round(profit, 2), + "margin_percent": round(profit / revenue * 100, 1) if revenue else None, + } + + @staticmethod + def _budget_variance(company_id, projects) -> float | None: + """Spend against approved budget, on completed projects only.""" + spend_by_project = dict( + db.session.query(Transaction.project_id, func.sum(Transaction.amount)) + .filter( + Transaction.company_id == company_id, + Transaction.transaction_type == TransactionType.EXPENSE, + ) + .group_by(Transaction.project_id) + .all() + ) + + # Completed projects only. Spend under budget on a live project is work + # not yet done, not a favourable variance, and averaging the two makes + # an overrunning portfolio look healthy. + finished = [ + p for p in projects if p.status == "completed" and p.budget and p.id in spend_by_project + ] + budget = sum(p.budget for p in finished) + spend = sum(float(spend_by_project[p.id]) for p in finished) + + if not budget: + return None + return round((spend - budget) / budget * 100, 1) + @staticmethod def _fleet_utilization(company_id) -> float: """Mean 30-day utilisation across a company's active equipment.""" @@ -108,59 +349,127 @@ def _grade_overdue(overdue: int, total: int) -> str: return "high" def get_financial_performance(self, company_id, months=12): - """Get financial performance trends. + """Monthly revenue, cost and margin, aggregated from the ledger. - WARNING: the monthly figures below are generated, not measured. Real - trends need transaction history the platform does not yet aggregate - by month. The payload is flagged ``simulated`` so callers can label it - rather than present invented revenue as fact. - """ - - monthly_data = [] - base_revenue = 2500000 # Base monthly revenue + This used to generate the whole series:: - for i in range(months): - month_date = datetime.now() - timedelta(days=30 * i) + base_revenue = 2500000 + growth_factor = 1 + (i * 0.02) + variance = 0.8 + (i % 3) * 0.1 + revenue = base_revenue * growth_factor * variance + costs = revenue * 0.75 - # Simulate revenue growth with some variation - growth_factor = 1 + (i * 0.02) # 2% monthly growth - variance = 0.8 + (i % 3) * 0.1 # Add some variation + Twelve months of invented revenue on a two-percent growth curve, the + same for every company, carrying a ``simulated`` flag that the UI was + free to ignore. It is measured now. - revenue = base_revenue * growth_factor * variance - costs = revenue * 0.75 # 75% cost ratio - profit = revenue - costs + Where the ledger is empty the series is empty and ``available`` is + False, with the reason stated. **Extension path:** nothing further is + needed in this module — post ``Transaction`` rows of type INCOME or + EXPENSE with a ``transaction_date``, or raise ``Invoice`` records, and + every figure below populates. That is the whole contract. + """ + end_date = date.today() + start_date = end_date - timedelta(days=31 * months) + + # Bucketed in Python, not with func.strftime: that is SQLite-only and + # would have worked in development and failed on the PostgreSQL this + # deploys to. date_trunc and to_char are the Postgres spellings and + # neither exists in SQLite, so there is no portable SQL form. + rows = ( + db.session.query( + Transaction.transaction_date, + Transaction.transaction_type, + Transaction.amount, + ) + .filter( + Transaction.company_id == company_id, + Transaction.transaction_date >= start_date, + ) + .all() + ) - monthly_data.append( + buckets: dict[str, dict[str, float]] = {} + for transaction_date, kind, amount in rows: + if transaction_date is None: + continue + month = transaction_date.strftime("%Y-%m") + bucket = buckets.setdefault(month, {"revenue": 0.0, "costs": 0.0}) + if kind == TransactionType.INCOME: + bucket["revenue"] += float(amount or 0) + elif kind == TransactionType.EXPENSE: + bucket["costs"] += float(amount or 0) + + # Invoices count as revenue in months where no income was posted, so a + # deployment that invoices without keeping a ledger still gets a trend. + invoiced_by_month: dict[str, float] = {} + for issue_date, total in ( + db.session.query(Invoice.issue_date, Invoice.total_amount) + .filter(Invoice.company_id == company_id, Invoice.issue_date >= start_date) + .all() + ): + if issue_date is None: + continue + month = issue_date.strftime("%Y-%m") + invoiced_by_month[month] = invoiced_by_month.get(month, 0.0) + float(total or 0) + + for month, invoiced in invoiced_by_month.items(): + bucket = buckets.setdefault(month, {"revenue": 0.0, "costs": 0.0}) + bucket["revenue"] = max(bucket["revenue"], invoiced) + + if not buckets: + return { + "available": False, + "reason": ( + "No income, expense or invoice records in the last " + f"{months} months, so there is no financial trend to report." + ), + "how_to_populate": ( + "Record Transaction rows of type INCOME or EXPENSE against projects, " + "or raise Invoices. This report reads them directly; no configuration " + "or code change is required." + ), + "monthly_trends": [], + "year_to_date": None, + } + + monthly_trends = [] + for month in sorted(buckets): + revenue = round(buckets[month]["revenue"], 2) + costs = round(buckets[month]["costs"], 2) + profit = round(revenue - costs, 2) + monthly_trends.append( { - "month": month_date.strftime("%Y-%m"), + "month": month, "revenue": revenue, "costs": costs, "profit": profit, - "margin": (profit / revenue * 100) if revenue > 0 else 0, + "margin": round(profit / revenue * 100, 1) if revenue else None, } ) - monthly_data.reverse() # Show chronological order + total_revenue = sum(m["revenue"] for m in monthly_trends) + total_profit = sum(m["profit"] for m in monthly_trends) return { - "simulated": True, - "simulated_note": ( - "Generated illustrative data. Not derived from recorded transactions." - ), - "monthly_trends": monthly_data, + "available": True, + "months_with_data": len(monthly_trends), + "monthly_trends": monthly_trends, "year_to_date": { - "revenue": sum(m["revenue"] for m in monthly_data[-12:]), - "profit": sum(m["profit"] for m in monthly_data[-12:]), - "margin": sum(m["margin"] for m in monthly_data[-12:]) / len(monthly_data[-12:]), + "revenue": round(total_revenue, 2), + "profit": round(total_profit, 2), + "margin": round(total_profit / total_revenue * 100, 1) if total_revenue else None, }, } def get_project_portfolio_analysis(self, company_id): - """Analyze project portfolio performance. + """Analyse project portfolio performance. - Project counts by size band are real. Margins, completion rates, - durations and the geographic breakdown are illustrative constants and - are flagged as such in the payload. + Everything here is measured. Size bands, completion rates, durations, + margins, the geographic split and the sector split all come from + projects and transactions. Figures that nothing supports are ``None``, + not zero — a band with no data and a band at zero percent are different + statements and an executive dashboard must not merge them. """ projects = Project.query.filter_by(company_id=company_id).all() @@ -178,42 +487,25 @@ def get_project_portfolio_analysis(self, company_id): else: large_projects.append(project) - # Performance by project type + # Completion rate, duration and margin per band, all measured. These + # were constants — small projects always 15.2% margin, 95.8% complete, + # 3.2 months, for every company that ever loaded the page. + spend = self._spend_by_project(company_id) performance_by_size = { - "small": { - "count": len(small_projects), - "avg_margin": 15.2, # Higher margin for small projects - "completion_rate": 95.8, - "avg_duration": 3.2, # months - }, - "medium": { - "count": len(medium_projects), - "avg_margin": 12.1, - "completion_rate": 89.4, - "avg_duration": 8.7, - }, - "large": { - "count": len(large_projects), - "avg_margin": 8.9, # Lower margin but higher volume - "completion_rate": 82.1, - "avg_duration": 18.3, - }, + band: self._band_performance(members, spend) + for band, members in ( + ("small", small_projects), + ("medium", medium_projects), + ("large", large_projects), + ) } - # Geographic distribution (simulated) - geographic_data = [ - {"region": "Northeast", "projects": 12, "revenue": 8500000}, - {"region": "Southeast", "projects": 8, "revenue": 6200000}, - {"region": "Midwest", "projects": 15, "revenue": 11200000}, - {"region": "West", "projects": 10, "revenue": 9800000}, - ] + # Grouped by the location recorded on each project. This was four + # hardcoded US regions with invented project counts and revenue — + # figures that did not move when the portfolio did. + geographic_data = self._by_location(projects, spend) return { - "partially_simulated": True, - "simulated_note": ( - "Project counts and values are real. Margins, completion rates, " - "durations and the geographic split are illustrative constants." - ), "portfolio_summary": { "total_projects": len(projects), "total_value": sum(p.budget for p in projects if p.budget), @@ -224,12 +516,7 @@ def get_project_portfolio_analysis(self, company_id): }, "performance_by_size": performance_by_size, "geographic_distribution": geographic_data, - "sector_analysis": [ - {"sector": "Commercial", "projects": 15, "revenue": 18500000, "margin": 11.2}, - {"sector": "Residential", "projects": 12, "revenue": 8900000, "margin": 14.8}, - {"sector": "Industrial", "projects": 8, "revenue": 12600000, "margin": 9.3}, - {"sector": "Infrastructure", "projects": 10, "revenue": 15200000, "margin": 7.9}, - ], + "sector_analysis": self._by_sector(projects, spend), } def get_operational_efficiency(self, company_id): diff --git a/services/azure_ai.py b/services/azure_ai.py index 1cef2ec..ea98882 100644 --- a/services/azure_ai.py +++ b/services/azure_ai.py @@ -92,7 +92,7 @@ def _grounding(self, project_id: int) -> dict[str, Any]: def analyze_project_schedule(self, project_id: int) -> dict[str, Any]: """Interpret a computed schedule: risks, bottlenecks, recommendations.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: return {"success": False, "error": f"Project {project_id} not found"} @@ -162,7 +162,7 @@ def analyze_project_schedule(self, project_id: int) -> dict[str, Any]: def optimize_schedule(self, project_id: int, parameters: dict[str, Any]) -> dict[str, Any]: """Propose sequence and duration changes against a computed baseline.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: return {"success": False, "error": f"Project {project_id} not found"} @@ -239,7 +239,7 @@ def optimize_schedule(self, project_id: int, parameters: dict[str, Any]) -> dict def predict_completion_date(self, project_id: int) -> dict[str, Any]: """Forecast completion from measured progress against the CPM baseline.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: return {"success": False, "error": f"Project {project_id} not found"} diff --git a/services/fabric_service.py b/services/fabric_service.py index d58037c..8836306 100644 --- a/services/fabric_service.py +++ b/services/fabric_service.py @@ -4,6 +4,7 @@ import requests +from extensions import db from models import Project, Resource, Task @@ -69,7 +70,7 @@ def _make_api_request(self, endpoint: str, method: str = "GET", data: dict = Non def sync_project_data(self, project_id: int) -> dict[str, Any]: """Sync project data to Microsoft Fabric data lake.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) tasks = Task.query.filter_by(project_id=project_id).all() resources = Resource.query.filter_by(project_id=project_id).all() diff --git a/services/foundry_service.py b/services/foundry_service.py index fb6e57a..942f51f 100644 --- a/services/foundry_service.py +++ b/services/foundry_service.py @@ -5,6 +5,7 @@ import requests +from extensions import db from models import Project, Task, TaskStatus @@ -30,7 +31,7 @@ def _make_foundry_request(self, endpoint: str, data: dict) -> dict: def predict_project_outcomes(self, project_id: int, prediction_type: str) -> dict[str, Any]: """Predict project outcomes using Azure AI Foundry.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) tasks = Task.query.filter_by(project_id=project_id).all() # Prepare historical data for prediction @@ -269,7 +270,7 @@ def _predict_resource_needs(self, request_data: dict) -> dict[str, Any]: def generate_schedule_insights(self, project_id: int) -> dict[str, Any]: """Generate comprehensive schedule insights using Foundry.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) tasks = Task.query.filter_by(project_id=project_id).all() insights_request = { diff --git a/services/schedule_analysis.py b/services/schedule_analysis.py index 4a344ae..ceadeb5 100644 --- a/services/schedule_analysis.py +++ b/services/schedule_analysis.py @@ -26,7 +26,7 @@ def load_network(project_id: int) -> tuple[list[Activity], list[Relationship], WorkCalendar]: """Read a project out of the database as a pure logic network.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: raise LookupError(f"Project {project_id} not found") @@ -165,7 +165,7 @@ def health_check(project_id: int) -> dict[str, Any]: # not, they are omitted and the checks report as skipped. Deriving # "actuals" from the CPM forward pass would make them look assessed while # measuring nothing but the plan against itself. - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) calendar = WorkCalendar(project.start_date) tasks = Task.query.filter(Task.id.in_(task_ids)).all() @@ -203,7 +203,7 @@ def health_check(project_id: int) -> dict[str, Any]: def progress_report(project_id: int) -> dict[str, Any]: """Measure the project against its baseline: BEI, variance, slippage.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: return {"success": False, "error": f"Project {project_id} not found"} @@ -255,7 +255,7 @@ def offset(value): def set_baseline(project_id: int, name: str, user_id: int | None = None, notes: str = ""): """Freeze the current plan as the baseline everything is measured against.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: raise LookupError(f"Project {project_id} not found") diff --git a/services/schedule_io.py b/services/schedule_io.py index 7f9720d..8ae82ef 100644 --- a/services/schedule_io.py +++ b/services/schedule_io.py @@ -500,7 +500,7 @@ def import_into_project( def export_project(project_id: int) -> ExchangeSchedule: """Read a stored project out as an :class:`ExchangeSchedule`.""" - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: raise LookupError(f"Project {project_id} not found") diff --git a/services/schedule_optimizer.py b/services/schedule_optimizer.py index aaf42ee..b16bb50 100644 --- a/services/schedule_optimizer.py +++ b/services/schedule_optimizer.py @@ -9,6 +9,7 @@ calculate_cpm, longest_path, ) +from extensions import db from models import Project, Resource, ResourceAssignment, Task, TaskDependency @@ -34,7 +35,7 @@ def optimize_project_schedule( if optimization_type not in self.optimization_methods: raise ValueError(f"Unsupported optimization type: {optimization_type}") - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if project is None: return {"success": False, "error": f"Project {project_id} not found"} diff --git a/tasks/background_tasks.py b/tasks/background_tasks.py index 7f00636..154e396 100644 --- a/tasks/background_tasks.py +++ b/tasks/background_tasks.py @@ -1,6 +1,7 @@ import json from datetime import date, datetime +from extensions import db from services.azure_ai import AzureAIService from services.fabric_service import FabricService from services.foundry_service import FoundryService @@ -20,7 +21,7 @@ def process_project_file(self, project_id, file_path, file_type): from extensions import db from models import Project, Task - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if not project: raise Exception(f"Project {project_id} not found") @@ -81,7 +82,7 @@ def sync_azure_services(self, project_id, services=None): from extensions import db from models import AzureIntegration, Project - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if not project: raise Exception(f"Project {project_id} not found") @@ -139,7 +140,7 @@ def generate_project_report(project_id, report_type="comprehensive"): try: from models import Project, Resource, Task - project = Project.query.get(project_id) + project = db.session.get(Project, project_id) if not project: raise Exception(f"Project {project_id} not found") @@ -184,7 +185,7 @@ def send_notification(user_id, notification_type, data): try: from models import User - user = User.query.get(user_id) + user = db.session.get(User, user_id) if not user: return {"status": "failed", "error": "User not found"} diff --git a/templates/admin/audit_logs.html b/templates/admin/audit_logs.html index c8e6920..20254a2 100644 --- a/templates/admin/audit_logs.html +++ b/templates/admin/audit_logs.html @@ -71,14 +71,14 @@

Audit log

  • + href="{% if logs.has_prev %}{{ url_for('admin.audit_logs', page=logs.prev_num) }}{% else %}#{% endif %}"> Previous
    • {% for page_number in logs.iter_pages(left_edge=1, left_current=2, right_current=2, right_edge=1) %} {% if page_number %}
  • - {{ page_number }} + {{ page_number }}
    • {% else %}
  • …
    • @@ -86,7 +86,7 @@

    Audit log

    {% endfor %}
  • + href="{% if logs.has_next %}{{ url_for('admin.audit_logs', page=logs.next_num) }}{% else %}#{% endif %}"> Next
    • diff --git a/templates/admin/company_settings.html b/templates/admin/company_settings.html index 3c7f384..3fbae8a 100644 --- a/templates/admin/company_settings.html +++ b/templates/admin/company_settings.html @@ -89,7 +89,7 @@

    Microsoft cloud

    - Back to users + Back to users {% endif %} diff --git a/templates/admin/edit_user.html b/templates/admin/edit_user.html index b64b3d6..4cc9a5e 100644 --- a/templates/admin/edit_user.html +++ b/templates/admin/edit_user.html @@ -8,7 +8,7 @@
    @@ -93,7 +93,7 @@

    Edit user

    - Cancel + Cancel
    @@ -113,7 +113,7 @@

    Account status

    {{ user.role.value.replace('_', ' ')|title }}

    {% if user.is_active %} -
    + {% if csrf_token is defined %} {% endif %} @@ -122,7 +122,7 @@

    Account status

    {% else %} -
    + {% if csrf_token is defined %} {% endif %} diff --git a/tests/test_all_routes.py b/tests/test_all_routes.py index b5091d7..ed7a6ba 100644 --- a/tests/test_all_routes.py +++ b/tests/test_all_routes.py @@ -142,11 +142,13 @@ def test_the_pages_that_used_to_be_500s_now_render(walk_context): """ client, values = walk_context + # The /management paths these used to live at now 308 to /admin; that + # redirect is covered by tests/test_mutating_routes.py. pages = [ - f"/management/users/{values['user_id']}/edit", - "/management/company/settings", - "/management/audit-logs", - "/management/system-status", + f"/admin/users/{values['user_id']}/edit", + "/admin/company/settings", + "/admin/audit-logs", + "/admin/system-status", "/azure/dashboard", f"/azure/configure/{values['project_id']}", "/project-templates/my-templates", diff --git a/tests/test_executive_dashboard.py b/tests/test_executive_dashboard.py new file mode 100644 index 0000000..b35089d --- /dev/null +++ b/tests/test_executive_dashboard.py @@ -0,0 +1,328 @@ +"""The executive dashboard reports what happened, or says it cannot. + +This was the last facade. Twelve months of revenue were generated from a +constant:: + + base_revenue = 2500000 + growth_factor = 1 + (i * 0.02) + revenue = base_revenue * growth_factor * variance + costs = revenue * 0.75 + +along with a 12% profit margin, a 5% budget variance, four hardcoded US regions +with invented project counts, four sectors with invented margins, and per-size +performance bands where small projects were always 15.2% margin and 95.8% +complete. Identical for every company, unmoved by anything anyone did. A +``simulated`` flag was attached, which a chart is free to ignore. + +Everything is measured now, and where nothing supports a figure it is ``None`` +with the reason stated — never zero. Zero profit and unknown profit are +different statements, and the whole point of this page is that someone acts on +what it says. +""" + +from datetime import date, timedelta + +import pytest + +from extensions import db +from models import Company, Invoice, InvoiceStatus, Transaction, TransactionType + + +@pytest.fixture +def dashboard(): + from reports.executive_dashboard import ExecutiveDashboard + + return ExecutiveDashboard() + + +@pytest.fixture +def empty_company(app_context): + """A company with no projects, transactions or invoices at all.""" + company = Company(name="Brand New Contractors") + db.session.add(company) + db.session.commit() + return company + + +def _expense(project, amount, when, company_id, number): + db.session.add( + Transaction( + transaction_number=number, + transaction_type=TransactionType.EXPENSE, + amount=amount, + description="Test expense", + transaction_date=when, + project_id=project.id, + company_id=company_id, + created_by_id=project.created_by, + ) + ) + + +def _income(project, amount, when, company_id, number): + db.session.add( + Transaction( + transaction_number=number, + transaction_type=TransactionType.INCOME, + amount=amount, + description="Test income", + transaction_date=when, + project_id=project.id, + company_id=company_id, + created_by_id=project.created_by, + ) + ) + + +# ── nothing recorded ───────────────────────────────────────────────────── + + +def test_no_ledger_means_no_trend_rather_than_a_generated_one(dashboard, empty_company): + result = dashboard.get_financial_performance(empty_company.id) + + assert result["available"] is False + assert result["monthly_trends"] == [] + assert result["year_to_date"] is None + # And it says what to do about it. + assert "Transaction" in result["how_to_populate"] + + +def test_no_ledger_means_margin_is_unknown_not_zero(dashboard, empty_company): + """A zero margin reads as a company breaking even. An unknown margin reads + as a company that has not recorded anything. They must not be confused.""" + financial = dashboard.get_company_overview(empty_company.id)["financial"] + + assert financial["measured"] is False + assert financial["profit_margin"] is None + assert financial["estimated_profit"] is None + assert financial["recorded_income"] is None + assert financial["note"] is not None + + +def test_the_generated_series_is_gone(dashboard, seeded): + """The old implementation always produced exactly `months` entries from a + 2,500,000 base. A real ledger produces one entry per month that has data.""" + result = dashboard.get_financial_performance(seeded.company_id, months=12) + + assert result["available"] is True + assert len(result["monthly_trends"]) < 12, "12 entries suggests a generated series" + assert all(m["revenue"] != 2500000 for m in result["monthly_trends"]) + + +# ── measured from the ledger ───────────────────────────────────────────── + + +def test_monthly_trends_come_from_transactions(dashboard, seeded): + company_id = seeded.company_id + when = date.today().replace(day=15) + + _income(seeded, 400000, when, company_id, "TXN-IN-1") + _expense(seeded, 250000, when, company_id, "TXN-EX-1") + db.session.commit() + + result = dashboard.get_financial_performance(company_id) + month = when.strftime("%Y-%m") + entry = next(m for m in result["monthly_trends"] if m["month"] == month) + + assert entry["revenue"] >= 400000 + assert entry["costs"] >= 250000 + assert entry["profit"] == round(entry["revenue"] - entry["costs"], 2) + + +def test_an_invoice_counts_as_revenue_without_an_income_transaction(dashboard, seeded): + """A deployment may invoice without keeping a ledger; it still gets a trend.""" + when = date.today().replace(day=10) + db.session.add( + Invoice( + invoice_number="INV-EXEC-1", + client_name="Acme", + issue_date=when, + due_date=when + timedelta(days=30), + subtotal=750000, + total_amount=750000, + status=InvoiceStatus.SENT, + project_id=seeded.id, + company_id=seeded.company_id, + created_by_id=seeded.created_by, + ) + ) + db.session.commit() + + result = dashboard.get_financial_performance(seeded.company_id) + entry = next(m for m in result["monthly_trends"] if m["month"] == when.strftime("%Y-%m")) + assert entry["revenue"] >= 750000 + + +def test_income_and_invoices_are_not_double_counted(dashboard, seeded): + """Both are revenue. Adding them together would report twice the money.""" + when = date.today().replace(day=12) + _income(seeded, 500000, when, seeded.company_id, "TXN-IN-2") + db.session.add( + Invoice( + invoice_number="INV-EXEC-2", + client_name="Acme", + issue_date=when, + due_date=when + timedelta(days=30), + subtotal=500000, + total_amount=500000, + status=InvoiceStatus.SENT, + project_id=seeded.id, + company_id=seeded.company_id, + created_by_id=seeded.created_by, + ) + ) + db.session.commit() + + result = dashboard.get_financial_performance(seeded.company_id) + entry = next(m for m in result["monthly_trends"] if m["month"] == when.strftime("%Y-%m")) + assert entry["revenue"] < 1000000, "income and invoices were summed instead of reconciled" + + +# ── margin is not budget remaining ─────────────────────────────────────── + + +def test_an_unfinished_project_reports_budget_consumed_not_margin(dashboard, seeded): + """The trap this page was built to avoid. The demo project is four months + into a two-year job with 14% of its budget spent. Reported as margin that + is "86% profitable" — the least advanced project looks like the best one.""" + analysis = dashboard.get_project_portfolio_analysis(seeded.company_id) + band = analysis["performance_by_size"]["large"] + + assert band["avg_margin"] is None + assert "no completed project" in band["margin_basis"].lower() + assert band["budget_consumed_percent"] is not None + assert 0 < band["budget_consumed_percent"] < 100 + + +def test_a_completed_project_does_report_realised_margin(dashboard, seeded): + seeded.status = "completed" + db.session.commit() + + analysis = dashboard.get_project_portfolio_analysis(seeded.company_id) + band = analysis["performance_by_size"]["large"] + + assert band["avg_margin"] is not None + assert band["completion_rate"] == 100.0 + + +def test_budget_variance_ignores_work_still_in_flight(dashboard, seeded): + """Underspend on a live project is work not yet done. Averaging it with + completed work makes an overrunning portfolio look healthy.""" + financial = dashboard.get_company_overview(seeded.company_id)["financial"] + assert financial["budget_variance"] is None + + seeded.status = "completed" + db.session.commit() + financial = dashboard.get_company_overview(seeded.company_id)["financial"] + assert financial["budget_variance"] is not None + + +# ── geography and sector come from the projects ────────────────────────── + + +def test_geography_is_grouped_from_recorded_locations(dashboard, seeded): + """Was four hardcoded US regions with invented counts and revenue.""" + analysis = dashboard.get_project_portfolio_analysis(seeded.company_id) + regions = {entry["region"] for entry in analysis["geographic_distribution"]} + + assert seeded.location in regions + assert regions != {"Northeast", "Southeast", "Midwest", "West"} + + +def test_a_project_without_a_location_is_unspecified_not_invented(dashboard, seeded): + seeded.location = None + db.session.commit() + + analysis = dashboard.get_project_portfolio_analysis(seeded.company_id) + assert [e["region"] for e in analysis["geographic_distribution"]] == ["Unspecified"] + + +def test_sector_comes_from_the_template_the_project_was_created_from(dashboard, seeded): + seeded.template_used = "industrial_warehouse" + db.session.commit() + + analysis = dashboard.get_project_portfolio_analysis(seeded.company_id) + sectors = {entry["sector"] for entry in analysis["sector_analysis"]} + assert sectors == {"Industrial"} + + +def test_a_project_from_no_template_is_unspecified(dashboard, seeded): + """Rather than assigned to whichever sector looks plausible.""" + seeded.template_used = None + db.session.commit() + + analysis = dashboard.get_project_portfolio_analysis(seeded.company_id) + assert [e["sector"] for e in analysis["sector_analysis"]] == ["Unspecified"] + + +def test_an_empty_band_reports_nothing_rather_than_zero(dashboard, seeded): + """A band with no projects has no completion rate. Reporting 0% would read + as a portfolio in trouble rather than a portfolio without data.""" + analysis = dashboard.get_project_portfolio_analysis(seeded.company_id) + small = analysis["performance_by_size"]["small"] + + assert small["count"] == 0 + assert small["measured"] is False + assert small["completion_rate"] is None + assert small["avg_margin"] is None + + +# ── the payload no longer claims to be simulated ───────────────────────── + + +def test_nothing_is_flagged_simulated_any_more(dashboard, seeded): + """The flags existed because the data was invented. Their absence is the + claim that it no longer is.""" + company_id = seeded.company_id + payloads = [ + dashboard.get_company_overview(company_id), + dashboard.get_financial_performance(company_id), + dashboard.get_project_portfolio_analysis(company_id), + ] + for payload in payloads: + assert "simulated" not in payload + assert "partially_simulated" not in payload + assert "simulated_note" not in payload + + +def test_the_source_computes_rather_than_asserts_the_numbers(): + """The specific values the old implementation returned. + + Checked against the AST, not the raw text: the docstrings above quote the + removed code on purpose, so a substring search would match its own + explanation of what was deleted. + """ + import ast + from pathlib import Path + + source = ( + Path(__file__).resolve().parent.parent / "reports" / "executive_dashboard.py" + ).read_text(encoding="utf-8") + + invented = {2500000, 8500000, 18500000, 6200000, 11200000, 9800000, 15.2, 95.8, 78.5, 8.9, 12.1} + + found = set() + for node in ast.walk(ast.parse(source)): + # A number that appears as a literal in executable code. Docstrings are + # str constants and never match a numeric comparison. + if isinstance(node, ast.Constant) and isinstance(node.value, (int, float)): + if node.value in invented: + found.add(node.value) + + assert not found, f"still hardcoded in executable code: {sorted(found)}" + + +def test_every_dashboard_endpoint_answers(signed_in): + client, _, _ = signed_in + for url in ( + "/api/executive/overview", + "/api/executive/financial", + "/api/executive/portfolio", + "/api/executive/efficiency", + "/api/executive/risk", + ): + response = client.get(url) + # 403 is a legitimate answer for a non-executive role; a 500 is not. + assert response.status_code < 500, ( + f"{url} -> {response.status_code}: {response.get_data(as_text=True)[:160]}" + ) diff --git a/tests/test_mutating_routes.py b/tests/test_mutating_routes.py new file mode 100644 index 0000000..9e0e26a --- /dev/null +++ b/tests/test_mutating_routes.py @@ -0,0 +1,386 @@ +"""The half of the application that writes. + +Every GET route was walked; none of the 32 POST/PUT/DELETE routes were. That +is the half where authorisation lives, and it is where the damage is done when +authorisation is wrong. + +It hid a live bug. Two admin blueprints each implemented ``create_user``, and +they disagreed about how to read the role field: ``UserRole(role)`` keys on the +enum value, ``UserRole[role]`` on its name. The form sends ``ADMIN``, a name, so +``POST /admin/users/create`` raised ``ValueError: 'ADMIN' is not a valid +UserRole`` on every submission. No test had ever posted to it. + +This repository has also had a tenant-isolation bypass before — +``blueprints/projects.py`` checked ``role.name != 'ADMIN'`` and skipped the +company check for administrators, so any admin could read any other company's +project. The intruder tests below exist because that already happened once. +""" + +from datetime import date, timedelta + +import pytest +from werkzeug.security import generate_password_hash + +from extensions import db +from models import Company, Project, Task, User, UserRole + + +@pytest.fixture +def intruder(seeded): + """An administrator of a different company, and a client signed in as them. + + Deliberately an ADMIN: the bypass this guards against was specifically that + an administrator skipped the company check. A viewer would be refused by + the role check and prove nothing. + """ + rival = Company(name="Rival Contractors") + db.session.add(rival) + db.session.flush() + + user = User( + username="intruder", + email="intruder@rival.test", + password_hash=generate_password_hash("intruder-password"), + first_name="In", + last_name="Truder", + role=UserRole.ADMIN, + company_id=rival.id, + is_active=True, + ) + db.session.add(user) + db.session.commit() + + from flask import current_app + + client = current_app.test_client() + with client.session_transaction() as session: + session["_user_id"] = str(user.id) + session["_fresh"] = True + return client, user + + +def _victim(seeded): + task = Task.query.filter_by(project_id=seeded.id).first() + return seeded, task + + +# ── tenant isolation on writes ─────────────────────────────────────────── + + +def test_a_rival_admin_cannot_write_to_another_companys_project(intruder, seeded): + """The headline. Each of these is a write against a project the caller's + company does not own, and each must be refused.""" + client, _ = intruder + project, task = _victim(seeded) + + attempts = [ + ("post", f"/api/schedule/projects/{project.id}/baseline", {"name": "stolen baseline"}), + ("post", f"/azure/configure/{project.id}", {"service_type": "ai", "configuration": "{}"}), + ("put", f"/scheduling/api/tasks/{task.id}/update", {"name": "HACKED"}), + ("post", "/api/projects/quick-task", {"project_id": project.id, "name": "HACKED"}), + ] + + allowed = [] + for method, url, payload in attempts: + send = getattr(client, method) + use_json = url.startswith("/api") or method == "put" + response = send(url, json=payload) if use_json else send(url, data=payload) + # 2xx means the write went through. Anything else refused it. + if 200 <= response.status_code < 300: + allowed.append(f"{url} -> {response.status_code}") + + assert not allowed, "A rival company's admin was allowed to write: " + ", ".join(allowed) + + +def test_the_victims_data_is_untouched_after_the_attempts(intruder, seeded): + """Status codes can lie. This checks the database.""" + client, _ = intruder + project, task = _victim(seeded) + original_name = task.name + original_task_count = Task.query.filter_by(project_id=project.id).count() + + client.put(f"/scheduling/api/tasks/{task.id}/update", json={"name": "HACKED"}) + client.post("/api/projects/quick-task", json={"project_id": project.id, "name": "HACKED"}) + + db.session.expire_all() + assert db.session.get(Task, task.id).name == original_name + assert Task.query.filter_by(project_id=project.id).count() == original_task_count + + +def test_a_rival_admin_cannot_edit_a_user_in_another_company(intruder, seeded): + """Administrators administer their own company. The role alone is not a + licence over every tenant.""" + client, _ = intruder + victim = User.query.filter_by(username="demo").first() + original_email = victim.email + + client.post( + f"/admin/users/{victim.id}/edit", + data={ + "first_name": "Taken", + "last_name": "Over", + "email": "attacker@rival.test", + "role": "VIEWER", + "is_active": "on", + }, + ) + + db.session.expire_all() + refreshed = db.session.get(User, victim.id) + assert refreshed.email == original_email + assert refreshed.role is not UserRole.VIEWER + + +def test_a_rival_admin_cannot_deactivate_another_companys_user(intruder, seeded): + client, _ = intruder + victim = User.query.filter_by(username="demo").first() + + client.post(f"/admin/users/{victim.id}/deactivate") + client.post(f"/admin/api/users/{victim.id}/toggle-status") + + db.session.expire_all() + assert db.session.get(User, victim.id).is_active is True + + +# ── the writes an owner is entitled to make ────────────────────────────── + + +def test_an_admin_can_create_a_user(signed_in): + """The bug the missing coverage hid: POST here raised ValueError on every + submission, because the role was read by value and the form sends a name.""" + client, _, user = signed_in + user.role = UserRole.ADMIN + db.session.commit() + + response = client.post( + "/admin/users/create", + data={ + "username": "newjoiner", + "email": "newjoiner@example.test", + "first_name": "New", + "last_name": "Joiner", + "role": "PROJECT_MANAGER", + "password": "a-long-enough-password", + }, + follow_redirects=True, + ) + assert response.status_code == 200 + + created = User.query.filter_by(username="newjoiner").first() + assert created is not None, "the user was not created" + assert created.role is UserRole.PROJECT_MANAGER + assert created.company_id == user.company_id + + +def test_an_unknown_role_is_refused_rather_than_raising(signed_in): + """A hand-crafted POST with a bogus role must not produce a 500.""" + client, _, user = signed_in + user.role = UserRole.ADMIN + db.session.commit() + + response = client.post( + "/admin/users/create", + data={ + "username": "bogus", + "email": "bogus@example.test", + "first_name": "B", + "last_name": "B", + "role": "SUPREME_OVERLORD", + "password": "a-long-enough-password", + }, + ) + assert response.status_code < 500 + assert User.query.filter_by(username="bogus").first() is None + + +def test_an_admin_cannot_deactivate_their_own_account(signed_in): + """Recoverable only by editing the database directly, so it is refused.""" + client, _, user = signed_in + user.role = UserRole.ADMIN + db.session.commit() + + client.post(f"/admin/users/{user.id}/deactivate", follow_redirects=True) + db.session.expire_all() + assert db.session.get(User, user.id).is_active is True + + response = client.post(f"/admin/api/users/{user.id}/toggle-status") + assert response.status_code == 400 + db.session.expire_all() + assert db.session.get(User, user.id).is_active is True + + +def test_an_admin_can_update_their_own_company(signed_in): + client, _, user = signed_in + user.role = UserRole.ADMIN + db.session.commit() + + response = client.post( + "/admin/company/settings", + data={ + "name": "Renamed Contractors", + "address": "1 New Street", + "phone": "0100 000000", + "email": "hello@renamed.test", + "azure_tenant_id": "", + "fabric_workspace_id": "", + }, + follow_redirects=True, + ) + assert response.status_code == 200 + + db.session.expire_all() + assert db.session.get(Company, user.company_id).name == "Renamed Contractors" + + +# ── the retired /management surface ────────────────────────────────────── + + +def test_every_retired_management_path_redirects(signed_in): + """Existing links and bookmarks must not 404 after the merge.""" + from admin.user_management import REDIRECTS, REDIRECTS_WITH_USER + + client, _, user = signed_in + user.role = UserRole.ADMIN + db.session.commit() + + for path in REDIRECTS: + response = client.get(f"/management{path}") + assert response.status_code == 308, f"/management{path} -> {response.status_code}" + assert "/admin" in response.headers["Location"] + + for path in REDIRECTS_WITH_USER: + concrete = path.replace("", str(user.id)) + response = client.get(f"/management{concrete}") + assert response.status_code == 308, f"/management{concrete} -> {response.status_code}" + + +def test_the_redirect_preserves_the_method(signed_in): + """308, not 302. A 302 turns a POST into a GET and drops the form body, so + a bookmarked form would silently stop working instead of failing loudly.""" + client, _, user = signed_in + user.role = UserRole.ADMIN + db.session.commit() + + response = client.post(f"/management/users/{user.id}/deactivate") + assert response.status_code == 308 + + +def test_no_endpoint_is_defined_twice(flask_app): + """The merge exists to stop two blueprints implementing the same view. This + fails if a third copy appears.""" + seen = {} + for rule in flask_app.url_map.iter_rules(): + seen.setdefault(str(rule), []).append(rule.endpoint) + + duplicates = {url: names for url, names in seen.items() if len(set(names)) > 1} + assert not duplicates, f"Several endpoints serve the same URL: {duplicates}" + + +# ── the walk ───────────────────────────────────────────────────────────── + + +def test_no_mutating_route_returns_a_server_error(signed_in): + """Post an empty body to every write route and require that nothing + explodes. A 400 or 404 is a correct answer to nonsense; a 500 is not. + + This is deliberately shallow — it cannot know what each route wants — but + it is what catches an import error, a missing column or an unguarded + ``request.form[...]`` on a route nobody has opened in months. + """ + client, project, user = signed_in + user.role = UserRole.ADMIN + db.session.commit() + + task = Task.query.filter_by(project_id=project.id).first() + substitutions = { + "project_id": project.id, + "task_id": task.id, + "user_id": user.id, + "id": project.id, + "equipment_id": 1, + "invoice_id": 1, + "company_id": user.company_id, + "template_id": "commercial_office", + } + + import re + + from flask import current_app + + failures, walked = [], 0 + for rule in current_app.url_map.iter_rules(): + methods = rule.methods - {"HEAD", "OPTIONS", "GET"} + if not methods or rule.endpoint == "static": + continue + + url = str(rule) + skip = False + for argument in rule.arguments: + if argument not in substitutions: + skip = True + break + url = re.sub( + r"<[^<>]*\b" + re.escape(argument) + r">", str(substitutions[argument]), url + ) + if skip or "<" in url: + continue + # Signing out mid-walk would invalidate every later result. + if "logout" in url or "login" in url: + continue + + walked += 1 + try: + response = client.post(url, data={}, follow_redirects=False) + except Exception as exc: + failures.append(f"{url} raised {type(exc).__name__}: {exc}") + continue + if response.status_code >= 500: + failures.append( + f"{url} -> {response.status_code}: {response.get_data(as_text=True)[:160]}" + ) + + assert walked > 15, f"only {walked} mutating routes walked" + assert not failures, "Mutating routes returning a server error:\n " + "\n ".join(failures) + + +def test_a_write_route_rejects_an_anonymous_caller(seeded): + """Every mutating route except login and register requires a session.""" + from flask import current_app + + anonymous = current_app.test_client() + task = Task.query.filter_by(project_id=seeded.id).first() + + for method, url in ( + ("put", f"/scheduling/api/tasks/{task.id}/update"), + ("post", "/admin/users/create"), + ("post", "/admin/users/1/deactivate"), + ): + response = getattr(anonymous, method)(url, json={}) + assert response.status_code in (302, 401, 403), ( + f"{url} answered an anonymous write with {response.status_code}" + ) + + +def test_a_project_created_through_the_api_belongs_to_the_caller(signed_in): + """A create route that trusts a company_id in the body would let a caller + plant a project in someone else's tenant.""" + client, _, user = signed_in + other = Company(name="Somebody Else") + db.session.add(other) + db.session.commit() + + client.post( + "/api/projects/create", + json={ + "name": "Planted", + "company_id": other.id, + "start_date": date.today().isoformat(), + "end_date": (date.today() + timedelta(days=30)).isoformat(), + }, + ) + + planted = Project.query.filter_by(name="Planted").first() + if planted is not None: + assert planted.company_id == user.company_id, ( + "a project was created in another company because the request body said so" + ) diff --git a/tests/test_static_integrity.py b/tests/test_static_integrity.py index bf56eb8..638be64 100644 --- a/tests/test_static_integrity.py +++ b/tests/test_static_integrity.py @@ -174,3 +174,36 @@ def test_the_known_gaps_are_still_real(): assert not fixed, ( f"{relative}::{class_name} now defines {sorted(fixed)} — remove them from KNOWN_MISSING" ) + + +# ── SQLAlchemy legacy API ──────────────────────────────────────────────── + + +def test_nothing_uses_the_legacy_query_get(): + """``Model.query.get(id)`` is legacy in SQLAlchemy 2.0 and removed in 2.1. + + The bound in pyproject is ``sqlalchemy<3``, so 2.1 arrives as a Dependabot + proposal and takes 22 call sites with it — on code nobody touched. This is + the third instance of that shape in this repository, after + ``db.engine.execute`` (removed in 2.0, and it had been failing every health + check since) and MPXJ renaming its Java package. Use + ``db.session.get(Model, id)``. + + ``get_or_404`` is Flask-SQLAlchemy's own and is not affected. + """ + import re + + offenders = [] + # Matches `Something.query.get(` but not `.query.get_or_404(`. + pattern = re.compile(r"\.query\.get\(") + + for path in _python_files(): + for number, line in enumerate(path.read_text(encoding="utf-8").splitlines(), 1): + if pattern.search(line): + relative = str(path.relative_to(REPO_ROOT)).replace("\\", "/") + offenders.append(f"{relative}:{number}") + + assert not offenders, ( + "Query.get() is removed in SQLAlchemy 2.1; use db.session.get(Model, id): " + + ", ".join(offenders) + )