294/Add field type to custom fields #397
Conversation
AdamVig
force-pushed
the
294/add-field-type-to-custom-fields
branch
from
5f3a214
to
f82ed6d
Compare
|
Test coverage may be significantly reduced by this PR due to the addition of several public methods in |
|
I ended up writing the additional tests I mentioned, and ended up increasing test coverage by 0.1% as a result. |
Custom field values are automatically cast to a string when being inserted into the database. Now, with the addition of `customField.fieldTypeID` and the table `fieldType`, custom field values are type cast back to the correct type before sending them to the user. This implementation uses the partial hack of overriding `res.send` before calling the endpoint service. A better way to implement it would be to pass the endpoint service a function that modifies results before sending them back to the user, but this is the only controller that has this use case, so it makes more sense to rely on the current implementation for now.
AdamVig
force-pushed
the
294/add-field-type-to-custom-fields
branch
from
47a812c
to
6290b8a
Compare
| if (fieldTypeID === fieldType.number) { | ||
| return Number.parseInt(value, 10) | ||
| } else if (fieldTypeID === fieldType.currency) { | ||
| return Number.parseFloat(value) |
There was a problem hiding this comment.
Would it be worth making sure the value here has two digits?
There was a problem hiding this comment.
Also, if dealing with digits is too much of a hassle, something that we do at work is to convert all currencies * 100. This way, you can still send a number without having to worry about the digits. Then the front end just has to / 100 to display the correct value.
There was a problem hiding this comment.
Hm, I'm not sure I understand. What would that tell us about the data if we know it has two digits?
There was a problem hiding this comment.
Oh maybe I don't understand what this does correctly. Is it meant to format the data sent to the client, or format the data stored in the db sent from the client?
There was a problem hiding this comment.
This function is run before a response is sent, so it formats the data before it gets sent to the client. All it actually does is convert a string to the correct type. Does that help?
There was a problem hiding this comment.
Oh ok, so I just wanted to make sure we display currencies with two digits on the app, so it could be formatted on either the client or the api (it also might not be worth sending a float with many digits when we only need 2). Is that enforced when storing values to the database?
There was a problem hiding this comment.
The backend casts it to a string on the way into the database, so it should preserve all digits, then on the way back it should have exactly the same number of digits. That seems like a presentational constraint rather than a data constraint - I think that you should enforce the number of digits on the front-end, either by validating the input or or formatting the response from the backend. What do you think?
There was a problem hiding this comment.
Ok yeah that makes sense
Closes #294.
Note: this branch is based on #396, so you will see commits from both branches in this PR. The relevant commit for this branch is f82ed6d.