-
Notifications
You must be signed in to change notification settings - Fork 27
Credit system for jobs #8352
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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
📝 WalkthroughWalkthroughThis pull request implements a comprehensive AI credit system by introducing new controllers, services, models, and frontend components. The changes add functionality for managing credit transactions, including adding, refunding, and revoking credits. Several SQL schema modifications and configuration updates were introduced to support credit tracking and pricing metrics. Additionally, the PR integrates new email templates, frontend actions, and UI updates to facilitate credit ordering and display job credit costs. Changes
Assessment against linked issues
Possibly related issues
Suggested labels
Suggested reviewers
Poem
🪧 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.log
with 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 onlyOk
might 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.
reserveCredits
sets the transaction’s state toPending
but 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.
refundTransactionForJob
silently 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_organization
and_paidJob
fields 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.
findAll
returns 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.
insertNewPendingTransaction
does 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.
insertTransaction
largely 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.
TheTODO
comment 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.
TheTODO
suggests 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.
useEffect
is 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
requestedUsers
inhandleOrderCredits
is actually capturing the number of credits. Consider renaming it tocreditsToOrder
for clarity.- The modal's default value is set to
1
, but themin
is set to0
. This slight mismatch can cause confusion if a user enters 0. Verify if a minimum of1
is 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
parseInt
fails.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>
withonClick
can hinder keyboard navigation and screen-reader support. Consider usingButton
or 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
creditBalance
should be nullable (e.g.,number | null
). Otherwise, declaring it asnumber
is 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
requestedCredits
with 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.snap
is 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
ImportingBoundingBox
appears correct for bounding box calculations.
111-111
: Optional property addition
Introducing an optionaljobCreditCostsPerGVx
property inStartJobFormProps
is consistent with dynamic cost calculations.
118-118
: New property for volume rendering
AddingshowVolume
toBoundingBoxSelectionProps
is a clear, self-explanatory approach for controlling volume display.
Line range hint
123-142
: Rendering bounding box & volume
PassingshowVolume
to conditionally display formatted volume inrenderUserBoundingBox
is clear and intuitive. Code logic is consistent with the bounding box usage.
160-160
: Default parameter value
The defaultfalse
forshowVolume
prevents accidental volume display.
166-166
: Type definition
UsingshowVolume: boolean;
aligns properly with the usage and default.
185-185
: Prop forwarded to bounding box render
ForwardingshowVolume
makes the bounding box rendering more flexible.
196-196
: Consistent default parameter
Again, defaultingshowVolume
tofalse
ensures backward compatibility.
259-259
: Prop propagation
Explicitly passingshowVolume={showVolume}
to theBoundingBoxSelection
is 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
DestructuringjobCreditCostsPerGVx
from props is consistent with the rest of the code.
674-682
: Bounding box selection logic
The ternary fallback forselectedBoundingBox
is sensible. This ensures the bounding box is derived properly for cost calculations.
846-846
: Feature-based cost retrieval
PullingneuronInferralCostsPerGVx
fromfeatures
is consistent with the global approach for dynamic pricing.
856-856
: Passing cost prop
SendingneuronInferralCostsPerGVx
toStartJobForm
properly links to the credit-based pricing system.
884-884
: Feature-based cost retrieval
Similarly, pullingmitochondriaInferralCostsPerGVx
is consistent.
894-894
: Passing cost prop
ProvidingjobCreditCostsPerGVx={mitochondriaInferralCostsPerGVx}
correctly configures the segmentation form.
999-999
: Feature-based cost retrieval
GettingalignmentCostsPerGVx
fromfeatures()
follows the established pattern.
1012-1012
: Integration of alignment cost
Aligning sections also consumes credits, usingalignmentCostsPerGVx
for 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.
WhilehasEnoughCredits
returns 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.
completeTransactionOfJob
depends 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.
addJobIdToTransaction
updates 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.
readAccessQ
broadens 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.
findOne
callsparseFirst
which 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.
findTransactionForJob
retrieves 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 newCreditTransactionService
integrated here. Ensure that calls to it are always made from secure, authenticated endpoints.
67-68
: Constructor injection verified
InjectingorganizationService
andcreditTransactionService
is 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_Trial
orPower_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.Date
does not store time components, so converting it to an epoch millisecondInstant
may 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.
AddingcreditTransactionDAO
here 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
sendOrderCreditsEmail
is properly utilized inhandleOrderCredits
below. No issues found here.app/models/job/JobService.scala (2)
242-247
: Ensure fractional bounding boxes are intended.Dividing the parsed bounding box by
parsedMag
may 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/requestCredits
relies 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/chargeUpCredits
andPATCH /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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, Postgres automatically wraps single statements in transaction. See my comment referencing the docs above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
app/models/job/JobService.scala
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
tools/postgres/schema.sql
Outdated
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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_transactions
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_change
andspent_money
- When
expiration_date
should 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
state
andexpiration_date
will 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd say this is not important / relevant. But this should be discussed in the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fm3 Can we discuss the isolation levels needed for the credit transaction table a little, please? I am a little unsure about what kind of problems might arise in lower isolation levels 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is always a little hard to wrap your head around. Have a look at this document https://www.postgresql.org/docs/current/transaction-iso.html and possibly this discussion https://stackoverflow.com/questions/150177/transaction-isolation-problem-or-wrong-approach
This is mostly about concurrent interdependent queries. And yes, I think it might be a good idea to use the Serializable level for our transaction table. While it’s true that spending and adding credits is fairly infrequent at this time (at least per organization), I’d rather be safe here. We also use it for a couple of other transactions already. We will also get notified if serializing fails for any queries. (At least for queries set to serializable from slick. There we also set a retry number)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be done now. I chose every SQL statement that seemed like a potential error source to me. Even single updates are not running in isolation level serializable, so I also adjusted some of their isolation levels. That's alright, correct?
tools/postgres/schema.sql
Outdated
-- 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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.sql
to 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
tools/postgres/schema.sql
Outdated
|
||
|
||
--- Stored procedure to revoke temporary credits from an organization | ||
--- TODO !!!!!! Fix refunded free credits not being revoked !!!!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (3)
frontend/javascripts/admin/organization/upgrade_plan_modal.tsx (2)
351-353
: Strengthen input validation for credit purchase.While the InputNumber component has a min=1 constraint, it's advisable to add additional validation before sending the request, especially if the amount could be modified through browser devtools.
const handleOrderCredits = async () => { if (userInputRef.current) { const requestedUsers = Number.parseInt(userInputRef.current.value); + if (requestedUsers <= 0 || !Number.isInteger(requestedUsers)) { + Toast.error("Please enter a valid positive integer for credit amount"); + return; + } try { await sendOrderCreditsEmail(requestedUsers); Toast.success(messages["organization.credit_request_sent"]); } catch (e) { Toast.error(`Could not request credits: ${e}`); console.log(e); } } destroy(); };
366-366
: Improve readability of the total cost display.The current implementation displays the total cost directly in the component. Consider using a styled component or paragraph tag for better visual hierarchy.
- Total resulting cost: {creditCostAsString} + <div style={{ marginTop: '10px', fontSize: '16px' }}> + <strong>Total resulting cost:</strong> {creditCostAsString} + </div>frontend/javascripts/types/api_flow_types.ts (1)
708-712
: Consider documenting the GVx unit definitionThese cost configurations are important for the new credit system, but the definition of a Gigavoxel (GVx) isn't documented. Based on the PR comments, there was a discussion about whether 1 GVx = 1000³ or 1024³. Consider adding a documentation comment to clarify which definition was chosen.
+// GVx (Gigavoxel) is defined as 1000³ voxels (decimal) for consistency with how byte amounts are displayed readonly neuronInferralCostPerGVx: number; readonly mitochondriaInferralCostPerGVx: number; readonly alignmentCostPerGVx: number;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
MIGRATIONS.unreleased.md
(1 hunks)frontend/javascripts/admin/organization/upgrade_plan_modal.tsx
(6 hunks)frontend/javascripts/types/api_flow_types.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- MIGRATIONS.unreleased.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (13)
frontend/javascripts/admin/organization/upgrade_plan_modal.tsx (11)
9-9
: Good import addition for the credit system implementation.The addition of the
sendOrderCreditsEmail
import aligns with the credit system functionality being implemented.
17-18
: Appropriate imports for feature access and currency formatting.The imports of
features
andformatCurrency
are well-chosen for implementing the credit ordering functionality, providing access to configuration values and proper formatting utilities.
23-23
: React hooks added for state management in the new component.The addition of
useEffect
anduseState
to the imports is necessary for managing the credit amount and cost calculation in the new OrderWebknossosCreditsModal component.
40-40
: Export functions to improve reusability across components.Making these functions exportable improves the modularity of the codebase and allows them to be reused in other components, which is a good practice for maintaining a clean architecture.
Also applies to: 73-73, 118-118, 251-251
302-306
: Well-structured entry point function for ordering credits.This function follows the same pattern as the other modal-triggering functions in the file, maintaining consistency in the codebase.
310-314
: Inconsistent state variable naming.The state setter function
setCreditCostsAsString
uses plural "Costs" while the state variablecreditCostAsString
uses singular "Cost".- const [creditCostAsString, setCreditCostsAsString] = useState<string>( + const [creditCostAsString, setCreditCostAsString] = useState<string>(
320-323
: Add Unicode thin space between number and currency symbol.There should be a thin space between the number and the currency symbol as mentioned in previous review comments.
- setCreditCostsAsString(`${totalCostInEuro}€/${totalCostInDollar}$`); + setCreditCostAsString(`${totalCostInEuro}\u2009€/${totalCostInDollar}\u2009$`);
325-338
: Good error handling for credit order requests.The implementation includes proper error handling with try/catch, displaying an error toast to the user if the credit request fails. This addresses a previous review comment that highlighted the need for error handling.
343-343
: Use Unicode thin space in the OK button text.Apply the same formatting recommendation to ensure consistency with currency display throughout the UI.
- okText={`Buy more WEBKNOSSOS Credits for ${creditCostAsString}`} + okText={`Buy more WEBKNOSSOS Credits for ${creditCostAsString.replace('€', '\u2009€').replace('$', '\u2009$')}`}
356-365
: Good implementation of the credit amount input.The InputNumber component is properly configured with appropriate minimums, steps, and onChange handlers. The min=1 setting prevents ordering zero credits, which was a good recommendation from a previous review.
386-386
: Good export addition for the new functionality.The new function is properly added to the default exports, making it accessible to other parts of the application.
frontend/javascripts/types/api_flow_types.ts (2)
652-652
: Type consistency concern for financial valuesThe
creditBalance
is typed asstring | null | undefined
, which might lead to confusion when performing arithmetic operations. While this appears to be a deliberate choice (based on PR comments about precision in financial calculations), it's important to ensure that the codebase consistently handles these string-based financial values.
770-770
:❓ Verification inconclusive
Ensure consistent credit value handling
Similar to
creditBalance
,creditCost
is typed asstring | null | undefined
. Make sure the application has consistent utility functions for performing operations on these string-based credit values to avoid floating-point precision issues.
🏁 Script executed:
#!/bin/bash # Check for credit handling utility functions in the codebase echo "Searching for credit-related utility functions..." rg -A 5 "format.*Credit|calculate.*Credit|convert.*Credit" --type tsLength of output: 5058
Ensure consistent usage of credit utilities for creditCost
It looks like the codebase already includes a centralized utility—
formatCreditsString
infrontend/javascripts/libs/format_utils.ts
—that handles string-based credit values. Please verify that all operations involvingcreditCost
(typed asstring | null | undefined
infrontend/javascripts/types/api_flow_types.ts
) consistently make use of these dedicated utilities (or other equivalent functions likeformatCurrency
where appropriate) to avoid floating-point precision issues. In particular:
- Make sure any arithmetic or formatting on
creditCost
leverages the centralized functions.- Confirm that nullish values are correctly handled before operations are applied.
- Cross-check that usage in components (e.g., in admin modals and job-related views) remains consistent with how
creditBalance
is managed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (1)
87-94
: Consider using the APIJobType enum for key values.The
jobNameToImagePath
object uses string literals for keys while the rest of the code has been migrated to use theAPIJobType
enum.const jobNameToImagePath = { - infer_neurons: "neuron_inferral_example.jpg", - infer_nuclei: "nuclei_inferral_example.jpg", - infer_mitochondria: "mito_inferral_example.jpg", + [APIJobType.INFER_NEURONS]: "neuron_inferral_example.jpg", + [APIJobType.INFER_NUCLEI]: "nuclei_inferral_example.jpg", + [APIJobType.INFER_MITOCHONDRIA]: "mito_inferral_example.jpg", align_sections: "align_example.png", materialize_volume_annotation: "materialize_volume_annotation_example.jpg", invisible: "", - infer_with_model: "", + [APIJobType.INFER_WITH_MODEL]: "", } as const;
📜 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.snap
is excluded by!**/*.snap
📒 Files selected for processing (2)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
(27 hunks)frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (39)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (39)
2-7
: LGTM: API imports updated for credit system support.The imports have been correctly expanded to include credit-related functionality from the admin REST API.
15-15
: LGTM: Added import for credit ordering.Adding this import correctly enables the functionality for users to order WEBKNOSSOS credits.
38-40
: LGTM: Added useful formatting imports.The new imports for formatting credits and voxels are appropriate for displaying currency values and volumes in a user-friendly way.
56-59
: LGTM: Added Redux action imports for organization.These organization action imports are necessary for updating the organization's credit balance in the Redux store.
66-69
: LGTM: Updated imports and fixed React ordering.The imports are properly organized, and React is imported correctly. The addition of
Link
from React Router is appropriate for the credit purchasing links.
79-82
: LGTM: Updated modal state types to use enum.The code has been correctly updated to use the
APIJobType
enum values instead of string literals for better type safety.
113-114
: LGTM: Updated type to use APIJobType enum.Correctly updated to use the APIJobType enum instead of string literals for better type safety.
124-125
: LGTM: Added jobCreditCostPerGVx prop.The new prop allows passing job credit cost per Gigavoxel, which is necessary for credit cost calculation.
131-132
: LGTM: Added showVolume prop to control bounding box display.This new property appropriately controls whether to display volume information in the bounding box selection UI.
136-156
: LGTM: Updated function signature to support volume display.The function has been properly updated to accept the new
showVolume
parameter and conditionally display volume information.
173-174
: LGTM: Added showVolume parameter with appropriate default.The
showVolume
parameter has been correctly added with a default value offalse
to maintain backward compatibility.Also applies to: 179-180
198-199
: LGTM: Correctly passing showVolume to the rendering function.The
renderUserBoundingBox
function now receives theshowVolume
parameter as expected.
209-210
: LGTM: Added showVolume parameter to BoundingBoxSelectionFormItem.Consistently added the
showVolume
parameter with a proper default value.
272-273
: LGTM: Properly passing showVolume to the BoundingBoxSelection component.The parent component correctly forwards the
showVolume
prop to the child component.
403-404
: LGTM: Updated to use APIJobType enum.The code has been properly updated to use the APIJobType enum instead of string literals.
419-436
: LGTM: Added Mitochondria detection option.The UI for mitochondria detection has been properly implemented with appropriate styling and event handling.
441-443
: LGTM: Updated to use APIJobType enum for nuclei detection.The code has been properly updated to use the APIJobType enum.
459-464
: LGTM: Updated conditional rendering to use APIJobType enum.The conditional rendering of form components based on the job type has been correctly updated to use the APIJobType enum.
548-654
: LGTM: Well-designed JobCreditCostInformation component.The new component effectively displays:
- Job cost information based on bounding box size
- Organization's current credit balance
- Information about how costs are calculated
- Error messages when credits are insufficient
The TODO comment on line 613 is deliberately left for after the testing phase, as confirmed in previous review discussions.
728-756
: LGTM: Added helpful utility functions for bounding box management.These well-designed utility functions:
getBoundingBoxesForLayers
: Generates bounding boxes for complete layersuseCurrentlySelectedBoundingBox
: Manages the currently selected bounding box stateBoth functions are properly implemented with appropriate types and logic.
783-785
: LGTM: Added organization credit balance from Redux store.The code correctly retrieves the organization's credit balance from the Redux store.
788-796
: LGTM: Using new utility functions for bounding box selection.The code properly utilizes the new helper functions for bounding box management.
797-808
: LGTM: Added credit cost fetching functionality.The
useFetch
hook is properly implemented to fetch credit cost information when the bounding box changes.
809-814
: LGTM: Added effect to update organization credits.This effect ensures the Redux store is updated with the latest credit balance from the backend.
858-869
: LGTM: Added organization credit refresh after job start.The code properly refreshes the organization's credit information after a job is started, with good error handling.
927-928
: LGTM: Conditionally showing volume based on job credits.The code correctly passes the
showVolume
property based on whether the job has a credit cost.
937-945
: LGTM: Conditionally rendering the JobCreditCostInformation component.The component is only rendered when a credit cost is specified, with all necessary props passed correctly.
949-951
: LGTM: Added credit cost information to the button label.The button now includes credit cost information when available, improving user experience.
993-994
: LGTM: Added neuron inferral cost per GVx from features.The cost per gigavoxel is correctly retrieved from the features configuration.
1000-1001
: LGTM: Updated to use APIJobType enum for job name.The code consistently uses the APIJobType enum.
1005-1006
: LGTM: Passing neuron inferral cost to StartJobForm.The credit cost per gigavoxel is properly passed to the form component.
1070-1071
: LGTM: Added mitochondria inferral cost per GVx from features.The cost information is consistently retrieved from the features configuration.
1075-1076
: LGTM: Updated to use APIJobType enum for job name.The code consistently uses the APIJobType enum.
1080-1081
: LGTM: Passing mitochondria inferral cost to StartJobForm.The credit cost per gigavoxel is properly passed to the form component.
1129-1130
: LGTM: Updated to use APIJobType enum for job name.The code consistently uses the APIJobType enum for custom AI model inference.
1185-1186
: LGTM: Added alignment cost per GVx from features.The cost information is consistently retrieved from the features configuration.
1189-1190
: LGTM: Updated to use APIJobType enum for job name.The code consistently uses the APIJobType enum.
1198-1199
: LGTM: Passing alignment cost to StartJobForm.The credit cost per gigavoxel is properly passed to the form component.
1293-1294
: LGTM: Updated to use APIJobType enum for job name.The code consistently uses the APIJobType enum for materializing volume annotations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/models/organization/CreditTransaction.scala (1)
72-103
:⚠️ Potential issueParameter order mismatch in getCreditTransactions implementation.
There's a mismatch between how the parameters are ordered in the case class constructor at line 24 and how they're used in the getCreditTransactions implementation. The
_relatedTransaction
and_paidJob
parameters are swapped.CreditTransaction( transactionId, organizationId, - relatedTransaction, - paidJobId, + paidJobId, + relatedTransaction, creditChange, comment, transactionState, creditState, expiresAt, createdAt, updatedAt, isDeleted )
🧹 Nitpick comments (1)
app/models/organization/CreditTransaction.scala (1)
292-339
: Complex business logic needs additional documentation.The credit revocation logic is sophisticated, particularly the calculation of available credits. Consider adding more detailed comments explaining the business rules behind the SQL query at lines 295-305, especially regarding the classification of transactions.
private def revokeExpiredCreditsTransactionQuery(transaction: CreditTransaction): DBIO[Unit] = for { - // Query: Sums up all spent credits since the transaction which are completed and subtracts refunded transactions. + // Query: Calculates the available free credits by summing: + // 1. All negative credit transactions (spent credits) + // 2. Refunding transactions (added back to balance) + // 3. Expired transactions (to be revoked) + // This gives us the net amount of free credits still available for revocation freeCreditsAvailableResult <- q"""
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/organization/CreditTransaction.scala
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/models/organization/CreditTransaction.scala (5)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-03-19T13:07:43.053Z
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.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-03-19T13:07:43.053Z
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:0-0
Timestamp: 2025-03-19T13:07:43.053Z
Learning: PostgreSQL automatically wraps single SQL statements in transactions, making explicit transaction blocks unnecessary for single statements.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-03-19T13:07:43.053Z
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.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-03-19T13:07:43.053Z
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.
🧬 Code Definitions (1)
app/models/organization/CreditTransaction.scala (4)
app/models/organization/CreditState.scala (1)
CreditState
(6-9)app/models/organization/CreditTransactionState.scala (1)
CreditTransactionState
(6-9)app/models/organization/CreditTransactionService.scala (2)
addJobIdToTransaction
(67-71)refundTransaction
(56-64)app/models/organization/FreeCreditTransactionService.scala (1)
handOutMonthlyFreeCredits
(24-24)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (5)
app/models/organization/CreditTransaction.scala (5)
24-35
: Well-structured case class with appropriate field types.The
CreditTransaction
case class is well-designed with appropriate types for financial data, includingBigDecimal
for monetary values and proper nullable fields with Option types.
130-138
: Implementation of getCreditBalance is robust.The method correctly uses COALESCE to handle empty results and returns a BigDecimal, which is appropriate for financial calculations. Based on the learnings provided, credit balance consistency is maintained through PostgreSQL triggers that prevent negative balances, making this implementation reliable.
139-152
: Transaction isolation properly implemented for financial operations.Using Serializable isolation level for credit transactions is the right approach to prevent race conditions and ensure data consistency. This is especially important for financial operations.
266-291
: Strong transaction handling for credit revocation process.The implementation properly handles each organization's expired credits within a single transaction, ensuring atomicity while allowing progress even if some organizations' revocations fail. The pattern of collecting failures and reporting them is a good practice.
358-362
: Efficient implementation of monthly free credits distribution.Using a database function to distribute free credits is a clean and efficient approach. The serializable isolation level ensures that this operation is atomic and prevents race conditions.
The value from
conf.Jobs.monthlyFreeCredits
is correctly passed to the stored procedure, allowing for configuration-driven credit allocation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
app/models/organization/CreditTransaction.scala (4)
24-35
: Consider renaming fields for idiomatic Scala.
Currently, fields like_id
and_organization
mirror DB column names. While this is consistent with the database schema, removing underscores may improve readability in Scala code.
48-70
: Handle unknown states more gracefully.
An exception is thrown iftransactionState
orcreditState
is invalid. Consider introducing a fallback, or storing unknown values as a separate state to avoid runtime crashes in production.
206-246
: Refund logic is limited to fully pending negative transactions.
If partial or multi-stage refunds are ever required, or if multiple refunds might happen concurrently, consider expanding this logic to handle those scenarios safely (e.g., row-level locks or stricter checks).
265-290
: Evaluate error handling in batch revocation flow.
A failure during a single transaction’s revocation only appends that item to a failed list, but partial revocations may leave the organization in a half-updated state. Consider a more comprehensive rollback or retry strategy to ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
app/models/organization/CreditTransaction.scala
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
app/models/organization/CreditTransaction.scala (5)
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-03-19T13:07:43.053Z
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.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-03-19T13:07:43.053Z
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:0-0
Timestamp: 2025-03-19T13:07:43.053Z
Learning: PostgreSQL automatically wraps single SQL statements in transactions, making explicit transaction blocks unnecessary for single statements.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransactionService.scala:0-0
Timestamp: 2025-03-19T13:07:43.053Z
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.
Learnt from: MichaelBuessemeyer
PR: scalableminds/webknossos#8352
File: app/models/organization/CreditTransaction.scala:80-88
Timestamp: 2025-03-19T13:07:43.053Z
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.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (5)
app/models/organization/CreditTransaction.scala (5)
105-111
: Review access control for credit transactions.
All organization members can read and update these records. Confirm this is intended or if stricter role-based controls (e.g., admin/manager) are desired for sensitive financial transactions.
130-138
: Concurrency concern for credit balance calculation.
This closely matches a previously reported issue regarding race conditions on reading the credit sum.
72-103
:❓ Verification inconclusive
Verify column order with
SELECT *
.
UsingSELECT *
heavily relies on the schema’s column order. If the table structure changes, this can lead to deserialization issues. It’s safer to explicitly list columns.
🏁 Script executed:
#!/bin/bash # Check all code references to SELECT * across the repository rg -A 5 "SELECT \*"Length of output: 67317
Action Required: Column Order Dependency Risk
The deserialization code inapp/models/organization/CreditTransaction.scala
(lines 72–103) depends on an exact column order in the result set. In our system, many views (e.g., the one defined inconf/evolutions/129-credit-transactions.sql
) useSELECT *
, which returns columns in a schema-dependent order. This approach can become problematic if the underlying table schema changes, causing mismatches in the expected column order. Please verify that the columns returned by the currentSELECT *
are ordered exactly as assumed by the deserialization logic. In the long term, explicitly listing the columns in your SQL queries would improve resilience against schema changes.
291-339
:❓ Verification inconclusive
Validate partial revocation math under concurrency.
ThefreeCreditsAvailable
calculation governs partial vs. full revocation. Under high concurrency, multiple processes may alter the same baseline, so confirming the states remain consistent is essential.
🏁 Script executed:
#!/bin/bash #!/bin/bash # 1. Locate definitions of `revokeExpiredCreditsTransactionQuery` and references to it # 2. Inspect test coverage for concurrent updates rg -A 10 "def revokeExpiredCreditsTransactionQuery" rg -A 15 "revokeExpiredCreditsTransactionQuery"Length of output: 5864
Action Required: Validate Concurrency Handling for Partial Revocation Math
The current implementation computes the available free credits and decides between full or partial revocation based on that result. We see that the function is used within a transactional context (using Serializable isolation in
runRevokeExpiredCredits
), which should mitigate typical concurrency issues. However, please ensure that:
- The calculation of
freeCreditsAvailable
remains accurate under high-concurrency scenarios.- There is explicit test coverage or simulation (e.g., stress/integration tests) for concurrent updates to verify that overlapping updates do not lead to inconsistent states.
- If any edge cases arise from the race conditions, additional safeguards or adjustments to the transaction logic may be necessary.
357-361
:❓ Verification inconclusive
Ensure the monthly credit distribution is race-condition safe.
The stored procedurehand_out_monthly_free_credits
is called in a single transaction, but confirm that parallel executions cannot trigger double allocations.
🏁 Script executed:
#!/bin/bash # Look for references to hand_out_monthly_free_credits definition in SQL files to verify concurrency safeguards rg -A 10 "CREATE OR REPLACE FUNCTION hand_out_monthly_free_credits"Length of output: 69
Action: Manually verify stored procedure's concurrency safeguards
The code executes
hand_out_monthly_free_credits
inside a SERIALIZABLE transaction, which is a good step toward mitigating race conditions. However, our automated search for the definition (using a search for"CREATE OR REPLACE FUNCTION hand_out_monthly_free_credits"
) produced no results. This means we couldn’t confirm via automation that the stored procedure’s implementation itself includes the necessary safeguards to prevent parallel executions from triggering duplicate credit allocations.Next Steps:
- Manually Review the Stored Procedure: Please check the definition of
hand_out_monthly_free_credits
(possibly located in a migration file or a different SQL script) to confirm that it correctly handles concurrency.- Verify Concurrency Mechanisms: Ensure that the procedure employs locking or other concurrency-safe mechanisms to avoid double allocations when multiple transactions are executed in parallel.
…dit-system-for-jobs
This reverts commit b26afd5.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
frontend/javascripts/admin/job/job_list_view.tsx
(2 hunks)frontend/javascripts/admin/organization/organization_cards.tsx
(3 hunks)frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
(28 hunks)frontend/javascripts/types/api_flow_types.ts
(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/javascripts/admin/job/job_list_view.tsx
- frontend/javascripts/admin/organization/organization_cards.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (30)
frontend/javascripts/types/api_flow_types.ts (3)
652-652
: Type choice consistency for credit valuesThe
creditBalance
is typed as a string, which is appropriate for preserving precision in financial calculations. However, ensure consistent handling of this string representation throughout the codebase when performing arithmetic operations.
708-712
: Looks good: Cost configuration properties addedThe addition of cost configuration properties for various job types is well-structured, using the appropriate number type for these configuration values.
770-770
: Consistent pattern for credit-related fieldsUsing string type for
creditCost
matches the approach used forcreditBalance
in theAPIOrganization
type, providing consistency in how credit values are represented across the application.frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (27)
3-3
: Imports for job cost logic
The new imports forJobCreditCostInfo
,getJobCreditCost
, andgetOrganization
look consistent with the introduced credit-related functionality.Also applies to: 5-6
37-37
: Additional utility imports
UsingformatCreditsString
,formatVoxels
,useFetch
,useGuardedFetch
, andpluralize
appears straightforward and suitable for the new credit cost display logic.Also applies to: 39-39, 45-45
56-59
: Organization actions import
These imports for updating the active organization and credit balance are aligned with the credit system enhancements.
61-61
: BoundingBox import
ImportingBoundingBox
to compute bounding box volumes is a clean and direct approach.
66-66
: React hooks import
AddinguseEffect
,useState
, anduseMemo
is appropriate for managing state and side effects in the new credit-based features.
68-68
: Extended API Flow Types
ImportingAPIJobType
aligns with using typed job constants throughout the file.
79-81
: Expanded StartAIJobModalState
The addition ofINFER_NEURONS
,INFER_NUCLEI
, andINFER_MITOCHONDRIA
to the modal state clearly enumerates new AI job options.
86-88
: New image paths for job types
Defining image paths for newly added job types (infer_neurons
,infer_nuclei
,infer_mitochondria
, andinfer_with_model
) is consistent.Also applies to: 92-92
112-112
: Updated jobName to APIJobType
Switching toAPIJobType
ensures consistent typing for job names.
123-123
: Optional credit cost prop
AddingjobCreditCostPerGVx?: number
to the form props is a clear extension for cost-based logic.
130-130
: showVolume property
IntroducingshowVolume: boolean
in the props indicates an explicit option to display bounding box volume.
135-135
: Render bounding box volume conditionally
The revisedrenderUserBoundingBox
function now computes and shows the bounding box volume ifshowVolume
is true. This is a neat way to provide more info conditionally.Also applies to: 142-142, 153-154
172-172
: Default showVolume to false
This default indicates that volume rendering is disabled unless specifically set.
197-197
: Pass showVolume to render function
PassingshowVolume
clarifies the bounding box details displayed at runtime.
208-208
: Another default assignment for showVolume
Repeated setting ofshowVolume = false
ensures no volume rendering unless configured.
550-583
: Dedicated JobCreditCostInformation component
This component elegantly displays cost data and current org credits. The informational alert is a helpful approach for the testing phase.
657-667
: Synthetic bounding boxes for layers
getBoundingBoxesForLayers
sensibly constructs bounding boxes covering entire layers, assigning unique negative IDs per layer.
669-685
: Custom hook to retrieve current bounding box
useCurrentlySelectedBoundingBox
encapsulates bounding box logic neatly, improving clarity and reusability.
706-706
: Dispatch usage
Initializingdispatch
is straightforward for credit-related state updates.
718-718
: Combining default and user bounding boxes
ConcatenatingdefaultBBForLayers
withrawUserBoundingBoxes
ensures the user can select either a layer-wide box or custom user bounding boxes.
719-725
: Applying the useCurrentlySelectedBoundingBox hook
This usage properly reads the watch values from the form and returns the correct bounding box.
726-736
: Fetching jobCreditCost
UsinguseFetch
to lazily load job cost data for the chosen bounding box is a solid approach.
787-798
: Re-fetch organization after job start
Refetching and updating the active organization's credits upon job creation provides accurate balance data.
916-916
: Neuron segmentation cost
InjectingneuronInferralCostPerGVx
fromfeatures()
into the NeuronSegmentationForm ensures cost-based logic for neuron segmentation.Also applies to: 928-928
993-993
: Mitochondria segmentation cost
Likewise, passingmitochondriaInferralCostPerGVx
to the MitochondriaSegmentationForm maintains consistent billing representation for this job type.Also applies to: 1003-1003
1108-1108
: Alignment job cost
The alignment job now also respects per-GVx charging withalignmentCostPerGVx
, ensuring uniform billing coverage.Also applies to: 1121-1121
1216-1216
: Materialization job name
AssigningjobName={APIJobType.MATERIALIZE_VOLUME_ANNOTATION}
clearly identifies the volume annotation materialization job.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (1)
86-93
: Consider standardizing job type references with APIJobType enumWhile the component now uses
APIJobType
for job type references,jobNameToImagePath
still uses string literals. This inconsistency could lead to errors if string literals don't match enum values.const jobNameToImagePath = { - infer_neurons: "neuron_inferral_example.jpg", - infer_nuclei: "nuclei_inferral_example.jpg", - infer_mitochondria: "mito_inferral_example.jpg", + [APIJobType.INFER_NEURONS]: "neuron_inferral_example.jpg", + [APIJobType.INFER_NUCLEI]: "nuclei_inferral_example.jpg", + [APIJobType.INFER_MITOCHONDRIA]: "mito_inferral_example.jpg", align_sections: "align_example.png", materialize_volume_annotation: "materialize_volume_annotation_example.jpg", invisible: "", - infer_with_model: "", + [APIJobType.INFER_WITH_MODEL]: "", } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
(28 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (4)
frontend/javascripts/libs/format_utils.ts (2)
formatVoxels
(515-538)formatCreditsString
(559-563)frontend/javascripts/admin/api/jobs.ts (2)
JobCreditCostInfo
(78-84)getJobCreditCost
(86-95)frontend/javascripts/types/api_flow_types.ts (1)
APIDataLayer
(97-97)frontend/javascripts/oxalis/model/actions/organization_actions.ts (2)
setActiveOrganizationsCreditBalance
(14-19)setActiveOrganizationAction
(8-12)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
🔇 Additional comments (11)
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx (11)
3-7
: Good job migrating to the APIJobType enum!The code now consistently uses
APIJobType
enums instead of string literals for job types, which improves type safety and maintainability. The implementation of credit-related imports and types is also well-structured.Also applies to: 68-68
135-157
: Good enhancement to display volume information!The
renderUserBoundingBox
function has been enhanced to display volume information when requested, which provides valuable context for users about the size of the bounding box they're selecting. This is especially important for credit-based operations.
550-583
: Well-implemented credit cost information componentThe
JobCreditCostInformation
component clearly communicates job credit costs, organization credit balance, and indicates that billing is not active during the testing phase. This provides excellent transparency to users.
657-685
: Good implementation of helper functions for bounding box handlingThe added helper functions
getBoundingBoxesForLayers
anduseCurrentlySelectedBoundingBox
improve code organization and reusability. The custom hook pattern is well-implemented and follows React best practices.
726-736
: Well-implemented credit cost fetchingThe implementation uses
useFetch
appropriately to retrieve job credit cost information when the bounding box or job type changes. The dependency array is correctly specified to trigger refetches when needed.
738-743
: Good synchronization of credit balanceThe implementation correctly updates the organization's credit balance in the store when new information is received from the API. This ensures the UI always displays the most current credit information.
787-798
: Well-handled organization refetch after job submissionThe code properly refetches the organization data after a job is submitted to ensure the credit balance is up-to-date. The error handling is appropriate, showing a user-friendly toast message if the refresh fails.
856-856
: Good conditional volume display based on credit relevanceThe code now shows the volume information in the bounding box selection only when it's relevant for credit calculations, which provides a cleaner UI when credits aren't involved.
866-871
: Well-integrated credit information displayThe credit cost information component is conditionally displayed only when job credit costs apply, keeping the UI clean and focused on relevant information.
916-916
: Successfully integrated cost per GVx for each job typeThe code correctly retrieves job-specific credit costs from the features configuration and passes them to the appropriate components. This modularity will make it easier to adjust pricing for specific job types in the future.
Also applies to: 993-993, 1108-1108, 928-928, 1003-1003, 1121-1121
571-575
:❓ Verification inconclusive
Remember to address the testing phase messaging
The JobCreditCostInformation component includes a notice that "Billing for this job is not active during testing phase". Ensure this is removed or updated when the credit system goes live.
🏁 Script executed:
#!/bin/bash # Search for testing phase references that need to be updated before go-live grep -n "testing phase" frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsxLength of output: 195
Action Required: Update "Testing Phase" Messaging for Go-Live
We've verified that the testing phase notice still appears at line 571 in
frontend/javascripts/oxalis/view/action-bar/starting_job_modals.tsx
:
- Line 571: "Billing for this job is not active during testing phase. This job is billed at …"
Please update or remove this message once the credit system goes live so that the billing information accurately reflects the live behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (2)
conf/application.conf (2)
156-160
: Consider adding explanatory comments for the new cost parametersThese new configuration parameters for the credit system are clear, but adding explanatory comments would improve maintainability and help future developers understand their purpose. For example, you could explain that these costs represent the pricing for different job types in the credit system and clarify what "GVx" stands for (Gigavoxel).
+ # Cost parameters for the credit system (per Gigavoxel) neuronInferralCostPerGVx = 1 mitochondriaInferralCostPerGVx = 0.5 alignmentCostPerGVx = 0.5 + # Conversion rates for credit purchases costPerCreditInEuro = 5 costPerCreditInDollar = 5.75
294-294
: Consider adding a comment for the monthlyFreeCredits parameterAdding a comment would clarify the purpose of this configuration parameter and how it's used in the credit system.
workerLivenessTimeout = 1 minute + # Number of free credits allocated to organizations each month monthlyFreeCredits = 2
📜 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.snap
is excluded by!**/*.snap
📒 Files selected for processing (2)
conf/application.conf
(2 hunks)frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md
(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/javascripts/test/snapshots/public-test/test-bundle/test/backend-snapshot-tests/misc.e2e.js.md
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
16-16
: Improve clarity and grammatical correctness.Consider hyphenating “long running” to “long‑running” for proper adjective usage. Additionally, inserting a comma after “For now” and adding “the” before “testing phase” will improve the sentence flow and clarity.
- Added a credit system making payment for long running jobs possible. For now it is in testing phase. [#8352](https://github.com/scalableminds/webknossos/pull/8352) + Added a credit system making payment for long‑running jobs possible. For now, it is in the testing phase. [#8352](https://github.com/scalableminds/webknossos/pull/8352)🧰 Tools
🪛 LanguageTool
[misspelling] ~16-~16: This word is normally spelled with a hyphen.
Context: ...dded a credit system making payment for long running jobs possible. For now it is in testing...(EN_COMPOUNDS_LONG_RUNNING)
[typographical] ~16-~16: Consider adding a comma here.
Context: ...payment for long running jobs possible. For now it is in testing phase. [#8352](https:/...(FOR_NOW_COMMA)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ...running jobs possible. For now it is in testing phase. [#8352](https://github.com/scala...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CHANGELOG.unreleased.md
(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[misspelling] ~16-~16: This word is normally spelled with a hyphen.
Context: ...dded a credit system making payment for long running jobs possible. For now it is in testing...
(EN_COMPOUNDS_LONG_RUNNING)
[typographical] ~16-~16: Consider adding a comma here.
Context: ...payment for long running jobs possible. For now it is in testing phase. [#8352](https:/...
(FOR_NOW_COMMA)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ...running jobs possible. For now it is in testing phase. [#8352](https://github.com/scala...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
CHANGELOG.unreleased.md (1)
16-16
: Refine the Changelog Entry Description.To improve clarity and consistency:
- Hyphenate "long running" as "long-running".
- Add a comma after "For now" for better readability.
- Optionally, consider inserting "the" before "payment" for improved grammatical flow.
For example, consider applying the following diff:
- Added a credit system making payment for long running jobs possible. For now it is in testing phase. + Added a credit system making payment for long-running jobs possible. For now, it is in testing phase.🧰 Tools
🪛 LanguageTool
[misspelling] ~16-~16: This word is normally spelled with a hyphen.
Context: ...dded a credit system making payment for long running jobs possible. For now it is in testing...(EN_COMPOUNDS_LONG_RUNNING)
[typographical] ~16-~16: Consider adding a comma here.
Context: ...payment for long running jobs possible. For now it is in testing phase. [#8352](https:/...(FOR_NOW_COMMA)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ...running jobs possible. For now it is in testing phase. [#8352](https://github.com/scala...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CHANGELOG.unreleased.md
(1 hunks)conf/messages
(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- conf/messages
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.unreleased.md
[misspelling] ~16-~16: This word is normally spelled with a hyphen.
Context: ...dded a credit system making payment for long running jobs possible. For now it is in testing...
(EN_COMPOUNDS_LONG_RUNNING)
[typographical] ~16-~16: Consider adding a comma here.
Context: ...payment for long running jobs possible. For now it is in testing phase. [#8352](https:/...
(FOR_NOW_COMMA)
[uncategorized] ~16-~16: You might be missing the article “the” here.
Context: ...running jobs possible. For now it is in testing phase. [#8352](https://github.com/scala...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: circleci_build
forgot to give green after final frontend remarks
This PR adds 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:
General Testing
Testing failed & successful jobs
throw Exception
statement).state = 'Completed'
.TODOs:
test postgres scheduler (pg_cron) to run automaticallyIssues:
(Please delete unneeded items, merge only when none are left open)