-
Notifications
You must be signed in to change notification settings - Fork 76
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
Ocrvs 8397 #8565
Ocrvs 8397 #8565
Conversation
Oops! Looks like you forgot to update the changelog. When updating CHANGELOG.md, please consider the following:
|
@@ -21,8 +21,6 @@ module.exports = { | |||
'@typescript-eslint/no-unnecessary-boolean-literal-compare': 2, | |||
'@typescript-eslint/no-non-null-assertion': 2, | |||
'@typescript-eslint/no-unnecessary-condition': 1, | |||
'@typescript-eslint/no-unsafe-argument': 1, |
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.
Let's reintroduce them when ts versions match
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.
For now, for each action add the same form.
/** | ||
* Quick-and-dirty mock data generator for event actions. | ||
*/ | ||
function mapTypeToMockValue(field: FieldConfig, i: number) { |
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.
Will be moved to same place with others on the next PR
input: ActionInputWithType | ||
} | ||
|
||
function mapTypeToZod(type: FieldConfig['type'], required?: boolean) { |
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.
Will be moved to same place with others on the next PR
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.
Looks great 👍
packages/commons/src/events/utils.ts
Outdated
configuration: EventConfig, | ||
action: ActionType | ||
) => { | ||
const actionConfig = configuration?.actions.find((a) => a.type === action) |
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.
const actionConfig = configuration?.actions.find((a) => a.type === action) | |
const actionConfig = configuration.actions.find((a) => a.type === action) |
await getEventConfigurationById({ | ||
token, | ||
eventType | ||
}), |
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.
Previously the result expectation convention in the codebase has been so that if a method starts with get..
it's implied to throw an error if said thing is not found. find..
starting ones are ones that return a null or undefined but do not throw. Let's change this to either internally throw or be a find... function
After merging, all partial form submissions will result to 400.
To keep this easy to review, I suggest separate PRs for:
FieldValue
refactoring and gather FieldType based mappers to one place on the next oneallowedWhen
conditionals