Page MenuHomekolab.org

Further changes to the reseller implementation
ClosedPublic

Authored by vanmeeuwen on Jun 30 2021, 8:55 AM.

Details

Reviewers
machniak
vanmeeuwen
Group Reviewers
Restricted Project
Commits
rKa8f0795ee00f: Further changes to the reseller implementation
Summary

See T452951

Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 35497
Build 13897: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
vanmeeuwen requested review of this revision.Jun 30 2021, 8:56 AM
vanmeeuwen updated this revision to Diff 7411.Jun 30 2021, 9:08 AM
  • Remove unnecessary fillable tenant_id
vanmeeuwen updated this revision to Diff 7423.Jun 30 2021, 10:22 AM
  • Rebase macro
machniak requested changes to this revision.Jun 30 2021, 10:40 AM
machniak added a subscriber: machniak.
machniak added inline comments.
src/app/Console/Command.php
65

We do this in getObjectModel() already.

src/app/Console/ObjectDeleteCommand.php
95

Shouldn't we be less-automatic in the scalpel mode?

src/app/Discount.php
33 ↗(On Diff #7405)

We don't do this in other models. We should be consistent in handling this property.

src/app/Domain.php
439

This query must have a tenant context.

446

This will not list the "admin without mailbox" user. Just saying.

src/app/Http/Controllers/API/V4/Reseller/StatsController.php
29

Well, we're in the V4/Reseller namespace. So, this check is redundant.

src/app/Observers/DomainObserver.php
63 ↗(On Diff #7405)

I wonder if this is not a problem that UserObserver deletes domains and DomainObserver deletes users. I'm thinking about some infinite loop. I hope Laravel is smart to handle attempts to delete already deleted object.

src/app/Observers/UserAliasObserver.php
33

Maybe we should also check if $alias->user is not null.

src/app/Providers/AppServiceProvider.php
100

I guess we could be more specific on the $object type. We expect a Model.

100–103

I don't like this method. 'withObjectTenantContext' means "with object's tenant" not "with object's tenant depending on who's asking".

src/app/Sku.php
45 ↗(On Diff #7405)

Inconsistent handling of this property, as above.

src/database/migrations/2020_05_05_095212_create_tenants_table.php
39–40

You set $tenantId above, you don't have to read the config again and again.

src/database/seeds/local/DiscountSeeder.php
29

Really, I like these commas. It makes the diff simpler if you add another entry to the array later.

src/database/seeds/local/DomainSeeder.php
37

tenant_id is set by the observer. And tenant_id is not fillable.

src/database/seeds/local/PackageSeeder.php
29

Redundant, not fillable.

src/tests/Browser/Admin/DashboardTest.php
50

I think we should use \config('app.passphrase') in tests. If it is not set, the test will fail anyway because the password will be different.

src/tests/Feature/UserTest.php
684

2 -> 5 in the comment

This revision now requires changes to proceed.Jun 30 2021, 10:40 AM
vanmeeuwen planned changes to this revision.Jun 30 2021, 11:12 AM
vanmeeuwen marked 16 inline comments as done.

Submitting the changes in just a moment.

src/app/Console/ObjectDeleteCommand.php
95

We establish here the difference between:

./artisan domain:delete kolab.org

only ever resulting in Domain('kolab.org')->deleted_at getting set with observers getting executed, and

./artisan domain:force-delete kolab.org

wiping the database records all-together, again with observers getting executed.

Thus leaving the space for scalpel commands to do the same without observers getting executed.

I.e. ./artisan scalpel:domain:delete on a deleted domain will, without observers, execute a force-delete.

src/app/Discount.php
33 ↗(On Diff #7405)

Artifact from reversing on adding tenant_id as a fillable. Will remove.

src/app/Observers/DomainObserver.php
63 ↗(On Diff #7405)

Removed for now.

src/app/Providers/AppServiceProvider.php
100–103

Only administrators are supposed to be allowed to switch tenant contexts from their own, or the APP_TENANT_ID, in to the object's tenant context. This is a sanity check against abuse of the macro.

That said, we should also add a CLI check.

src/app/Sku.php
45 ↗(On Diff #7405)

Artifact from trying to do seeders, will remove.

src/tests/Browser/Admin/DashboardTest.php
50

This is what \App\Utils::generatePassphrase() does for us -- it takes \config('app.passphrase') if available.

vanmeeuwen updated this revision to Diff 7429.Jun 30 2021, 11:13 AM
vanmeeuwen marked 5 inline comments as done.
  • Fix from review feedback
machniak added inline comments.Jun 30 2021, 11:42 AM
src/app/Console/ObjectDeleteCommand.php
95

That was preciseely my point. So, ./artisan scalpel:domain:delete does not "switch" into force-deleting "mode" if not told specifically.

src/tests/Browser/Admin/DashboardTest.php
50

I know, but if you read the code of the test it says "generate passphrase". It does not make sense here, just from the code readability point of view.

machniak requested changes to this revision.Jun 30 2021, 12:27 PM
machniak added inline comments.
src/app/Providers/AppServiceProvider.php
131

There's no $object defined here.

This revision now requires changes to proceed.Jun 30 2021, 12:27 PM
vanmeeuwen planned changes to this revision.Jun 30 2021, 1:31 PM
vanmeeuwen added inline comments.
src/app/Console/ObjectDeleteCommand.php
95

What do you mean it doesn't?

src/tests/Browser/Admin/DashboardTest.php
50

This is why I wanted to get away from tests using seeded fixtures -- but that is also a different effort.

vanmeeuwen updated this revision to Diff 7435.Jun 30 2021, 1:32 PM
  • Fix c/p error
vanmeeuwen marked an inline comment as done.Jun 30 2021, 1:44 PM

Marking a comment thread as done in the app service provider.

machniak requested changes to this revision.Jun 30 2021, 2:47 PM
[14:20:34] <alec> I'm getting quite a lot of failing tests: Errors: 20, Failures: 51, Skipped: 7, Incomplete: 14. just from --testsuite=Feature 
[14:20:34] <alec> I guess you might be running tests with a different APP_TENANT_ID or sth 
[14:20:54] <alec> I have APP_TENANT_ID=1 in .env 
[14:22:57] <alec> damn, and I've run it with production mollie key, there 4 new users created, can you remove them? 
[14:27:32] <alec> also APP_PASSPHRASE=simple123 is needed when seeding and running tests 
[14:41:48] <alec> for browser tests: Errors: 16, Failures: 8, Skipped: 14, Incomplete: 1. 
[14:43:14] <alec> this is what I expected, changing default set of entitlements/skus is breaking a lot of tests 
[14:46:14] <alec> also phpstan has a lot to say
This revision now requires changes to proceed.Jun 30 2021, 2:47 PM
vanmeeuwen updated this revision to Diff 7441.Jun 30 2021, 3:45 PM
  • Pretend that commandPrefix is available in Command
  • Label the use of builder for the sake of phpstan
  • Update phpstan.neon patterns
  • Model -> object
vanmeeuwen updated this revision to Diff 7447.Jun 30 2021, 4:25 PM
  • Adjust some more of them numbers
vanmeeuwen updated this revision to Diff 7465.Jul 2 2021, 7:15 AM
  • Correct some more numbers
  • Correct tests
machniak updated this revision to Diff 7483.Jul 7 2021, 2:51 PM
  • Fix browser tests
vanmeeuwen updated this revision to Diff 7489.Jul 8 2021, 11:19 AM
  • Fix some of the feature tests
machniak updated this revision to Diff 7495.Jul 9 2021, 10:33 AM
  • Fix obvious test regressions and a bug in the signup controller
machniak updated this revision to Diff 7501.Jul 9 2021, 12:38 PM
  • Fix various places where tenant context wasn't applied when selecting a sku record
machniak updated this revision to Diff 7507.Jul 9 2021, 3:16 PM
  • Fix reseller's GroupsController and tests for it
machniak updated this revision to Diff 7513.Jul 9 2021, 4:46 PM
  • Fix InvitationsController
machniak updated this revision to Diff 7519.Jul 9 2021, 8:00 PM
  • Fix Group and Domain controllers, use new helper checkTenant()
machniak updated this revision to Diff 7525.Jul 12 2021, 11:44 AM
  • Remove unused Packages controllers for admin and reseller API
  • Fix SKU controllers
  • Fix Wallet controllers
  • Fix Users controllers
  • /api/discounts -> /api/users/{id}/discounts as we need the user context for this
vanmeeuwen accepted this revision.Jul 30 2021, 7:40 AM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 30 2021, 7:52 AM
This revision was automatically updated to reflect the committed changes.