Improved Object Validations with New ValidateObject #13301
Conversation
github-actions
bot
added
APP: Learn
Abhishek-Punhani
force-pushed
the
add-new-validators
branch
from
2b94b6b to
7f03167
Compare
Build Artifacts
|
There was a problem hiding this comment.
Some possible cleanup here - I think @nucleogenesis has commented on this elsewhere as well, that maybe a content node spec could be standardized?
| return validateObject(content, { | ||
| id: { type: String, required: true }, | ||
| title: { type: String, required: true }, | ||
| thumbnail: { type: String, required: false, default: '' }, |
There was a problem hiding this comment.
We should avoid using the 'default' specification when using this for a VueJS component validator, as it does not update the value, and might mislead a developer to think that the defaulted values are always present on the object.
There was a problem hiding this comment.
@Abhishek-Punhani I think that this (and the other example @rtibbles noted in his review) should be addressed before merge to avoid committing unnecessary / potentially confusing code
There was a problem hiding this comment.
@rtibbles , I have removed defaults
| id: { type: String, required: true }, | ||
| title: { type: String, required: true }, | ||
| is_leaf: { type: Boolean, required: true }, | ||
| copies: { type: Array, required: false, default: () => [] }, |
There was a problem hiding this comment.
removing this default triggers vue-errors in the console
| preset: { type: String, required: true }, | ||
| lang: { | ||
| type: Object, | ||
| required: false, |
There was a problem hiding this comment.
For objects, you can nest a further spec for the object under the spec key. See here for an example: https://github.com/learningequality/kolibri/blob/develop/packages/kolibri/styles/internal/themeSpec.js#L49
| @@ -56,6 +57,15 @@ | |||
| content: { | |||
| type: Object, | |||
| required: true, | |||
| validator: function (content) { | |||
There was a problem hiding this comment.
Seems like we are already validating a content object elsewhere in the code - it might be worth consolidating a definition, rather than repeating parts of this over and over again:
There was a problem hiding this comment.
I have added spec for nested object validation
There was a problem hiding this comment.
It'd be nice to have a consistent spec for a base content node. It's worth noting though that we'd want to be sure that in certain cases, such as where the spec in LibraryAndChannelBrowserMainContent.vue expects a copies key. So we'd want to be sure we include fields like that directly in the component.
I'm not super sure where we should put the specs - my first sense was alongside the objectSpecs.js file in packages/kolibri/utils (maybe in a subdir) -- but would be interested in @rtibbles thoughts for where it should live
@nucleogenesis , It seems a good idea to have a function like |
|
@nucleogenesis @rtibbles , How do you think we should proceed? I believe we can separate the spec for contentNode validations to improve consistency and reduce redundancy. |
Abhishek-Punhani
force-pushed
the
add-new-validators
branch
from
9158a12 to
85e257e
Compare
WalkthroughThe changes introduce stricter runtime validation for props in several Vue components. The Changes
Sequence Diagram(s)sequenceDiagram
participant ParentComponent
participant VueComponent
ParentComponent->>VueComponent: Passes prop (object or array)
VueComponent->>VueComponent: Runs custom validator (using validateObject)
alt Validation passes
VueComponent-->>ParentComponent: Accepts prop, proceeds as normal
else Validation fails
VueComponent-->>ParentComponent: Emits Vue warning/error
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@nucleogenesis @rtibbles , Will this PR be merged after #13315 is resolved or not? |
|
I'm sorry for delay @Abhishek-Punhani, we will follow-up with you. |
There was a problem hiding this comment.
@Abhishek-Punhani the changes here all LGTM thank you again and I apologize for failing to come back to this sooner.
I think that this is probably good to merge, but we should make note in #13315 that we've implemented some specs here that we can update to use any future consolidated specification.
We're probably going to talk a bit more internally about how to consolidate specs (I particularly like @rtibbles 's suggestion to have the spec track the API endpoint). Thanks again for all of your effort and input on this!
|
@nucleogenesis , Yes, I completely agree! Once #13315 is finalised, we can clear all the tech debt and update the implementation to use the consolidated |
|
Hi @Abhishek-Punhani, thanks a lot for the offer to work on #13315. Looking at the conversation there, it still needs some decisions and I am rather unsure if / when those can be done. Meanwhile, if it's interesting for you, you're welcome to keep an eye on learningequality/studio#5060 where I am adding new issues on weekly basis specifically for our community, and this work has generally higher priority and so we are able to follow-up on everything. I am glad to have you there particularly for more complex issues - you already have lots of experience in our codebases and I've enjoyed collaborating with you. |
|
Sure @MisRob , thanks for the heads-up! |
|
Can this be merged @nucleogenesis? |
Summary
Implemented improved object validations using validateObject function
Objectprops #8903…
References
#8903
…
Reviewer guidance
1. HybridLearningLessonCard.vue
2. DownloadButton.vue
3. LibraryAndChannelBrowserMainContent.vue
Check for any validation failed or invalid prop type erros in console while navigating Learn App
Summary by CodeRabbit