Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Command, allow to search User by Email #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

n3ss
Copy link

@n3ss n3ss commented May 28, 2019

Add a new option for the "biig:jwt:generate"
This option allow to search an User by its Email

@n3ss n3ss self-assigned this May 28, 2019
@n3ss n3ss added the enhancement New feature or request label May 28, 2019
@n3ss n3ss requested a review from Nek- May 28, 2019 13:58
Copy link
Contributor

@Nek- Nek- left a comment

Choose a reason for hiding this comment

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

Nice contribution thanks. Please take a look to the feedback I did.

if ($input->hasOption('role')
if ($input->hasOption('email')
&& null !== $input->getOption('email'))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

wow, how did it happen like that? Please fix indentation.

if (!$find) {
$output->writeln(sprintf('<error>User with role "%s" doesn\'t exist.<error>', $input->getOption('role')));
&& !in_array($input->getOption('role'), $user->getRoles()))
{
Copy link
Contributor

Choose a reason for hiding this comment

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

👀 same here

&& !in_array($input->getOption('role'), $user->getRoles()))
{
$user = $this->findOneByRole($input, $output, $users);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we specify email and role, role will be ignored. This is due to refactoring in splitted methods. You probably should consider to add an error message or drop this refactoring and use a single (fat, I admit) condition.

Copy link
Author

Choose a reason for hiding this comment

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

I'm assuming that if you use the 'email' flag you must know which user you want and therefore you need to know which role this user have

@Nek-
Copy link
Contributor

Nek- commented Jun 11, 2019

@megan3ss ping :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants