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
- dev/T348758
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 29500 Build 10801: 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 ↗ | (On Diff #3370) | "Map the EXISTING skus" or "Get the existing skus indexed by id" |
| 372 ↗ | (On Diff #3370) | I guess we could group and count in sql here. |
| 384 ↗ | (On Diff #3370) | $existing and $requested, right? |
| 388 ↗ | (On Diff #3370) | Throw an exception instead of abort(500), it will land in the log. |
| 393 ↗ | (On Diff #3370) | this IF is redundant. |
| 403 ↗ | (On Diff #3370) | elseif |
| src/tests/Feature/Controller/UsersTest.php | ||
| 48 ↗ | (On Diff #3370) | Uncomment. |
| src/app/Http/Controllers/API/V4/UsersController.php | ||
|---|---|---|
| 372 ↗ | (On Diff #3370) | 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 | ||
|---|---|---|
| 473 ↗ | (On Diff #3418) | We fetched the list already, so I'd prefer to not count using sql, on every iteration. |
| src/app/User.php | ||
|---|---|---|
| 470 ↗ | (On Diff #3418) | 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). |