-
Notifications
You must be signed in to change notification settings - Fork 13k
chore: standardize schema definition for livechat/custom-fields.save #38376
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
base: develop
Are you sure you want to change the base?
Conversation
|
|
Looks like this PR is not ready to merge, because of the following issues:
Please fix the issues and try again If you have any trouble, please check the PR guidelines |
WalkthroughRemoved strict customFieldId/name validation and made custom-field ID nullable across REST types and model methods, allowing creation flows to proceed without pre-checking the identifier. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #38376 +/- ##
===========================================
+ Coverage 70.77% 70.79% +0.01%
===========================================
Files 3159 3159
Lines 109401 109401
Branches 19675 19674 -1
===========================================
+ Hits 77428 77448 +20
+ Misses 29944 29928 -16
+ Partials 2029 2025 -4
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
e8e1db7 to
08e89d1
Compare
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.
No issues found across 4 files
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.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/models/src/models/LivechatCustomField.ts (1)
52-70: Use explicit null/undefined check for_idparameter.With
_id: string | null, the truthy checkif (_id)treats empty string as "create," which could cause unintended inserts if an invalid ID slips through. Useif (_id != null)instead.Suggested fix
- if (_id) { + if (_id != null) { await this.updateOne({ _id }, { $set: record }); } else { record._id = field; await this.insertOne(record); }
🧹 Nitpick comments (1)
packages/rest-typings/src/v1/omnichannel.ts (1)
4502-4504: Align type optionality with schema.The schema doesn’t require
customFieldId, but the compiled type does. Consider making it optional to keep the type guard sound.✅ Suggested fix
-export const isPOSTLivechatSaveCustomFieldsParams = ajv.compile<{ - customFieldId: string | null; +export const isPOSTLivechatSaveCustomFieldsParams = ajv.compile<{ + customFieldId?: string | null; customFieldData: Omit<ILivechatCustomField, '_id' | '_updatedAt'> & { field: string }; }>(POSTLivechatSaveCustomFieldsSchema);
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/rest-typings/src/v1/omnichannel.ts`:
- Around line 4446-4449: The JSON Schema for the property customFieldId
currently only allows a string but the TypeScript type permits string | null;
update the schema for customFieldId (the property in the omnichannel schema) to
accept null as well — for example by adding "nullable: true" to the
customFieldId schema object (or change type to ["string","null"]) while keeping
the pattern validation for non-null strings.
| customFieldId: { | ||
| type: 'string', | ||
| pattern: '^[0-9a-zA-Z_-]+$', | ||
| }, |
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.
Schema missing nullable: true for customFieldId.
The TypeScript type at line 4502 declares customFieldId: string | null, but the JSON Schema defines it as type: 'string' without nullable: true. AJV will reject requests where customFieldId is explicitly set to null, causing a mismatch with the declared TypeScript type.
🐛 Proposed fix
customFieldId: {
type: 'string',
+ nullable: true,
pattern: '^[0-9a-zA-Z_-]+$',
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| customFieldId: { | |
| type: 'string', | |
| pattern: '^[0-9a-zA-Z_-]+$', | |
| }, | |
| customFieldId: { | |
| type: 'string', | |
| nullable: true, | |
| pattern: '^[0-9a-zA-Z_-]+$', | |
| }, |
🤖 Prompt for AI Agents
In `@packages/rest-typings/src/v1/omnichannel.ts` around lines 4446 - 4449, The
JSON Schema for the property customFieldId currently only allows a string but
the TypeScript type permits string | null; update the schema for customFieldId
(the property in the omnichannel schema) to accept null as well — for example by
adding "nullable: true" to the customFieldId schema object (or change type to
["string","null"]) while keeping the pattern validation for non-null strings.
|
@KevLehman and I found out there is bug in the endpoint validation, as it was incorrectly migrated from its meteor method counterpart
The method implementation validated the Moved back to draft so I can take a better look at this |
Proposed changes (including videos or screenshots)
Livechat portion extracted from #38227
Issue(s)
Steps to test or reproduce
Further comments
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.