[O2B-1602] Shared BeamTypesDto should return split input rather than simply validation - #2202
[O2B-1602] Shared BeamTypesDto should return split input rather than simply validation#2202graduta wants to merge 8 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2202 +/- ##
==========================================
- Coverage 46.20% 46.17% -0.03%
==========================================
Files 1039 1038 -1
Lines 17131 17120 -11
Branches 3123 3121 -2
==========================================
- Hits 7915 7905 -10
+ Misses 9216 9215 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| const Joi = require('joi'); | ||
| const { validateBeamTypes, BEAM_TYPE_INVALID } = require('../../../utilities/beamTypeUtils'); | ||
| const { CustomJoi } = require('../CustomJoi.js'); |
There was a problem hiding this comment.
I understand this was not introduced in this PR, so not to be implemented here, but would you agree that CustomJoi is a rather nondescript name and should be changed to something like `StringArrayJoi? If so, I will make a note and then the change.
There was a problem hiding this comment.
I do not believe it should be changed to "StringArrayJoi" because the name CustomJoi refers to the wrapper defined in that file.
If you look at the implementation of the wrapper, you will see it defines the method stringArray which is being called in this file.
Thus, to me:
- "CustomJoi" - informs the developer that it is a custom wrapper around the Joi library
- "CustomJoi.stringArray()" - it is the call that returns the string array
| exports.BeamTypesDto = CustomJoi.stringArray() | ||
| .items(Joi.string() | ||
| .trim() | ||
| .min(2) |
There was a problem hiding this comment.
Is this minimum check necessary as the pattern enforces >2 chars with char dash char?
There was a problem hiding this comment.
While the pattern will enforce that, I think it is also important to be self-explanatory when writing code, so that it is easy to maintain it.
Moreover, regexes are in general slow, so in this case a length-based match may be faster for scenarios in which we receive thousands of requests within a second.
| .min(2) | ||
| .max(15) | ||
| .pattern(/^[A-Za-z0-9]+ ?- ?[A-Za-z0-9]+$/) | ||
| .messages({ |
There was a problem hiding this comment.
The custom validator was the only thing raising BEAM_TYPE_INVALID error so I believe this is now dead message handler code.
Supported by the fact the test/api/runs.test.js LN196 test has been changed to assert the default JOI message format.
Do you agree?
There was a problem hiding this comment.
You are correct. I have modified the DTO to have more specific error messages now
I have a JIRA ticket
Notable changes for users:
Notable changes for developers: