message/messages response field inconsistency

[mjs]

Validation error responses (see customErrors.js) have a message field while other responses have a messages field. This is confusing and annoying. We should settle on a single way of returning messages. An array (i.e. messages) is a better choice as it gives the client more flexibility in terms of presenting multiple errors to the user.

To aid the transition period as we update client code the API server should probably return both message and messages for a while for validation errors. Once all client code has been updated the message field can go.

[m-ahsan-nazer]

In lines 266 to 268 middleware.js has
if (!("user" in request)) { throw new customErrors("No user specified.", 422); }

Is this meant to be
if (!("user" in request)) { throw new customErrors.ClientError("No user specified.", 422); }

[mjs]

Questions from email:

My questions:

• Can I document my relevant notes from this email on the issue page on github? Or no is that not the proper use of the issue page.

That is the perfect use of the issue.

• True or false? The javascript files in api/V1 are tested using the python scripts in test directory i.e tests are not performed using javascript scripts.

True. The existing tests are "functional" tests which exercise the API server as a whole from the outside. These tests are written in Python.

It would be helpful to also have lower level unit tests to check finer details but we don't have those yet. Tests like this would be best implemented in JavaScript.

• If 2 is True then there is no test script for customErrors.js. (a) I could/should create a python test script (pytest) to throw errors replicating the calls in the scripts that use customErrors.js? (b) I could/should instead create a javascript file say test_customErrors.js that throws errors replicating the calls in the scripts that use customErrors.js (this sounds easier).

You could try to create a test which triggers an error with the message field and fails because it doesn't use a messages array, and then fix the problem so that the test passes. Triggering this error could be tricky due to the external, functional style of our tests, but it might be possible.

Alternatively, it might be easier to test this change using a JavaScript based unit test. Doing this will require you doing a lot of extra work because we don't currently have any unit tests so there's lots of infrastructure to set up.

How about you try and trigger one of the problematic errors using our existing test infrastructure and see how you go. Writing this test in a new Python test file is fine. If that doesn't work out, let me know and we can figure out an alternative approach.

(More general question) I do all of the above on my fork of the cacophony-api and once I resolve the issue I ask for a merge? How are you/cacophony-people able to follow the details of the problem solving attempt? Or in general you are not interested and only review issues when a merge request is made. 

The process is:

• Fix the problem in a git branch on your machine.
• When it's ready, push the branch to your fork of the repo on Github.
• Create a pull request from the branch. Mention Fixes #168 in the pull request description along with a description of what you've done and why.
• The team will be notified of the pull request and review. The information you've supplied as well as the reference to the github issue will give us the context we need.
• There might be some back and forth in terms of reviews and updates to the PR.
• The PR gets approved and merged.

Does that make sense?

[mjs]

!("user" in request)) { throw new customErrors("No user specified.", 422); }

Is this meant to be
if (!("user" in request)) { throw new customErrors.ClientError("No user specified.", 422); }

Almost certainly! That looks like a bug.

You could fix that and propose it as an initial pull request in order to get familiar with the Github process if you like.