Page MenuHomePhorge

Further changes to the reseller implementation
ClosedPublic

Authored by vanmeeuwen on Jun 30 2021, 8:55 AM.
Tags
None
Referenced Files
F11586678: D2635.id7423.diff
Thu, Mar 28, 3:33 PM
Unknown Object (File)
Wed, Mar 27, 7:53 PM
Unknown Object (File)
Fri, Mar 22, 3:14 PM
Unknown Object (File)
Wed, Mar 20, 9:00 PM
Unknown Object (File)
Mon, Mar 18, 5:20 AM
Unknown Object (File)
Sun, Mar 10, 10:49 PM
Unknown Object (File)
Sun, Mar 10, 6:00 PM
Unknown Object (File)
Sun, Mar 10, 8:01 AM
Subscribers

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 Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 35449
Build 13855: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Remove unnecessary fillable tenant_id
machniak subscribed.
machniak added inline comments.
src/app/Console/Command.php
58

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

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

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
83

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

86

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

Inconsistent handling of this property, as above.

src/database/migrations/2020_05_05_095212_create_tenants_table.php
39

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

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

src/app/Observers/DomainObserver.php
63

Removed for now.

src/app/Providers/AppServiceProvider.php
86

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

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 marked 5 inline comments as done.
  • Fix from review feedback
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 added inline comments.
src/app/Providers/AppServiceProvider.php
108

There's no $object defined here.

This revision now requires changes to proceed.Jun 30 2021, 12:27 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.

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

[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
  • Pretend that commandPrefix is available in Command
  • Label the use of builder for the sake of phpstan
  • Update phpstan.neon patterns
  • Model -> object
  • Adjust some more of them numbers
  • Correct some more numbers
  • Correct tests
  • Fix some of the feature tests
  • Fix obvious test regressions and a bug in the signup controller
  • Fix various places where tenant context wasn't applied when selecting a sku record
  • Fix reseller's GroupsController and tests for it
  • Fix InvitationsController
  • Fix Group and Domain controllers, use new helper checkTenant()
  • 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
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.