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

[fider/581] Invalid posts should return a 404 and not a 500 #589

Merged
merged 10 commits into from Oct 21, 2018

Conversation

Projects
None yet
3 participants
@peterver
Copy link
Contributor

peterver commented Oct 20, 2018

Issue: #581

  • Adjust the handlers to send back a 404 if the ParamAsInt("number") validation fails.
  • Add an endpoint to retrieve a post (Rest)
@goenning
Copy link
Member

goenning left a comment

Just two comments, it's looking great! Thank you!

@@ -159,6 +159,7 @@ func routes(r *web.Engine) *web.Engine {
api.Use(middlewares.IsAuthenticated())

api.Post("/api/v1/posts", apiv1.CreatePost())
api.Get("/api/v1/posts/:number", apiv1.GetPost())

This comment has been minimized.

@goenning

goenning Oct 20, 2018

Member

Wow, how did I miss this API? Thank you!

Few comments:

  1. We can move it before the IsAuthenticated middleware, right next to /api/v1/posts
  2. Align it with the rest of the code.
@@ -52,6 +52,23 @@ func CreatePost() web.HandlerFunc {
}
}

// GetPost retrieves the existing post by id

This comment has been minimized.

@goenning

goenning Oct 20, 2018

Member

id -> number

// GetPost retrieves the existing post by number

@peterver

This comment has been minimized.

Copy link
Contributor

peterver commented Oct 20, 2018

@goenning will take a look at the rfc tomorrow :) !

peterver added some commits Oct 21, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 21, 2018

Codecov Report

Merging #589 into master will decrease coverage by 0.02%.
The diff coverage is 46.15%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #589      +/-   ##
==========================================
- Coverage   75.13%   75.11%   -0.02%     
==========================================
  Files         100      100              
  Lines        6397     6407      +10     
==========================================
+ Hits         4806     4812       +6     
- Misses       1247     1249       +2     
- Partials      344      346       +2
Impacted Files Coverage Δ
app/handlers/post.go 62.12% <0%> (ø) ⬆️
app/cmd/routes.go 87.18% <100%> (+0.11%) ⬆️
app/handlers/apiv1/post.go 60.28% <45.45%> (-0.32%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6ffc1b...497b884. Read the comment docs.

@peterver

This comment has been minimized.

Copy link
Contributor

peterver commented Oct 21, 2018

@goenning changes are done :)

@peterver

This comment has been minimized.

Copy link
Contributor

peterver commented Oct 21, 2018

@goenning I also added a test for the new GetPost route

@peterver peterver force-pushed the ValkyrieStudios:master branch from a3620cc to 374e1a2 Oct 21, 2018

peterver added some commits Oct 21, 2018

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 21, 2018

@peterver Add a Expect(query.String("title")).Equals("My First Post") assertion to that unit test :)

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 21, 2018

@peterver one more thing, you need to move the api.Get("/api/v1/posts/:number", apiv1.GetPost()) before the api.Use(middlewares.IsAuthenticated()). Posts are public and without that only authenticated users would be able to use that API.

@peterver

This comment has been minimized.

Copy link
Contributor

peterver commented Oct 21, 2018

there ya go @goenning :)

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 21, 2018

Approved! But looks like CircleCI didn't run the build :(

@peterver

This comment has been minimized.

Copy link
Contributor

peterver commented Oct 21, 2018

@goenning probably stuck in a queue somewhere :/. Anyway to retrigger a circleci build or let's just way for the queue to do its thing ?

@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 21, 2018

I don't see any queue on CircleCI. Can you change the title or description on that test and push another commit? That should force a new build.

@peterver

This comment has been minimized.

Copy link
Contributor

peterver commented Oct 21, 2018

@goenning There we go, now circle ci is rollin :)

@goenning goenning merged commit ccb077a into getfider:master Oct 21, 2018

5 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: push Your tests passed on CircleCI!
Details
ci/circleci: setup Your tests passed on CircleCI!
Details
ci/circleci: test-server Your tests passed on CircleCI!
Details
ci/circleci: test-ui Your tests passed on CircleCI!
Details
@goenning

This comment has been minimized.

Copy link
Member

goenning commented Oct 21, 2018

That's merged, thank you!

I'll prepare and cut the 0.16.0 release now! 🎆

oxynux added a commit to oxynux/fider that referenced this pull request Oct 24, 2018

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