Skip to content

Conversation

@turegjorup
Copy link
Contributor

@turegjorup turegjorup commented Jul 7, 2023

Link to ticket

https://jira.itkdev.dk/browse/SUPP0RT-1109

Description

Updated add user command to ask which tenants user belongs to

Screenshot of the result

If your change affects the user interface you should include a screenshot of the result with the pull request.

Checklist

  • My code is covered by test cases.
  • My code passes our test (all our tests).
  • My code passes our static analysis suite.
  • My code passes our continuous integration process.

If your code does not pass all the requirements on the checklist you have to add a comment explaining why this change
should be exempt from the list.

Additional comments or questions

If you have any further comments or questions for the reviewer please add them here.

@turegjorup turegjorup self-assigned this Jul 7, 2023
@turegjorup turegjorup requested a review from rimi-itk July 11, 2023 08:19
Comment on lines +84 to +85
->addArgument('role', InputArgument::OPTIONAL, 'The role of the user [editor|admin]')
->addArgument('tenant-keys', InputArgument::IS_ARRAY | InputArgument::OPTIONAL, 'The keys of the tenants the user should belong to (separate multiple keys with a space)')
Copy link
Contributor

Choose a reason for hiding this comment

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

I would use options for this (rather than arguments) to make it easier to handle multiple values, e.g.

app:user:add … --role=admin --tenant=hat --tenant=briller

but that's a matter of opinion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally my preference as well. This was originally adapted from https://github.com/symfony/demo/blob/main/src/Command/AddUserCommand.php

Copy link
Contributor

Choose a reason for hiding this comment

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

And that example uses an option, --admin, for (implicitly) setting the role. This is not really important, so let's leave it as it is for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed 😄

Changed because

  1. there might be more than 2 roles at some point,
  2. none of the people who has had to use the command have figured out the --admin option. So tried to make it more obvious how to select role

$this->io->text(' > <info>Tenant Keys</info>: '.$tenantKeys);
} else {
$question = new ChoiceQuestion(
'Please select the tenant(s) the user should belong to (to select multiple answer with a list. E.g: "key1, key3")',
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what “(to select multiple answer with a list. E.g: "key1, key3")” means. Doesn't the helper help the user picking multiple values?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. You have to answer with a list of comma separated values. There is no "UI".

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I thought the terminal presented a checkbox like user experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://symfony.com/doc/current/components/console/helpers/questionhelper.html#multiple-choices

use Symfony\Component\Console\Question\ChoiceQuestion;

// ...
public function execute(InputInterface $input, OutputInterface $output): int
{
    // ...
    $helper = $this->getHelper('question');
    $question = new ChoiceQuestion(
        'Please select your favorite colors (defaults to red and blue)',
        ['red', 'blue', 'yellow'],
        '0,1'
    );
    $question->setMultiselect(true);

    $colors = $helper->ask($input, $output, $question);
    $output->writeln('You have just selected: ' . implode(', ', $colors));

    return Command::SUCCESS;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Notice implode(', ', $colors)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But yes, before testing it I also expected a checkbox like feature

throw new InvalidArgumentException('The role can not be empty.');
}

$allowedRoles = ['editor', 'admin'];
Copy link
Contributor

Choose a reason for hiding this comment

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

For future-proofing you could set these roles in an environment variable. Or at least put the list into a class variable; it's also used in interact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to a class constant.

Originally I tried to get the values through DI and the one of the Security services. Didn't work as I wanted because it's a subset of the defined roles we allow here.

I don't like moving them to env because they would still have to match what is defined in security.yaml. Also I consider env values to be something you can configure. But Symfony's docs encourage you to do stuff like #[IsGranted('ROLE_ADMIN')] in code, so having the roles configurable would clash with that.

@turegjorup turegjorup requested a review from rimi-itk July 11, 2023 09:13
@turegjorup turegjorup merged commit 5daea4e into develop Jul 11, 2023
@turegjorup turegjorup deleted the feature/SUPP0RT-1109_user_tenants_commands branch July 11, 2023 09:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants