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

Check for existing username or email #72

Closed
mccombs opened this issue May 16, 2013 · 19 comments
Closed

Check for existing username or email #72

mccombs opened this issue May 16, 2013 · 19 comments

Comments

@mccombs
Copy link

mccombs commented May 16, 2013

It would be great if Confide would check for either an existing username or email so you don't end up with registration containing the same information.

P.S. We're loving this extension. It's really helped us get up and going.

@andrew13
Copy link
Collaborator

That was added: 5e75221

But it was breaking updates soit was rolled back. It's actually a semi bug in Ardent that is getting addressed. A work around is to cite a different rule set when updating. See https://github.com/andrew13/Laravel-4-Bootstrap-Starter-Site/blob/master/app/models/User.php for an example.

In short, it'll be added soon. Once it can be handled properly.

@andrew13
Copy link
Collaborator

Leaving open, until addressed.

@andrew13
Copy link
Collaborator

Maybe as a stop gap I'll include the update params and an update function in ConfideUser to allow the unique check to happen in Confide.

@andrew13
Copy link
Collaborator

More info: #64

@andrew13
Copy link
Collaborator

I'm going to go ahead and do something like laravel-ardent/ardent#37 (comment)

in confideUser

@richlove1
Copy link

Interesting reading that Ardent issue, that's the exact thing I was wondering about enforcing uniqueness on update vs creation.

@andrew13
Copy link
Collaborator

Yep, I'm basically going to add the option to add to the params that get sent to save to specify update. If that update param is set to true it'll use the updateRules, otherwise it'll use the normal rules. Seems like the cleanest solution.

@richlove1
Copy link

Yeah I was wondering along those line but thinking maybe there might be a cleaner way but you're probably right, I couldn't think of a better/cleaner solution than that.

Will the solution allow the user to change their email and/or username whilst also checking for uniqueness? (I know some systems don't allow username changes but I don't see why the user shouldn't be able to if their new choice is available)

@andrew13
Copy link
Collaborator

You're correct that it would allow a user to change their email to an existing email. I suppose there might be a way to check the email/username if it changed from the original if you could compare it to a user id field or something of that nature.

I think I decided on a slightly different route for the update rules.

Add a protected variable and getter/setter function

protected $updateRules = array();

public function getUpdateRules() 
{
return $updateRules;
}

public function setUpdateRules($set) 
{
$this->updateRules = $set;
}

Also add what is basically an alias of save

    public function amend( array $rules = array(), array $customMessages = array(), array $options = array(), \Closure $beforeSave = null, \Closure $afterSave = null )
    {
if(empty($rules)) {$rules=$this->getUpdateRules();}
        return $this->real_save( $this->, $customMessages, $options, $beforeSave, $afterSave );
    }

Using amend instead of update, ad update is already used in Eloquent.

@andrew13
Copy link
Collaborator

Added those: 4b0bb2c

Use the amend function to update a user.

@andrew13
Copy link
Collaborator

Need to add the check if a user has edited a unique field that field should be checked.

I'll do this by grabbing the rules array, finding the unique fields, then comparing two users, if unique fields are different then the updateRules array will get the unique param added to it.

This will happen within a separate function. Which means there will be two calls when updating.

$user->prepareRules($Auth::user(),$user);
//Save
$user->save($this->getUpdateRules());
// Or Amend
$user->amend();

@andrew13 andrew13 reopened this May 21, 2013
@richlove1
Copy link

Looking sensible Andrew 👍

@Zizaco
Copy link
Owner

Zizaco commented May 29, 2013

I'm not sure if this was addressed very well. I don't think the methods: amend(), prepareRules() and getRules() are a good idea. We could, inside the save/beforeSave, check if the user exists in the database (the original attribute i guess) and address the issues regarding the validation errors when updating.

Plus, I don't think is a good idea for us to try to "patch" an Ardent bug in Confide.

@Zizaco Zizaco reopened this May 29, 2013
@andrew13
Copy link
Collaborator

I'd 100% that patching a bug in Ardent within Confide isn't the best solution. However, the bug in Ardent has been there for sometime without action on it and not having a way to update within Confide is an issue. I'd welcome implementing a better solution.

@Zizaco
Copy link
Owner

Zizaco commented May 30, 2013

I'm seriously thinking about droping Ardent use. We can do an simpler self-validating model that will address this or we can use Aware (that is now available for laravel4)

@andrew13
Copy link
Collaborator

I'd vote for dropping it. I haven't used Aware. Is it more stable than Ardent?

@andrew13
Copy link
Collaborator

Also, I'd disagree with checking if the user exists on each save request. For two reasons:

Simply because it unnecessary in most cases. As a developer I'd likely know whether I'm creating a user or modifying one. I'd rather save a query on save/update and use two different functions. (save/update) (Maybe we specify a different set of rules for the update function. Instead of using amend.)

Further, I'm not sure how we would differentiate save vs update. Checking if the user exists isn't good enough. That would effectively allow a user creating a user to update a user. If we added a flag to the save function, that's a possibility, but simply checking if a user exists isn't going to work.

@Zizaco
Copy link
Owner

Zizaco commented May 31, 2013

What if we check if there is an id in the user? Id there is not, than we query for it's username OR email before saving it.
It's one more query in the saving process, but maybe it's a worth one.

@Zizaco
Copy link
Owner

Zizaco commented May 31, 2013

Done: d764178

:)

@Zizaco Zizaco closed this as completed May 31, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants