Harden the four remaining gaps - #21
Merged
Merged
Conversation
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/<id>/tasks/create and /projects/<id>/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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The four things I flagged as outstanding. Three of them turned out to be hiding live bugs.
1. A breakage with a known arrival date
22 call sites used
Query.get()— legacy in SQLAlchemy 2.0, removed in 2.1 — with the bound atsqlalchemy<3. Dependabot would have proposed 2.1 and broken CI on code nobody touched. Third instance of that shape here, afterdb.engine.executeand MPXJ's package rename, sotests/test_static_integrity.pynow fails on the next one.Deprecation warnings across the suite: 188 → 29.
2. Two admin blueprints, and the bug they hid
blueprints/admin.pyat/adminandadmin/user_management.pyat/managementboth implementedmanage_users,create_userandcompany_settingsagainst the same templates, redirecting into each other.The two
create_usercopies disagreed about the role field:The form sends
ADMIN. SoPOST /admin/users/createraisedValueError: 'ADMIN' is not a valid UserRoleon every submission, while the/managementcopy worked. Verified before and after.One blueprint now.
/management/*returns 308 to/admin/*— not 302, because a 302 turns a POST into a GET and silently drops the form body.3. The untested half
32 mutating routes had never been exercised. Walking them found five that answered an empty POST with a 500 — all writing before validating, or reading
request.jsonon a non-JSON body and letting a bareexceptconvert a client error into a server one:/auth/register·/azure/configure/<id>·/api/projects/quick-task·/projects/<id>/tasks/create·/projects/<id>/resources/createRegistration let a stranger into an existing tenant
Worse than the 500. It looked the company up by name and, when it found one, attached the new user to it:
Anyone who guessed a customer's company name received a scheduler account inside that tenant, and with it every project the tenant owns. Registration creates a company now; joining one is an invitation an administrator issues.
Tenant isolation is tested, not assumed
A rival-company admin fixture (deliberately an admin — the bypass this repo had before was specifically that admins skipped the company check) asserts cross-tenant writes are refused and that the victim's rows are unchanged afterwards. Status codes can lie; the tests check the database.
4. The executive dashboard
The last facade. It generated twelve months of revenue from a 2,500,000 base on a 2% growth curve, a 12% profit margin, four hardcoded US regions, and per-size bands fixed at 15.2% margin / 95.8% complete — identical for every company, carrying a
simulatedflag a chart is free to ignore.Now measured, with an explicit empty state and a documented extension path, as asked:
Revenue and cost come from
TransactionandInvoice, reconciled rather than summed so a deployment that does both isn't double-counted. Geography groups byProject.location; sector by the construction template a project came from, withTEMPLATE_SECTORSand a note on adding a realsectorcolumn. Anything unsupported isNonewith a reason — never zero, because zero profit and unknown profit are different statements.Bucketed by month in Python, not
func.strftime— that is SQLite-only and would have worked in development and failed on the PostgreSQL this deploys to.One I caught in my own work
My first version reported
avg_margin: 86.1for the demo project. That project is four months into a two-year job having spent 14% of its budget —(budget - spend) / budgeton unfinished work is money not yet spent, and calling it margin made the least advanced project look like the most profitable. Realised margin (completed work only) is now separate frombudget_consumed_percent, andbudget_varianceignores work still in flight for the same reason.Verification
359 passed, 2 skipped (was 327).
ruff format --checkandruff checkclean.🤖 Generated with Claude Code