Do not catch exception when adding user with Nut

[dantleech]

Currently, if you try and register a user with a short name, you get the following error:

./app/nut user:add admin admin admin@example.com admin admin
Error creating user: admin

Which is not very helpful. By letting the exception get thrown:

  [Bolt\Exception\AccessControlException]                           
  Can not save a password with a length shorter than 6 characters!

We could show the exception message, but I personally prefer to let exceptions be thrown, especially here as you can get their stack trace with --verbose. But happy to change.

[GwendolenLynch]

Yeah, works for me. I don't suppose you could fix the one test that this breaks too?

[dantleech]

Updated. I removed the test which tested the exception catching.

Tests were failing on 5.5 for an unrelated dependency problem:

  Problem 1
    - Installation request for lstrojny/phpunit-function-mocker dev-master@dev -> satisfiable by lstrojny/phpunit-function-mocker[dev-master].
    - lstrojny/phpunit-function-mocker dev-master requires php >=5.6.0 -> your PHP version (5.5.33) does not satisfy that requirement.

[GwendolenLynch]

Tests were failing on 5.5 for an unrelated dependency problem:

Yep, see #5484 for the details on that.

Otherwise, thank you!

[GwendolenLynch]

Just a slight tweak/process request. That test is still valid and shouldn't be removed, but can be simply:

    public function testFailure()
    {
        $app = $this->getApp();
        $command = new UserAdd($app);
        $tester = new CommandTester($command);

        $this->setExpectedException('Bolt\Exception\AccessControlException', 'Can not save a password with a length shorter than 6 characters!');

        $tester->execute(
            [
                'username'    => 'koala',
                'displayname' => '',
                'email'       => '',
                'password'    => '',
                'role'        => '',
            ]
        );
    }

[dantleech]

That test is still valid and shouldn't be removed

I thought about that, but imo this is the wrong level to test it, we are just testing the domain model by proxy, throwing that exception is not a responsiblity of this class. I would move the test back to the model if it does not already exist. wdyt?

[GwendolenLynch]

OK, I don't know what I did with my last reply… but here goes again 😆

Agree that our tests need a lot of work, and if you're willing to move that test to a better location as part of this PR please be my guest. But that test was added to catch a real bug that was reported so needs to exists… somewhere.

[dantleech]

Would it be enough to add a unit test for the Bolt\EventListenerBolt\EventListener\StorageEventListener class which emmitts this exception?

Going further, could this be better handled by a validator on the model?

[GwendolenLynch]

Would it be enough to add a unit test for the Bolt\EventListenerBolt\EventListener\StorageEventListener class which emmitts this exception?

Absolutely! 👍

Going further, could this be better handled by a validator on the model?

Potentially, please feel very welcome to RFC a proposed approach… Gives everyone visibility 😄

[Nitpick-CI]

Whitespace found at end of line

[Nitpick-CI]

Whitespace found at end of line

[dantleech]

Added a unit test -- I made no attempt here to copy existing style - as wanted to show how I would do this, can change as required of course.

Notable I am using Prophecy for mocking, and I do not extend the BoltTestCase - as that test case would (it seems to me) make this a functional test.

[GwendolenLynch]

Notable I am using Prophecy for mocking, and I do not extend the BoltTestCase - as that test case would make this a functional test.

Our tests need love, but we must keep the level of testing intact (regardless of mixed approaches currently).

For this I have no problem, and also prefer the approach but if you would like to expand on this approach, a short RFC on the subject would get it moving. Also if you're keen to jump on some of this (and you're help is wanted, have I mentioned that yet 😉 ), please feel free to join the dev meetings on IRC on Tuesday nights.

[GwendolenLynch]

Failure just only due to #5484 … otherwise GTG.