Return resource.errors.full_messages in addition to resource.errors

[jasonswett]

In RegistrationsController#create, might it be a good idea to return resource.errors.full_messages instead of resource.errors? The full_messages version seems to me to be easier to consume on the client side.

One reservation I do have is that maybe devise_token_auth isn't always used in client-server-architected apps. "Traditional" apps would of course still probably want resource.errors as opposed to resource.errors.full_messages. So if we were to make a change, we would probably only want to make it for JSON requests.

I forked the project and changed resource.errors to resource.errors.full_messages and it works well for me.

What do you think?

[lynndylanhurley]

I think the current error-handling system is mostly haphazard. I've been wanting to formalize the errors, but I keep getting distracted. I'll make this a priority for the next release.

I do have some ideas on this, I'll post back to this issue ASAP.

[jasonswett]

I'm willing to help in whatever way makes sense. Let me know if you think any of that work is delegable to me.

[lynndylanhurley]

Sorry for the delay. I think the best way to handle this will be to return the full error object, including messages and full_messages.

Just to be clear:

• messages look like this: {:password_confirmation=>["doesn't match Password"]}
• full_messages look like this: ["Password confirmation doesn't match Password"]

The messages hash is useful for matching by field name, so the error can be easily appended above the input that is causing the issue.

For example:

<fieldset ng-class="{error: errors.full_messages.length}">
  <div>
    <label>Email</label>
    <p class="errors" ng-show="errors.messages['email']">Email {{ errors.messages['email'] }}</p>
    <input type="text" name="email" ng-model="form.email" />
  </div>
</fieldset>

The full_messages array is useful for throwing alerts/modals, or showing an error summary at the top of forms.

So I think it's probably best to standardize all the errors so that they look like this:

{
  data: {
    errors: {
      messages: {
        // messages hash contents
      },
      full_messages: [
        // full_messages array contents
      ]
    }
  }
}

Do you agree? Is this something that you would like to take on?

[jasonswett]

Totally agree, and yes.

Is there any certain process I should be following? Create a branch, implement the feature, issue pull request?

[lynndylanhurley]

I would prefer this process:

1. Create feature branch
2. Write test cases
3. Make tests pass
4. Issue pull request

You can run the test suite using the following steps:

1. clone this repo
2. bundle install
3. rake db:migrate
4. RAILS_ENV=test rake db:migrate
5. guard

This will open the guard test-runner. Guard will re-run each test suite when changes are made to its corresponding files.

I should probably add a CONTRIBUTING.md or something :)

[jasonswett]

Okay, I'm with you. I was including steps 2 and 3 in "implement the feature", I just didn't explicitly say it. And I already cloned your repo and ran your tests a long time ago. :)

I'm actually on vacation right now but let me take a closer look at this later in the week and give you a rough feel for timeline. Thanks for the opportunity to help.

[jasonswett]

I checked my schedule: I can get this in place by the end of next week (the week ending 10/24).

[lynndylanhurley]

Awesome, thanks @jasonswett! Let me know if you run into any trouble.

[jasonswett]

Hmm, after getting into the implementation a little bit I'm realizing that splitting

{
  data: {
    errors: [
      // whatever
    ]
  }
}

into

{
  data: {
    errors: {
      messages: {
        // messages hash contents
      },
      full_messages: [
        // full_messages array contents
      ]
    }
  }
}

might carry some unwanted baggage. The issues to me are:

• It seems to me not-backwards-compatible. People would have to know to change any instance of response.errors to response.errors.messages.
• response.errors.messages seems kind of non-standard compared to response.errors
• There are a number of places in the controller (I counted 6) that would have to change in order to match this, and that might result in some awkwardness

What would you think about this structure instead?

{
  data: {
    errors: {
      // messages hash contents
      full_messages: [
        // full_messages array contents
      ]
    },
  }
}

[lynndylanhurley]

@jasonswett - I agree! Good work 👍

[jasonswett]

Okay, the change is in place. I'm new to open source contribution so I don't know if I did all the steps right. Here's a link to my pull request: #50

[lynndylanhurley]

Cool, thanks @jasonswett! I'll review ASAP.

[lynndylanhurley]

Merged! Check out:

• ng-token-auth 0.0.24-beta2
• devise_token_auth 0.1.30.beta1

Please test and confirm that everything works!

[jasonswett]

Will do. Thanks!

[lynndylanhurley]

@jasonswett - you may want to hold off testing until I fix a bug that was introduced regarding #51. I'll let you know when everything has stabilized.

[jasonswett]

10-4

[lynndylanhurley]

Ok @jasonswett - the problem should be fixed in 0.1.30.beta3.

[lynndylanhurley]

@jasonswett - this seems to be working ok. I'm closing this, but please re-open if you find any issues.

Thanks again for the PR 😺

[jasonswett]

@lynndylanhurley You're welcome for the PR! Sorry for my slowness here - been a bit underwater lately.