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

Add showAPIErrors to available notifications. #2920

Closed
wants to merge 1 commit into from
Closed

Add showAPIErrors to available notifications. #2920

wants to merge 1 commit into from

Conversation

shindakun
Copy link
Contributor

ref: #2890

  • adds a pluralized version of showAPIError

ref: #2890
- adds a pluralized version of showAPIError
@shindakun
Copy link
Contributor Author

This came up in discussion of #2890, I'm submitting it separately just in case anyone has any comments. Do we need a test for this as well, if it's good to go?

@novaugust
Copy link
Contributor

Looks good.


Y'know, this kind of leads to another discussion. the notifications client-side api now exposes four (!) error handling methods. We should change it to expose only one, and standardize its use throughout the ember app. Looks like your showApiErrors would be the place to start. I already have something like 10 issues opened in my name, so I'd be happy to let someone else open that :p

@jaswilli
Copy link
Contributor

jaswilli commented Jun 9, 2014

I just don't get having both a showAPIError and showAPIErrors methods (same goes for showError and showErrors) that are almost identical. There doesn't seem to be any reason to distinguish between a response with one error or a response with multiple errors.

I think error handling may have gone off the rails a little bit in utils/ajax.js by making the differentiation and decorating the response object with either .error or .errors instead of a single property that contains any and all error information.

@novaugust
Copy link
Contributor

Yeah, this stuff really needs to get whittled down to 1

@shindakun
Copy link
Contributor Author

I'll take a look at it after work and see if I can trim it down.

@novaugust
Copy link
Contributor

Sorry for giving you the run-around man. It sure would be sweet to get that
standardized

On Mon, Jun 9, 2014 at 12:04 PM, Steve Layton notifications@github.com
wrote:

I'll take a look at it after work and see if I can trim it down.


Reply to this email directly or view it on GitHub
#2920 (comment).

@shindakun
Copy link
Contributor Author

@novaugust no worries, I've been sort of thinking about that after I submitted the PR.

edit: that's kinda why I submitted this one on its own too, wanted feedback.

@ErisDS
Copy link
Member

ErisDS commented Jun 11, 2014

So I think that this is not quite ready, and #2890 is also waiting on this? Just making sure I'm up-to-date.

@ErisDS
Copy link
Member

ErisDS commented Jun 22, 2014

Is it worth merging this so we can get #2890 shipped, and cleaning up afterwards?

@ErisDS
Copy link
Member

ErisDS commented Jun 23, 2014

@jaswilli Seeing as you've taken on #2847 is any of this PR or #2890 useful to you? Let me know if you want any of it merged.

@jaswilli
Copy link
Contributor

@ErisDS I don't think #2890 will survive the cleanup of the Debug page so it's probably not necessary.

My gut feeling on errors is that the Ember admin still probably needs a comprehensive once-over to get some clarity on how errors are being delivered from the API and then transported around through Ember/Ember Data. From there we can do any necessary cleanup in the RESTAdapter to provide a consistent .errors properties on response objects, and then get a set of notification methods in place.

I don't think this PR gets us there so I don't think it's necessary, but If someone else sees some immediate value from it then by all means let's get it merged.

@ErisDS
Copy link
Member

ErisDS commented Jun 25, 2014

Sorry @shindakun gonna close this now #3091 is in.

@ErisDS ErisDS closed this Jun 25, 2014
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.

None yet

4 participants