Page MenuHomePhorge

Show functioning without quota table
ClosedPublic

Authored by vanmeeuwen on Feb 10 2020, 12:27 PM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 22, 8:41 PM
Unknown Object (File)
Sun, Mar 17, 10:48 PM
Unknown Object (File)
Sun, Mar 17, 3:00 AM
Unknown Object (File)
Wed, Feb 28, 3:00 AM
Unknown Object (File)
Wed, Feb 28, 3:00 AM
Unknown Object (File)
Feb 21 2024, 2:10 PM
Unknown Object (File)
Feb 15 2024, 12:17 PM
Unknown Object (File)
Feb 12 2024, 8:52 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 Lint Coverage
Unit
No Test Coverage
Build Status
Buildable 28045
Build 10006: arc lint + arc unit

Event Timeline

vanmeeuwen created this revision.
machniak subscribed.
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
  • Clear almost all phpstan issues
  • Stubbornly omit any changes to packages-lock.json from the differential
  • Remove superfluous debug commands
  • Remove debugging
  • Must return a relationship
  • Dusk's installation will think it's production
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 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.

  • Correct standards and inconsistencies
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

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.

  • 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.