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 "Auth error" status code #1083

Merged
merged 21 commits into from
Jun 9, 2016
Merged

Add "Auth error" status code #1083

merged 21 commits into from
Jun 9, 2016

Conversation

frenchbread
Copy link
Contributor

@frenchbread frenchbread commented Jun 8, 2016

Closes #918

Changes

  • Add 401 status code check

@frenchbread
Copy link
Contributor Author

@jykae Please review since you reported the bug.

@jykae
Copy link
Contributor

jykae commented Jun 8, 2016

@frenchbread reviewing

@jykae
Copy link
Contributor

jykae commented Jun 8, 2016

@frenchbread I think this still needs some more.. Committing soon to this PR, and plelase comment what you think.

@frenchbread
Copy link
Contributor Author

@jykae Agree but not sure what else we can add.. Some more status codes?

@jykae
Copy link
Contributor

jykae commented Jun 8, 2016

@frenchbread what do you think? Not sure what to show on client side when getting 401

@jykae jykae added the WIP label Jun 8, 2016
@jykae
Copy link
Contributor

jykae commented Jun 8, 2016

Put WIP, so we are not merging yet, did not test each if-else statement that it works as intended.

@frenchbread
Copy link
Contributor Author

@jykae We could show different message depending on what status code is received but we are basically checking the status of the host. Do you think we should add custom message output depending on status code?

@frenchbread
Copy link
Contributor Author

@jykae Based on our discussion, I tried showing up the status code within the status panel but all the cases I've tested it returned "0" value so I decided to leave it as it is.

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

There seems to be several categories of HTTP status codes.

We could show, for instance, red, yellow, or green based on the status code category and display the specific status code message on hover. One idea would be the following:

  • 200s - green
  • 300s - yellow
  • 400s - yellow
  • 500s - red

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

@frenchbread where is the line of code where you are handling the server response? What does the response object contain?

@frenchbread
Copy link
Contributor Author

@brylie Status object returned from the server looks like this.

I've tested displaying different kind of info depending on status code but the in most cases it has zero value.

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

What are some example cases where you are getting a zero value? What value is zero?

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

HTTP.call("GET", "http://domain.com");

The above should return an object with a status code. That should work for non-existing domains as well, since it will throw an error.

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

Is there a better JS utility for checking a website? E.g. one that would pass us any relevant errors?

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

Also, check where you are trying to use the return value. Meteor methods do not return a value as you might expect from normal funcitons. Method values are returned in a callback funciton, wherein you can set a template reactive variable.

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

Lets clean up the isUp code, since we can check the status code directly in the template.

@jykae
Copy link
Contributor

jykae commented Jun 9, 2016

I was just about to write to check if result exists, and maybe do things on client in callback like @brylie says above.

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

Also, we might not need a server method here, since we are not storing the data.

@jykae
Copy link
Contributor

jykae commented Jun 9, 2016

That's also a good point :) Less code.

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

Right, I just tried it, but it has some difficulties about cross-origin requests.

@frenchbread
Copy link
Contributor Author

Added some refactoring, cleanups & modified the layout of status:

screen shot 2016-06-09 at 13 37 06

So not it's super easy to reuse the status indicator, e.g. :

{{> viewApiBackendStatus apiBackend=apiBackend width="0.4"  }}

Since this is a circle, width==heigh. In ems.

@apinf/developers Please review/test/merge.

@jykae
Copy link
Contributor

jykae commented Jun 9, 2016

@frenchbread All right, reviewing :)

@55 55 added this to the Sprint 24 milestone Jun 9, 2016
@jykae
Copy link
Contributor

jykae commented Jun 9, 2016

Great now it works and it gives feedback for user 👍
Suggestion for text "Code 401. API requires authorization."
I think "client error" is not that clear what it means if 401 is not clear at first for the user.

nayttokuva 2016-06-09 kello 13 51 31

// Init indicator element
const apiStatusIndicator = $('#api-status-indicator');

// Init regEx for status codes
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, wow, you use regexes for status codes, cool

@brylie
Copy link
Contributor

brylie commented Jun 9, 2016

We would have to generate a help text for each status code, since the response object only contains the number. Lets discuss filing that as an enhancement.

@frenchbread
Copy link
Contributor Author

Added internationalisation.

@jykae
Copy link
Contributor

jykae commented Jun 9, 2016

Cool. Tested, still works, merging.

@jykae jykae merged commit 1673647 into develop Jun 9, 2016
@jykae jykae deleted the hotfix/api-statuscode-check branch June 9, 2016 11:26
@jykae jykae removed the in progress label Jun 9, 2016
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