CA Task Id 37 - Validate Response from Persistence Layer #282
Conversation
codyseibert
commented
Dec 9, 2018
•
codyseibert
commented
- adding a joiValidationDecorator function which we can pass constructor functions to in order to decorate the prototype with joi specific validation code
- added a new method called 'validateRawCollection' which allows you to pass an array and each entity inside the array will be valided.
- adding tests over all the use cases to verify these entities are validated before they are returned.
… entities returned from persistence
There was a problem hiding this comment.
This is great work. Thanks for the work Cody.
The general point I make below is that we sometimes return things from a use case that don't really make sense. For example, if an IRS attorney files an answer, why are we returning the case rather than a success or failure code with some metadata about the filing (like who and when).
I've raised this question with Will, and he addressed it to some degree in the "Send to IRS" use case.
As a general point, we want to avoid premature optimization and returning something base on speculation about what the user will want to see next. I left a few related comments below.
| userId, | ||
| applicationContext, | ||
| }); | ||
|
|
||
| return new Case(updatedCase).validate().toJSON(); |
There was a problem hiding this comment.
Why does file answer return an entire case? I would expect this to return the result of the filing plus a little metadata about the answer (when it was filed and who filed it).
There was a problem hiding this comment.
Changing it to return the document metadata associated with the created document
| applicationContext, | ||
| }); | ||
|
|
||
| return new Case(caseAfterUpdate).validate().toJSON(); |
There was a problem hiding this comment.
I don't know of any standard that says you shouldn't return the updated object. It's usually done because the backend has made some alteration to the data that the client needs to get back. Is there some standard we are violating here?
There was a problem hiding this comment.
Yeah, I'm not to sure about this. I've always seen update methods return the update objects.
There was a problem hiding this comment.
@sadlerw Can you share a "standard" that says you should? I can see that before the age of SPAs, a web/application server would do this because of the way web forms worked or as an optimization. But there is nothing about the use case interactor's semantic that suggests that this is what it should return.
For example, when I submit an order to Amazon, I just want to see that it was successfully placed. I don't need (or want) to see the order actually refreshed, especially since I just looked at it and it's still showing on the page.
| @@ -3,6 +3,7 @@ const assert = require('assert'); | |||
| const Case = require('./Case'); | |||
|
|
|||
| const A_VALID_CASE = { | |||
| docketNumber: '00101-18', | |||
There was a problem hiding this comment.
We've updated the docket number pattern to remove leading zeroes. Although these tests will probably pass, you may want to adjust based on the very-recently-merged code.
| @@ -0,0 +1,41 @@ | |||
| const joi = require('joi-browser'); | |||
|
|
|||
| exports.joiValidationDecorator = function(entityConstructor, schema) { | |||
| caseRecord: new Case({ | ||
| userId: userId, | ||
| docketNumber: docketNumber, | ||
| documents: documents, |
There was a problem hiding this comment.
These could be shortened to {userId, docketNumber, documents}
| @@ -25,7 +25,11 @@ describe('Create case', () => { | |||
| getPersistenceGateway: () => { | |||
| return { | |||
| createCase: () => | |||
| Promise.resolve({ caseId: 'c54ba5a9-b37b-479d-9201-067ec6e335bb' }), | |||
| Promise.resolve({ | |||
| docketNumber: '00101-18', | |||
There was a problem hiding this comment.
see previous mention of leading zeroes.
…om/flexion/ef-cms into task/CA_37_verify_response_objects
