standardize validator functions to all `Object` props #8903

indirectlylit · 2021-12-13T06:50:43Z

Observed behavior

Following up from: #8640, #8878, and #8902 which added validation functions.

There are a large number of Object-type props with no validator function. (Search the codebase using a multi-line regex to help find them: {[^}]*type: Object[^}]*})

Normal component props are well-documented using type, required, and sometimes validator. The keys of Object props are as much a part of the component API as the other ones, but they are often under-documented.

Even when validation of Object props is done in Kolibri, it is performed in an ad-hoc manner with little consistency and the logging is less useful because usually Vue will simply say the validator failed without specifying why. Some examples:

kolibri/kolibri/core/assets/src/views/sync/FacilityAdminCredentialsForm.vue

Lines 54 to 60 in c3238a8

    
                 device: { 
        
                   type: Object, 
        
                   required: true, 
        
                   validator(val) { 
        
                     return val.name && val.id && val.baseurl; 
        
                   }, 
        
                 },

kolibri/kolibri/plugins/setup_wizard/assets/src/views/importFacility/SelectFacilityForm.vue

Lines 67 to 73 in c3238a8

    
                 device: { 
        
                   type: Object, 
        
                   required: true, 
        
                   validator(val) { 
        
                     return isString(val.name) && isString(val.id) && isString(val.baseurl); 
        
                   }, 
        
                 },

kolibri/kolibri/plugins/learn/assets/src/views/EmbeddedSidePanel/SelectGroup.vue

Lines 72 to 79 in c3238a8

    
                 value: { 
        
                   type: Object, 
        
                   required: true, 
        
                   validator(value) { 
        
                     const inputKeys = ['channels', 'accessibility_labels', 'languages', 'grade_levels']; 
        
                     return inputKeys.every(k => Object.prototype.hasOwnProperty.call(value, k)); 
        
                   }, 
        
                 },

Expected behavior

Component object props should have a validator and fill in default values which should simplify internal logic of components using these props

The text was updated successfully, but these errors were encountered:

nikkuAg · 2024-02-09T16:00:08Z

Is this issue still open? I would like to work on this

MisRob · 2024-02-12T07:23:45Z

Hi, @nikkuAg, yes, thanks for volunteering

MisRob · 2024-02-12T07:28:54Z

This seems to be relevant https://github.com/learningequality/kolibri/pull/8902/files#diff-294e45dbea9c24d93f492cafd5cf86476c419f189cbb3bf0d77f7ffb9f8ac220

indirectlylit · 2024-02-12T23:44:03Z

This seems to be relevant

yes, that was the intent - thank you

Introduces two new functions, validateObject and objectWithDefaults, which bring Vue prop-like validation and default behavior to nested javascript objects. This can be used with component props, vuex and API validation, and other scenarios

a stopgap for lack of TypeScript, perhaps :)

rtibbles · 2024-02-13T02:10:43Z

a stopgap for lack of TypeScript

Except we could use this for runtime validation, which TypeScript does not provide!

nikkuAg · 2024-03-01T18:12:12Z

Hi,
I am not entirely able to understand what I have to do.

rtibbles · 2024-03-04T17:23:17Z

The basic idea is to use the Object validation specification work that was done in the referenced PRs, and use it to fully document each of the cases where an Object type is used as a prop.

This allows for better specification of the intended properties and shape of the prop, and to give more semantic error messages when these expectations are violated.

I would start by looking at one of the examples listed in the issue, and replacing the validator function with the Object validation specification.

MisRob · 2024-04-05T13:19:06Z

Hi @nikkuAg, are you still planning to work on this at some point or would it be better to unassign?

nikkuAg · 2024-04-07T08:26:00Z

Hi @MisRob,
I'm currently occupied with another task, but once I complete it, I'll be able to pick this up. Therefore, I believe it would be best to unassign me for now so that someone else can take over.

MisRob · 2024-04-08T03:13:27Z

Okay @nikkuAg, thank you, let us know any time if you'd like to return to this

tiffcoding · 2024-12-09T19:38:25Z

hello, can i be assigned to work on a few of these validators?

rtibbles · 2024-12-09T20:22:24Z

Hi @tiffcoding - yes, and please do feel free to open a PR with just a few of them, there is no need to do all of these all at once!

MisRob · 2024-12-17T18:34:06Z

Hi @tiffcoding, I wanted to mention that Learning Equality will be closed from December 23 to January 5.

Ammog-Warrier · 2024-12-27T18:58:52Z

Hello there. Is this still being worked on?Asking since its been 3 weeks. I'm open to working on it if it's open. Thank You

akolson · 2025-01-03T17:49:56Z

Great! Thank you! I'll assign it to you shortly!

nucleogenesis · 2025-01-15T00:43:51Z

@Ammog-Warrier do you still plan to tackle this issue?

In any case, @tiffcoding 's work in #12921 may be helpful for whoever ends up working on this.

Ammog-Warrier · 2025-01-15T03:13:28Z

@Ammog-Warrier do you still plan to tackle this issue?

In any case, @tiffcoding 's work in #12921 may be helpful for whoever ends up working on this.

Yes yes, I'm currently working on it. I just glt sidetracked for few days because of some life stuff. Thank you for the advice. I'll look into it

akolson · 2025-01-15T07:06:50Z

Thank you @Ammog-Warrier for your confirmation!

Ammog-Warrier · 2025-01-18T03:47:19Z

Hey there! sorry for this, but I was free for a bit but my schedule just got even worse loaded than I expected. I sincerely apologise and request you to assign this issue to someone else.

Abhishek-Punhani · 2025-01-18T04:31:53Z

@MisRob Can i work on this issue?

MisRob · 2025-01-20T07:59:10Z

Hi @Ammog-Warrier, no problem at all, thank you for letting us know.

Hi @Abhishek-Punhani, yes I will assign you. Thanks!

MisRob · 2025-01-20T08:01:34Z

@Abhishek-Punhani I'd recommend you study #12921 and @nucleogenesis's review - it has helpful information for this task

Abhishek-Punhani · 2025-01-20T11:16:03Z

@MisRob I was looking into this issue and I found that changes could be done ContentPage.vue,SelectGroup.vue and FacilityAdminCredentialsForm.vue
I didnt find any validator used in kolibri/plugins/setup_wizard/assets/src/views/importFacility/SelectFacilityForm.vue

Is this what I am supposed to do?

MisRob · 2025-01-21T15:19:52Z

Hi @Abhishek-Punhani, I think there's many many places to update in Kolibri, basically any time there's an Object prop. I'd definitely recommend your start with just a very small number of files for your first pull request. After we confirm it's all good, you can continue with more places if you wish to. Your screenshot looks like a good direction to me.

MisRob · 2025-01-21T15:34:50Z

@Abhishek-Punhani We don't only want to update existing validators but also to validate objects that don't have any validators yet. But as said, in smaller chunks ideally :) Thanks!

Abhishek-Punhani · 2025-01-21T18:57:24Z

@MisRob Thank you for your feedback. I’ve raised a PR where I modified the validators function in three Vue files to incorporate the validateObject function. Please review it at your convenience. I believe I can implement the validateObject and objectWithDefaults functions in upcoming PRs, but I would greatly appreciate your guidance to ensure they are implemented effectively.

MisRob · 2025-02-12T17:40:59Z

Hi @Abhishek-Punhani, thank you! I think you can feel free to continue working on this issue.

Abhishek-Punhani · 2025-02-12T17:58:56Z

@MisRob Thanks! Since this approach has been applied to a few files in the merged PR, should I continue making similar changes across other relevant files, or would you prefer a different approach? Let me know how you'd like me to proceed!

MisRob · 2025-02-12T18:23:51Z

Yes @Abhishek-Punhani, feel free to gradually work through this as you wish. The only thing I'd recommend opening pull requests of reasonable size since reviews will be easier and faster than having a huge PR, and that's all. However you can definitely do a bit more than in the first one, perhaps around 10 files in a single PR? That's just my opinionated recommendation, you're welcome to choose.

Abhishek-Punhani · 2025-02-13T04:57:01Z

Got it . Thanks @MisRob . I will try to raise a PR soon

Abhishek-Punhani · 2025-02-14T11:06:59Z

Hi @MisRob I think we can begin by resolving all the files in the Learn App. I have raised a PR modifying 10 files—please review it at your convenience.

MisRob · 2025-02-17T03:28:28Z

Sure, thanks @Abhishek-Punhani

EshaanAgg · 2025-02-18T19:37:04Z

Hi! Since the scope of this issue is the entire codebase, would it be okay if I, too, joined to help? I can coordinate with @Abhishek-Punhani to work on disjoint directories, so that there is no duplication of efforts and we can work on the issue parallely.

Abhishek-Punhani · 2025-02-20T12:16:42Z

Hi @EshaanAgg , I think it would it better if we both collaborate together and coordinate to avoid any conflicts in our PRs. Let me know how you'd like to split things up! I think with this we can close this issue at a faster pace . @MisRob Please let me know how would you like me and @EshaanAgg to proceed with this issue!

EshaanAgg · 2025-02-20T13:02:09Z

I was able to jot up a quick script that listed all the files that match the given regex and probably require intervention. One way to open up the issue to multiple contributors would be to treat this message (or we can copy this into the issue description itself) and treat it as the issue tracker. Any contributor can message here to claim a folder that he/she wants to work on, and the maintainers can edit the message to add a WIP @Contributor in front of the folder to indicate the same. Once the folder is done, we can delete it from the markdown or check the checkbox!

How does this approach sound?

Files with Object-Type Props Missing Validators

MisRob · 2025-02-24T04:04:09Z

Hi @EshaanAgg, thanks a lot for helping to coordinate, this would be very helpful.

This was referenced Dec 13, 2021

add object validation and apply it to themes #8902

Merged

add core validator generator for Object-type props #8878

Closed

marcellamaki added this to the upcoming patch milestone Jan 5, 2022

marcellamaki modified the milestones: upcoming patch, Planned Patch 6 Jul 12, 2022

marcellamaki removed this from the Planned Patch 6 milestone Aug 29, 2022

rtibbles added the help wanted Open source contributors welcome label Jan 3, 2024

rtibbles added this to the Kolibri 0.17: General maintenance, bugs, misc. important fixes milestone Jan 3, 2024

MisRob assigned nikkuAg Feb 12, 2024

MisRob unassigned nikkuAg Apr 8, 2024

marcellamaki modified the milestones: Kolibri 0.17: General maintenance, bugs, misc. important fixes, Kolibri 0.17: Planned Patch 1 Jun 24, 2024

rtibbles modified the milestones: Kolibri 0.17: Planned Patch 1, Kolibri 0.18: General maintenance Aug 9, 2024

rtibbles assigned tiffcoding Dec 9, 2024

tiffcoding mentioned this issue Dec 9, 2024

Standardizing Validator Functions for Object props #12921

Closed

akolson assigned Ammog-Warrier and unassigned tiffcoding Jan 3, 2025

marcellamaki modified the milestones: Kolibri 0.18: General maintenance, upcoming patch Jan 9, 2025

MisRob assigned Abhishek-Punhani and unassigned Ammog-Warrier Jan 20, 2025

Abhishek-Punhani mentioned this issue Jan 21, 2025

Modified validators in few files according to new ValidateObject #13015

Merged

Abhishek-Punhani mentioned this issue Feb 14, 2025

Improved Object Validations with New ValidateObject #13081

Open

standardize validator functions to all Object props #8903

standardize validator functions to all Object props #8903

Comments

indirectlylit commented Dec 13, 2021

Observed behavior

Expected behavior

nikkuAg commented Feb 9, 2024

MisRob commented Feb 12, 2024

MisRob commented Feb 12, 2024

indirectlylit commented Feb 12, 2024

rtibbles commented Feb 13, 2024

nikkuAg commented Mar 1, 2024 • edited Loading

rtibbles commented Mar 4, 2024

MisRob commented Apr 5, 2024

nikkuAg commented Apr 7, 2024

MisRob commented Apr 8, 2024

tiffcoding commented Dec 9, 2024

rtibbles commented Dec 9, 2024

MisRob commented Dec 17, 2024

Ammog-Warrier commented Dec 27, 2024

akolson commented Jan 3, 2025

nucleogenesis commented Jan 15, 2025

Ammog-Warrier commented Jan 15, 2025 • edited Loading

akolson commented Jan 15, 2025

Ammog-Warrier commented Jan 18, 2025

Abhishek-Punhani commented Jan 18, 2025

MisRob commented Jan 20, 2025

MisRob commented Jan 20, 2025

Abhishek-Punhani commented Jan 20, 2025 • edited Loading

MisRob commented Jan 21, 2025

MisRob commented Jan 21, 2025

Abhishek-Punhani commented Jan 21, 2025

MisRob commented Feb 12, 2025

Abhishek-Punhani commented Feb 12, 2025

MisRob commented Feb 12, 2025

Abhishek-Punhani commented Feb 13, 2025

Abhishek-Punhani commented Feb 14, 2025

MisRob commented Feb 17, 2025

EshaanAgg commented Feb 18, 2025

Abhishek-Punhani commented Feb 20, 2025 • edited Loading

EshaanAgg commented Feb 20, 2025 • edited Loading

Files with Object-Type Props Missing Validators

MisRob commented Feb 24, 2025

standardize validator functions to all `Object` props #8903

standardize validator functions to all `Object` props #8903

nikkuAg commented Mar 1, 2024 •

edited

Loading

Ammog-Warrier commented Jan 15, 2025 •

edited

Loading

Abhishek-Punhani commented Jan 20, 2025 •

edited

Loading

Abhishek-Punhani commented Feb 20, 2025 •

edited

Loading

EshaanAgg commented Feb 20, 2025 •

edited

Loading