Page MenuHomekolab.org

Deleting users/accounts
ClosedPublic

Authored by machniak on Mar 5 2020, 1:59 PM.

Details

Reviewers
vanmeeuwen
Group Reviewers
Restricted Project
Commits
rK1035ab7af7aa: Deleting users/accounts
Summary
  • Implement proper users listing, added tests for it
  • Deleting users/accounts

TODO: more browser tests

Test Plan

./phpunit

Diff Detail

Repository
rK kolab
Branch
dev/user-delete
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 28435
Build 10189: arc lint + arc unit

Event Timeline

machniak requested review of this revision.Mar 5 2020, 1:59 PM
machniak created this revision.
machniak retitled this revision from Implement proper users listing, added tests for it to Deleting users/accounts.Mar 5 2020, 2:01 PM
machniak edited the summary of this revision. (Show Details)
machniak edited the test plan for this revision. (Show Details)
machniak added a reviewer: Restricted Project.
machniak added a subscriber: Restricted Project.
vanmeeuwen requested changes to this revision.Mar 5 2020, 3:58 PM
vanmeeuwen added a subscriber: vanmeeuwen.
vanmeeuwen added inline comments.
src/app/Observers/DomainObserver.php
54

Shouldn't this be a progression on the dispatched deletion process having completed?

57

How about $domain->save()?

This revision now requires changes to proceed.Mar 5 2020, 3:58 PM
machniak added inline comments.Mar 5 2020, 6:46 PM
src/app/Observers/DomainObserver.php
54

It could, indeed. Then we should change isDeleted() so it also returns true when !empty($this->deleted_at), or not? I don't want to make things more complicated at the moment.

57

save() would work, but would generate updated/updating event which we don't need/want.

vanmeeuwen added inline comments.Mar 5 2020, 9:09 PM
src/app/Observers/DomainObserver.php
54

The question is then how the complete process of deleting a domain (or a user for that matter) is reflected in real life, vs. the database record.

I believe I intended for the database to reflect in 'deleted_at' the semantics for UI/UX -- including billing and pro rata temporis if you will, and for STATUS_DELETED to be the final confirmation.

In a sense, deleting could spawn the dispatched process -- it creates a series of actions to need to have occurred much like creating a user does (ldap ready, imap ready).

I think it is there where I figured STATUS_DELETED would add value compared to just deleted_at.

57

I think we need to settle what deleting and deleted is compared to deleted_at vs. STATUS_DELETED, before we need to consider $domain->save() and its triggering of updating()/updated().

src/app/Observers/UserObserver.php
95

Think... "remove entitlements billed to wallets owned".

When john@kolab.org owns a wallet to which all entitlements associated with any @kolab.org-related entitlements are billed -- and how would he not?? --, then iterating over his wallets and entitlements billed to said wallets is a very predictable path.

machniak updated this revision to Diff 2491.Mar 6 2020, 1:34 PM
  • Set domain/user STATUS_DELETED after removing from ldap
  • Added more browser tests (that are failing at the moment)
machniak updated this revision to Diff 2497.Mar 6 2020, 3:40 PM
  • Fixes regarding wallets
machniak updated this revision to Diff 2512.Mar 9 2020, 11:02 AM
  • Use wallets to control access to UI elements
machniak updated this revision to Diff 2524.Mar 9 2020, 11:55 AM
  • Fix dispatching DomainDelete job - use ID instead of the Model object
vanmeeuwen requested changes to this revision.Mar 9 2020, 2:18 PM

Will require rebasing on to current master.

src/resources/vue/components/User/Profile.vue
60

This is where a merge on to current origin/master fails. Please double-check the intended result.

This revision now requires changes to proceed.Mar 9 2020, 2:18 PM
vanmeeuwen added inline comments.Mar 10 2020, 7:51 AM
src/app/Jobs/DomainDelete.php
30

$domain -> $domain_id.

src/app/User.php
373

This should really be:

return $this->select(['users.*', 'entitlements.wallet_id'])

because Query\Builder::select() only supports 0-1 parameters (see phpstan output).

machniak updated this revision to Diff 2530.Mar 10 2020, 10:01 AM
  • Rebase
  • Small refactoring regarding wallets/accounts
  • Remove redundant code
machniak updated this revision to Diff 2536.Mar 10 2020, 10:49 AM
  • Fix tests regressions
machniak updated this revision to Diff 2542.Mar 10 2020, 11:01 AM
  • Make UserVerifyTest less fragile - wait up to 10 seconds
vanmeeuwen accepted this revision.Mar 10 2020, 11:08 AM
This revision is now accepted and ready to land.Mar 10 2020, 11:08 AM
This revision was automatically updated to reflect the committed changes.