Route + log for organization plan updates #9129
Conversation
📝 WalkthroughWalkthroughThis PR adds infrastructure for tracking organization pricing plan updates. It introduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
fm3
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
schema/schema.sql (1)
359-371: Consider adding a primary key and index for query performance.The
organization_plan_updatestable lacks a primary key. While it may function as an append-only log, having a primary key is a best practice for data integrity and potential future references. Additionally, consider adding an index on_organizationfor efficient lookups when listing plan updates.CREATE TABLE webknossos.organization_plan_updates( + _id TEXT CONSTRAINT _id_objectId CHECK (_id ~ '^[0-9a-f]{24}$') PRIMARY KEY DEFAULT webknossos.generate_object_id(), _organization TEXT NOT NULL, description TEXT DEFAULT NULL, pricingPlan webknossos.PRICING_PLANS DEFAULT NULL, paidUntil TIMESTAMPTZ DEFAULT NULL, paidUntilChanged BOOLEAN NOT NULL, -- bool is necessary because set to null is distinct from did not change includedUsers INTEGER DEFAULT NULL, includedUsersChanged BOOLEAN NOT NULL, -- bool is necessary because set to null is distinct from did not change includedStorage BIGINT DEFAULT NULL, includedStorageChanged BOOLEAN NOT NULL, -- bool is necessary because set to null is distinct from did not change created TIMESTAMPTZ NOT NULL DEFAULT NOW(), CONSTRAINT validOrganizationId CHECK (_organization ~* '^[A-Za-z0-9\-_. ]+$') );Add an index after the table definition:
CREATE INDEX ON webknossos.organization_plan_updates(_organization);app/controllers/OrganizationController.scala (1)
319-329: Consider adding logging for audit purposes.Given this is a sensitive operation (modifying pricing plans), consider adding a log statement similar to other privileged operations in this codebase (e.g.,
deleteaction at line 191)._ <- userService.assertIsSuperUser(request.identity) organization <- organizationDAO.findOne(request.body.organizationId) ?~> Messages( "organization.notFound", request.body.organizationId) ~> NOT_FOUND + _ = logger.info(s"Superuser ${request.identity._id} updating plan for organization ${organization._id}") _ <- organizationDAO.insertPlanUpdate(organization._id, request.body) _ <- organizationDAO.updatePlan(organization._id, request.body) } yield Okapp/models/organization/Organization.scala (1)
13-13: Avoid DAO depending oncontrollers.OrganizationPlanUpdatePulling a controller‑package type into the DAO inverts the usual layering (models/DAO should not depend on controllers). Consider moving
OrganizationPlanUpdateinto a sharedmodelsordtopackage and having both controller and DAO depend on that instead.schema/evolutions/146-organization-plan-updates.sql (1)
5-21: Consider indexing forfindPlanUpdatesquery
findPlanUpdatesqueries:
WHERE _organization = ?ORDER BY createdOn a table that may grow over time this can degrade to a full scan + sort. It would be worth adding an index such as:
CREATE INDEX organization_plan_updates_org_created_idx ON webknossos.organization_plan_updates (_organization, created);to keep the listing endpoint responsive as more updates accumulate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
app/controllers/OrganizationController.scala(3 hunks)app/models/organization/Organization.scala(2 hunks)app/models/user/UserService.scala(1 hunks)conf/webknossos.latest.routes(1 hunks)schema/evolutions/146-organization-plan-updates.sql(1 hunks)schema/evolutions/reversions/146-organization-plan-updates.sql(1 hunks)schema/schema.sql(3 hunks)unreleased_changes/9129.md(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
app/controllers/OrganizationController.scala (2)
util/src/main/scala/com/scalableminds/util/time/Instant.scala (3)
Instant(14-45)Instant(47-103)now(48-48)app/models/team/PricingPlan.scala (1)
PricingPlan(5-10)
app/models/user/UserService.scala (2)
util/src/main/scala/com/scalableminds/util/tools/Fox.scala (1)
assertTrue(192-198)app/models/analytics/AnalyticsService.scala (1)
isSuperUser(103-108)
🔇 Additional comments (9)
app/models/user/UserService.scala (1)
86-90: LGTM! Clean extraction of the super-user check logic.This refactor follows the same pattern used in
AnalyticsService.scalaand enables reuse of theisSuperUsercheck in the newlistPlanUpdatesendpoint without duplicating DAO calls.schema/evolutions/reversions/146-organization-plan-updates.sql (1)
1-9: LGTM! Standard reversion script following established patterns.The script correctly validates the schema version, drops the new table, and reverts the version number within a transaction.
unreleased_changes/9129.md (1)
1-5: LGTM! Changelog entry is clear and complete.conf/webknossos.latest.routes (1)
258-259: LGTM! Routes are well-placed and follow existing naming conventions.The new endpoints are logically grouped with other pricing-related routes.
schema/schema.sql (2)
24-24: Schema version increment is correct.
954-955: Foreign key constraint is correctly defined with ON DELETE CASCADE.app/controllers/OrganizationController.scala (2)
331-338: Verify intended scope for superuser access.Currently,
listPlanUpdatesonly returns updates for the requesting user's current organization. Since superusers can update any organization's plan (viaupdatePlan), should they also be able to list updates for any organization (perhaps via an optionalorganizationIdquery parameter)?If the current behavior is intentional (superusers see only their org's updates unless they switch), this is fine. Otherwise, consider:
- def listPlanUpdates: Action[AnyContent] = + def listPlanUpdates(organizationId: Option[String]): Action[AnyContent] = sil.SecuredAction.async { implicit request => for { isSuperUser <- userService.isSuperUser(request.identity._multiUser) _ <- Fox.fromBool(isSuperUser || request.identity.isAdmin) ?~> "organization.listPlanUpdates.onlyAdmin" - planUpdates <- organizationDAO.findPlanUpdates(request.identity._organization) + targetOrgId = if (isSuperUser) organizationId.getOrElse(request.identity._organization) + else request.identity._organization + _ <- Fox.fromBool(!organizationId.isDefined || isSuperUser) ?~> "notAllowed" ~> FORBIDDEN + planUpdates <- organizationDAO.findPlanUpdates(targetOrgId) } yield Ok(Json.toJson(planUpdates)) }
43-46: TristateOptionJsonHelper correctly handles tristate Option parsing.The
tristateOptionParsingconfiguration properly differentiates between absent JSON keys (producesNone) and explicitnullvalues (producesSome(None)). The case class default values ofSome(None)for fields likepaidUntil,includedUsers, andincludedStorageBytesare only used for programmatic construction; JSON deserialization is controlled entirely by theInvertedDefaultHandlerlogic, which treats absent keys and null values distinctly.app/models/organization/Organization.scala (1)
346-375: Plan update parsing and change‑flag semantics look consistent
parsePlanUpdatecorrectly reconstructs the “changed vs unchanged vs set‑to‑null” semantics:
paidUntil = Some(Some(ts))→ changed to concrete date.paidUntil = Some(None)→ changed to unlimited (NULL).paidUntil = None→ unchanged (perpaidUntilChangedflag).The same pattern for
includedUsers/includedStoragematches the described API behavior, andfindPlanUpdatesselects all columns in the order defined in the evolution, so mapping toOrganizationPlanUpdatesRowshould be stable.
| CREATE TABLE webknossos.organization_plan_updates( | ||
| _organization TEXT NOT NULL, | ||
| description TEXT DEFAULT NULL, | ||
| pricingPlan webknossos.PRICING_PLANS DEFAULT NULL, | ||
| paidUntil TIMESTAMPTZ DEFAULT NULL, | ||
| paidUntilChanged BOOLEAN NOT NULL, -- bool is necessary because set to null is distinct from did not change | ||
| includedUsers INTEGER DEFAULT NULL, | ||
| includedUsersChanged BOOLEAN NOT NULL, -- bool is necessary because set to null is distinct from did not change | ||
| includedStorage BIGINT DEFAULT NULL, | ||
| includedStorageChanged BOOLEAN NOT NULL, -- bool is necessary because set to null is distinct from did not change | ||
| created TIMESTAMPTZ NOT NULL DEFAULT NOW(), | ||
| CONSTRAINT validOrganizationId CHECK (_organization ~* '^[A-Za-z0-9\-_. ]+$') | ||
| ); | ||
|
|
||
| ALTER TABLE webknossos.organization_plan_updates | ||
| ADD CONSTRAINT organization_ref FOREIGN KEY(_organization) REFERENCES webknossos.organizations(_id) ON DELETE CASCADE DEFERRABLE; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find references to organization_plan_updates table in codebase
rg "organization_plan_updates" --type sql --type scala --type java --type ts --type js -A 5 -B 2Repository: scalableminds/webknossos
Length of output: 5396
🏁 Script executed:
# Search for related issues, PRs, or requirements documentation
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "*.rst" \) | xargs rg "organization_plan_updates\|pricing.*plan.*updates\|audit.*organization" 2>/dev/null || trueRepository: scalableminds/webknossos
Length of output: 50
🏁 Script executed:
# Check for any DAO or SQL query files related to organization plans
fd -e scala -e java -e ts -e js | xargs rg "organization.*plan|PlanUpdate" --type scala --type java --type ts --type js -l 2>/dev/null | head -20Repository: scalableminds/webknossos
Length of output: 543
🏁 Script executed:
# Look at the git history of this migration file
git log --oneline --all -- "schema/evolutions/146-organization-plan-updates.sql" 2>/dev/null | head -10Repository: scalableminds/webknossos
Length of output: 129
🏁 Script executed:
# Check if there are any related migrations before or after 146
ls -la schema/evolutions/14[5-7]*.sql 2>/dev/null || ls -la schema/evolutions/ | grep -E "14[5-7]"Repository: scalableminds/webknossos
Length of output: 264
🏁 Script executed:
# Get more context on the Organization.scala file, especially around insertPlanUpdate and getPlanUpdates
sed -n '1,50p' app/models/organization/Organization.scalaRepository: scalableminds/webknossos
Length of output: 1683
🏁 Script executed:
# Find the full insertPlanUpdate and related methods
rg -A 20 "insertPlanUpdate" app/models/organization/Organization.scalaRepository: scalableminds/webknossos
Length of output: 1117
🏁 Script executed:
# Find any references to "actor" or "changedBy" in organization-related files
rg "actor|changedBy|performedBy|performedAt|changed_by" app/models/organization/ --type scalaRepository: scalableminds/webknossos
Length of output: 337
🏁 Script executed:
# Search for audit-related comments or documentation in the codebase
rg "audit.*log|audit.*table" --type scala --type sql -iRepository: scalableminds/webknossos
Length of output: 50
🏁 Script executed:
# Check the OrganizationController to see how insertPlanUpdate is used
rg -B 5 -A 10 "insertPlanUpdate" app/controllers/OrganizationController.scalaRepository: scalableminds/webknossos
Length of output: 873
🏁 Script executed:
# Look for any PR or issue references in commit messages related to organization plan updates
git log --all --oneline --grep="plan.*update\|organization.*plan" 2>/dev/null | head -20Repository: scalableminds/webknossos
Length of output: 566
🏁 Script executed:
# Search for the original PR number or issue
git log --all --format="%H %s" -- schema/evolutions/146-organization-plan-updates.sql | head -5Repository: scalableminds/webknossos
Length of output: 161
🏁 Script executed:
# Get commit details for the merge commit
git show c052a7af --stat 2>/dev/null | head -50Repository: scalableminds/webknossos
Length of output: 3019
🏁 Script executed:
# Look for TODOs or FIXMEs related to organization plan updates
rg "TODO|FIXME" app/models/organization/ app/controllers/OrganizationController.scala -B 2 -A 2Repository: scalableminds/webknossos
Length of output: 50
🏁 Script executed:
# Check the OrganizationPlanUpdate case class to understand the structure
rg -B 5 -A 15 "case class OrganizationPlanUpdate" app/controllers/Repository: scalableminds/webknossos
Length of output: 2049
🏁 Script executed:
# Search for any documentation or requirements about audit logging
find . -type f \( -name "*.md" -o -name "*.txt" \) | xargs rg -l "plan.*update\|organization.*audit" 2>/dev/null | head -10Repository: scalableminds/webknossos
Length of output: 50
🏁 Script executed:
# Look at the full listPlanUpdates implementation to see what data is returned
rg -B 3 -A 20 "def listPlanUpdates|def findPlanUpdates" app/models/organization/Organization.scalaRepository: scalableminds/webknossos
Length of output: 708
Missing actor and old values prevent this table from serving as a complete audit log
The table stores only new values plus boolean flags indicating what changed, with no column recording which superuser performed each update or previous values. The findPlanUpdates() query filters by _organization and orders by created without a supporting index.
To make this table self-contained, add:
- A column for the acting superuser (e.g.,
_changedByreferencing the multi-user table) - Either "old" value columns or a
JSONBdiff field capturing before/after state for each changed property - An index on
(_organization, created)to optimize the primary query pattern
🤖 Prompt for AI Agents
In schema/evolutions/146-organization-plan-updates.sql around lines 5 to 21, the
table only records new values and boolean change flags but lacks the acting
superuser and previous-state data and has no index for the primary query
pattern; add a _changedBy column that references the multi-user table (FK to the
appropriate users table), add either explicit "<field>_old" columns
(paidUntil_old, includedUsers_old, includedStorage_old, pricingPlan_old,
description_old) or a single JSONB column (e.g., diff JSONB) to capture
before/after values, and create an index on (_organization, created) to optimize
findPlanUpdates() queries; ensure the new FK is nullable if needed and the
chosen old-values field(s) have appropriate types matching the new-value
columns.
There was a problem hiding this comment.
@normanrz do you think we should also store old values, and the changedBy reference? My guess would be that the current implementation is sufficient for your use case?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| import models.organization.{FreeCreditTransactionService, OrganizationDAO, OrganizationService} | ||
| import models.user.{InviteDAO, MultiUserDAO, UserDAO, UserService} | ||
| import models.team.PricingPlan | ||
| import models.team.PricingPlan.PricingPlan |
|
|
||
| import scala.concurrent.ExecutionContext | ||
|
|
||
| case class OrganizationPlanUpdate( |
Superusers can now edit organization pricing plans via API.
Admins and superusers can list organization pricing plan updates.
Example:
curl 'http://localhost:9000/api/pricing/updatePlan' -X 'POST' --data '{"organizationId":"sample_organization","description":"a description!","paidUntil":"2025-12-12T00:00:00Z","includedUsers": null}' -H 'X-Auth-Token: secretSampleUserToken' --header "Content-Type: application/json"Note that for the fields
paidUntil,includedUsersandincludedStorageBytesthe valuenullin the json means that the value is updated tonull(which means unlimited). If the key is absent from the json, the value is unchanged. Description and pricingPlan can also be omitted, meaning unchanged.in the update list route
GET http://localhost:9000/api/pricing/planUpdates, the same differentiation between null and absent applies for the response body.URL of deployed dev instance (used for testing):
Steps to test:
Issues:
$PR_NUMBER.mdfile inunreleased_changesor use./tools/create-changelog-entry.py)