codefortulsa / courtbot-engine Goto Github PK

Unit tests = good.

getCaseParties should return the unique set of party names. Right now it will contain duplicates if multiple services return the same name.

Example case that has had a warrant dropped: CM-14-3319.

The way they're written, unit tests do not currently simulate network latency. They should, if only to test for improperly handled race conditions in higher-level modules.

[Aside: Because the calls to the event handlers are synchronous/blocking, it's possible that poorly written data modules could stall execution of server code for an indeterminate amount of time.

As far as making sure data sources put Promises into return.promises, my instinct is to make that explicit in the unit testing for the data sources themselves, and have a soft assert that all data modules which handle retrieve-parties and retrieve-party-events must do this. Am I missing anything here?]

(Edited: the functions don't return promises, they put them into the result.promises Array. Updated to reflect that.)

We should decide how to handle partial failures (one data service fails to retrieve but others succeed). We should, at minimum:

1. Log all errors to log4js
2. Continue with the partial data we DO have.

It might be nice to advise the user somehow that we can't retrieve data correctly? A status page would be nice to. Maybe collect failures and show some sort of health indicator?

Move the default messaging out of courtbot into courtbot-engine. Add events to allow the user to hook in and change messages if they want to. This could allow us to create modules like

• courtbot-engine-messaging-spanish

etc.

#6 The Promise.all() calls inside module.exports.sendDueReminders() and module.exports.checkMissingCases() would cancel message sending without warning if one of the promises in the array fails before all the others complete.

Use of the Router pattern will clean up some code and allow for better testability.

Right now DST is not handled. We need to handle it before March 11th (the 12th is DST)

When scrubbing an object, scrubObject() still calls any getter functions that are attached to enumerable properties. This could include random code that was passed in earlier.

Given how the default options are used in the engine, is this worth addressing?

CI = better.

Courtbook issue on this: codefortulsa/courtbook#7