Adding a "create user" to the api #370

nir-sopher · 2017-03-15T20:59:53Z

No description provided.

mitchell852

some comments for you. thanks!

mitchell852 · 2017-03-22T19:09:20Z

traffic_ops/app/lib/API/User.pm

+		return $self->alert("full-name is required.");
+	}
+
+	if ( !defined($params->{email}) ) {


email is a unique column. do you want to check to make sure this email is not already in use? kind of like you did for username?

nevermind, i guess email uniqueness is checked in the is_valid() function

mitchell852 · 2017-04-07T03:02:43Z

docs/source/development/traffic_ops_api/v12/user.rst

+  +----------------------+--------+------------------------------------------------+
+  |``uid``               | int    |                                                |
+  +----------------------+--------+------------------------------------------------+
+  | ``tenantId``         | int    | Owning tenant ID                               |


Can you add tenant to this response?

Ori plans to do so in a separate PR or on the merge with pr 434

mitchell852 · 2017-04-07T03:07:19Z

traffic_ops/app/lib/API/User.pm

+	my $tenant_id = exists($params->{tenantId}) ? $params->{tenantId} :  $self->current_user_tenant();
+
+	my $values = {
+		address_line1 			=> defined_or_default($params->{addressLine1}, ""),


I don't think you need to do all these defined_or_default()

it's fine if they leave out addressLine1 which will just set address_line1 to undef.

mitchell852 · 2017-04-07T03:09:23Z

traffic_ops/app/lib/API/User.pm

+		country 			=> defined_or_default($params->{country}, ""),
+		email 				=> $params->{email},
+		full_name 			=> $params->{fullName},
+		new_user            		=> defined_or_default($params->{newUser}, 0),		


just set this to

new_user => \0, # false

it's confusing, i know but "new_user" is used for users that were created via a registrationSent.

Done. "0" an not "\0" as setting it to "\0" failed.
Will need to look further into it to understand why

mitchell852 · 2017-04-07T03:14:58Z

traffic_ops/app/lib/API/User.pm

+		$response->{stateOrProvince} 		= $rs->state_or_province;
+		$response->{uid} 			= $rs->uid;
+		$response->{username} 			= $rs->username;
+		$response->{tenantId} 			= $rs->tenant_id;


can you return the tenant name as well?

Ori plans to add it in a separate PR

nir-sopher · 2017-04-07T10:57:01Z

Indeed

…

On Apr 7, 2017 6:16 AM, "Jeremy Mitchell" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In traffic_ops/app/lib/API/User.pm <#370 (comment)> : > + my $name = $params->{username}; + if ( !defined($name) ) { + return $self->alert("Username is required."); + } + + my $existing = $self->db->resultset('TmUser')->search( { username => $name } )->single(); + if ($existing) { + return $self->alert("A user with username \"$name\" already exists."); + } + + + if ( !defined($params->{fullName}) ) { + return $self->alert("full-name is required."); + } + + if ( !defined($params->{email}) ) { nevermind, i guess email uniqueness is checked in the is_valid() function — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#370 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AXGkYLXxSpb8U3fcfR6JyCEygD0RjcT5ks5rtaqDgaJpZM4MehR1> .

mitchell852 · 2017-04-20T21:11:28Z

you might want to run your code thru the perl formatter. I see some formatting errors. Here's more info about that:

https://github.com/apache/incubator-trafficcontrol/blob/master/CONTRIBUTING.md#code-formatting

http://trafficcontrol.apache.org/docs/latest/development/traffic_ops.html#perl-formatting-conventions

mitchell852 · 2017-04-20T21:16:00Z

traffic_ops/app/lib/API/User.pm

+
+	my $name = $params->{username};
+	if ( !defined($name) ) {
+		return $self->alert("Username is required.");


you check for username, fullName and email in the is_valid method. so you don't have to do that here.

I'll remove the checks for full-name and email.
As "username" value is use for searching the DB later on, I prefer to validate it.

I don't understand. fullname, email and username are validated here:

https://github.com/apache/incubator-trafficcontrol/blob/master/traffic_ops/app/lib/API/User.pm#L463

also, you should probably add role to line 463

mitchell852 · 2017-04-24T17:16:51Z

traffic_ops/app/t/api/1.2/user_admin.t

+	my $tenant_name = defined ($tenant_id) ? $schema->resultset('Tenant')->find( { id => $tenant_id } )->get_column('name') : "null";
+
+	# Verify the user
+	ok my $user = $schema->resultset('TmUser')->find( { username => $login_user } ), 'Does the portal user exist?';


Can you update this message? not sure what portal user has to do with this.

mitchell852 · 2017-04-24T17:18:43Z

traffic_ops/app/t/api/1.2/user_admin.t

+		->json_is( "/response/username" =>  $addedUserName )
+		->json_is( "/response/email" =>  $addedUserEmail)
+		->json_is( "/response/tenantId" =>  $tenant_id) #tenant Id not set - getting the tenant id from the user
+        	    , 'Failed adding user?';


this test message doesn't seem right either. Maybe "Successfully added user?" would be better.

mitchell852 · 2017-05-31T03:40:35Z

@nir-sopher - can you fix this PR? "This branch cannot be rebased safely" - i'd like to get it merged if possible

mitchell852

some comments for you. please address these comments as well as rebase and we can get this merged. thanks.

mitchell852 · 2017-05-31T16:05:11Z

traffic_ops/app/lib/API/User.pm

+
+	my $name = $params->{username};
+	if ( !defined($name) ) {
+		return $self->alert("Username is required.");


I don't understand. fullname, email and username are validated here:

https://github.com/apache/incubator-trafficcontrol/blob/master/traffic_ops/app/lib/API/User.pm#L463

also, you should probably add role to line 463

mitchell852 · 2017-05-31T16:06:01Z

traffic_ops/app/lib/API/User.pm

+		return $self->alert("confirm-local-password is required.");
+	}
+
+	if ($params->{localPassword} ne $params->{confirmLocalPassword}){


this is not necessary, it is validated in the is_valid method

mitchell852 · 2017-05-31T16:06:30Z

traffic_ops/app/lib/API/User.pm

+		return $self->alert("local-password and confirmed-local-password mismatch.");
+	}
+
+	if ( !defined($params->{role}) ) {


if you add 'role' to line 463, you can take this out as well....

…response the value is bool like everywhere else

nir-sopher · 2017-06-01T20:08:45Z

Rebase was done a PR review changes were made, except for a test for "username" in the beginning of the create function. The test is done as the value is in use by the function logic.

nir-sopher · 2017-06-01T20:13:06Z

The change of the "role" broke the UT and was reverted.
It changes current behavior of user-update.
I prefer not to make such changes in this PR

… some errors in existing logic when checking uniqueness of email and username and did some formatting

removed some irrelevant props, leveraged the is_valid() method, fixed…

mitchell852 · 2017-06-05T15:00:00Z

yes, there is a UT failure:

t/user.t (Wstat: 256 Tests: 21 Failed: 1)
Failed test: 5
Non-zero exit status: 1

but i'm pretty sure this was fixed in master so i'll pull this in and if it's still broken, i can fix it.

it only changes the behavior of user-update in the sense that role is now checked which in my opinion is a very valid check. you should not be able to update a user and leave out a required field - role.

mitchell852 requested changes Apr 7, 2017

View reviewed changes

mitchell852 reviewed Apr 20, 2017

View reviewed changes

mitchell852 reviewed Apr 24, 2017

View reviewed changes

mitchell852 requested changes May 31, 2017

View reviewed changes

nir-sopher added 12 commits June 1, 2017 17:10

User REST UPI - update, registration_sent cannot be 0/1

f2fdf35

Adding user create API

329578c

remove new_user redundant set

5e0623f

user does specify not set the uid/gid on user create

6c86bab

user does specify not set the 'registration sent' on user create. on …

c275dd8

…response the value is bool like everywhere else

Forgoten util

0cd3442

User create doc

49f7e46

Merge of user creation with user tenancy

1a926fe

create user from API - PR comments fixes

4759971

Remove redundant tests on user creation API

6c33f48

Minor changes in adding users UT - follwoing PR reviews

49c22c6

Create user API. PR review fixes

681dbab

mitchell852 and others added 2 commits June 2, 2017 14:47

removed some irrelevant props, leveraged the is_valid() method, fixed…

f15f2bd

… some errors in existing logic when checking uniqueness of email and username and did some formatting

Merge pull request #5 from mitchell852/nir-create-user-rest-api

86cbe68

removed some irrelevant props, leveraged the is_valid() method, fixed…

asfgit closed this in 5913c53 Jun 5, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding a "create user" to the api #370

Adding a "create user" to the api #370

nir-sopher commented Mar 15, 2017

mitchell852 left a comment

mitchell852 Mar 22, 2017

mitchell852 Apr 7, 2017

nir-sopher Apr 7, 2017

mitchell852 Apr 7, 2017

nir-sopher Apr 7, 2017

mitchell852 Apr 7, 2017

nir-sopher Apr 10, 2017

mitchell852 Apr 7, 2017

nir-sopher Apr 10, 2017

mitchell852 Apr 7, 2017

nir-sopher Apr 10, 2017 •

edited

nir-sopher commented Apr 7, 2017 via email

mitchell852 commented Apr 20, 2017

mitchell852 Apr 20, 2017

nir-sopher Apr 24, 2017

mitchell852 May 31, 2017

mitchell852 Apr 24, 2017

nir-sopher Apr 24, 2017

mitchell852 Apr 24, 2017

nir-sopher Apr 24, 2017

mitchell852 commented May 31, 2017

mitchell852 left a comment

mitchell852 May 31, 2017

mitchell852 May 31, 2017

mitchell852 May 31, 2017

nir-sopher commented Jun 1, 2017

nir-sopher commented Jun 1, 2017

mitchell852 commented Jun 5, 2017

Adding a "create user" to the api #370

Adding a "create user" to the api #370

Conversation

nir-sopher commented Mar 15, 2017

mitchell852 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nir-sopher Apr 10, 2017 • edited

Choose a reason for hiding this comment

nir-sopher commented Apr 7, 2017 via email

mitchell852 commented Apr 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell852 commented May 31, 2017

mitchell852 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nir-sopher commented Jun 1, 2017

nir-sopher commented Jun 1, 2017

mitchell852 commented Jun 5, 2017

nir-sopher Apr 10, 2017 •

edited