-
Notifications
You must be signed in to change notification settings - Fork 249
Adds defensive check when generating language text in the language dropdown #5465
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: hotfixes
Are you sure you want to change the base?
Conversation
| languageText(item) { | ||
| const firstNativeName = item.native_name.split(',')[0].trim(); | ||
| const nativeName = item?.native_name || ''; | ||
| const firstNativeName = nativeName.split(',')[0].trim(); |
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 similarly to the node title defensive check, we should be trying to understand why this is happening and whether it's indicative of another issue. I imported shared/leUtils/Languages.js in a Node REPL and all of them had a native_name.
> Array.from(LanguagesMap.values()).filter(l => !l.native_name)
[]
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 bug is triggered when trying to edit multiple resources with differing languages. In such cases, and empty object ({}) is returned, thus the bug. The above should be an acceptable fix, I think. However, I have posted here for designers to have their thoughts on UX.
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'll see what design says. If they prefer to keep it as is, perhaps we can use a Symbol to explicitly handle this situation, which will be specific enough that we wouldn't suppress any other possible issues.
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.
Based on designs recommendation, a "new" language "Mixed (Mix)" will be added Languages.vue. The wording could change after string review but should be sufficient for now.
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.
Update: Looks like we don't need to add a new type afterall. We already have "Multiple languages (mull)" option in places.
…nguages are selected
|
@bjester I think we should be good here. Noting here that the display of the Multple languages option is cosmetic and no saving is done. |
bjester
left a comment
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.
Left some comments based off my understanding of the code and the UX behavior, but I didn't actually test it. Let me know if I misinterpreted anything.
contentcuration/contentcuration/frontend/channelEdit/components/edit/DetailsTabView.vue
Outdated
Show resolved
Hide resolved
contentcuration/contentcuration/frontend/channelEdit/components/edit/DetailsTabView.vue
Outdated
Show resolved
Hide resolved
|
@bjester I think we should be good. Thanks for the recommendations--all made sense to me! |

Summary
This pr adds a defensive on the code for the error reported in sentry as detailed here. The bug is triggered when trying to edit multiple resources with differing languages.
References
Fixes #5434
Reviewer guidance