-
Notifications
You must be signed in to change notification settings - Fork 11k
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
chore!: Remove meteor/check
from custom-user-status
endpoints
#32549
chore!: Remove meteor/check
from custom-user-status
endpoints
#32549
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 |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #32549 +/- ##
============================================
+ Coverage 58.59% 75.70% +17.10%
============================================
Files 2739 432 -2307
Lines 65696 19919 -45777
Branches 14826 5084 -9742
============================================
- Hits 38496 15079 -23417
+ Misses 24420 4269 -20151
+ Partials 2780 571 -2209
Flags with carried forward coverage won't be shown. Click here to find out more. |
@@ -35,14 +36,55 @@ const CustomUserStatusListSchema = { | |||
|
|||
export const isCustomUserStatusListProps = ajv.compile<CustomUserStatusListProps>(CustomUserStatusListSchema); | |||
|
|||
type CustomUserStatusCreateProps = { name: string; statusType?: UserStatus }; |
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.
The validation expects statuses not 100% equal to this type as you can see here: https://github.com/RocketChat/Rocket.Chat/blob/develop/apps/meteor/app/user-status/server/methods/insertOrUpdateUserStatus.ts#L67
Are we good with this change?
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've created a new CustomizableUserStatus
type/enum to fix this
name: { | ||
type: 'string', | ||
}, | ||
statusType: { |
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.
nullable?
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.
This is a required field (as you can check here). Since this is upposed to be breaking, I believe we can fix this (make it required even though it didn't use to be)
name: { | ||
type: 'string', | ||
}, | ||
statusType: { |
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.
nullable?
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.
This is a required field (as you can check here). Since this is upposed to be breaking, I believe we can fix this (make it required even though it didn't use to be)
ea82dad
to
7e69ab1
Compare
bc4fad7
to
b484993
Compare
b285fd4
to
39a6fe1
Compare
ce44882
to
d94b784
Compare
30f6f98
to
8c447cf
Compare
1ef21d2
to
8c447cf
Compare
05b65e0
to
ee50d3f
Compare
Co-authored-by: Douglas Gubert <[email protected]>
* regression(Marketplace): Restore missing button on empty state * regression(Marketplace): Restore missing text in `AppsUsageCard` * regression(Marketplace): Fix misalignment on marketplace header * regression(Marketplace): Fix wrong import on uninstall app modal * regression(Marketplace): Restore link to uninstall app modal
…point (#31889) * Do not allow unused joinDefaultChannels param in users.update * Do not allow user creation on users.update endpoint --------- Co-authored-by: Marcos Spessatto Defendi <[email protected]>
* fix: Update translation key used by 'Forgot Password' e-mail body setting --------- Co-authored-by: Marcos Spessatto Defendi <[email protected]>
…cardTranscript methods (#33432)
ca695dc
to
d2ef9e6
Compare
|
bfefe41
to
204272c
Compare
08d4324
to
57dd899
Compare
Proposed changes (including videos or screenshots)
meteor/check
fromcustom-user-status
endpoints by using thevalidateParams
property.Issue(s)
CORE-318
Steps to test or reproduce
Further comments
The intention for this PR is to improve code readability and make invalid params errors more standardized. No side effects are expected. We're considering this a breaking change since invalid params errors are being changed to follow the same standard when possible.