The UserSeeder shows how to assign a package to a user, and it resulting in multiple storage SKUs
Details
- Reviewers
vanmeeuwen machniak - Group Reviewers
Restricted Project - Commits
- rKb554235ea392: Show functioning without quota table
None yet
Diff Detail
- Repository
- rK kolab
- Branch
- dev/without-quota-table
- Lint
No Lint Coverage - Unit
No Test Coverage - Build Status
Buildable 28057 Build 10012: arc lint + arc unit
Event Timeline
src/app/Domain.php | ||
---|---|---|
224 | This debug line is redundant. | |
src/app/Handlers/Domain.php | ||
7 | This is the same namespace, so just "Base" here. | |
src/app/Handlers/Resource.php | ||
14 | I guess $entitlement could be typed as it's always App\Entitlement. The 2nd argument is indeed mixed type. | |
src/app/Package.php | ||
37 | My old method used entitleableClass() to make the code more universal. I.e. the calling code didn't have to know with with arguments to call the method. I mean, look at SignupController, it calls $package->assign($user), which means it will assing the domain-hosting SKU to a user not the domain. This is a bug. | |
61 | I'm pretty sure this will make a new query on every iteration. We should move that out of the foreach loop, so the query is executed only once. | |
src/database/migrations/2019_09_17_102628_create_sku_entitlements.php | ||
40 | Why these are commented? They should be nullable. | |
src/tests/Feature/Controller/UsersTest.php | ||
93 ↗ | (On Diff #2236) | This debug is not needed anymore. |
src/tests/Feature/SkuTest.php | ||
26 | As registerEntitlement() method does not exist now, I think we need tests for Package::assign() method in a similar manner. |
- Clear almost all phpstan issues
- Stubbornly omit any changes to packages-lock.json from the differential
- Remove superfluous debug commands
- Remove debugging
src/app/Entitlement.php | ||
---|---|---|
79 | This is against what Laravel documentation does. I think this is wrong. | |
src/app/Http/Controllers/API/PasswordResetController.php | ||
109 | This is not consistent with other places. I propose to use the full namespace here. | |
src/app/Observers/EntitlementObserver.php | ||
22 | I think that maybe we should throw exceptions instead of returning false here. | |
src/app/User.php | ||
213 | Really? One-liner looked much better. |
src/app/Entitlement.php | ||
---|---|---|
79 | Yeah, it is indeed. | |
src/app/Http/Controllers/API/PasswordResetController.php | ||
109 | Fine with me either way. | |
src/app/Observers/EntitlementObserver.php | ||
22 | Returning false will prohibit the "transaction" from succeeding / being committed. | |
src/app/User.php | ||
213 | Yeah, I did the one-liner for app/Domain.php. | |
src/database/migrations/2019_09_17_102628_create_sku_entitlements.php | ||
40 | They weren't before, and they are not now. The entitleable_id and entitleable_type are effectively the object of the entitlement, not the subject. It basically implies every SKU entitlement applies to either a \App\Domain or a \App\User. |
src/app/Handlers/Domain.php | ||
---|---|---|
7 | I dislike these "incomplete" references tbh. They're sometimes necessary to maintain legibility of non-rendered documentation (i.e. inline while editing), if the absolute path is very long. However, should in no way be considered a show-stopper. The point is that observers rely on functions existing, and therefore we need to extend an abstract class by convention. | |
src/app/Package.php | ||
37 | It should be the inverse, i.e. User->assignPackage($package), to preserve the narrative of a customer taking something off the shelf. | |
61 | I'll move it out. |
- Use the iterable as intended
- Use the iterable as intended
- Remove superfluous query log output
- List sensible entitlements for a user
- Add back "verified" with DNS_ANY
- Use contains() on the collection
- Move obtaining the wallet ID out of the for loop
According to domain verification. You did not uncomment all relevant parts. First remove <exclude>tests/Feature/Jobs/DomainVerifyTest.php</exclude> from phpunit.xml. Then check:
- app/Http/Controllers/API/UsersController.php $steps['domain-verified'] = 'isVerified'
- app/Observers/DomainObserver.php
- tests/Feature/Controller/UsersTest.php testStatusInfo()
- tests/Feature/DomainTest.php - first /* */ comment in testCreateJobs()
- tests/Unit/DomainTest.php
- Provide a function or two that deletes users and domains for realz
- Exclude additional directories from documentation
- Recommended way to run phpstan
- Recommended way to run phpunit
- Do not verify peer, peer name nor host by default
- Coverage reporting
- It's much more comfortable to not have to type ../bin/ and instead go with ./
- Assorted changes
src/app/Http/Controllers/API/SignupController.php | ||
---|---|---|
230 | I guess, this is not finished yet, cause I see you ignore the package plan. | |
src/tests/Feature/DomainOwnerTest.php | ||
44 ↗ | (On Diff #2266) | As it is really testing assignPackage() method I'd rather see this in UsersTest.php. Or if you want to keep it this way maybe we need Feature/UserStories subfolder. The place where it is in just does not look right to me. |
src/tests/Feature/DomainTest.php | ||
64 ↗ | (On Diff #2266) | This test was supposed to test that the job has been created, that's it. The job execution is tested in Featuire/Jobs. What's the point in executing the job here if you don't check the result, anyway. |
src/tests/Feature/Jobs/DomainCreateTest.php | ||
35 ↗ | (On Diff #2266) | Why this? DomainCreate job does not start other jobs. |
src/tests/Feature/Jobs/UserVerifyTest.php | ||
34 ↗ | (On Diff #2266) | The test was testing failure and success case. After mocks removal I expect this assertion to fail. The expected result should be that isImapReady() is true. As I mention in another comment this test file is disabled in phpunit.xml. |
src/tests/Feature/SkuTest.php | ||
65 | This line wasn't really that long, should we change the limit? |
- Simplify sku testing code in handlers, add documentation for entitlement
- Add ../bin/phpdoc and symbolic link
- Assert the mailbox SKU is part of (currently) two packages (lite, kolab)
- Pivot with qty
- Use qty to calculate minimal costs if qty_min isn't set
- Add the qty to the database
- Alphabetical ;-)
- 10 users is enough
- Test costs calculation
- Ensure the tests leave a clean installation
- Only update a password if its actually empty
- Correct typehinting
- Change over to user->assignPlan()
- Provide user->assignPlan()
- Cleanup before and after browser tests
- Add delete test users and domains
- Also execute browser tests
- Remove package-lock.json from git
- Package->assign() is no longer needed
- Add browser tests to a complete phpunit run
- Ensure the second test user is also deleted
src/app/Auth/LDAPUserProvider.php | ||
---|---|---|
109 | This whole if statement is wrong. 1. I don't see $user->save() here after ldap_password is set. 2. It should be password_ldap not ldap_password. 3. Because of User::setPasswordAttribute() it shouldn't be needed at all. | |
src/app/Http/Controllers/API/SignupController.php | ||
228–231 | The comment does not say truth now | |
src/app/Observers/UserObserver.php | ||
89 ↗ | (On Diff #2281) | You don't need the loop, you can just cal ->delete() instead of ->get(). |
src/app/User.php | ||
100 | The $user argument can be a domain object too. And I see the method body is still not aware of that. | |
150 | Without checking entitleable_type this may return wrong results, I guess. Also, that method might end up to be not that useful if it does not return the domain entitlement. Something's looking fishy or just unfinished here. |