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

Force create of admin user. #4604

Merged
merged 6 commits into from
Jul 6, 2020
Merged

Force create of admin user. #4604

merged 6 commits into from
Jul 6, 2020

Conversation

tjventurini
Copy link
Contributor

There is a conflict with eg. Spark. Users can't set the password during create. I changed to forceCreate and added a check if the user exists already. I hope that fixes the issue for the future 😃

There is a conflict with eg. Spark. Users can't set the password during create. I changed to `forceCreate` and added a check if the user exists already. I hope that fixes the issue for the future 😃
Copy link
Collaborator

@MrCrayon MrCrayon left a comment

Choose a reason for hiding this comment

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

Never used Spark so not sure what's the problem but you should move your check.

@@ -123,6 +123,13 @@ protected function getUser($create = false)

// If we need to create a new user go ahead and create it
if ($create) {
// check if user with given email exists
$User = $model::where('email', $email)->first();
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should move this check later on because at this point you might still not have the email.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved it further down, after the check for the email.

Ensure that the command asked for the email before running the check.
@@ -2,12 +2,13 @@

namespace TCG\Voyager\Commands;

use App\User;
use Illuminate\Support\Str;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please avoid committing changes like these (rearranging "use" statements). They provide no benefit, and simply make StyleCI unhappy. I realize it likely wasn't intentional, so don't worry about it. StyleCI will auto-fix it after merge anyway.

@@ -2,12 +2,13 @@

namespace TCG\Voyager\Commands;

use App\User;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need to include this line. You're not using it anywhere, and it's not guaranteed to exist.

@@ -131,6 +132,13 @@ protected function getUser($create = false)
if (!$email) {
$email = $this->ask('Enter the admin email');
}

// check if user with given email exists
$User = $model::where('email', $email)->first();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use lowercase (camel-case, though it doesn't matter here) for variable names.

Also, can't you use ->exists() on the model instead since you're not using the object anywhere. This will allow you to combine this line with the conditional below it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. Updated it.


// check if user with given email exists
$User = $model::where('email', $email)->first();
if ($User) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here

emptynick
emptynick previously approved these changes Jul 6, 2020
@emptynick emptynick dismissed stale reviews from fletch3555 and MrCrayon July 6, 2020 10:40

Implemented

@emptynick emptynick changed the base branch from 1.3 to 1.4 July 6, 2020 10:40
@emptynick emptynick dismissed their stale review July 6, 2020 10:40

The base branch was changed.

@codecov
Copy link

codecov bot commented Jul 6, 2020

Codecov Report

Merging #4604 into 1.4 will decrease coverage by 0.04%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                1.4    #4604      +/-   ##
============================================
- Coverage     62.96%   62.91%   -0.05%     
- Complexity     1372     1374       +2     
============================================
  Files           194      194              
  Lines          4007     4010       +3     
============================================
  Hits           2523     2523              
- Misses         1484     1487       +3     
Impacted Files Coverage Δ Complexity Δ
src/Commands/AdminCommand.php 8.33% <0.00%> (-0.56%) 12.00 <0.00> (+1.00) ⬇️
src/Traits/Translatable.php 37.65% <75.00%> (ø) 61.00 <0.00> (ø)
src/Models/Menu.php 92.06% <100.00%> (ø) 30.00 <16.00> (ø)
src/Models/User.php 100.00% <100.00%> (ø) 7.00 <0.00> (+1.00)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6063f09...f795df0. Read the comment docs.

@emptynick emptynick merged commit b9535ed into thedevdojo:1.4 Jul 6, 2020
@tjventurini
Copy link
Contributor Author

Baam ... half a year later it is merged ... Not sure if I should party or complain 😅

@emptynick
Copy link
Collaborator

Sorry for the long delay, but we are quite busy with v2 at the moment.

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.

4 participants