Use the observer to bill the wallet
Details
- Reviewers
vanmeeuwen - Group Reviewers
Restricted Project - Commits
- rKfbf3f75e27ea: Make sure we bill for entitlements that are deleted
./phpunit
Diff Detail
- Repository
- rK kolab
- Branch
- arcpatch-D1297
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 29521 Build 10819: arc lint + arc unit
Event Timeline
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.
- Additional tests
- Correct how many SKUs to add at zero cost, beyond existing ones
- With doc
- Improve finding the entitleable object
- Adjust tests
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().
I think I may have fixed this by adding a \App\User::removeSku($sku, $count = 1) function.
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(... |
- 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
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). |