Page MenuHomePhorge

Make sure we bill for entitlements that are deleted
ClosedPublic

Authored by vanmeeuwen on May 27 2020, 11:36 AM.
Tags
None
Referenced Files
F11584488: D1297.id3418.diff
Thu, Mar 28, 8:13 AM
F11581101: D1297.id.diff
Thu, Mar 28, 1:54 AM
F11575737: D1297.id3352.diff
Wed, Mar 27, 6:48 PM
Unknown Object (File)
Wed, Mar 20, 10:32 AM
Unknown Object (File)
Fri, Mar 1, 1:02 AM
Unknown Object (File)
Feb 25 2024, 5:15 PM
Unknown Object (File)
Feb 23 2024, 2:44 AM
Unknown Object (File)
Feb 23 2024, 2:12 AM
Subscribers
Restricted Project

Details

Reviewers
vanmeeuwen
Group Reviewers
Restricted Project
Commits
rKfbf3f75e27ea: Make sure we bill for entitlements that are deleted
Summary

Use the observer to bill the wallet

Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Branch
arcpatch-D1297
Lint
No Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 29836
Build 11047: arc lint + arc unit

Event Timeline

vanmeeuwen created this revision.

This will need to be rebased on top of an accepted D1102.

So far so good. I have two stories to consider:

  1. User is deleting and adding the same SKU every 13 days - got it for free.
  2. (After 14 days of using his account) user raises quota, then in 14 days he lowers it - got the "rised quota period" for free.

So far so good. I have two stories to consider:

  1. User is deleting and adding the same SKU every 13 days - got it for free.
  2. (After 14 days of using his account) user raises quota, then in 14 days he lowers it - got the "rised quota period" for free.

Yes to both.

  • 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

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().

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.

  • Add testUpdateEntitlements

Tests look good.

src/app/Http/Controllers/API/V4/UsersController.php
370–371

"Map the EXISTING skus" or "Get the existing skus indexed by id"

379

I guess we could group and count in sql here.

382

$existing and $requested, right?

386

Throw an exception instead of abort(500), it will land in the log.

391

this IF is redundant.

401

elseif

src/tests/Feature/Controller/UsersTest.php
48

Uncomment.

This revision now requires changes to proceed.May 28 2020, 10:45 AM
src/app/Http/Controllers/API/V4/UsersController.php
370–371

Oops, forgot to change the comment back after I attempted to select a more narrow set of SKUs using rSkus.

379

How?

382

Yes.

391

Ghe, I had a log statement here ;-)

src/app/Http/Controllers/API/V4/UsersController.php
379

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
474

We fetched the list already, so I'd prefer to not count using sql, on every iteration.

This revision now requires changes to proceed.Jun 5 2020, 12:50 PM
src/app/User.php
471

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).

  • Rollback transactions on exceptions
  • Performance optimization
This revision is now accepted and ready to land.Jun 15 2020, 2:50 PM
This revision was automatically updated to reflect the committed changes.