The UserSeeder shows how to assign a package to a user, and it resulting in multiple storage SKUs
- Group Reviewers
- rKb554235ea392: Show functioning without quota table
This debug line is redundant.
This is the same namespace, so just "Base" here.
I guess $entitlement could be typed as it's always App\Entitlement. The 2nd argument is indeed mixed type.
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.
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.
Why these are commented? They should be nullable.
|93 ↗||(On Diff #2236)|
This debug is not needed anymore.
As registerEntitlement() method does not exist now, I think we need tests for Package::assign() method in a similar manner.
This is against what Laravel documentation does. I think this is wrong.
This is not consistent with other places. I propose to use the full namespace here.
I think that maybe we should throw exceptions instead of returning false here.
Really? One-liner looked much better.
Yeah, it is indeed.
Fine with me either way.
Returning false will prohibit the "transaction" from succeeding / being committed.
Yeah, I did the one-liner for app/Domain.php.
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.
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.
It should be the inverse, i.e. User->assignPackage($package), to preserve the narrative of a customer taking something off the shelf.
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'
- tests/Feature/Controller/UsersTest.php testStatusInfo()
- tests/Feature/DomainTest.php - first /* */ comment in testCreateJobs()
- 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
I guess, this is not finished yet, cause I see you ignore the package plan.
|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.
|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.
|35 ↗||(On Diff #2266)|
Why this? DomainCreate job does not start other jobs.
|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.
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
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.
The comment does not say truth now
|89 ↗||(On Diff #2281)|
You don't need the loop, you can just cal ->delete() instead of ->get().
The $user argument can be a domain object too. And I see the method body is still not aware of that.
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.