Skip to content

Conversation

@duk3luk3
Copy link
Contributor

This pull request

  • fixes outdated comments in the POST /users api call
  • adds the parameter expired_password to the call which if set will cause the password to be set expired
  • adds the parameter force_random_password to the call which if set will cause the password to be randomly generated

To properly handle the more complex logic needed to make this change backwards-compatible and robust, some code had to be added. Comments were inserted where expedient.

I believe it is self-evident that adding the "generate temp password" feature that is already in the admin ui to the api call is useful. It enables for example batch-adding users with random passwords.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling a1d7353 on duk3luk3:useradd-api-extension into 0e38791 on gitlabhq:master.

@dosire
Copy link
Member

dosire commented Sep 11, 2013

Can you please include tests for the new functionality? If you can transform if/else statements to object calls that would be great too.

Conflicts:
	lib/api/users.rb

Maintained changes for useradd api
Added a test for the new features introduced to the user api in ba10a34
Parameter reference in error string was typo'd.
Several tests for the /user api call were succeeding for the wrong reason.
The tests were all missing required parameters, so the api would return
an error because of that and not because of the specific condition
intended to be tested.

Also fixed a small typo.
@duk3luk3
Copy link
Contributor Author

I've updated my PR with a test and some bugfixes.

I've also changed the other tests for the POST /users API to work as intended; this leads to one of them failing.

I don't know which of the if-statements I should change. Do you mean using validators in the user model instead of testing for correct parameters in the API call handler?

lib/api/users.rb Outdated
Copy link
Member

Choose a reason for hiding this comment

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

semi-required => required unless force_random is set

@coveralls
Copy link

Coverage Status

Coverage decreased (-31.46%) when pulling 2462294 on duk3luk3:useradd-api-extension into 3fe2558 on gitlabhq:master.

replaced "semi-required" description for password field to proper
description.
@coveralls
Copy link

Coverage Status

Coverage decreased (-31.06%) when pulling 0e83ffe on duk3luk3:useradd-api-extension into 3fe2558 on gitlabhq:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-31.22%) when pulling 0e83ffe on duk3luk3:useradd-api-extension into 3fe2558 on gitlabhq:master.

J. Random Hacker (duk3luk3) added 2 commits September 22, 2013 20:01
Removed parentheses in if-statements
Added documentation for the extended api.
@duk3luk3
Copy link
Contributor Author

I integrated the last suggestions from @dosire. Thanks for helping me mature this PR!

@dosire
Copy link
Member

dosire commented Sep 23, 2013

@duk3luk3 You're welcome, thank you for your efforts. I've asked Dmitriy to take a look at this.

@duk3luk3
Copy link
Contributor Author

Pulled in 6-1-stable and added a fix for a syntax error I had introduced. Will pull in 6-2-stable and then master as soon as I get to it.

@jvanbaarsen
Copy link
Contributor

@dosire What do you think about this PR?

@dosire
Copy link
Member

dosire commented Dec 9, 2013

@duk3luk3 We want to start sending users an activation link instead of a temporary password when they create an account. Therefore it doesn't seem wise to add the temporary password to the API anymore. Also something I should have mentioned earlier, please try to keep PR's as small as possible. If you want to update the documentation do that in one PR.

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

Labels

None yet

Development

Successfully merging this pull request may close these issues.

8 participants