Improved Object Validations with New ValidateObject #13081
Conversation
github-actions
bot
added
APP: Learn
Build Artifacts
|
|
Thanks @Abhishek-Punhani! I skimmed briefly and overall looks as what I would expect. I will invite my colleague @AllanOXDi for more detailed reviews. |
| description: { type: String, required: false }, | ||
| duration: { type: Number, required: false }, | ||
| grade_levels: { type: Array, required: false }, | ||
| lang: { type: Object, required: false }, |
There was a problem hiding this comment.
I think lang store a language code ( "en" or "es"), it should be a String? @MisRob What do you think here?
There was a problem hiding this comment.
Hi, @AllanOXDi In the same file, at line 94, I noticed that content.lang.lang_name is being accessed, which is why I defined lang as an object. Thanks for pointing out this; please let me know your suggestion on this.
There was a problem hiding this comment.
Thanks both. In the screenshot I posted below lang is one of the console errors, so let's see if it's resolved after the PR is updated with fixes.
| description: { type: String, required: false }, | ||
| duration: { type: Number, required: false }, | ||
| grade_levels: { type: Array, required: false }, | ||
| lang: { type: Object, required: false }, |
There was a problem hiding this comment.
The lang object also needs a default value that creates a new object each time. It should be default: () => ({})
There was a problem hiding this comment.
I have implemented the requested changes by setting the lang object's default value to default: () => ({}). Let me know if any further modifications are needed.
Abhishek-Punhani
force-pushed
the
Issue8903
branch
from
887962c to
4beb8f8
Compare
| @@ -148,18 +148,31 @@ | |||
| type: Array, | |||
| required: true, | |||
| default: () => [], | |||
| /** Content node with the following properties: id, is_leaf, title */ | |||
There was a problem hiding this comment.
I think we could even remove comments like this. Nice thing about validateObject is that it is self-documenting.
There was a problem hiding this comment.
Hi @Abhishek-Punhani, I see many errors in the console that appear to be caused by the updates in this pull request. They are not present in the develop branch.
The goal here is to implement validations in a way that fits exactly how the objects are used, or fix problematic usages that fail the validations.
This is quite a lot of cumbersome work that requires thorough examination of code as well as manual testing, so feel free to go slowly step by step - there is really no pressure around amounts of work and such.
Abhishek-Punhani
force-pushed
the
Issue8903
branch
from
4beb8f8 to
4700aaa
Compare
|
@MisRob , There were some incorrect type assignments; I think now all errors are resolved; please check! |
There was a problem hiding this comment.
looks good to me. Thanks @Abhishek-Punhani - I will let my colleague pass a second eye to it
|
Thank you, I will have a look. @Abhishek-Punhani we are now in the period of time shortly before release, so I wanted to note even if all will look good, it may take a while to merge - I will take some time to coordinate with relevant people. |
There was a problem hiding this comment.
Hi @Abhishek-Punhani and @AllanOXDi,
there are still issues, for example here (there may be more)
During development and review, I think it'd be best use Vue Devtools + examining imports in code to find what places need to be previewed, and carefully test step by step.
@Abhishek-Punhani Some components are used at more places, and this task is a bit more tricky than I expected - you may consider decreasing the size of this PR to move this work on smoothly. Perhaps just take care of one or two components, but find a way to test it everywhere where it's used.
|
@MisRob , I am unable to reproduce these console errors. Could you please guide me in doing so? |
|
@MisRob , I didn't import all resources earlier; I saw errors on the console; I will look into them and make changes asap. |
Improved Object Validations with New ValidateObject
Corrected Some Incorrect Type Assignments
Abhishek-Punhani
force-pushed
the
Issue8903
branch
from
4700aaa to
b578bfe
Compare
|
@MisRob , I don't see errors now, but there's some sort of confusion around accessibility_labels and grade_levels regarding whether these should be assigned Object or Array. I have made some changes and don't see any errors now; please check at your convenience! |
|
Thanks for the updates @Abhishek-Punhani. @AllanOXDi would you be available to re-review? Before we proceed with the new review, @Abhishek-Punhani I'd like to confirm if you were able to test this PR fully as suggested here or do we need to consider decreasing the scope? |
Ah yes that could be the cause, thanks for looking into it before I could reply. Well, we will try as best as we can to take care of testing as many places we can, even though we may miss some contexts. I am quite not sure yet how to best organize around these kinds of changes, let's keep trying :) I also plan to invite QA team later, however we will need to provide them with information about what pages to preview since they won't read this from the code. Perhaps @Abhishek-Punhani in your upcoming PRs, you may jot down brief note with the list of places that were affected and then we can all use it to sync? |
Yes, @MisRob, I saw all those errors and fixed them; I was confused over accessibility labels and grade levels. However, assigning them type Array and validating them as both Array and Object fixes the errors. Some errors were due to my previous PR for the exact cause of type assignment to accessibility_labels and grade_levels. I have fixed them too. While reviewing, if you see new errors, please let me know; thanks! |
Yes, @MisRob , It sounds like a good idea; I’ll make sure to include a brief note in my upcoming PRs about the affected areas to help with syncing. |
Implemented improved object validations using validateObject function
Issue Addressed : #8903