Page MenuHomekolab.org

Make sure we bill for entitlements that are deleted
ClosedPublic

Authored by vanmeeuwen on May 27 2020, 11:36 AM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

vanmeeuwen requested review of this revision.May 27 2020, 11:36 AM
vanmeeuwen created this revision.
vanmeeuwen planned changes to this revision.May 27 2020, 11:37 AM

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.

vanmeeuwen updated this revision to Diff 3343.May 27 2020, 2:18 PM
  • Additional tests
  • Correct how many SKUs to add at zero cost, beyond existing ones
  • With doc
  • Improve finding the entitleable object
  • Adjust tests
vanmeeuwen updated this revision to Diff 3346.May 27 2020, 2:20 PM
  • Avoid unintentional changes
vanmeeuwen updated this revision to Diff 3352.May 27 2020, 2:38 PM
  • Fix phpstan issues

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.

vanmeeuwen updated this revision to Diff 3370.May 28 2020, 10:30 AM
  • Add testUpdateEntitlements
machniak requested changes to this revision.May 28 2020, 10:45 AM

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
vanmeeuwen added inline comments.May 28 2020, 11:28 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 ;-)

vanmeeuwen updated this revision to Diff 3388.May 28 2020, 11:28 AM
  • Feedback
machniak added inline comments.May 28 2020, 11:54 AM
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(...
vanmeeuwen updated this revision to Diff 3418.May 28 2020, 12:31 PM
  • 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
machniak requested changes to this revision.Fri, Jun 5, 12:50 PM

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
478

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

This revision now requires changes to proceed.Fri, Jun 5, 12:50 PM
machniak added inline comments.Fri, Jun 5, 1:09 PM
src/app/User.php
475

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

machniak updated this revision to Diff 3646.Fri, Jun 12, 11:01 AM
  • Rollback transactions on exceptions
  • Performance optimization
vanmeeuwen accepted this revision.Mon, Jun 15, 2:49 PM
vanmeeuwen removed a reviewer: machniak.
This revision is now accepted and ready to land.Mon, Jun 15, 2:50 PM
This revision was automatically updated to reflect the committed changes.