Skip to content

occ commands to list users and groups#28312

Merged
DeepDiver1975 merged 3 commits into
masterfrom
occ-list-users-groups
Jul 6, 2017
Merged

occ commands to list users and groups#28312
DeepDiver1975 merged 3 commits into
masterfrom
occ-list-users-groups

Conversation

@phil-davis

Copy link
Copy Markdown
Contributor

Description

Provide the following new items for the occ command:

  1. ListGroupMembers.php - given a group name, list the members in that group
  2. ListGroups.php - list all groups on the system, with the ability to provide a string and filter the list to show only groups containing that string
  3. ListUserGroups.php - given a User ID, lists the groups which that user is i
  4. ListUsers.php - list all users on the system, with the ability to provide a string and filter the list to show only User IDs containing that string

Related Issue

#28311

Motivation and Context

Help with managing users and groups from the occ command line.

How Has This Been Tested?

Running the occ commands on a test system with various filters, no filtering, with non-existent user and group names.

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.

Signed-off-by: Phil Davis <phil@jankaritech.com>
@phil-davis phil-davis requested a review from mmattel July 5, 2017 16:15
@phil-davis

Copy link
Copy Markdown
Contributor Author

I don't see where there are existing "acceptance tests" that run various occ command line commands and check that the required actions or output actually happened.
@SergioBertolinSG do have that yet?

Because it would be good to know that occ commands do what they claim, both existing ones and these new ones.

@SergioBertolinSG

Copy link
Copy Markdown
Contributor

@phil-davis there are no tests for occ commands currently. There is this PR https://github.com/owncloud/automated-tests/pull/17
Not sure if it is worthy to look at it. They are in python and old.

@PVince81 a couple of questions:

  1. Do we want to check the output of the occ commands?
  2. Do we want to add this to the behat tests?

@phil-davis

Copy link
Copy Markdown
Contributor Author

Good to know. I would think that occ command tests should be quite easy to add to the integration tests and have behat scenarios. Just needs to go in the backlog of acceptance/integration tests to be written.

@mmattel

mmattel commented Jul 5, 2017

Copy link
Copy Markdown
Contributor

@phil-davis
a big thanks for that, really needed.
cant test, businesstrip...
pls add a documentatuon PR ( @settermjd - fyi)

@phil-davis

Copy link
Copy Markdown
Contributor Author

Doc issue owncloud-archive/documentation#3203 which I will follow up once people have had a chance to review and comment on this PR.

@phil-davis

Copy link
Copy Markdown
Contributor Author

Feel free to call in anyone else who should give review or feedback on expanding the occ command.

Comment thread core/Command/Group/ListGroupMembers.php Outdated

use OC\Core\Command\Base;
use OCP\IGroupManager;
use Symfony\Component\Console\Command\Command;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused

Comment thread core/Command/Group/ListGroupMembers.php Outdated
use OCP\IGroupManager;
use Symfony\Component\Console\Command\Command;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused

Comment thread core/Command/Group/ListGroupMembers.php Outdated
parent::configure();

$this
->setName('group:listmembers')

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

list-members

please adjust this naming convention to all group commands - THX

@phil-davis phil-davis Jul 6, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following existing occ commands do not have the "-":
app:getpath
user:lastseen
user:resetpassword
I guess these should not be changed, as that would break backward-compatibility if users have scripted/depend on them.

The following are new commands awaiting release in 10.0.3, so I can change those easily as there are no users depending on them yet:
group:addmember
group:removemember
see #28318

And I will fix the new commands introduced here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes - please fix all the new ones - and yes - we failed to name those old ones 😠

anyway - THX

Comment thread core/Command/Group/ListGroupMembers.php Outdated
$groupName = $input->getArgument('group');
$group = $this->groupManager->get($groupName);
if (!$group) {
$output->writeln('<error>Group "' . $groupName . '" does not exist</error>');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer interpolated strings fro better readability

$output->writeln("<error>Group $groupName does not exist</error>");

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copy-pasted this style from other code in these core/Command/... files.
Do I update everywhere so the code will look consistent?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally speaking I prefer to enhance/change code only if there is a need.
changing code 'just because of making it nice' is not the way to go.

thx

Signed-off-by: Phil Davis <phil@jankaritech.com>
@phil-davis

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975

  1. Unused "use" removed from the various files
  2. Hyphens added between command words
  3. output string building refactored
    It works for me in local testing. Hopefully Travis and Jenkins agree.

Comment thread core/Command/Group/ListGroups.php Outdated
->setName('group:list')
->setDescription('list groups')
->addArgument(
'group',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

search pattern or something would match better I guess

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I had 'pattern' in there at one stage yesterday.
If I make it 'searchpattern' do I write it:
'searchpattern' or
'search-pattern' or
'searchPattern'?
(and will do the same style of thing for ListUsers.php)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same naming schema search-pattern - THX

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@phil-davis phil-davis added this to the development milestone Jul 6, 2017
Signed-off-by: Phil Davis <phil@jankaritech.com>
@phil-davis

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975 IMHO this is good to go

@DeepDiver1975 DeepDiver1975 merged commit de2ef05 into master Jul 6, 2017
@DeepDiver1975 DeepDiver1975 deleted the occ-list-users-groups branch July 6, 2017 14:24
phil-davis added a commit that referenced this pull request Jul 6, 2017
* occ commands to list users and groups

Signed-off-by: Phil Davis <phil@jankaritech.com>

* Add hyphens to list-members and list-groups occ group commands

Signed-off-by: Phil Davis <phil@jankaritech.com>

* Use search-pattern as arg in group:list and user:list

Signed-off-by: Phil Davis <phil@jankaritech.com>
@mmattel

mmattel commented Jul 6, 2017

Copy link
Copy Markdown
Contributor

👍 thanks a lot
As always - pls do not forget documentation... ( @settermjd fyi)

@phil-davis

Copy link
Copy Markdown
Contributor Author

Doc issue owncloud-archive/documentation#3203 already raised, so it won;t get lost.
I will make the doc additions tomorrow and submit a PR to documentation repo.

DeepDiver1975 pushed a commit that referenced this pull request Jul 6, 2017
* occ commands to list users and groups

Signed-off-by: Phil Davis <phil@jankaritech.com>

* Add hyphens to list-members and list-groups occ group commands

Signed-off-by: Phil Davis <phil@jankaritech.com>

* Use search-pattern as arg in group:list and user:list

Signed-off-by: Phil Davis <phil@jankaritech.com>
phil-davis added a commit to owncloud-archive/documentation that referenced this pull request Jul 7, 2017
settermjd pushed a commit to owncloud-archive/documentation that referenced this pull request Oct 20, 2017
@lock

lock Bot commented Aug 3, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Aug 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants