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

Determine if tasks/:task_id/comments/:id is necessary #340

Closed
joshsmith opened this issue Oct 10, 2016 · 3 comments · Fixed by #372
Closed

Determine if tasks/:task_id/comments/:id is necessary #340

joshsmith opened this issue Oct 10, 2016 · 3 comments · Fixed by #372

Comments

@joshsmith
Copy link
Contributor

We have a /tasks/:task_id/comments/:id that our Ember app does not appear to be hitting in tests (is not in the Mirage config), but which the API does serve.

We should determine if this route is strictly necessary, and if not, remove it.

@begedin
Copy link
Contributor

begedin commented Oct 10, 2016

See my reply on #341 (comment)

The same applies here.

@begedin
Copy link
Contributor

begedin commented Oct 18, 2016

I took another look at this and our ember app, within the project/tasks/task route actually does explicitly fetch comments using a taskId query.

However, the only route the ember app needs is tasks/:task_id/comments. The tasks/:task_id/comments/:id is completely unnecessary and would be to begin with, since the :id is more than enough to identify a comment (It's an actual id, not a number).

@begedin
Copy link
Contributor

begedin commented Oct 27, 2016

Just to add some info to this issue, so there's no confusion in the future, pr #372 removed both tasks/:task_id/comments and tasks/:task_id/comments/:id routes.

tasks/:task_id/comments/:id was unnecessary, so it has been removed completely.

tasks/:task_id/comments has been replaced with /comments/, which allows fetching multiple comments using a coalesced id filter, in line with our other endpoints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants