-
Notifications
You must be signed in to change notification settings - Fork 3.2k
fix: BROS-592: Correctly handle whenRole for required controls
#8848
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
Basically it's just a duplication of the similar check in `Visibility` mixin.
✅ Deploy Preview for label-studio-docs-new-theme canceled.
|
✅ Deploy Preview for label-studio-playground ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for heartex-docs canceled.
|
|
/git merge
|
✅ Deploy Preview for label-studio-storybook ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #8848 +/- ##
===========================================
- Coverage 66.35% 57.54% -8.81%
===========================================
Files 789 561 -228
Lines 63529 40888 -22641
Branches 10810 11045 +235
===========================================
- Hits 42153 23529 -18624
+ Misses 21376 17355 -4021
- Partials 0 4 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| return { | ||
| validate() { | ||
| const whenRoles = self.whenrole ? self.whenrole.split(",") : null; |
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.
might worth doing toLowerCase here and below when you check includes so that user won't mess it up
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.
we have different visibility params like whenChoiceValue and whenLabelValue and they all are case-sensitive, so I'd leave it like this for consistency. better to keep these values strict to avoid disambiguity.
| if (label && label !== self.whentagname) continue; | ||
| } | ||
|
|
||
| if (whenRoles && !whenRoles.includes(reg.chatmessage?.role)) continue; |
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.
if (whenRoles && !whenRoles.includes(reg.chatmessage?.role)) continue;
This feels oddly specific to include chatmessage in a base mixin
- use less specific `role` instead of `chatmessage.role` - move condition out of "region-selected" one - look for `whenrole` in parent `View` tags as well
|
/git merge
|
Basically it's just a duplication of the similar check in
Visibilitymixin.