-
Notifications
You must be signed in to change notification settings - Fork 8.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Cases] Custom fields feature fix tests #167472
Changes from 44 commits
0a67b83
dab8dbc
b6b39a6
815c168
4ec0ca7
9a09fbd
9b2b1cd
1fb4348
dfa0495
af5b0f9
e0d0622
c9bc8d3
24bfc06
18d39dd
98e025a
57ffcfc
dd4c36b
77e8545
5706bc8
2ef7158
f4efc20
6aae78d
4d0d554
d500dff
503ccc4
18ecb86
90f8561
3bf2f42
4979cc8
df339ed
af8d52c
debf8fb
fa5b1ed
2628c94
0b2d3b3
01ee8c8
e2ce598
f10dd1b
79c3de0
28304c1
35df316
28209db
0c2de5e
75d2589
0b05b5c
e63c07f
b5f2ffb
df19932
e5ceb36
2249e67
4035897
756405f
f254ca7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -95,12 +95,12 @@ export const validateRequiredCustomFields = ({ | |
requestCustomFields, | ||
customFieldsConfiguration, | ||
}: CustomFieldValidationParams) => { | ||
if (!Array.isArray(requestCustomFields) || !requestCustomFields.length) { | ||
return; | ||
} | ||
|
||
if (customFieldsConfiguration === undefined) { | ||
throw Boom.badRequest('No custom fields configured.'); | ||
if (!Array.isArray(requestCustomFields) || !requestCustomFields.length) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am a bit confused about this change. What scenario are we trying to cover? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is strange, this doesn't seem to be my change 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems that @adcoelho tried to fix some tests in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that if there is no configuration or no custom fields we should return. The function validates only required custom fields so no configuration means no custom fields are required. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We discussed offline and we decided to leave it as it is. |
||
return; | ||
} else { | ||
throw Boom.badRequest('No custom fields configured.'); | ||
} | ||
} | ||
|
||
const requiredCustomFields = customFieldsConfiguration.filter( | ||
|
@@ -109,7 +109,7 @@ export const validateRequiredCustomFields = ({ | |
|
||
const invalidCustomFieldKeys = differenceWith( | ||
requiredCustomFields, | ||
requestCustomFields, | ||
requestCustomFields ?? [], | ||
(requiredVal, requestedVal) => requiredVal.key === requestedVal.key | ||
).map((e) => e.key); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should keep this test and the new test you added ("should throw an error when required `customFields are undefined"). For this test, we need to make all fields in the configuration optional.