oppia / oppia

A free, online learning platform to make quality education accessible for all.
https://www.oppia.org
Apache License 2.0
5.85k stars 4.11k forks source link

Write frontend validators for all interactions. #3446

Open seanlip opened 7 years ago

seanlip commented 7 years ago

In order to prevent entry of bad input, we have a frontend validator for the customization args of every interaction (this was broken out in #3445; see extensions/interactions/{{INTERACTION_NAME}}/directives/XXX-validation.service.ts). However, some of these validators are not yet written. The aim of this issue is to write these validation functions, as well as tests for them in the corresponding *spec file.

To be assigned to a file or for any queries, comment on the thread and tag @kevintab95 .

Up for grabs:

Completed:

Note that the validator files for other interactions already exist, and can be used as examples for what's needed here.

PR's for reference: #3470 #4832 #4828 #3580

/cc @juansaba

Note: For a guide on how to access Oppia's webpages, see this.

seanlip commented 7 years ago

@zpuller -- while waiting for review on the docstrings PR, what do you think about taking up one of these (let me know which one you'd prefer)? It seems like a good opportunity to practice handling multiple git branches; just make sure to checkout develop and sync to upstream before checking out a fresh branch.

zpuller commented 7 years ago

I can try any of these, no preference really.

seanlip commented 7 years ago

OK! How about starting with NumericInput and TextInput? Those are pretty canonical, so it seems sensible to do them first.

zpuller commented 7 years ago

Sounds good.

zpuller commented 7 years ago

@seanlip @juansaba I'm having trouble understanding what exactly needs to be validated based on the existing validators. Can you clarify? Is there maybe a good exploration I can do that would demonstrate it?

seanlip commented 7 years ago

Hi @zpuller -- try creating an exploration in your local dev server. When you select an interaction (i.e. a question type), you'll notice that some of these interactions can be customized, e.g. you can specify placeholder text for the TextInput interaction. This form needs to be validated.

For example, in the MultipleChoiceInput validator, we check that all choices have something in them, and that there are no duplicates. You can find the actual list of customization args in the *.py file in the relevant folder in extensions/interactions.

Now, for NumericInput -- I just realized that that has no customization args, so there is actually nothing to do! So, just removing the TODO would suffice.

On the other hand, for TextInput, there are two such args: placeholder and rows. We want to make the checks as strict as possible (such that any set of inputs that pass the checks can be considered a valid input). So, in this case you'll want to check that 'rows' is a positive integer. If you're paranoid, you could also check that the placeholder is a string. It's also worth adding a baseInteractionValidationService.requireCustomizationArguments(...) check, like in the other validators, to make sure that the args are present in the first place.

Does this help? The validation for these interactions is simple, but some of the others might later end up being a bit more complicated.

zpuller commented 7 years ago

Yes, that is perfect. Thanks!

vibhor98 commented 6 years ago

Hi @seanlip , I want to take up one of the above listed interactions for writing validators. As the remaining ones appear a bit tricky to me, can you suggest which one should I take up first?

seanlip commented 6 years ago

Hi @vibhor98 -- if you're looking for easiness, I think EndExploration and MathExpressionInput are probably the best options. Thanks!

vibhor98 commented 6 years ago

Hi @seanlip , I've filed PR for End Exploration. MathExpressionInput does not contain any customization arguments so, I think there are no validators to be added there. I'll pick another one then. Thanks!

nipan09 commented 5 years ago

Hi @seanlip, I want to take up the issue for MusicNotesInput. Can you assign me the issue?

seanlip commented 5 years ago

Sure, I have assigned you. Go ahead!

nipan09 commented 5 years ago

Thank you @seanlip . I will keep updating my progress.

BenHenning commented 4 years ago

@MegrezZhu I suggest this as a frontend starter issue (maybe take SetInput?)

MegrezZhu commented 4 years ago

@kevintab95 Hi, I would like to work on SetInput. :)

kevintab95 commented 4 years ago

Sure, I've assigned you @MegrezZhu!

ghost commented 4 years ago

May i do something useful here? forking and cloning now... @kevintab95

kevintab95 commented 4 years ago

Hi @cernoit, welcome to Oppia! Before you can work on this issue, can you please confirm that you have followed the steps mentioned on our wiki page? If not, I would encourage you to do so. New contirbutors are required to fill up the CLA and complete the contributor survery (steps 1 & 2). Once that is done, you'll be assigned a mentor who will help you with your first contributions to Oppia. Thanks!

Bhawana15 commented 3 years ago

Hey, I am Bhawana. I am new to Oppia & open source community. I have signed the CLA & completed the contributor survey. I am interested in solving issue #3446. What should be my next step @kevintab95 ?

ChitvanRamani22 commented 3 years ago

Hi @seanlip I am interested in working on this issue. But i want to start with a easy one from which should i start. Please suggest!! And please tell how to fill the CLA form, where's the link to it.

Bhawana15 commented 3 years ago

Hi @seanlip I am interested in working on this issue. But i want to start with a easy one from which should i start. Please suggest!! And please tell how to fill the CLA form, where's the link to it.

Hi @ChitvanRamani22, I am Bhawana, also a new contributor. Here you can find the link to fill the CLA. After filling it you will get a mentor, who will guide you towards your first contribution. Also #10306 is an easy first issue you can solve. Thanks.

ChitvanRamani22 commented 3 years ago

Hello Bhavna I filled the CLA form but how will i get to know who is my mentor.

On Wed, 9 Dec 2020, 6:23 am Bhawana Tiwari, notifications@github.com wrote:

Hi @seanlip https://github.com/seanlip I am interested in working on this issue. But i want to start with a easy one from which should i start. Please suggest!! And please tell how to fill the CLA form, where's the link to it.

Hi @ChitvanRamani22 https://github.com/ChitvanRamani22, I am Bhawana, also a new contributor. Here https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#setting-things-up you can find the link to fill the CLA. After filling it you will get a mentor, who will guide you towards your first contribution. Also #10306 https://github.com/oppia/oppia/issues/10306 https://github.com/oppia/oppia/issues/10306 http://url is an easy first issue you can solve. Thanks.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/oppia/oppia/issues/3446#issuecomment-741347201, or unsubscribe https://github.com/notifications/unsubscribe-auth/AQOFI67NQCEKEVC3XWDNALDST3C63ANCNFSM4DMEZMYQ .

sachinchauhan2889 commented 3 years ago

@kevintab95 please assign "PencilCodeEditor" to me.

PranshuSrivastava commented 3 years ago

Hey @kevintab95, could you assign me the MusicNotesInput file?

oppiabot[bot] commented 3 years ago

Hi @oppia/core-maintainers, this issue is not assigned to any project. Can you please update the same? Thanks!

Rijuta-s commented 3 years ago

Hi @seanlip , I think PRs adding validation to both setInput and PencilCodeEditors are merged(#12033 and #9439). Only MusicNotesInput is left. Can you please confirm? If MusicNotesInput is available, @brian-kim1 is willing to work on it. Thank you.

seanlip commented 3 years ago

Yup, I confirm this. Thanks @Rijuta-s! And thanks for your interest in working on it, @brian-kim1 -- I've assigned it to you!

KarishmaVanwari commented 2 years ago

Hey @seanlip , I'd like to work on "MusicNotesInput", since the assignee does not seem to be active currently. Could you please assign it to me?

seanlip commented 2 years ago

Sure -- done!

KarishmaVanwari commented 2 years ago

@seanlip I am working on writing validators for "MusicNotesInput". Can you please help me figure out what type is assignable to ReadableMusicNote[] and ReadableMusicNote? I assumed it to be a 'string' and 'number' respectively, but it throws errors when I try to push the changes.

seanlip commented 2 years ago

Hi @KarishmaVanwari, you will need to read the code (and perhaps do console logs when running a local web server in order to see the structure of the items for yourself) -- don't assume.

Once you've done so, feel free to reply to this thread with your reasoning and conclusions, and if you're still running into issues, we can take a look at them.

gp201 commented 2 years ago

Unassigning @KarishmaVanwari since no PR was opened.

lucyduan commented 2 years ago

Hi @seanlip , interested in picking this up -- was the validator for musicnotesinput ever completed?

ayushsuryan commented 2 years ago

Hi @seanlip , wanted to work on: validator for musicnotesinput.

seanlip commented 2 years ago

I don't think it was -- assigning to @lucyduan since she asked first :)

@ayushsuryan, I recommend following these instructions to get started!

lucyduan commented 1 year ago

Hi @seanlip, based on previous comments, validators, and the music notes input code, I've gathered that I should validate the custom arg types for music notes input -- however, for music notes we have a custom type. I see that there is a MusicPhrase obj_type, I'm assuming this defines the components of the object type that should be validated, but I'm not quite sure where this is defined. Any help would be appreciated!

seanlip commented 1 year ago

Did you try searching for MusicPhrase in the codebase? I think that should pick up the occurrence in extensions/objects/models/objects.py where the different custom objects are defined.

SanjaySajuJacob commented 1 year ago

Hi @lucyduan , are you currently working on this issue?

seanlip commented 1 year ago

@SanjaySajuJacob On looking at the PRs made for this, I don't think so. Deassigning.

SanjaySajuJacob commented 1 year ago

Oh I see, thanks!

Gurinderp commented 1 year ago

Hello. I would like to try taking this issue on, but I am having an issue seeing the validator.js file in the ~/extensions/interactions folder. I was hoping someone could clarify this for me since the only .js file I see is webdriverio.js. Thanks.

seanlip commented 1 year ago

@Gurinderp Sorry about that, the link in the original issue was obsolete and referred to an older version of the codebase. I've updated it, please take a look!

Gurinderp commented 1 year ago

Thank you for the update. I have looked at the code and would like to try taking on this issue. Does the validation only need to encompass the readableNoteName choices 'C4', 'D4', 'E4', 'F4', 'G4', 'A4', 'B4', 'C5', 'D5', 'E5', 'F5', 'G5', 'A5' within the MusicPhrase(BaseObject) class?

seanlip commented 1 year ago

@Gurinderp You'll need to figure out what validation is needed for the interaction customization args so that everything is consistent. Try playing around with it and provide a list here of what validation checks you will add, and if those look good we'll assign this to you.

(You can also look at previous PRs done for this issue to get a sense of what those files should end up looking like.)

Thanks!

neilhanda commented 1 year ago

@seanlip Hello Sir, I am starting my contributor journey and came across this good first issue. I would like to try to work on it, could you please give some more context about what exactly is "interaction customisation args" so that I can come up with a list of validation checks for the same.

Is it related to something on Oppia website that I can try ? I would like to get understanding of the issue before working on it and finally getting myself assigned to it.

seanlip commented 1 year ago

@neilhanda Interaction customization args are ways that you can customize a question type, e.g. by specifying the multiple-choice options or the placeholder text. You can play with this functionality by setting up a local dev server, going to the /creator-dashboard, and creating a new exploration. Then, you can add a new music notes interaction to it and poke around at it.

It might also be worth looking at the previous successfully-merged PRs for this issue to get an idea of where the code is and what to do. I hope this helps!

neilhanda commented 1 year ago

Ok sir thanks, it was helpful. I will look into it.

yash1378 commented 11 months ago

@anthkris sir is this issue still open ?

ShatilKhan commented 11 months ago

I see MusicNotesInput is up for grabs @kevintab95 please assign me to it

seanlip commented 11 months ago

@ShatilKhan Per the guidance at https://github.com/oppia/oppia/wiki/Contributing-code-to-Oppia#choosing-a-good-first-issue, please provide an explanation of what your PR will do (with names of files you're changing, what you plan to change in each file, etc.). If it looks good, we can assign you to this issue.

Please also follow the other instructions on that wiki page if you have not yet done so. Thanks!