New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add options to the user:sync command to handle accounts #27798

Merged
merged 2 commits into from May 8, 2017

Conversation

Projects
None yet
4 participants
@jvillafanez
Member

jvillafanez commented May 2, 2017

Description

Handle missing accounts during user syncing. Requires #27796 to work properly.

Related Issue

Motivation and Context

How Has This Been Tested?

Manual tested all the options asked, plus without interaction.

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jvillafanez jvillafanez added this to the 10.0.1 milestone May 2, 2017

Show outdated Hide outdated core/Command/User/SyncBackend.php
foreach ($toBeDeleted as $u) {
$output->writeln($u);
$userAccount = $this->userManager->get($u);

This comment has been minimized.

@jvillafanez

jvillafanez May 2, 2017

Member

$syncService->getNoLongerExistingUsers should return Account object in order to avoid this.

@jvillafanez

jvillafanez May 2, 2017

Member

$syncService->getNoLongerExistingUsers should return Account object in order to avoid this.

@DeepDiver1975 DeepDiver1975 self-requested a review May 2, 2017

Show outdated Hide outdated core/Command/User/SyncBackend.php
@@ -99,7 +99,7 @@ protected function execute(InputInterface $input, OutputInterface $output) {
$helper = $this->getHelper('question');
$question = new ChoiceQuestion(
'If unknown users are found, what do you want to do with their accounts? (removing the account will also remove its data)',
['nothing', 'disable accounts', 'remove accounts'],
['nothing', 'disable accounts', 'remove accounts', 'ask later'],

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 3, 2017

Member

we agreed on disabling by default and delete as option.
the option has to be confirmed in interactive more.

why are there two more modes?

@DeepDiver1975

DeepDiver1975 May 3, 2017

Member

we agreed on disabling by default and delete as option.
the option has to be confirmed in interactive more.

why are there two more modes?

Show outdated Hide outdated core/Command/User/SyncBackend.php
case 'nothing':
$output->writeln("Please delete them after careful verification.");
$this->doActionForAccountUids($toBeDeleted,
function($uid, $ac) use ($output) { $output->writeln($uid); },

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 May 3, 2017

Member

no onliners - nerver - ever

@DeepDiver1975

DeepDiver1975 May 3, 2017

Member

no onliners - nerver - ever

Show outdated Hide outdated core/Command/User/SyncBackend.php
$helper = $this->getHelper('question');
$question = new ChoiceQuestion(
'If unknown users are found, what do you want to do with their accounts? (removing the account will also remove its data)',
['disable accounts', 'remove accounts', 'ask later'],

This comment has been minimized.

@jvillafanez

jvillafanez May 3, 2017

Member

The "ask later" option is used to make a decision about what to do with the users after the admin knows who is affected.

  • If the admin wants to remove the accounts maybe he want to make sure what accounts will be removed.
  • If the admin wants to disable the accounts he can answer the question before starting the process and let the command run.
@jvillafanez

jvillafanez May 3, 2017

Member

The "ask later" option is used to make a decision about what to do with the users after the admin knows who is affected.

  • If the admin wants to remove the accounts maybe he want to make sure what accounts will be removed.
  • If the admin wants to disable the accounts he can answer the question before starting the process and let the command run.
@wravenm

This comment has been minimized.

Show comment
Hide comment
@wravenm

wravenm commented May 3, 2017

n

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 4, 2017

Member

The action is not controlable via arguments/options? Please adjust

Member

DeepDiver1975 commented May 4, 2017

The action is not controlable via arguments/options? Please adjust

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 4, 2017

Member

And please test your code - this is not working ....

Member

DeepDiver1975 commented May 4, 2017

And please test your code - this is not working ....

@davitol

This comment has been minimized.

Show comment
Hide comment
@davitol

davitol May 4, 2017

Contributor

LGTM with the tests done until now adding #27796 + owncloud/user_ldap#85

Contributor

davitol commented May 4, 2017

LGTM with the tests done until now adding #27796 + owncloud/user_ldap#85

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 4, 2017

Member

rebased and adjustments to make occ usage easier

Member

DeepDiver1975 commented May 4, 2017

rebased and adjustments to make occ usage easier

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 4, 2017

Member

a disabled users will be reported again in case the command is rerun. but I guess this is okay since in a second run the admin might want to remove that users

Member

DeepDiver1975 commented May 4, 2017

a disabled users will be reported again in case the command is rerun. but I guess this is okay since in a second run the admin might want to remove that users

jvillafanez and others added some commits May 2, 2017

Add options to the user:sync command to handle accounts
Allow answering after seeing the unknown users

Remove "nothing" option and avoid using oneliners

Add cli options for the available actions for missing accounts
@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 May 4, 2017

Member

and I squashed them

Member

DeepDiver1975 commented May 4, 2017

and I squashed them

@DeepDiver1975 DeepDiver1975 merged commit 84d1220 into master May 8, 2017

4 checks passed

Scrutinizer 4 new issues, 2 updated code elements
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@DeepDiver1975 DeepDiver1975 deleted the sync_with_delete branch May 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment