Groups for occ command and UI testing by phil-davis · Pull Request #28190 · owncloud/core

phil-davis · 2017-06-22T06:09:58Z

Description

Add group add, delete and removeuser to occ command (group add can also add users to a group)
Add support for creating and removing groups withing the UI testing framework
Use this to change/add some sharing autocompletion tests to include sharing with groups as well as users

Related Issue

Addresses #20629 to provide occ group command
for which some initial work happened in #21627

Also part of providing base infrastructure for UI testing #27858

Motivation and Context

It is useful to be able to manage groups from occ as well as users
UI testing needs to be able to easily create groups and put users into groups during scenario setup
File Firewall needs this to be able to test group rules

How Has This Been Tested?

Self-testing - Travis does it

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.

Once this is accepted, the documentation of the occ group command needs to be done.

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

phil-davis · 2017-06-22T06:13:57Z

@SergioBertolinSG this implements issue #20629 which you raised and is in 10.0 backlog. Having group commands in occ is very useful for the UI testing scenario steps, and will be useful for other things also (even sysadmins who want to create and manage groups from the command line)

phil-davis · 2017-06-22T11:14:39Z

@PVince81 @DeepDiver1975 this expands functionality of occ to be able to manage groups and their members, so I called you in for review here.
The functionality is useful for acceptance testing - a simple and well-defined way to set up groups and membership ready for testing their behavior. But also would become available to real-life sysdamins who use occ. Actually a place I used to work at wanted to manage group membership for certain groups by script that could read from some outside file and make the oC groups and their membership match. This occ group functionality would have made it easy.

felixboehm · 2017-06-28T11:34:47Z

@PVince81 please review and set proper milestone. Thanks

PVince81 · 2017-06-29T07:37:42Z

+					$group->removeUser($user);
+					$output->writeln('User "' . $user->getUID() . '" removed from group "' . $group->getGID() . '"');
+				} else {
+					$output->writeln('<error>User "' . $userName . '" could not be found in group "' . $group->getGID() . '"</error>');


should this maybe affect the command's return code ? should we abort ?

I wonder what to do in these sort of cases. The sysadmin has asked to remove user1 from group1. Actually user1 is already not in group1, so the requested state is already the case and the occ command is a NOOP.
Do we tell the sysadmin "the user is already not in the group"?
Do we mark the text as an error? (because actually what the sysadmin asked is done)
Do we return an error code, or success?

not sure, maybe check what other system tools do. Does "groupdel" on Linux return an error code ?

PVince81 · 2017-06-29T07:39:16Z

As a CLI user I'd be confused as to what command to use for adding users to a group. I know that add does it too but seeing it, it's not clear that the command can do both.

I'd prefer having separate commands with clearer names:

group:add
group:delete
group:add-member
group:remove-member

PVince81 · 2017-06-29T07:40:04Z

+		$group = $this->groupManager->get($groupName);
+		if (is_null($group)) {
+			$output->writeln('<error>Group does not exist</error>');
+			return;


return error code

Similar thing here - should this text even be highlighted as an error?
The sysadmin asked to delete the group, the group is not even existing, so it is indeed deleted.

PVince81 · 2017-06-29T07:40:15Z

+			return;
+		}
+
+		$output->writeln('<error>The specified group could not be deleted. Please check the logs.</error>');


return error code

Yes, I better return an error code here - this one is the system having trouble!

PVince81 · 2017-06-29T07:42:12Z

+			$output->writeln('The group "' . $group->getGID() . '" already exists');
+		}
+
+		$users = $input->getOption('user');


please move this to a separate command, much clearer from a user's perspective.

Also I guess that the current impl would not let you add users to an existing group as you'd get "Group already exists" which sounds like an error even though the code carries on.

PVince81

See comments.

Make sure to return an error code in all execute commands when non-zero. It can help with scripting.

Also note that runOcc can check said error code in case it's useful for testing.

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

phil-davis · 2017-06-29T13:03:44Z

I can do group:add group:addmember group:removemember and group:delete OK from the command line.
If I try to add a group that exists, or delete a group that does not exist I get an error.
If I try to add/remove members from a group that does not exist I get an error.
If I try to addmember or removemember using a user name that does not exist, I get an error.

If I addmember and some users are already in the group, I do not emit an error. IMHO that is an OK condition - a scripting person can do group:addmember for all the users that they want to be in the group and do not need to try and thin if they are already in the group.

Similarly, if I removemember and some users were already removed from the group, I do not emit an error. That lets a scripting person do removemember for any user(s) that they want to make sure are not in the group without worrying if they were already removed.

Let's see if CI is happy...

individual-it

LGTM 👍

PVince81

👍

PVince81 · 2017-06-30T09:00:27Z

@phil-davis please backport to stable10.

Ignoring the old rule "don't backport new features". cc @DeepDiver1975 @felixboehm

phil-davis · 2017-06-30T09:17:38Z

Actually, the reason to backport this is that

it provides infrastructure that allows both core and apps to implement UI tests that deal with groups
apps need to be able to find this infrastructure when being tested against stable10

the fact that it also provides an enhancement/feature to the occ command, which end-users will see and can use in 10.0.3, is actually just "collateral damage" :)

phil-davis · 2017-06-30T09:19:51Z

and this has been, and will be, a common thing for both new test infrastructure and new tests. Generally-speaking it will be "a good thing" that a more extensive set of automated tests runs in the 10.0.* series.

PVince81 · 2017-06-30T09:21:15Z

@phil-davis thanks, makes sense

phil-davis · 2017-06-30T09:34:12Z

@PVince81 to backport, it needs #28188 backport to be merged first into stable10.

mmattel · 2017-07-04T16:31:03Z

@PVince81 @phil-davis @settermjd
Please add a documentation PR for this occ command.
I do not see a reference nor a PR in the documentation repository...

that were added by owncloud/core#28190 and backported to stable10 by owncloud/core#28276 so the occ group commands will become available from 10.0.3

phil-davis · 2017-07-05T03:42:15Z

Doc PR owncloud-archive/documentation#3201 created.

mmattel · 2017-07-05T07:38:47Z

Q: maybe I missed that:
why don't we have a parameter for listing existing groups and listing existing users in a particular group?

phil-davis · 2017-07-05T07:51:23Z

@mmattel that is a good question that I also asked myself. But I did not want to start expanding beyond what I really needed because then people might review and say "why did you add xyz?"

IMHO it would be very handy for oC admins to be able to list users and groups from the command line.

mmattel · 2017-07-05T08:46:43Z

@phil-davis mind to add this list functinality?

lock · 2019-08-02T09:02:41Z

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.

phil-davis added 3 commits June 22, 2017 11:36

Implement group add delete removeuser in occ command

595013c

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

Infrastructure to use groups in UI tests

0813985

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

Test groups in sharee autocompletion

760e570

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

phil-davis requested review from SergioBertolinSG and individual-it June 22, 2017 06:10

phil-davis added comp:console-occ dev:acceptance-tests labels Jun 22, 2017

phil-davis requested review from DeepDiver1975 and PVince81 June 22, 2017 11:09

phil-davis mentioned this pull request Jun 27, 2017

Automated UI Tests #27858

Closed

phil-davis added the 3 - To Review label Jun 27, 2017

phil-davis added this to the 10.1 milestone Jun 27, 2017

PVince81 reviewed Jun 29, 2017

View reviewed changes

PVince81 suggested changes Jun 29, 2017

View reviewed changes

phil-davis added 2 commits June 29, 2017 18:30

Provide addmember removemember and return statuses

59ebd76

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

Fix AddMember class name

8ec0b9f

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

individual-it approved these changes Jun 30, 2017

View reviewed changes

PVince81 approved these changes Jun 30, 2017

View reviewed changes

PVince81 merged commit 3df9dc9 into master Jun 30, 2017

PVince81 deleted the occ-manage-groups branch June 30, 2017 08:59

phil-davis mentioned this pull request Jun 30, 2017

[stable10] UI tests - autocomplete sharing #28188

Merged

This was referenced Jun 30, 2017

[stable10] Groups for occ command and UI testing #28272

Closed

[stable10] Groups for occ command and UI testing #28276

Merged

phil-davis added a commit to owncloud-archive/documentation that referenced this pull request Jul 5, 2017

Document occ group commands

1a70891

that were added by owncloud/core#28190 and backported to stable10 by owncloud/core#28276 so the occ group commands will become available from 10.0.3

phil-davis mentioned this pull request Jul 5, 2017

Document occ group commands owncloud-archive/documentation#3201

Merged

This was referenced Jul 5, 2017

Provide occ commands to list users and groups #28311

Closed

Commands for managing groups with occ. #20629

Closed

DeepDiver1975 modified the milestones: 10.1, development Oct 10, 2017

lock Bot locked as resolved and limited conversation to collaborators Aug 2, 2019

Uh oh!

Conversation

phil-davis commented Jun 22, 2017

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

phil-davis commented Jun 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-davis commented Jun 22, 2017

Uh oh!

felixboehm commented Jun 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jun 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

phil-davis commented Jun 29, 2017

Uh oh!

individual-it left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 left a comment

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Jun 30, 2017

Uh oh!

phil-davis commented Jun 30, 2017

Uh oh!

phil-davis commented Jun 30, 2017

Uh oh!

PVince81 commented Jun 30, 2017

Uh oh!

phil-davis commented Jun 30, 2017

Uh oh!

mmattel commented Jul 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phil-davis commented Jul 5, 2017

Uh oh!

mmattel commented Jul 5, 2017

Uh oh!

phil-davis commented Jul 5, 2017

Uh oh!

mmattel commented Jul 5, 2017

Uh oh!

lock Bot commented Aug 2, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

phil-davis commented Jun 22, 2017 •

edited

Loading

mmattel commented Jul 4, 2017 •

edited

Loading