Page MenuHomekolab.org

Show functioning without quota table
ClosedPublic

Authored by vanmeeuwen on Feb 10 2020, 12:27 PM.

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
Branch
dev/without-quota-table
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 28045
Build 10006: arc lint + arc unit

Event Timeline

vanmeeuwen requested review of this revision.Feb 10 2020, 12:27 PM
vanmeeuwen created this revision.
machniak requested changes to this revision.Feb 10 2020, 2:37 PM
machniak added a subscriber: machniak.
machniak added inline comments.
src/app/Domain.php
225

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.

This revision now requires changes to proceed.Feb 10 2020, 2:38 PM
vanmeeuwen updated this revision to Diff 2242.Feb 10 2020, 2:54 PM
  • Clear almost all phpstan issues
  • Stubbornly omit any changes to packages-lock.json from the differential
  • Remove superfluous debug commands
  • Remove debugging
vanmeeuwen updated this revision to Diff 2248.Feb 10 2020, 3:40 PM
  • Must return a relationship
  • Dusk's installation will think it's production
machniak added inline comments.Feb 10 2020, 4:08 PM
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.

vanmeeuwen planned changes to this revision.Feb 10 2020, 6:05 PM
vanmeeuwen added inline comments.
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.

vanmeeuwen updated this revision to Diff 2254.Feb 10 2020, 6:12 PM
  • Correct standards and inconsistencies
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.

vanmeeuwen updated this revision to Diff 2260.Feb 11 2020, 8:31 AM
  • 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
vanmeeuwen updated this revision to Diff 2266.Feb 12 2020, 7:40 AM
  • 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
machniak added inline comments.Feb 12 2020, 9:52 AM
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?

This comment was removed by vanmeeuwen.
vanmeeuwen updated this revision to Diff 2281.Feb 14 2020, 7:44 AM
  • 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
machniak added inline comments.Feb 14 2020, 11:56 AM
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.

vanmeeuwen updated this revision to Diff 2287.Feb 19 2020, 10:33 AM
  • ldap_password -> password_ldap
  • Correct plan package assignment
This revision was not accepted when it landed; it landed in state Needs Review.Feb 20 2020, 3:44 PM
This revision was automatically updated to reflect the committed changes.