Use the observer to bill the wallet
Details
Details
- Reviewers
vanmeeuwen - Group Reviewers
Restricted Project - Commits
- rKfbf3f75e27ea: Make sure we bill for entitlements that are deleted
./phpunit
Diff Detail
Diff Detail
- Repository
- rK kolab
- Branch
- arcpatch-D1297
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 29542 Build 10837: arc lint + arc unit
Event Timeline
Comment Actions
So far so good. I have two stories to consider:
- User is deleting and adding the same SKU every 13 days - got it for free.
- (After 14 days of using his account) user raises quota, then in 14 days he lowers it - got the "rised quota period" for free.
Comment Actions
- Additional tests
- Correct how many SKUs to add at zero cost, beyond existing ones
- With doc
- Improve finding the entitleable object
- Adjust tests
Comment Actions
Looking at the code in context of removing entitlements, I see we might not handle removing storage entitlements properly. We have to add some tests to tests/Feature/Controller/UsersTest::testUpdateEntitlements().
Comment Actions
I think I may have fixed this by adding a \App\User::removeSku($sku, $count = 1) function.
Comment Actions
Tests look good.
| src/app/Http/Controllers/API/V4/UsersController.php | ||
|---|---|---|
| 363 | "Map the EXISTING skus" or "Get the existing skus indexed by id" | |
| 372 | I guess we could group and count in sql here. | |
| 384 | $existing and $requested, right? | |
| 388 | Throw an exception instead of abort(500), it will land in the log. | |
| 393 | this IF is redundant. | |
| 403 | elseif | |
| src/tests/Feature/Controller/UsersTest.php | ||
| 48 | Uncomment. | |
| src/app/Http/Controllers/API/V4/UsersController.php | ||
|---|---|---|
| 372 | Not tested and not that pretty: Entitlement::selectRaw('sku_id, count(*) as entitlement_count')
->where('entitleable_id', $user->id)
->groupBy('sku_id')
->get()
->each(... | |
Comment Actions
- Additional tests
- Correct how many SKUs to add at zero cost, beyond existing ones
- With doc
- Improve finding the entitleable object
- Adjust tests
- Avoid unintentional changes
- Fix phpstan issues
- Add testUpdateEntitlements
- Feedback
- Use counting on the sql server side
Comment Actions
This learned us that an exception does not rollback uncommitted transactions. Here's a general fix for this issue:
--- a/src/app/Exceptions/Handler.php +++ b/src/app/Exceptions/Handler.php @@ -5,6 +5,7 @@ namespace App\Exceptions; use Exception; use Illuminate\Auth\AuthenticationException; use Illuminate\Foundation\Exceptions\Handler as ExceptionHandler; +use Illuminate\Support\Facades\DB; class Handler extends ExceptionHandler { @@ -49,6 +50,11 @@ class Handler extends ExceptionHandler */ public function render($request, Exception $exception) { + // Rollback uncommitted transactions + while (DB::transactionLevel() > 0) { + DB::rollBack(); + } + return parent::render($request, $exception); }
| src/app/User.php | ||
|---|---|---|
| 472 | We fetched the list already, so I'd prefer to not count using sql, on every iteration. | |
| src/app/User.php | ||
|---|---|---|
| 469 | I'm also thinking that maybe we should sort them also by created_by. First, to have the process more defined. Second, to delete them in an order that will give us potenially more money (although, that's probably another rare corner-case). | |