Skip to content
This repository has been archived by the owner on Dec 13, 2018. It is now read-only.

Fix/default response content type #563

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

the-overengineer
Copy link

Update the controller responses (via handleAcceptRequests) to use config.web.produces[0] as the default, instead of text/html, if this array is defined (if not, it will explode in a later step, anyway). Adds tests to check for this behaviour, and some of the other behaviour of that helper, while I was at it.

While the commits thus far do not deal with this, it might be prudent to exchange the next default handlers (which result in a 404) with something like 400 Bad Request default handlers, which is something like what @rdegges mentioned in #487 . A not found is not really semantically correct. @robertjd, thoughts regarding this?

Another important thing to note is that this is a breaking change - a good number of our tests failed because they expected it to default to text/html. This can be solved by either defining a ['text/html', 'application/json'] value to the config.web.produces array, or by explicitly setting the Accept header. So far, it was exactly the reverse (explicit setting to application/json was required).

Fixes #487

@coveralls
Copy link

coveralls commented Dec 19, 2016

Coverage Status

Coverage increased (+0.02%) to 68.555% when pulling 005df40 on fix/default-response-content-type into 3b242a0 on master.

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

Successfully merging this pull request may close these issues.

2 participants