LF-3881 Return better errors by default from the API

[kathyavini]

Description

Context

I wrongly maligned our logger, Winston, as responsible for eating the missing message and stack trace properties on the Error object, because I did not know that those two properties on JS Error objects were not enumerable! (See this StackOverflow: Winston not displaying error details)

The reason I thought Winston was responsible was the addition of the level and services properties after logging, and indeed Winston v3 does by design mutate the original Error object. See this issue on the Winston repo issue tracker -- sorry linking to a search to avoid automatic cross-linking to that repo 😁

Fixing our useless error messages

As of Winston v3, there is a built-in format called error() that parses the non-enumerable Error properties for use with the Winston transports.

However, this does not make them available on the Error object for passing back in the response. To make message available to .send({ error }) or .json({ error }) there are two options:

1. Add them within each controller catch block, either with a dedicated function or just in the response, i.e.

Move from:

return res.status(400).json({ error })

To:

return res.status(400).json({
 ...error, // keep any richer context from custom errors like Model Validation
 message: error.message, // or error: { message: error.message }
});

This has the advantage of being explicit, but would require doing this in every controller.

2. (Implemented) Take advantage of Winston mutation and add the message as a custom property (formatted like the universal error handler in server.js) using one of the built-in Winston methods that has access to info, the Winston reference to the Error object. I used a custom format function, but a custom transport could be used the same way.

This will require console.log(error) or console.error(error) to be invoked in each catch() block before the res.send(), but this needs to be done anyway for the error to be included in our logfiles so it should always be done.

In this PR I therefore added it to the handful of catch blocks that were missing such logging. Although I didn't correct existing blocks, I added new logs at the error level (console.error), because this gives the messages the correct level to end up in the error log file, and makes more sense to me in a catch block.

Bonus content

The ticket was given a bonus task of logging these catch-block caught errors to Sentry. It is absolutely possible, but since it requires iterative testing on beta I will give it its own ticket and branch.

Jira link: https://lite-farm.atlassian.net/browse/LF-3881

Type of change

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• This change requires a documentation update

How Has This Been Tested?

• Passes test case
• UI components visually reviewed on desktop view
• UI components visually reviewed on mobile view
• Other (please explain)

I created a Model Validation Error situation and also looked at the error from LF-3884, which is adding a task targeting a wild crop. I made sure the messages were now visible in

• logfiles in API
• dev console
• response

Checklist:

• I have commented my code, particularly in hard-to-understand areas
• My changes generate no new warnings
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• The precommit and linting ran successfully
• I have added or updated language tags for text that's part of the UI
• I have added "MISSING" for all new language tags to languages I don't speak
• I have added the GNU General Public License to all new files

[antsgar]

Tested it forcing an error and it's working great, I got the right error message returned 🙂

[SayakaOno]

Great improvement 👏