Details

Reviewers

vanmeeuwen
machniak

Group Reviewers

Restricted Project

Commits

rKb554235ea392: Show functioning without quota table

Summary

The UserSeeder shows how to assign a package to a user, and it resulting in multiple storage SKUs

Test Plan

None yet

Diff Detail

Repository

rK kolab

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

vanmeeuwen requested review of this revision.Feb 10 2020, 12:27 PM

vanmeeuwen created this revision.

Harbormaster completed remote builds in B28027: Diff 2236.Feb 10 2020, 12:27 PM

machniak requested changes to this revision.Feb 10 2020, 2:37 PM

machniak subscribed.

machniak added inline comments.

src/app/Domain.php
257	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 ↗	(On Diff #2236)	Why these are commented? They should be nullable.
src/tests/Feature/Controller/UsersTest.php
155	This debug is not needed anymore.
src/tests/Feature/SkuTest.php
34	As registerEntitlement() method does not exist now, I think we need tests for Package::assign() method in a similar manner.

This revision now requires changes to proceed.Feb 10 2020, 2:38 PM

Clear almost all phpstan issues
Stubbornly omit any changes to packages-lock.json from the differential
Remove superfluous debug commands
Remove debugging

Harbormaster completed remote builds in B28033: Diff 2242.Feb 10 2020, 2:54 PM

Must return a relationship
Dusk's installation will think it's production

Harbormaster completed remote builds in B28039: Diff 2248.Feb 10 2020, 3:40 PM

machniak added inline comments.Feb 10 2020, 4:08 PM

src/app/Entitlement.php
84	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
265	Really? One-liner looked much better.

vanmeeuwen planned changes to this revision.Feb 10 2020, 6:05 PM

vanmeeuwen added inline comments.

src/app/Entitlement.php
84	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
265	Yeah, I did the one-liner for `app/Domain.php`.
src/database/migrations/2019_09_17_102628_create_sku_entitlements.php
40 ↗	(On Diff #2236)	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.

Correct standards and inconsistencies

Harbormaster completed remote builds in B28045: Diff 2254.Feb 10 2020, 6:12 PM

vanmeeuwen planned changes to this revision.Feb 10 2020, 6:21 PM

vanmeeuwen marked 5 inline comments as done.

vanmeeuwen added inline comments.

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

Harbormaster completed remote builds in B28057: Diff 2260.Feb 11 2020, 8:31 AM

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

Harbormaster completed remote builds in B28084: Diff 2266.Feb 12 2020, 7:40 AM

machniak added inline comments.Feb 12 2020, 9:52 AM

src/app/Http/Controllers/API/SignupController.php
227	I guess, this is not finished yet, cause I see you ignore the package plan.
src/tests/Feature/DomainOwnerTest.php
45	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
61	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	Why this? DomainCreate job does not start other jobs.
src/tests/Feature/Jobs/UserVerifyTest.php
41–42	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
74	This line wasn't really that long, should we change the limit?

• JessicaJones121 subscribed.Feb 12 2020, 1:48 PM

This comment was removed by vanmeeuwen.

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

Harbormaster completed remote builds in B28105: Diff 2281.Feb 14 2020, 7:44 AM

machniak added inline comments.Feb 14 2020, 11:56 AM

src/app/Auth/LDAPUserProvider.php
105	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
225–226	The comment does not say truth now
src/app/Observers/UserObserver.php
89	You don't need the loop, you can just cal ->delete() instead of ->get().
src/app/User.php
102	The $user argument can be a domain object too. And I see the method body is still not aware of that.
203	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.

ldap_password -> password_ldap
Correct plan package assignment

Harbormaster completed remote builds in B28111: Diff 2287.Feb 20 2020, 11:17 AM

This revision was not accepted when it landed; it landed in state Needs Review.Feb 20 2020, 3:44 PM

Closed by commit rKb554235ea392: Show functioning without quota table (authored by vanmeeuwen, committed by machniak). · Explain Why

This revision was automatically updated to reflect the committed changes.

Show functioning without quota tableClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2293

bin/phpdoc

bin/phpstan

bin/phpunit

bin/quickstart.sh

phpdoc.dist.xml

src/.env.example

src/.gitignore

src/app/Auth/LDAPUserProvider.php

src/app/Backends/IMAP.php

src/app/Backends/LDAP.php

src/app/Console/Commands/PackageSkusCommand.php

src/app/Console/Commands/PlanPackagesCommand.php

src/app/Console/Commands/UserDomains.php

src/app/Console/Commands/UserEntitlementsCommand.php

src/app/Domain.php

src/app/Entitlement.php

src/app/Handlers/Base.php

src/app/Handlers/Domain.php

src/app/Handlers/DomainHosting.php

src/app/Handlers/DomainRegistration.php

src/app/Handlers/Groupware.php

src/app/Handlers/Mailbox.php

src/app/Handlers/Resource.php

src/app/Handlers/SharedFolder.php

src/app/Handlers/Storage.php

src/app/Http/Controllers/API/DomainsController.php

src/app/Http/Controllers/API/PasswordResetController.php

src/app/Http/Controllers/API/SignupController.php

src/app/Http/Controllers/API/UsersController.php

src/app/Jobs/DomainDelete.php

src/app/Jobs/UserDelete.php

src/app/Mail/PasswordReset.php

src/app/Observers/DomainObserver.php

src/app/Observers/EntitlementObserver.php

src/app/Observers/UserObserver.php

src/app/Package.php

src/app/PackageSku.php

src/app/Plan.php

src/app/PlanPackage.php

src/app/Quota.php

src/app/SignupCode.php

src/app/Sku.php

src/app/User.php

src/app/UserSetting.php

src/app/Utils.php

src/app/VerificationCode.php

src/app/Wallet.php

src/composer.json

src/config/imap.php

src/database/migrations/2019_10_10_095050_create_quota_table.php

src/database/migrations/2019_12_10_105942_create_plan_packages_table.php

src/database/migrations/2020_02_11_110959_add_deleted_at.php

src/database/seeds/DatabaseSeeder.php

src/database/seeds/DomainSeeder.php

src/database/seeds/PlanSeeder.php

src/database/seeds/UserSeeder.php

src/package-lock.json

src/phpdoc

src/phpstan

src/phpstan.neon

src/phpunit

src/phpunit.xml

src/tests/Browser/PasswordResetTest.php

src/tests/Browser/SignupTest.php

src/tests/DuskTestCase.php

src/tests/Feature/Controller/DomainsTest.php

src/tests/Feature/Controller/PasswordResetTest.php

src/tests/Feature/Controller/SignupTest.php

src/tests/Feature/Controller/UsersTest.php

src/tests/Feature/DomainOwnerTest.php

src/tests/Feature/DomainTest.php

src/tests/Feature/EntitlementTest.php

src/tests/Feature/Jobs/DomainCreateTest.php

src/tests/Feature/Jobs/DomainVerifyTest.php

Show functioning without quota table
ClosedPublic
Actions

Revision Contents
Changeset List