turingschool-projects / lookingfor

Hello, is it me you're looking for?
2 stars 4 forks source link

Adds Total Pages to Recent Job API Endpoint #107

Closed ghost closed 8 years ago

ghost commented 8 years ago

@JaredRoth Fixes this issue

What does this add?

Adds a 'meta' key to the recent job's API endpoint that returns the total amount of pages for recent jobs.

Requested feedback

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.3%) to 59.626% when pulling e78c0345daa2994dda4fc3d4680258c3b8587699 on adds-total-pages-api-endpoint#102 into 635763bf8a9e5017f737ef742a0bf1c1e61ca47f on master.

ghost commented 8 years ago

@selfup, I added the .DS_Store to the .gitignore file that way it won't be a nuance to other devs to constantly delete it from their directory all the time

AllPurposeName commented 8 years ago

I would avoid adding a meta key. Using an envelope ie { data: {}, meta: {} } is non-trivial. Typically in pagination, things like total pages, page index, pages remaining... whatever.. get stored in the response header. I would only find myself justified in breaking that standard if the consumer (I am guessing just the front end) does not have access to the response headers.

Further meta is typically only used when including meta data that doesn't belong in the header. Usually with special case stuff that breaks RESTful-ness or something.

selfup commented 8 years ago

@Salvi6God You are totally right.

Haha my bad. I guess I need to read which file that was put it.

ghost commented 8 years ago

@justinabrahms, should I follow what @AllPurposeName said and figure out a way how to get the total pages result from the response header?

justinabrahms commented 8 years ago

I believe he is suggesting you put it into a response header, rather than in the json payload.

As for if/how you do that, I don't have a strong opinion.

ghost commented 8 years ago

@AllPurposeName, it doesn't seem to make sense in my case to add anything into the response header due to the discussion I just read from here from the front-end team. It seems like they expect to grab that information from the JSON output. But please feel free to correct me if I'm wrong!

Also, on the JSONAPI examples, there's an example that does exactly what I did where they don't find themselves having to add anything into the headers here.

justinabrahms commented 8 years ago

https://github.com/LookingForMe/lookingForFrontEnd/blob/6f397ed351273f47850d285e2d6fdc4d370c3893/lib/components/SearchBarAndListings.js#L19

According to this line, you have access to the full response on the front end, and should be able to read the headers and set state accordingly. It's purely an implementation choice, not a clear "this is the only right way to do it".

AllPurposeName commented 8 years ago

@Salvi6God Yes it looks like you have capability to do either implementation. You'll find big glorious APIs plugging pagination in either body meta keys or response headers. It's an application's preference and the important thing is your team picks one or the other right now to stick with.

My perspective is that headers already contain the metadata for the response, so why add another metadata container in the body? Additionally the introduction of envelopes can make your other endpoints inconsistent. Sometimes collections are within the data key, sometimes they're on the top level. Again, consider it a decision y'all should make before going much further.

ghost commented 8 years ago

@AllPurposeName just talked to the front-end team and we all agreed upon placing it within the JSON response.

I really like the way you were able to explain your case. Thanks for doing that.

AllPurposeName commented 8 years ago

Cool. In that case +1. I don't know how to order the JSON keys so meta is at top. Maybe a serializer or jbuilder??

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.4%) to 59.733% when pulling e675133b811a6a1a7890b78b19972c99ab8da7fc on adds-total-pages-api-endpoint#102 into 635763bf8a9e5017f737ef742a0bf1c1e61ca47f on master.

ghost commented 8 years ago

@AllPurposeName I looked into the docs for Active Model Serializer and from what I've read it only discusses how to display things for the model itself. In this instance, I don't think that will be the way on how to order the JSON keys 'meta' and 'recent_jobs' since the 'meta' key isn't part of the model nor would I want that key to be displaying multiple time per object.

justinabrahms commented 8 years ago

JSON keys do not have order.

An object is an unordered set of name/value pairs. via http://json.org/

AllPurposeName commented 8 years ago

@Salvi6God Yeah good points. I briefly looked at the pagination docs and didn't see anything in there. Beats me how to fix it. Can you explain why the order is important?

ghost commented 8 years ago

@justinabrahms ahhh, you have a point. I remember reviewing that website when I learned about APIs in Module 3 😅

@AllPurposeName, it doesn't matter I just wanted to try and have the 'meta' key appear on top that way the user won't have to scroll down the page all the time. Either way, it's the program that will make use of the API not a human haha.

ghost commented 8 years ago

Is this PR ready to merge? If so, that way @JaredRoth can merge it :)

JaredRoth commented 8 years ago

Looks good to me and the conversation has been enlightening. Merging.

rrgayhart commented 8 years ago

Good work ya'll!