-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Improve API routing #1557
Improve API routing #1557
Conversation
@Twixes when you get a chance take a look at the conflicts and then I could pick up the review on this, unless there's any objection to that |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. I made some changes to also make the internal use endpoints flexible, as well as made sure the tests were testing endpoints both with and without a trailing slash. Added comments so that people can make sense of the "inconsistency".
I made changes regarding endpoints that weren't in the scope of this PR but that I thought made sense. @Twixes see if the changes make sense and otherwise this should be good to go.
Makes sense. 👍 |
Will have to take a look at these Cypress tests finally… But this should be OK for the moment. |
Yeah I was thinking of doing that and then we got the debug bar PR merged, all new PRs pass, but old PRs that get merged with master don't? |
Changes
Our API endpoints in this form:
/api/events
have confusingly been not working… the difference is just the lack of a trailing slash. That has been a longstanding issue, hard to diagnose for users and annoying when developing. This PR fixes that,along with lacking support for personal API keys in(superseded by #1553), and also adds API-specific 404 handling (as to not confusingly 302 to the app login page when hitting a wrong endpoint)./decide
Updated URLs in a few tests to make this covered by tests.
Checklist