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

fix(api): added error handling for undefined api routes #187

Merged
merged 6 commits into from
Jul 27, 2019

Conversation

dingtaoliu
Copy link
Contributor

Added a 404 route that matches urls for undefined '/api' routes for the rails backend. Previously, undefined api routes are handled as frontend react router routes and rails would return a 200 status.

Copy link
Contributor

@siefkenj siefkenj left a comment

Choose a reason for hiding this comment

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

Can you add an javascript API test for this?

@@ -7,6 +7,10 @@ class ApplicationController < ActionController::Base

def index
render :file => 'public/index.html'
end
end
Copy link
Contributor

Choose a reason for hiding this comment

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

spaces, not tabs


end

def not_found
Copy link
Contributor

Choose a reason for hiding this comment

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

Spaces, not tabs

function unknownRouteTests(api = { apiGet, apiPost }) {
const { apiGet, apiPost } = api;

it("should succeed GET request with known '/api' routes", async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is not needed. It is already done elsewhere

@@ -352,6 +366,9 @@ describe("API tests", () => {
describe("`/applications` tests", () => {
applicationsTests({ apiGET, apiPOST });
});
describe("`/404` tests", () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Describe these tests better

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

2 participants