Adding prompts to admin:user:create cli command by steverobbins · Pull Request #2690 · magento/magento2

steverobbins · 2015-12-11T21:27:05Z

This will prompt for the admin info if not given in the options:

$ ./bin/magento admin:user:create
Admin Firstname: Foo
Admin Lastname: Bar
Admin User: foobar
Admin Email: foobar@example.com
Admin Password: 
Created Magento administrator user named foobar

Vinai · 2015-12-15T10:52:43Z

Thank you for the PR! We are currently waiting for the travis builds to work again, which should happen in a few days. Once that is running and the tests are green we can continue to process your contribution.

buskamuza · 2015-12-18T19:07:40Z

getOption() verified that such option is supported and that it is set. Accessing it directly in the array can be risky because there is no guarantee that such key exists. I'd suggest to add some validation for all such cases. Maybe a new private method can be dedicated to it.

At this point of execution it is guaranteed that this key exists in the array. It has gone through the optional prompting step and has been validated with addUserInfoRules().

I'm not totally against this approach in this particular case, so I'm ok, if it will be as is.
Just, in general, I'd prefer to not rely on internal logic of a method, but rather rely on the interface/signature. Even, if it is in the same class. Sometimes, it may happen that different developers touch the code and they may miss usage of data returned by a method.
Also, in this case, the method is protected, which means that it's behavior may be modified (close to 0 chance in this particular case, but I'm saying in general).

Again, it's ok to leave it as is in this case.

steverobbins · 2015-12-18T21:51:26Z

@buskamuza I've addressed two of your comments in an update commit.

buskamuza · 2015-12-22T23:28:18Z

@steverobbins , thanks for the update! I hope, we'll merge the PR soon.

mazhalai · 2016-01-12T15:50:26Z

@steverobbins Please merge latest from develop and rerun builds.

vancoz · 2016-01-12T22:59:25Z

Hello @steverobbins, would be nice to add tescase into setup/src/Magento/Setup/Test/Unit/Console/Command/AdminUserCreateCommandTest.php according to your changes.

Thank you.

steverobbins · 2016-01-12T23:47:14Z

Rebased on develop.

Looks like there are some issues with the validate() method as it's parameter type changed. I will address this and @vancoz's comment shortly.

vancoz · 2016-01-14T22:22:42Z

Internal ticket - MAGETWO-49808.
@steverobbins please fix unittests.

vancoz · 2016-01-15T20:13:39Z

I do not think we can change the signature of this method.

We can do anything if we set our minds to it 😛

Yes, I'm having a rethink on my implementation that will also address @buskamuza's array access comment.

steverobbins · 2016-01-18T20:19:02Z

Still working through this (see latest commit), but it's turning into a much bigger thing that I'm not sure you'll accept.

In order to keep the signature of the validate() method the same, I need to manipulate the InputInterface to set values with prompts. This requires a custom Input since $definition is protected. Otherwise validate() will always return errors even though we're getting values from prompts.

This seems to work in practice but there's still work to do due to how CommandTester instantiates the input class, which means I'll need to create our own CommandTester class, and a new Input that extends ArrayInput instead of ArgvInput.

vancoz · 2016-01-21T18:23:51Z

@steverobbins you are right, this is significant change. Looks like we have no good way how to implement user interaction now. Let us discuss this internally.

vrann · 2017-03-22T16:24:44Z

@steverobbins can you please merge this PR with the latest develop? Going to work on accepting it.

steverobbins · 2017-03-22T18:26:05Z

@vrann Squashed and rebased... Let's see if tests still pass a year later 😟

vrann · 2017-03-23T00:01:12Z

@steverobbins you have couple failures. Also adding one comment to the code

vrann · 2017-03-23T00:02:05Z

+        ];
+        foreach ($keys as $key) {
+            if (!$input->getOption($key)) {
+                $input->promptForOption($dialog, $output, $key, $key === AdminAccount::KEY_PASSWORD);


Please check that input actually implements the interface which has promptForOption method

vrann · 2017-03-23T00:19:52Z

+     */
+    protected function setMissingValues(InputInterface &$input, OutputInterface $output)
+    {
+        if (!method_exists($input, 'promptForOption')) {


sorry for not being more specific earlier, but better way to achieve this is to introduce new interface, i.e. InputInteractiveInterface, which extends InputInterface. Then here either check is $input an instance of this interface or even better do type hinting to InputInteractiveInterface in the method signature. The latter is not always possible, so either way.

Isn't that what this is? https://github.com/magento/magento2/pull/2690/files#diff-6c32687b440c3791df9e43cba7f888b3

Can you clarify the question please? Literally, there should be an interface with the promptForOption method and here you should either have it in protected function setMissingValues(InputInteractiveInterface &$input, OutputInterface $output) (better) or call instanceof (still ok).

Please let me know does this makes sense to you. I don't want to add too much work :)

davidalger · 2017-06-02T22:34:28Z

@steverobbins Sorry this has been open so long. I'm attempting to resurrect this and get it merged in, but it looks like the test this PR adds is failing:

vagrant@dev-web70:05:11 PM:/var/www/sites/m2engcom-ce.dev/dev/tests/unit$ ../../../vendor/phpunit/phpunit/phpunit ../../../setup/src/Magento/Setup/Test/Unit/Console/Command
PHPUnit 4.1.0 by Sebastian Bergmann.

Configuration read from /var/www/sites/m2engcom-ce.dev/dev/tests/unit/phpunit.xml.dist

..........F....................................................  63 / 136 ( 46%)
............................................................... 126 / 136 ( 92%)
..........

Time: 1.32 seconds, Memory: 16.00MB

There was 1 failure:

1) Magento\Setup\Test\Unit\Console\Command\AdminUserCreateCommandTest::testExecuteWithPrompts
Failed asserting that 'User Name is a required field.
First Name is a required field.
Last Name is a required field.
Please enter a valid email.
Password is required field.
Your password must be at least 7 characters.
Your password must include both numeric and alphabetic characters.
' contains "Created Magento administrator user named user".

/var/www/sites/m2engcom-ce.dev/setup/src/Magento/Setup/Test/Unit/Console/Command/AdminUserCreateCommandTest.php:145
                                         
FAILURES!                                
Tests: 136, Assertions: 488, Failures: 1.

In my manual test of the command, it appears to be working, so presumably it's the test that is broken. Can you fix this?

steverobbins · 2017-06-05T22:22:53Z

I don't know when I'll be able to get to this, someone else might want to take over.

okorshenko · 2017-06-09T23:09:04Z

Closing this PR.

buskamuza reviewed Dec 18, 2015
View reviewed changes

steverobbins force-pushed the feature/admin-user-create-prompts branch from 90f50b2 to fbd3313 Compare December 18, 2015 21:44

MomotenkoNatalia added the PS label Dec 21, 2015

vancoz added the Progress: needs update label Jan 12, 2016

steverobbins force-pushed the feature/admin-user-create-prompts branch from 99d1c88 to 91f93d1 Compare January 12, 2016 23:07

vancoz self-assigned this Jan 14, 2016

vancoz added Progress: accept Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development and removed Progress: needs update labels Jan 14, 2016

vancoz added improvement Progress: needs update and removed Progress: accept Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development labels Jan 14, 2016

vancoz reviewed Jan 15, 2016
View reviewed changes

sshrewz removed the Progress: needs update label Feb 22, 2016

vasiliyseleznev unassigned vancoz Aug 3, 2016

vasiliyseleznev removed the PS label Aug 3, 2016

vkorotun added the Admin Panel label Aug 3, 2016

sshrewz added the Component: Setup label Aug 5, 2016

Adding prompt to admin:user:create cli command

7e07366

steverobbins force-pushed the feature/admin-user-create-prompts branch from 0d2ee5f to 7e07366 Compare March 22, 2017 18:24

vrann self-requested a review March 23, 2017 00:01

vrann suggested changes Mar 23, 2017

View reviewed changes

Check for prompt option on input object

1a061cd

vrann suggested changes Mar 23, 2017

View reviewed changes

okorshenko modified the milestones: March 2017, April 2017 Apr 2, 2017

vrann approved these changes Apr 12, 2017

View reviewed changes

okorshenko modified the milestones: April 2017, May 2017 May 9, 2017

steverobbins mentioned this pull request May 30, 2017

Make bin/magento admin:user:create interactive #9794

Closed

davidalger self-assigned this May 31, 2017

okorshenko modified the milestones: May 2017, June 2017 Jun 1, 2017

okorshenko self-assigned this Jun 9, 2017

okorshenko closed this Jun 9, 2017

magento locked and limited conversation to collaborators Jun 9, 2017

Conversation

steverobbins commented Dec 11, 2015 • edited by davidalger Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Vinai commented Dec 15, 2015

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!

steverobbins commented Dec 18, 2015

Uh oh!

buskamuza commented Dec 22, 2015

Uh oh!

mazhalai commented Jan 12, 2016

Uh oh!

vancoz commented Jan 12, 2016

Uh oh!

steverobbins commented Jan 12, 2016

Uh oh!

vancoz commented Jan 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steverobbins commented Jan 18, 2016

Uh oh!

vancoz commented Jan 21, 2016

Uh oh!

vrann commented Mar 22, 2017

Uh oh!

steverobbins commented Mar 22, 2017

Uh oh!

vrann commented Mar 23, 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!

vrann Mar 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidalger commented Jun 2, 2017

Uh oh!

steverobbins commented Jun 5, 2017

Uh oh!

okorshenko commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

steverobbins commented Dec 11, 2015 •

edited by davidalger

Loading

vrann Mar 23, 2017 •

edited

Loading