Credit system for jobs #8352
Credit system for jobs #8352
Conversation
- add first version of data model - add DAO and WIP service
…dit-system-for-jobs
…dit-system-for-jobs
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request introduces a comprehensive credit transaction system for WEBKNOSSOS, enabling organizations to manage and track credit usage across various computational tasks. The implementation spans multiple components including controllers, services, models, and frontend interfaces. Key additions include credit transaction tracking, cost calculations for different job types, email notifications for credit requests, and mechanisms to validate and manage organizational credit balances. The system supports operations like reserving, charging, and refunding credits while ensuring organizations have paid plans. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (30)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (1)
551-637: Console log & placeholder link
- Consider removing or replacing the
console.logwith a structured logger to keep logs clean in production.- The user prompt to “order more credits !here!” lacks an actual link. Provide a proper URL to improve user clarity.
app/controllers/CreditTransactionController.scala (1)
1-29: Consider returning descriptive responses and adding logging.
Overall, the controller setup is clean and leverages Silhouette for security checks. However, returning onlyOkmight limit visibility into the outcome. Consider returning a JSON object containing status and transaction details, and potentially add logs for better traceability.app/models/organization/CreditTransactionService.scala (3)
21-36: Reserve strategy might need a balance check.
reserveCreditssets the transaction’s state toPendingbut does not verify if the organization actually has sufficient credits. If negative balances are disallowed, consider an additional check or a transaction-based logic that fails early when the balance is insufficient.
50-56: Consider meaningful logs or audits for refunds.
refundTransactionForJobsilently performs a refund. Logging or auditing the transaction details can help diagnose issues and provide traceability, particularly for financial operations.
61-76: Avoid exposing internal fields inpublicWrites.
Public-facing JSON might not need_id,_organization, or fields likeisDeleted. Consider filtering or redacting sensitive fields to reduce leakage.app/models/organization/CreditTransaction.scala (5)
1-26: Revisit naming of underscore-prefixed fields.
The_organizationand_paidJobfields suggest they are internal. Consider removing underscores or using Scala’s conventional naming to increase clarity.
64-70: Consider pagination or filtering for large data sets.
findAllreturns every credit transaction for the organization. As the table grows, loading all rows will become expensive. Implementing pagination or filtering can improve performance.
89-104: Enforce a check to prevent overspending.
insertNewPendingTransactiondoes not confirm if the organization has enough credits. If overspending is disallowed, consider verifying or reserving the balance here.
105-115: Clarify write-access checks.
There’s a TODO comment to “check write access.” Ensure that only privileged users or owners can update the_paid_job. This is critical to prevent unauthorized linking of jobs to transactions.
116-131: Consolidate transaction insertion logic.
insertTransactionlargely duplicates code frominsertNewPendingTransaction. Consider refactoring to reduce duplication and ease maintenance.app/controllers/JobController.scala (3)
90-90: Clarify the TODO or remove it
The inline TODO about adding paid credits to job public writes should be resolved or removed to avoid confusion. If credit usage information in job metadata is needed, consider including it in the job’s response for improved visibility.
300-306: Refactor duplicated credit transaction flow
The repeated code that reserves credits and attaches them to a job suggests an opportunity to abstract the logic in a helper function. This will improve maintainability and reduce the risk of synchronization errors.
327-330: Ensure dataset bounding box is always defined
If a dataset’s bounding box is missing, this code path fails with “dataset.boundingBox.unset.” Consider providing user-facing guidance or fallback logic in such cases.app/controllers/WKRemoteWorkerController.scala (1)
87-92: Conditional completion/refunding logic is clear, but consider concurrency checks.
Ensure that repeated or conflicting updates (e.g., multiple state transitions) cannot create inconsistent credit records. It may be beneficial to confirm job ownership or existence of a valid transaction before completing or refunding.app/models/organization/OrganizationService.scala (2)
53-54: Avoid using GlobalAccessContext if possible.
TheTODOcomment indicates a plan to refactor this to a more localized context. Consider passing the context as a parameter to keep the design consistent.
183-187: Paid plan check is good, but refine the error message.
TheTODOsuggests that the error message might be improved for clarity. Let me know if you'd like assistance with a user-friendly error.app/mail/DefaultMails.scala (2)
156-163: Order credits mail method is well-structured, but consider concurrency.
It might be worthwhile to confirm that the user’s email is valid or that the user is part of a paid organization before sending the mail.
164-172: Order credits request mail targets [email protected].
Everything looks fine, though consider mentioning the organization ID in the subject or body to help the support team.frontend/javascripts/admin/organization/upgrade_plan_modal.tsx (2)
21-21: Remove unused import.
useEffectis imported but never used. Removing it will keep the code clean and prevent confusion.- import { useEffect, useRef, useState } from "react"; + import { useRef, useState } from "react";
300-379: Refine naming and remove magic numbers.
- The variable
requestedUsersinhandleOrderCreditsis actually capturing the number of credits. Consider renaming it tocreditsToOrderfor clarity.- The modal's default value is set to
1, but theminis set to0. This slight mismatch can cause confusion if a user enters 0. Verify if a minimum of1is more appropriate.- You’ve noted a TODO about the hard-coded costs of 5€/5.75$. Storing these in configuration (similar to other cost parameters) or providing a well-documented constant will make the system more flexible and maintainable.
- Consider basic client-side validation (e.g., disallow negative values) and showing an error if
parseIntfails.app/models/job/JobService.scala (1)
227-234: Avoid throwing exceptions in service methods.By throwing an
IllegalArgumentException, you risk unhandled exceptions in asynchronous flows. Consider returning aFox.failure(...), anEither, or similar to keep errors consistent with the rest of the codebase.frontend/javascripts/admin/organization/organization_cards.tsx (3)
106-113: Address nested layout for improved semantic HTML.Using
<ul>directly inside a<p>can lead to HTML warnings. Wrapping the<ul>in a<span>is a workaround, but consider removing the<p>or using<div>to maintain clean structure.
256-260: Use a button for better accessibility.Wrapping text in a
<span>withonClickcan hinder keyboard navigation and screen-reader support. Consider usingButtonor a clickable link component instead of<span>to improve accessibility and UX.- <span key="buyMoreCreditsAction" onClick={UpgradePricingPlanModal.orderWebknossosCredits}> - Buy more credits - </span> + <Button type="link" onClick={UpgradePricingPlanModal.orderWebknossosCredits}> + Buy more credits + </Button>
263-312: Validate credit purchase visibility and streamline layout.
- Make sure only users with sufficient permissions can see and click the “Buy more credits” card. Otherwise, unauthorized users might attempt credit orders.
- Consider extracting the repeated card/row structure into smaller components for maintainability and consistent styling in the plan dashboard.
frontend/javascripts/types/api_flow_types.ts (1)
633-633: Consider nullable credit balanceConsider whether organizations might start with no credits at all and if so, whether
creditBalanceshould be nullable (e.g.,number | null). Otherwise, declaring it asnumberis fine if every organization is guaranteed to have a balance.frontend/javascripts/admin/admin_rest_api.ts (1)
1830-1835: Use request body instead of query parameters for POST requests.Although this implementation works, sending
requestedCreditswith a JSON body clarifies the request for future maintainers and avoids unusual practices. It also simplifies potential future extensions.Consider applying this diff:
-export async function sendOrderCreditsEmail(requestedCredits: number): Promise<void> { - return Request.receiveJSON(`/api/pricing/requestCredits?requestedCredits=${requestedCredits}`, { - method: "POST", - }); -} +export async function sendOrderCreditsEmail(requestedCredits: number): Promise<void> { + return Request.sendJSONReceiveJSON(`/api/pricing/requestCredits`, { + method: "POST", + data: { requestedCredits }, + }); +}app/views/mail/orderCredits.scala.html (1)
3-10: Consider extracting copy for future localization or styling changes.The body content is clear and functional. For large-scale deployments, extracting strings to a message file or resource bundle can simplify translations and maintain consistency.
app/views/mail/orderCreditsRequest.scala.html (1)
3-11: Improve the email body’s grammar and clarity.Consider making this line clearer:
-<p>There is a purchase WEBKNOSSOS credits.</p> +<p>There is a request to purchase additional WEBKNOSSOS credits.</p>frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md (1)
Line range hint
71-83: LGTM! Consider adding documentation for the cost parameters.The new cost parameters for alignment and inference operations are correctly added to the feature toggles. These values align with the credit system implementation.
Consider adding a comment in the test file explaining what these costs represent (e.g., cost per gigavoxel for different operations) and how they relate to the credit system.
tools/postgres/schema.sql (1)
352-368: Consider additional indexes for credit transaction queries.The table structure is well-designed, but could benefit from additional indexes to optimize common queries.
Consider adding the following indexes:
CREATE INDEX ON webknossos.organization_credit_transactions(created_at); CREATE INDEX ON webknossos.organization_credit_transactions(_organization, state); CREATE INDEX ON webknossos.organization_credit_transactions(expiration_date) WHERE state = 'Completed';These indexes would improve performance for:
- Historical transaction queries
- Organization-specific transaction status queries
- Expired credit identification
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.snapis excluded by!**/*.snap
📒 Files selected for processing (26)
app/controllers/CreditTransactionController.scala(1 hunks)app/controllers/JobController.scala(10 hunks)app/controllers/OrganizationController.scala(1 hunks)app/controllers/WKRemoteWorkerController.scala(3 hunks)app/mail/DefaultMails.scala(1 hunks)app/models/job/JobService.scala(1 hunks)app/models/organization/CreditTransaction.scala(1 hunks)app/models/organization/CreditTransactionService.scala(1 hunks)app/models/organization/CreditTransactionState.scala(1 hunks)app/models/organization/OrganizationService.scala(4 hunks)app/models/team/PricingPlan.scala(1 hunks)app/utils/WkConf.scala(1 hunks)app/views/mail/orderCredits.scala.html(1 hunks)app/views/mail/orderCreditsRequest.scala.html(1 hunks)conf/application.conf(1 hunks)conf/messages(1 hunks)conf/webknossos.latest.routes(2 hunks)frontend/javascripts/admin/admin_rest_api.ts(1 hunks)frontend/javascripts/admin/organization/organization_cards.tsx(2 hunks)frontend/javascripts/admin/organization/upgrade_plan_modal.tsx(3 hunks)frontend/javascripts/messages.tsx(1 hunks)frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx(18 hunks)frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md(2 hunks)frontend/javascripts/types/api_flow_types.ts(1 hunks)tools/postgres/schema.sql(7 hunks)util/src/main/scala/com/scalableminds/util/time/Instant.scala(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/models/organization/CreditTransactionState.scala
🔇 Additional comments (53)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (19)
31-31: Import usage is appropriate
These changes look correct and necessary for formatting voxel volumes.
50-50: Import usage is appropriate
ImportingBoundingBoxappears correct for bounding box calculations.
111-111: Optional property addition
Introducing an optionaljobCreditCostsPerGVxproperty inStartJobFormPropsis consistent with dynamic cost calculations.
118-118: New property for volume rendering
AddingshowVolumetoBoundingBoxSelectionPropsis a clear, self-explanatory approach for controlling volume display.
Line range hint
123-142: Rendering bounding box & volume
PassingshowVolumeto conditionally display formatted volume inrenderUserBoundingBoxis clear and intuitive. Code logic is consistent with the bounding box usage.
160-160: Default parameter value
The defaultfalseforshowVolumeprevents accidental volume display.
166-166: Type definition
UsingshowVolume: boolean;aligns properly with the usage and default.
185-185: Prop forwarded to bounding box render
ForwardingshowVolumemakes the bounding box rendering more flexible.
196-196: Consistent default parameter
Again, defaultingshowVolumetofalseensures backward compatibility.
259-259: Prop propagation
Explicitly passingshowVolume={showVolume}to theBoundingBoxSelectionis clear.
536-549: Check rounding logic
UsingMath.ceil(jobCosts * 100)enforces upward rounding. Verify that this behavior matches the intended billing policy, as it may differ from a standard round.
643-651: Prop destructuring
DestructuringjobCreditCostsPerGVxfrom props is consistent with the rest of the code.
674-682: Bounding box selection logic
The ternary fallback forselectedBoundingBoxis sensible. This ensures the bounding box is derived properly for cost calculations.
846-846: Feature-based cost retrieval
PullingneuronInferralCostsPerGVxfromfeaturesis consistent with the global approach for dynamic pricing.
856-856: Passing cost prop
SendingneuronInferralCostsPerGVxtoStartJobFormproperly links to the credit-based pricing system.
884-884: Feature-based cost retrieval
Similarly, pullingmitochondriaInferralCostsPerGVxis consistent.
894-894: Passing cost prop
ProvidingjobCreditCostsPerGVx={mitochondriaInferralCostsPerGVx}correctly configures the segmentation form.
999-999: Feature-based cost retrieval
GettingalignmentCostsPerGVxfromfeatures()follows the established pattern.
1012-1012: Integration of alignment cost
Aligning sections also consumes credits, usingalignmentCostsPerGVxfor cost calculation is consistent.app/controllers/CreditTransactionController.scala (1)
57-66: Validate transaction state before refunding.
The refund action checks if the transaction belongs to the organization but not whether the transaction is still pending or already refunded. Consider adding a state check to prevent multiple refunds or refunds for completed transactions.app/models/organization/CreditTransactionService.scala (3)
17-20: Double-check negative balance scenarios.
WhilehasEnoughCreditsreturns whether the balance is >=creditsToSpent, no further check prevents negative balances if multiple transactions run concurrently. You may want to enforce stronger locks or a transaction-based approach to ensure consistency.
43-49: Confirm job-to-transaction mapping consistency.
completeTransactionOfJobdepends on the transaction for a givenjobId. If there are multiple transactions per job, this could lead to unexpected results. Ensure only one pending transaction per job or handle parallel completions gracefully.
57-60: Validate job ID assignment for concurrency safety.
addJobIdToTransactionupdates an existing transaction, but multiple threads could map different job IDs if the transaction is used concurrently. Consider locking or transaction isolation if that’s a concern.app/models/organization/CreditTransaction.scala (4)
28-56: DAO extension is well-structured.
The DAO cleanly extendsSQLDAO, providing a consistent pattern for database interactions. Good approach for separation of concerns.
58-63: Review access control for read operations.
readAccessQbroadens access if the user is super user, but also includes anOR TRUE in (SELECT isSuperUser...)snippet. Verify that this logic doesn’t inadvertently grant access for unauthorized reads.
71-79: Check for non-existent transaction IDs.
findOnecallsparseFirstwhich may fail if the ID doesn’t exist. Make sure you handle the resulting Fox failure in any calling code.
171-179: Ensure single transaction retrieval per job.
findTransactionForJobretrieves only the first match, but multiple transactions per job could exist in edge cases. Decide whether that scenario is allowed, and if so, handle it explicitly.app/controllers/JobController.scala (7)
9-9: Import usage for credit management
It's good to see the newCreditTransactionServiceintegrated here. Ensure that calls to it are always made from secure, authenticated endpoints.
67-68: Constructor injection verified
InjectingorganizationServiceandcreditTransactionServiceis consistent with the rest of the codebase. Make sure to include unit tests that validate these dependencies.
239-248: Bounding box check vs. credits
There’s a TODO suggesting that the bounding box check should be disabled since credits are already charged. Verify whether bounding box restrictions serve an additional purpose (e.g., resource usage constraints). Also remember to add any missing error messages to messages.conf and ensure test coverage for boundary conditions.
285-289: Revisit bounding box logic for mitochondria inference
Similar to neuron inference, there is a TODO to potentially disable the bounding box check. Verify whether this is truly desired, as bounding box checks can prevent resource overuse. Also ensure any error messages are finalized in messages.conf.
333-334: Confirm error messaging for insufficient credits
A TODO indicates adding a messages.conf entry for “job.notEnoughCredits.” Ensure these errors are user-friendly and tested for large bounding box scenarios.
493-493: Verify watermark requirement for paid plans
This logic forces paid plans to include a watermark. Generally, paid subscriptions remove or relax watermark requirements. Double-check if this is intended or if the condition should be inverted.
496-496: Confirm SD resolution requirement for paid plans
For paid customers, requiring SD resolution might be counterintuitive if higher-resolution outputs are typically a premium feature. Confirm that this constraint is correct or invert the condition if needed.app/models/team/PricingPlan.scala (1)
8-9: Distinguish trial vs. paid plans
The current check lumps trial plans together with fully paid tiers. If a trial is not truly paid, consider updating the condition to excludeTeam_TrialorPower_Trial. Otherwise, clarify that trials are regarded as paid for the application’s logic.util/src/main/scala/com/scalableminds/util/time/Instant.scala (1)
62-63: Be mindful of date granularity
java.sql.Datedoes not store time components, so converting it to an epoch millisecondInstantmay produce unexpected times at midnight UTC. Ensure all consumers of this method are aware of the day-only precision.app/controllers/WKRemoteWorkerController.scala (2)
12-12: Import statement looks good.
No issues at this point.
24-24: New constructor parameter injection is appropriate.
This parameter dependency helps integrate credit transactions without cluttering the controller's code.app/models/organization/OrganizationService.scala (2)
25-25: Constructor extension for credit transaction handling is correct.
AddingcreditTransactionDAOhere is a logical move to keep all organization-related operations encapsulated.
68-69: Displaying owner name and credit balance is helpful.
Check whether any Personally Identifiable Information (PII) might be exposed unintentionally. If safe, this addition is clear and beneficial.conf/application.conf (1)
156-158: Cost configuration addition is clear.
Ensure the numeric values are correctly parsed as doubles and consumed appropriately for cost calculations.frontend/javascripts/admin/organization/upgrade_plan_modal.tsx (1)
9-9: Good addition for email sending.The new import of
sendOrderCreditsEmailis properly utilized inhandleOrderCreditsbelow. No issues found here.app/models/job/JobService.scala (2)
242-247: Ensure fractional bounding boxes are intended.Dividing the parsed bounding box by
parsedMagmay yield non-integer coordinates. Confirm that fractional bounding boxes are valid in your logic. If only integer voxels are acceptable, enforce rounding or add domain checks.
222-222: Double-check bounding box validity.Ensure zero or negative dimensions for boundingBox or mag are properly validated. Consider validating further (e.g., boundingBox corners or negative extents) to avoid unexpected behaviors.
✅ Verification successful
Bounding box validation is properly implemented.
The codebase already has comprehensive validation for bounding box dimensions:
- Non-negative width/height/depth enforced by parsing
- Zero/negative dimensions checked via
isEmpty- Volume and edge length limits prevent unreasonable extents
- Negative coordinates are properly supported
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for parseBoundingBoxWithMagOpt calls to confirm usage rg -A 3 'parseBoundingBoxWithMagOpt'Length of output: 2080
Script:
#!/bin/bash # Search for BoundingBox class and fromLiteral implementation ast-grep --pattern 'object BoundingBox { $$$ def fromLiteral($$$) { $$$ } $$$ }' # Also search for any BoundingBox validation related code rg -A 5 'BoundingBox.*valid|valid.*BoundingBox' # Search for Vec3Int fromMagLiteral implementation ast-grep --pattern 'object Vec3Int { $$$ def fromMagLiteral($$$) { $$$ } $$$ }'Length of output: 5404
Script:
#!/bin/bash # Search for BoundingBox class implementation rg -A 10 'case class BoundingBox|object BoundingBox' # Search for specific validation checks within BoundingBox rg -A 5 'width|height|depth.*[<=>]|[<=>].*width|height|depth'Length of output: 81972
conf/messages (1)
43-43: Consistent and user-focused messagingThe newly added error message is well-phrased and aligns with the existing message style, clearly explaining that only the organization owner can order credits. This helps users quickly understand the authorization requirements.
frontend/javascripts/messages.tsx (1)
495-496: Accurate feedback for credit requestsAdding
"organization.credit_request_sent"clarifies to users that their credit request email was sent. The message is concise and consistent with the rest of the file’s user-facing notifications.conf/webknossos.latest.routes (2)
249-249: Validate and handle potential edge cases for requested credits.The new route
POST /pricing/requestCreditsrelies on the query parameterrequestedCredits. Consider validating its value (e.g., ensuring it’s non-negative and within expected upper bounds) to avoid potential abuse or data inconsistencies.
294-296: Confirm access control checks for superuser-only credit transactions.Ensure that these newly introduced endpoints (
POST /creditTransactions/chargeUpCreditsandPATCH /creditTransactions/:transactionId) feature robust access control, restricting usage to super users or authorized admins. This prevents unauthorized credit modifications or refunds.app/views/mail/orderCredits.scala.html (1)
1-2: Template parameters look fine.No immediate concerns. Play’s template engine will automatically escape the supplied arguments, minimizing security issues like XSS if they are user-provided.
app/views/mail/orderCreditsRequest.scala.html (1)
1-2: Signature parameters appear correct.All parameters are strongly typed, and name/email are inserted safely in the Play template. This is good for maintainability and security.
tools/postgres/schema.sql (3)
23-23: LGTM! Schema version bump is appropriate.The schema version is correctly incremented for the new credit system changes.
1092-1093: Verify the credit revocation scheduling approach.The cron schedule is commented out. We need to confirm if this is intentional or if it should be enabled for production.
Please clarify:
- Should this be uncommented for production?
- Is pg_cron the preferred scheduling method, or should we use an external scheduler?
- Have we considered timezone implications for the 00:01 schedule?
998-998:⚠️ Potential issueCritical: Address the TODO comment about revoked free credits.
This TODO indicates a potential issue with credit revocation that needs to be fixed before deployment.
Let's verify the current behavior:
| def chargeUpCredits(organizationId: String, | ||
| creditAmount: Int, | ||
| moneySpent: String, | ||
| comment: Option[String], | ||
| expiresAt: Option[String]): Action[AnyContent] = sil.SecuredAction.async { implicit request => | ||
| for { | ||
| _ <- userService.assertIsSuperUser(request.identity) ?~> "Only super users can charge up credits" | ||
| moneySpentInDecimal <- tryo(BigDecimal(moneySpent)) ?~> s"moneySpent $moneySpent is not a valid decimal" | ||
| _ <- bool2Fox(moneySpentInDecimal > 0) ?~> "moneySpent must be a positive number" | ||
| _ <- bool2Fox(creditAmount > 0) ?~> "creditAmount must be a positive number" | ||
| commentNoOptional = comment.getOrElse(s"Charge up for $creditAmount credits for $moneySpent Euro.") | ||
| _ <- organizationService.ensureOrganizationHasPaidPlan(organizationId) | ||
| expirationDateOpt <- Fox.runOptional(expiresAt)(Instant.fromString) | ||
| chargeUpTransaction = CreditTransaction( | ||
| ObjectId.generate, | ||
| organizationId, | ||
| BigDecimal(creditAmount), | ||
| Some(moneySpentInDecimal), | ||
| commentNoOptional, | ||
| None, | ||
| CreditTransactionState.Completed, | ||
| expirationDateOpt | ||
| ) | ||
| _ <- creditTransactionService.doCreditTransaction(chargeUpTransaction) | ||
| } yield Ok | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential concurrency issues and partial rollback.
Though the code validates user permissions and input boundaries (e.g., creditAmount > 0), concurrent invocations of chargeUpCredits could lead to race conditions if the design expects real-time balance tracking. Consider using database transactions or locks for robust concurrency handling.
There was a problem hiding this comment.
This is not currently relevant to our domain. Charge-ups happen rarely and in case a user reads outdated values, this isn't a problem to use as long as the data in the database is consistent.
There was a problem hiding this comment.
@MichaelBuessemeyer Understood, thank you for the clarification. I'll note this for future reviews.
✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/controllers/CreditTransactionController.scala:30-55
Timestamp: 2025-01-27T12:51:23.807Z
Learning: In the WEBKNOSSOS credit system, charge-up operations are infrequent and temporary outdated credit balance reads are acceptable. Database-level consistency is sufficient, and additional concurrency control mechanisms are not required.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| package models.organization | ||
|
|
||
| import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContext} | ||
| import com.scalableminds.util.objectid.ObjectId | ||
| import com.scalableminds.util.tools.{Fox, FoxImplicits} | ||
| import com.typesafe.scalalogging.LazyLogging | ||
| import play.api.libs.json.{JsObject, Json} | ||
|
|
||
| import javax.inject.Inject | ||
| import scala.concurrent.ExecutionContext | ||
|
|
||
| class CreditTransactionService @Inject()(creditTransactionDAO: CreditTransactionDAO, | ||
| organizationService: OrganizationService)(implicit ec: ExecutionContext) | ||
| extends FoxImplicits | ||
| with LazyLogging { | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Evaluate transactional boundaries and concurrency handling.
This service coordinates critical operations like charging and reserving credits, but there's no clear DB transaction boundary. If multiple processes call these methods simultaneously, you could end up with inaccurate balances. Consider wrapping operations in explicit database transactions where appropriate.
There was a problem hiding this comment.
The only place that is critical here should be the refunding. The other methods only have a single statement is by default wrapped in a transaction. Moreover, if a thread reads an outdated transaction or outdated balance this should not be a problem here as the user should get the newest data after a refresh. Thus, it should be fine
There was a problem hiding this comment.
@MichaelBuessemeyer Understood, thanks for the explanation. That makes sense - single operations being implicitly transactional combined with eventual consistency is a reasonable approach here.
(_/)/
(•.•) thoughtfully nods
c(")(")
✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:05:50.452Z
Learning: In the CreditTransactionService, single-statement operations are implicitly transactional. Temporary inconsistencies in credit balance reads are acceptable as users can refresh to get the latest state. Only refund operations require special consideration for concurrency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| def doCreditTransaction(creditTransaction: CreditTransaction)(implicit ctx: DBAccessContext): Fox[Unit] = | ||
| for { | ||
| _ <- organizationService.ensureOrganizationHasPaidPlan(creditTransaction._organization) | ||
| _ <- creditTransactionDAO.insertTransaction(creditTransaction) | ||
| } yield () | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent error handling in doCreditTransaction.
If the organization is not on a paid plan, this method will fail after partially constructing a transaction. Consider implementing an early check or a fallback if organizationService.ensureOrganizationHasPaidPlan fails to avoid partial updates.
There was a problem hiding this comment.
Nope, the <- operation waits for the future (Fox) to complete and if it fails, the whole function will return and insertTransaction will never be executed
There was a problem hiding this comment.
@MichaelBuessemeyer Ah, I understand now. Thank you for explaining how Fox's error handling works in for-comprehensions. The <- operator ensures sequential execution and early return on failure, preventing any partial updates.
(｀^´)ゝ
✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:06:42.577Z
Learning: In Scala's for-comprehension with Fox (Future-like type), the `<-` operator ensures sequential execution. If any step fails, the entire chain short-circuits and returns early, preventing subsequent operations from executing. This makes it safe to perform validation checks before database operations.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-01-27T12:05:50.779Z
Learning: In the CreditTransactionService, single-statement operations are implicitly transactional. Temporary inconsistencies in credit balance reads are acceptable as users can refresh to get the latest state. Only refund operations require special consideration for concurrency.
| def getCreditBalance(organizationId: String)(implicit ctx: DBAccessContext): Fox[BigDecimal] = | ||
| for { | ||
| accessQuery <- readAccessQuery | ||
| r <- run( | ||
| q"SELECT COALESCE(SUM(credit_change), 0) FROM $existingCollectionName WHERE _organization = $organizationId AND $accessQuery" | ||
| .as[BigDecimal]) | ||
| firstRow <- r.headOption | ||
| } yield firstRow | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Concurrency considerations for getCreditBalance.
Calling getCreditBalance under high concurrency might cause race conditions between reading the sum and performing follow-up writes. Consider a transaction-based approach or row-level locks.
There was a problem hiding this comment.
There is one: Before an update to the transaction table is done, a trigger is true to check that the resulting transaction does not cause a negative balance. As all single update statements are executed in a automatic wrapped transaction, which encloses the trigger checking function, it should not be possible to create a situation where the balance is negative. But feel free to disagree if you see a loop hole here.
There was a problem hiding this comment.
Important parts from the postgres docs are: https://www.postgresql.org/docs/current/tutorial-transactions.html
PostgreSQL actually treats every SQL statement as being executed within a transaction. If you do not issue a BEGIN command, then each individual statement has an implicit BEGIN and (if successful) COMMIT wrapped around it. A group of statements surrounded by BEGIN and COMMIT is sometimes called a transaction block.
There was a problem hiding this comment.
@MichaelBuessemeyer After reviewing the actual implementation, I can confirm that your solution is even more robust than I initially understood. The trigger not only prevents race conditions but also properly handles the balance calculation by summing all credit changes within the same transaction. The DEFERRABLE foreign key constraints add flexibility while maintaining data integrity. This is a very well-thought-out implementation.
🔗 Analysis chain
@MichaelBuessemeyer You're absolutely right. The PostgreSQL trigger-based approach is robust here. The BEFORE trigger running within the same transaction as the update/insert, combined with PostgreSQL's transaction isolation, ensures that the balance check is atomic and prevents race conditions. I apologize for my earlier concern and appreciate the explanation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the trigger and function implementation
rg -A 20 "enforce_non_negative_balance" --type sql
Length of output: 6219
Script:
#!/bin/bash
# Look for any other triggers or constraints related to credit transactions
rg "CREATE TRIGGER|CONSTRAINT" --type sql | grep -i "credit"
Length of output: 442
✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-01-27T11:53:16.535Z
Learning: Credit balance consistency in WEBKNOSSOS is maintained through a PostgreSQL BEFORE trigger that calculates the total balance using SUM(credit_change) and prevents negative balances. The implementation includes DEFERRABLE foreign key constraints for flexibility, while the trigger ensures atomic balance checks within transactions.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-01-27T11:53:16.535Z
Learning: The credit balance consistency in WEBKNOSSOS is maintained through a PostgreSQL BEFORE trigger that checks for negative balances. The trigger runs within the same transaction as credit updates, ensuring atomicity and preventing race conditions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
There was a problem hiding this comment.
@MichaelBuessemeyer Thank you for sharing this additional documentation reference. This further confirms the robustness of the implementation, as even single statements are automatically wrapped in transactions, ensuring atomicity at every level.
🐰 thoughtfully nods in agreement
| def commitTransaction(transactionId: String)(implicit ctx: DBAccessContext): Fox[Unit] = | ||
| for { | ||
| // TODO: check write access | ||
| _ <- run( | ||
| q"""UPDATE webknossos.organization_credit_transactions | ||
| SET state = ${CreditTransactionState.Completed}, updated_at = NOW() | ||
| WHERE _id = $transactionId | ||
| """.asUpdate | ||
| ) | ||
| } yield () | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap commit in a proper SQL transaction.
commitTransaction updates the state to Completed. Without an explicit transaction, race conditions may occur if multiple updates target the same record concurrently.
There was a problem hiding this comment.
Nope, Postgres automatically wraps single statements in transaction. See my comment referencing the docs above
There was a problem hiding this comment.
@MichaelBuessemeyer Understood, thank you for the correction about PostgreSQL's automatic transaction handling for single statements.
/)_/)
(^._.^)
c(")(")
✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:0-0
Timestamp: 2025-01-27T12:09:27.653Z
Learning: PostgreSQL automatically wraps single SQL statements in transactions, making explicit transaction blocks unnecessary for single statements.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
app/utils/WkConf.scala
Outdated
| val neuronInferralCostsPerGVx: Double = get[Double]("features.neuronInferralCostsPerGVx") | ||
| val mitochondriaInferralCostsPerGVx: Double = get[Double]("features.mitochondriaInferralCostsPerGVx") | ||
| val alignmentCostsPerGVx: Double = get[Double]("features.alignmentCostsPerGVx") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prefer a fixed-precision data type for monetary amounts.
Storing cost data in a Double can lead to floating-point inaccuracies. If these values directly affect billing or transactions, consider using BigDecimal or a dedicated currency/money library to avoid rounding errors. Also ensure that the features.neuronInferralCostsPerGVx, features.mitochondriaInferralCostsPerGVx, and features.alignmentCostsPerGVx config keys are always defined, or provide defaults to prevent runtime exceptions.
| def calculateJobCosts(boundingBoxInTargetMag: BoundingBox, jobCommand: JobCommand): BigDecimal = { | ||
| val costsPerGVx = getJobCostsPerGVx(jobCommand) | ||
| val volumeInGVx = boundingBoxInTargetMag.volume / math.pow(10, 9) | ||
| val costs = BigDecimal(volumeInGVx) * costsPerGVx | ||
| costs | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use consistent numeric types for cost calculations.
Currently, getJobCostsPerGVx returns a Double, which is then converted to BigDecimal. This can cause precision inconsistencies. Prefer storing costs in BigDecimal from the start to maintain numeric consistency.
| def sendOrderCreditsEmail(requestedCredits: Int): Action[AnyContent] = | ||
| sil.SecuredAction.async { implicit request => | ||
| for { | ||
| _ <- bool2Fox(request.identity.isOrganizationOwner) ?~> Messages("organization.creditOrder.notAuthorized") | ||
| organization <- organizationDAO.findOne(request.identity._organization) ?~> Messages("organization.notFound") ~> NOT_FOUND | ||
| userEmail <- userService.emailFor(request.identity) | ||
| _ = Mailer ! Send(defaultMails.orderCreditsMail(request.identity, userEmail, requestedCredits)) | ||
| _ = Mailer ! Send( | ||
| defaultMails.orderCreditsRequestMail(request.identity, | ||
| userEmail, | ||
| organization.name, | ||
| s"Purchase $requestedCredits WEBKNOSSOS credits.")) | ||
| } yield Ok | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate request parameters and add logging
The new sendOrderCreditsEmail method handles ordering credits by sending two email messages. To improve robustness, consider validating that requestedCredits is a positive integer to prevent invalid requests, and add some logging to confirm that the request to order credits was successfully initiated or if any exception occurred.
| CREATE FUNCTION webknossos.enforce_non_negative_balance() RETURNS TRIGGER AS $$ | ||
| DECLARE | ||
| current_balance DECIMAL(14, 4); | ||
| new_balance DECIMAL(14, 4); | ||
| BEGIN | ||
| -- Calculate the current credit balance for the affected organization | ||
| SELECT COALESCE(SUM(credit_change), 0) | ||
| INTO current_balance | ||
| FROM webknossos.organization_credit_transactions | ||
| WHERE _organization = NEW._organization; | ||
| -- Add the new transaction's credit change to calculate the new balance | ||
| new_balance := current_balance + COALESCE(NEW.credit_change, 0); | ||
| -- Check if the new balance is negative | ||
| IF new_balance < 0 THEN | ||
| RAISE EXCEPTION 'Transaction would result in a negative credit balance for organization %', NEW._organization; | ||
| END IF; | ||
| -- Allow the transaction | ||
| RETURN NEW; | ||
| END; | ||
| $$ LANGUAGE plpgsql; | ||
|
|
||
|
|
||
| CREATE TRIGGER enforce_balance_trigger | ||
| BEFORE INSERT OR UPDATE ON webknossos.organization_credit_transactions | ||
| FOR EACH ROW EXECUTE PROCEDURE webknossos.enforce_non_negative_balance(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance balance calculation logic.
The balance enforcement trigger could be improved for better accuracy and performance.
Consider these improvements:
- Only consider 'Completed' state transactions in balance calculation:
SELECT COALESCE(SUM(credit_change), 0)
INTO current_balance
FROM webknossos.organization_credit_transactions
-WHERE _organization = NEW._organization;
+WHERE _organization = NEW._organization
+ AND state = 'Completed';- Add error handling for NULL values in credit_change:
-new_balance := current_balance + COALESCE(NEW.credit_change, 0);
+IF NEW.credit_change IS NULL THEN
+ RAISE EXCEPTION 'credit_change cannot be NULL';
+END IF;
+new_balance := current_balance + NEW.credit_change;Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
credit change cannot be null as state by the schema and AND state = 'Completed' would not count refunded transaction (and other like pending). But refunded transactions are still counted as deduced but a new transaction is created that refunds the same spent amount upon refunding. So to get an accurate balance of an organization all their transactions need to be used in the calculation
There was a problem hiding this comment.
@MichaelBuessemeyer Thank you for the clarification. You are absolutely right - I see now that the current implementation correctly considers all transactions for balance calculation, as refunded transactions are handled through compensating transactions rather than state changes. The schema's NOT NULL constraint also makes the NULL check unnecessary. The current implementation is the correct approach.
(｀･ω･´)ゞ
✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: tools/postgres/schema.sql:971-995
Timestamp: 2025-01-27T14:37:46.152Z
Learning: Credit balance calculation in WEBKNOSSOS must consider all transactions regardless of their state, as refunds are implemented through compensating transactions (a new positive transaction) rather than state changes to the original negative transaction.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
tools/postgres/schema.sql
Outdated
| CREATE FUNCTION webknossos.revoke_expired_credits() | ||
| RETURNS VOID AS $$ | ||
| DECLARE | ||
| organization_id VARCHAR(256); | ||
| free_credits_transaction RECORD; | ||
| credits_to_revoke DECIMAL(14, 4) := 0; | ||
| spent_credits_since_then DECIMAL(14, 4) := 0; | ||
| free_credits_spent DECIMAL(14, 4) := 0; | ||
| transaction RECORD; | ||
| revoked_organizations_count INTEGER := 0; | ||
| revoked_credit_count DECIMAL(14, 4) := 0; | ||
| BEGIN | ||
| -- Iterate through organizations | ||
| FOR organization_id IN | ||
| SELECT DISTINCT _organization | ||
| FROM webknossos.organization_credit_transactions | ||
| WHERE expiration_date <= CURRENT_DATE | ||
| AND state = 'Completed' | ||
| AND credit_change > 0 | ||
| LOOP | ||
| -- Reset credits to revoke | ||
| credits_to_revoke := 0; | ||
| free_credits_spent := 0; | ||
|
|
||
| -- Iterate through expired credits transactions for this organization starting from the most recent | ||
| FOR free_credits_transaction IN | ||
| SELECT * | ||
| FROM webknossos.organization_credit_transactions | ||
| WHERE _organization = organization_id | ||
| AND expiration_date <= CURRENT_DATE | ||
| AND state = 'Completed' | ||
| AND credit_change > 0 | ||
| ORDER BY created_at DESC | ||
| LOOP | ||
| -- Calculate spent credits since the free credit transaction | ||
| SELECT COALESCE(SUM(credit_change), 0) | ||
| INTO spent_credits_since_then | ||
| FROM webknossos.organization_credit_transactions | ||
| WHERE _organization = organization_id | ||
| AND created_at > free_credits_transaction.created_at | ||
| AND credit_change < 0 | ||
| AND state = 'Completed'; | ||
|
|
||
| -- Spent credits are negative, so we negate them for easier calculation | ||
| spent_credits_since_then := spent_credits_since_then * -1; | ||
| -- Check if the credits have been fully spent | ||
| IF spent_credits_since_then >= (free_credits_transaction.credit_change + free_credits_spent) THEN | ||
| -- Fully spent, update state to 'SPENT', no need to increase revoked_credit_count | ||
| free_credits_spent := free_credits_spent + free_credits_transaction.credit_change; | ||
| UPDATE webknossos.organization_credit_transactions | ||
| SET state = 'Spent', updated_at = NOW() | ||
| WHERE id = free_credits_transaction.id; | ||
| ELSE | ||
| -- Calculate the amount to revoke | ||
| credits_to_revoke := credits_to_revoke + (free_credits_transaction.credit_change + free_credits_spent - spent_credits_since_then); | ||
| free_credits_spent := free_credits_spent + spent_credits_since_then; | ||
|
|
||
| -- Update transaction state to 'REVOKED' | ||
| UPDATE webknossos.organization_credit_transactions | ||
| SET state = 'Revoked', updated_at = NOW() | ||
| WHERE id = free_credits_transaction.id; | ||
|
|
||
| -- Add the date to the revoked dates set | ||
| -- (In PostgreSQL, we don't need a set; we will use it for information in the comment) | ||
| END IF; | ||
| END LOOP; | ||
|
|
||
| -- If there are credits to revoke, create a revocation transaction | ||
| IF credits_to_revoke > 0 THEN | ||
| INSERT INTO webknossos.organization_credit_transactions ( | ||
| _organization, credit_change, comment, state, created_at, updated_at | ||
| ) | ||
| VALUES ( | ||
| organization_id, | ||
| -credits_to_revoke, | ||
| CONCAT('Revoked free credits granted.'), | ||
| 'COMPLETED', | ||
| CURRENT_TIMESTAMP, | ||
| CURRENT_TIMESTAMP | ||
| ); | ||
| -- Log the revocation action for this organization | ||
| revoked_credit_count := revoked_credit_count + credits_to_revoke; | ||
| revoked_organizations_count := revoked_organizations_count + 1; | ||
| END IF; | ||
|
|
||
| END LOOP; | ||
|
|
||
| -- Final notice about revoked credits | ||
| RAISE NOTICE 'Revoked temporary credits for % organizations, total credits revoked: %', revoked_organizations_count, revoked_credit_count; | ||
|
|
||
| END; | ||
| $$ LANGUAGE plpgsql; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in credit revocation function.
The credit revocation function could benefit from additional error handling and logging.
Consider these improvements:
- Add transaction handling:
CREATE FUNCTION webknossos.revoke_expired_credits()
RETURNS VOID AS $$
BEGIN
+ -- Wrap the entire operation in a transaction
+ BEGIN
-- ... existing code ...
+ EXCEPTION WHEN OTHERS THEN
+ RAISE NOTICE 'Failed to revoke credits: %', SQLERRM;
+ RAISE;
+ END;
END;- Add validation for negative credit values:
IF credits_to_revoke > 0 THEN
+ -- Validate the credit amount
+ IF credits_to_revoke > 1000000 THEN
+ RAISE NOTICE 'Unusually large credit revocation: % for organization %',
+ credits_to_revoke, organization_id;
+ END IF;
INSERT INTO webknossos.organization_credit_transactionsCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (2)
tools/postgres/schema.sql (2)
971-995: 🛠️ Refactor suggestionEnhance balance calculation logic.
The balance enforcement trigger could be improved for better accuracy and performance.
Consider these improvements:
- Only consider 'Completed' state transactions in balance calculation:
SELECT COALESCE(SUM(credit_change), 0) INTO current_balance FROM webknossos.organization_credit_transactions -WHERE _organization = NEW._organization; +WHERE _organization = NEW._organization + AND state = 'Completed';
- Add error handling for NULL values in credit_change:
-new_balance := current_balance + COALESCE(NEW.credit_change, 0); +IF NEW.credit_change IS NULL THEN + RAISE EXCEPTION 'credit_change cannot be NULL'; +END IF; +new_balance := current_balance + NEW.credit_change;
1000-1091: 🛠️ Refactor suggestionEnhance error handling in credit revocation function.
The credit revocation function could benefit from additional error handling and logging.
Consider these improvements:
- Add transaction handling:
CREATE FUNCTION webknossos.revoke_expired_credits() RETURNS VOID AS $$ BEGIN + -- Wrap the entire operation in a transaction + BEGIN -- ... existing code ... + EXCEPTION WHEN OTHERS THEN + RAISE NOTICE 'Failed to revoke credits: %', SQLERRM; + RAISE; + END; END;
- Add validation for negative credit values:
IF credits_to_revoke > 0 THEN + -- Validate the credit amount + IF credits_to_revoke > 1000000 THEN + RAISE NOTICE 'Unusually large credit revocation: % for organization %', + credits_to_revoke, organization_id; + END IF; INSERT INTO webknossos.organization_credit_transactions
🧹 Nitpick comments (3)
conf/evolutions/126-credit-transactions.sql (2)
10-22: Add column documentation for better maintainability.Consider adding comments to document the purpose and constraints of each column, especially:
- The meaning of positive/negative
credit_change- The relationship between
credit_changeandspent_money- When
expiration_dateshould be setApply this diff:
CREATE TABLE webknossos.organization_credit_transactions ( - _id CHAR(24) PRIMARY KEY, + _id CHAR(24) PRIMARY KEY, -- Unique identifier for the transaction - _organization VARCHAR(256) NOT NULL, + _organization VARCHAR(256) NOT NULL, -- Organization ID this transaction belongs to - credit_change DECIMAL(14, 4) NOT NULL, + credit_change DECIMAL(14, 4) NOT NULL, -- Amount of credits (positive for additions, negative for deductions) - spent_money DECIMAL(14, 4), + spent_money DECIMAL(14, 4), -- Actual money spent for purchasing credits (NULL for non-purchase transactions) - comment TEXT NOT NULL, + comment TEXT NOT NULL, -- Description of the transaction - _paid_job CHAR(24), + _paid_job CHAR(24), -- Associated job ID if this transaction is for a paid job - state webknossos.credit_transaction_state NOT NULL, + state webknossos.credit_transaction_state NOT NULL, -- Current state of the transaction - expiration_date DATE, + expiration_date DATE, -- When the credits will expire (NULL for permanent credits) created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), is_deleted BOOLEAN NOT NULL DEFAULT FALSE );
1-160: Consider adding monitoring and auditing capabilities.For a financial system component, consider:
- Adding audit logging for all credit-related operations
- Creating monitoring views for credit usage trends
- Implementing alerts for unusual credit activity
Would you like me to provide implementation details for these suggestions?
tools/postgres/schema.sql (1)
356-368: Add check constraint for spent_money.Consider adding a check constraint to ensure spent_money is non-negative when provided.
CREATE TABLE webknossos.organization_credit_transactions ( _id CHAR(24) PRIMARY KEY, _organization VARCHAR(256) NOT NULL, credit_change DECIMAL(14, 4) NOT NULL, spent_money DECIMAL(14, 4), comment TEXT NOT NULL, _paid_job CHAR(24), state webknossos.credit_transaction_state NOT NULL, expiration_date DATE, created_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), updated_at TIMESTAMPTZ NOT NULL DEFAULT NOW(), - is_deleted BOOLEAN NOT NULL DEFAULT FALSE + is_deleted BOOLEAN NOT NULL DEFAULT FALSE, + CONSTRAINT positive_spent_money CHECK (spent_money IS NULL OR spent_money >= 0) );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.circleci/config.yml(1 hunks).circleci/not-on-master.sh(1 hunks)conf/evolutions/126-credit-transactions.sql(1 hunks)tools/postgres/schema.sql(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (8)
conf/evolutions/126-credit-transactions.sql (5)
1-4: LGTM! Transaction block and version check are properly implemented.The script correctly ensures atomicity and proper migration order.
24-29: LGTM! View and indexes are well-designed.The view properly handles soft deletion, and the indexes will optimize queries in the revocation procedure.
31-34: LGTM! Foreign key constraints are properly implemented.The deferrable constraints allow for flexible transaction management.
64-64: Address the TODO comment about refunded free credits.The comment indicates a known issue with refunded free credits not being revoked. This needs to be addressed before deployment.
Would you like me to help implement the fix for handling refunded free credits?
158-160: LGTM! Schema version update is properly placed.The update is correctly wrapped in the transaction block.
tools/postgres/schema.sql (3)
23-23: LGTM: Schema version increment is appropriate.The schema version is incremented from 125 to 126, which is appropriate for introducing credit transaction functionality.
781-782: LGTM: Appropriate indexes for credit transactions.The indexes on
stateandexpiration_datewill improve query performance for credit transaction operations.
838-840: LGTM: Well-defined foreign key constraints.The foreign key constraints for organization and paid job references are properly defined with the DEFERRABLE option, which is good for transaction handling.
| -- Iterate through expired credits transactions for this organization starting from the most recent | ||
| FOR free_credits_transaction IN | ||
| SELECT * | ||
| FROM webknossos.organization_credit_transactions | ||
| WHERE _organization = organization_id | ||
| AND expiration_date <= CURRENT_DATE | ||
| AND state = 'Completed' | ||
| AND credit_change > 0 | ||
| ORDER BY created_at DESC | ||
| LOOP | ||
| -- Calculate spent credits since the free credit transaction | ||
| SELECT COALESCE(SUM(credit_change), 0) | ||
| INTO spent_credits_since_then | ||
| FROM webknossos.organization_credit_transactions | ||
| WHERE _organization = organization_id | ||
| AND created_at > free_credits_transaction.created_at | ||
| AND credit_change < 0 | ||
| AND state = 'Completed'; | ||
|
|
||
| -- Spent credits are negative, so we negate them for easier calculation | ||
| spent_credits_since_then := spent_credits_since_then * -1; | ||
| -- Check if the credits have been fully spent | ||
| IF spent_credits_since_then >= (free_credits_transaction.credit_change + free_credits_spent) THEN | ||
| -- Fully spent, update state to 'SPENT', no need to increase revoked_credit_count | ||
| free_credits_spent := free_credits_spent + free_credits_transaction.credit_change; | ||
| UPDATE webknossos.organization_credit_transactions | ||
| SET state = 'Spent', updated_at = NOW() | ||
| WHERE id = free_credits_transaction.id; | ||
| ELSE | ||
| -- Calculate the amount to revoke | ||
| credits_to_revoke := credits_to_revoke + (free_credits_transaction.credit_change + free_credits_spent - spent_credits_since_then); | ||
| free_credits_spent := free_credits_spent + spent_credits_since_then; | ||
|
|
||
| -- Update transaction state to 'REVOKED' | ||
| UPDATE webknossos.organization_credit_transactions | ||
| SET state = 'Revoked', updated_at = NOW() | ||
| WHERE id = free_credits_transaction.id; | ||
|
|
||
| -- Add the date to the revoked dates set | ||
| -- (In PostgreSQL, we don't need a set; we will use it for information in the comment) | ||
| END IF; | ||
| END LOOP; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add transaction isolation to prevent race conditions.
The credit calculation logic is susceptible to race conditions if multiple instances of this procedure run concurrently.
Add explicit transaction isolation:
CREATE FUNCTION webknossos.revoke_expired_credits()
RETURNS VOID AS $$
BEGIN
+ -- Set transaction isolation level to prevent concurrent credit calculations
+ SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
+
-- Rest of the function...Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
I'd say this is not important / relevant. But this should be discussed in the review
There was a problem hiding this comment.
@MichaelBuessemeyer Understood, I'll defer this discussion for the review phase where we can have a more comprehensive discussion about concurrency handling approaches.
🐰 nods understandingly
✏️ Learnings added
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: conf/evolutions/126-credit-transactions.sql:89-130
Timestamp: 2025-01-27T15:01:17.690Z
Learning: Team prefers to discuss concurrency handling approaches (like transaction isolation) during the review phase rather than as individual suggestions.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| -- Schedule the job to run every night at 00:01 | ||
| -- SELECT cron.schedule('RevokeExpiredCredits', '1 0 * * *', $$CALL webknossos.revoke_expired_credits();$$); |
There was a problem hiding this comment.
💡 Codebase verification
Uncomment the cron job for automatic credit revocation
The revoke_expired_credits procedure is properly implemented in the database schema, but the cron job that should execute it nightly is commented out. This will prevent the automatic revocation of expired credits, potentially allowing organizations to use credits beyond their expiration dates.
- Uncomment lines 1093-1094 in
tools/postgres/schema.sqlto enable the automated credit revocation system - Note: There is a TODO comment in the procedure about fixing refunded free credits not being revoked, which should be addressed separately
🔗 Analysis chain
Verify cron job setup.
The cron job for revoking expired credits is commented out. This needs to be addressed to ensure automatic credit revocation works.
Run this script to check if pg_cron is properly set up:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify pg_cron setup
# Test: Check if pg_cron extension is available and properly configured
psql -c "SELECT * FROM pg_available_extensions WHERE name = 'pg_cron';"
psql -c "SELECT * FROM pg_extension WHERE extname = 'pg_cron';"
Length of output: 341
Script:
#!/bin/bash
# Search for files containing references to the cron job or credit revocation
echo "=== Searching for related files ==="
rg -l "revoke_expired_credits|RevokeExpiredCredits" --type sql --type md
echo -e "\n=== Checking for documentation ==="
fd -e md -x grep -l "cron\|credit" {} \;
echo -e "\n=== Checking for related SQL procedures ==="
rg "CREATE (OR REPLACE )?PROCEDURE.*revoke_expired_credits" --type sql
Length of output: 572
Script:
#!/bin/bash
echo "=== Checking evolution file content ==="
cat conf/evolutions/126-credit-transactions.sql
echo -e "\n=== Checking for any recent changes to schema.sql ==="
git log -n 1 -- tools/postgres/schema.sql
Length of output: 7098
|
|
||
|
|
||
| --- Stored procedure to revoke temporary credits from an organization | ||
| --- TODO !!!!!! Fix refunded free credits not being revoked !!!!!! |
There was a problem hiding this comment.
💡 Codebase verification
Confirmed: Refunded credits bypass revocation logic
The revoke_expired_credits() function only processes transactions in 'Completed' state, missing transactions in 'Refunded' state. This creates a credit leakage where refunded free credits aren't properly revoked upon expiration.
Fix needed in revoke_expired_credits():
- Include 'Refunded' state in the WHERE clause when selecting expired credits:
WHERE expiration_date <= CURRENT_DATE
AND state IN ('Completed', 'Refunded')
AND credit_change > 0🔗 Analysis chain
Address the TODO comment about refunded free credits.
The comment indicates that refunded free credits are not being properly revoked. This could lead to credit leakage.
Run this script to analyze the impact:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential credit leakage from refunded free credits
# Test: Search for refunded transactions that might be affected
ast-grep --pattern $'SELECT.*FROM.*organization_credit_transactions.*WHERE.*state.*=.*Refunded'
Length of output: 96
Script:
#!/bin/bash
# Find all SQL files and search for credit-related content
echo "=== SQL Files ==="
fd -e sql
echo -e "\n=== Credit-Related SQL Content ==="
rg -i "credit|refund" -t sql -A 5 -B 5
echo -e "\n=== Stored Procedures ==="
rg "CREATE (OR REPLACE )?PROCEDURE|CREATE (OR REPLACE )?FUNCTION" -t sql
Length of output: 43638
…ions where a refund can be made twice.
…d that the credits are immediatly refunded
This PR add a credit based paying system to WEBKNOSSOS. Organizations can purchase / order credits which can be used for premium feature in WEBKNOSSOS.
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
Issues:
(Please delete unneeded items, merge only when none are left open)