See T452951
Details
- Reviewers
machniak vanmeeuwen - Group Reviewers
Restricted Project - Commits
- rKa8f0795ee00f: Further changes to the reseller implementation
./phpunit
Diff Detail
- Repository
- rK kolab
- Branch
- master
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 35497 Build 13897: arc lint + arc unit
Event Timeline
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 |
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. |
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. |
src/app/Providers/AppServiceProvider.php | ||
---|---|---|
131 | There's no $object defined here. |
[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
- Pretend that commandPrefix is available in Command
- Label the use of builder for the sake of phpstan
- Update phpstan.neon patterns
- Model -> object
- 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