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

Errors returned by schema.pre('remove') middleware don't display in the Admin UI #536

Closed
JedWatson opened this issue Aug 12, 2014 · 13 comments
Assignees
Labels

Comments

@JedWatson
Copy link
Member

As reported by David Vermeir in the Google Group:

When you try to delete an item but throw an error in a 'remove' hook it doesn't show up in the flashMessages in the detail view, or in the alert box in the list view.

Test case:

/*
* Stop deletion of challenge if already active
* */
Challenge.schema.pre('remove', function(next) {
    if(this.status === 1) { return next(new Error("You can't remove a challenge once it's been activated!")); }
    keystone.list('UserChallenge').model.findOne({challenge: this._id}).exec(function(err, userChallenge) {
        if(userChallenge) { return next(new Error("You can't remove a challenge that's in use by a player!")); }
        next();
    });
});

Return an error in the next() callback usually works for 'save' hooks but not for 'remove'.

In the detail view it shows the following error in the flash messages section:

There was an error deleting Challenge (logged to console)

In the list view it shows the following error in the alert popup:

There was an error deleting the challenge. ( error: database error)

So it's successfully stopping the 'remove' but not showing the errors I throw.

@JedWatson JedWatson added the bug label Aug 12, 2014
JedWatson added a commit that referenced this issue Oct 4, 2014
Custom deletion hook error messages: issue #536
@larsha
Copy link

larsha commented Feb 23, 2016

@morenoh149 @JedWatson I'm still having issues with this, I have a pre-save hook checking for duplicate e-mail adresses. If hit on a duplicate adress i call next with an error object.

I'm getting error info in console, but no flash:

Error saving changes to User 56cc44d1ba1638443b250fdf: [Error: E-mail already exists.]

The error message don't show up in the admin ui when creating a new user. Am I approaching this the wrong way?

@creynders
Copy link
Contributor

@larsha That should work. I do it all the time in my projects. Could you post a bare-bones example that reproduces this behaviour?

@larsha
Copy link

larsha commented Feb 24, 2016

Hi @creynders, here's an example of the code:

User.schema.pre('save', function(next) {
  User.model.findOne({ 'email': this.email }).exec()
    .then((user) => {
      if (user) {
        return next(new Error('E-mail already exists on another user.'));
      }

      return next();
    })
    .catch((err) => {
      next(new Error(err));
    });
});

Here's a screenshot of the admin ui part when no flash error shows after hitting create with an e-mail adress already registered.
skarmavbild 2016-02-24 kl 09 48 15

Although the error shows up in the console as mentioned: Error saving changes to User 56cd6e4a71770fe8624a3d38: [Error: E-mail already exists on another user.]

@creynders
Copy link
Contributor

Is this 0.3? or 0.4? Come to think of it, I don't think I've got any custom validation on initial fields, maybe the bug's only with them. I'll try to verify it asap.

@larsha
Copy link

larsha commented Feb 24, 2016

It's 0.3, that would be helpful, thanks @creynders !

@gaoyan10
Copy link

I have got the same error, with keystone 0.3.16. The error shows in the console, but the flash message not show.

@creynders
Copy link
Contributor

I can confirm the bug only happens when creating items, not when editing. Don't know whether it applies to 0.4 too. BTW @larsha this would always fail when trying to save an already existing user, since the query will return the user you're editing.

@larsha
Copy link

larsha commented Feb 26, 2016

Thanks @creynders, some more logic around that one I guess, maybe exclude that user by it's id should solve that problem?

_Edit:_
Solved that issue like this:
User.model.findOne({ 'email': this.email, '_id': { '$ne': this._id } }).exec()

@creynders
Copy link
Contributor

fixed in #2566 for v0.4.

@VinayaSathyanarayana
Copy link

Not working in 4.0.0-beta.5

@VinayaSathyanarayana
Copy link

Any update on this @creynders - Not working in 4.0.0-beta.5

@langovoi
Copy link

Any news, guys?

@Mukarrim
Copy link

Thanks @creynders, some more logic around that one I guess, maybe exclude that user by it's id should solve that problem?

_Edit:_
Solved that issue like this:
User.model.findOne({ 'email': this.email, '_id': { '$ne': this._id } }).exec()

thanks man for the 'id' part saved life :)

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

No branches or pull requests

9 participants