fix: lock before generating invoice number#1243
Conversation
WalkthroughThis change refactors invoice number generation in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CollectionCampService
participant DB
User->>CollectionCampService: generateInvoiceNumber(op, objectName, objectId, objectRef)
CollectionCampService->>DB: Begin Transaction
CollectionCampService->>DB: SELECT sequence row FOR UPDATE
alt Sequence not initialized
CollectionCampService->>User: Throw Exception
DB-->>CollectionCampService: Rollback
else
CollectionCampService->>DB: Increment sequence value
CollectionCampService->>DB: UPDATE sequence value
CollectionCampService->>DB: UPDATE contribution with new invoice number
CollectionCampService->>DB: Commit
CollectionCampService->>User: Log assignment
end
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (4)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
| if (!$contributionId) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This was removed because this was already being ensured earlier in the method:
if ($objectName !== 'Contribution' || !$objectRef->id) {
return;
}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (2)
1708-1712: Add validation for the invoice number componentsThe code assumes the database values are valid but doesn't validate the prefix format or check for integer overflow.
Consider adding validation:
$last = (int) $dao->value; $prefix = $dao->label; + + // Validate prefix format + if (!preg_match('/^[A-Z0-9_\/-]+$/', $prefix)) { + throw new \Exception("Invalid invoice prefix format"); + } + + // Check for potential overflow + if ($last >= PHP_INT_MAX - 1) { + throw new \Exception("Invoice sequence number overflow"); + } + $next = $last + 1; $newInvoice = $prefix . $next;
1695-1720: Consider using CiviCRM's API for consistencyThe method uses direct SQL queries for option value operations while using the API for contribution updates. This inconsistency makes the code harder to maintain.
While the current implementation works correctly for ensuring atomicity, consider investigating if CiviCRM's API supports the necessary locking mechanisms. If not, document why direct SQL is necessary here for future maintainers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php(2 hunks)
🔇 Additional comments (3)
wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php (3)
1539-1542: Good defensive programming practice!Adding this early return check prevents unnecessary processing when
sourceIDis falsy, improving code efficiency and preventing potential null reference issues downstream.
1544-1547: Excellent null check addition!This early return prevents further processing when
contributionCampaignIdis falsy, maintaining consistency with the defensive programming approach used throughout the method.
1693-1720: Excellent implementation of atomic invoice number generation!The use of database transactions with
SELECT ... FOR UPDATEproperly addresses the race condition issue mentioned in the PR title. This ensures that concurrent requests won't generate duplicate invoice numbers. The transaction management with proper rollback on failure is a best practice for maintaining data integrity.
| throw new \Exception("Invoice sequence not initialized for prefix $invoiceSeqName"); | ||
| } |
There was a problem hiding this comment.
Security consideration: Avoid exposing internal implementation details
The exception message reveals the internal naming convention of your invoice sequences, which could be useful information for attackers.
Consider using a more generic error message:
- throw new \Exception("Invoice sequence not initialized for prefix $invoiceSeqName");
+ throw new \Exception("Invoice sequence not properly initialized");Log the detailed error internally for debugging purposes while keeping the exception message generic.
🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php at
lines 1705-1706, the exception message exposes internal invoice sequence naming
details. Replace the detailed message with a generic error message like "Invoice
sequence initialization error." Log the original detailed message internally for
debugging without exposing it in the exception thrown.
| $params = [ | ||
| 1 => [$invoicePrefix . '%', 'String'], | ||
| ]; | ||
| $invoiceSeqName = 'GNJCRM_25_26'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the invoice sequence name configurable
The hardcoded value 'GNJCRM_25_26' appears to be fiscal year specific. This will require code changes every fiscal year, which violates the DRY principle and creates a maintenance burden.
Consider these alternatives:
- Make it a class constant that can be easily updated
- Retrieve it from a configuration setting
- Calculate it dynamically based on the current fiscal year
Example implementation for option 3:
- $invoiceSeqName = 'GNJCRM_25_26';
+ $currentYear = date('y');
+ $nextYear = ($currentYear + 1) % 100;
+ $invoiceSeqName = sprintf('GNJCRM_%02d_%02d', $currentYear, $nextYear);🤖 Prompt for AI Agents
In wp-content/civi-extensions/goonjcustom/Civi/CollectionCampService.php at line
1691, the invoice sequence name is hardcoded as 'GNJCRM_25_26', which is fiscal
year specific and requires manual updates each year. To fix this, refactor the
code to avoid hardcoding by either defining the sequence name as a class
constant, fetching it from a configuration setting, or dynamically generating it
based on the current fiscal year. This will reduce maintenance overhead and
adhere to the DRY principle.
2 participants
Update logic for invoice number generation.
Why using transaction directly won't work
In our previous logic, we were asking MySQL:
But even if we run this inside a transaction, there is nothing to lock, because:
FOR UPDATEon a known, specific row.So two processes still get the same max invoice, both think “I’ll use 153”, and you get duplicates.
Why saving in option value Fixes This
Inside a transaction, this row is locked — no one else can increment until we finish.
Summary by CodeRabbit