Skip to content
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

Validation fix #105

Closed
wants to merge 8 commits into from
Closed

Conversation

IanEisenberg
Copy link
Member

No description provided.

…ndividually and checks whether they have a check if required and not text, or have text if required and are a text input
@vsoch
Copy link
Member

vsoch commented Apr 5, 2016

could you please put the code on multiple lines so it is readable?

@vsoch
Copy link
Member

vsoch commented Apr 5, 2016

sorry I should be more specific - can you just put the final output (here is fine, if you indent 6 lines it will "codeify") so I can look at the final rendered validation code.

@IanEisenberg
Copy link
Member Author

Like this? I agree, this is difficult to read.

validation = "%s if (($.unique($('.page%s.required:text').map(function(){return this.name})).map(function() {return $('[name*=' + this + '].required:text').filter(function() { return $(this).val();}).length > 0}).get().indexOf(false) != -1) || ($.unique($('.page%s.required:not(:text)').map(function(){return this.name})).map(function() {return $('[name*=' + this + '].required:checked').length > 0}).get().indexOf(false) != -1));\nreturn false;\n" % (validation, page_number, page_number)

@vsoch vsoch mentioned this pull request Apr 5, 2016
@vsoch
Copy link
Member

vsoch commented Apr 5, 2016

This PR does not work:

image

In the future please test code fully before you ask someone else to review it.

@IanEisenberg
Copy link
Member Author

Sorry! There were two bugs - missing an ending "{" and a larger bug due to the change from text to number. Both should be fixed:

"%s if (($.unique($('.page%s.required[type=number],.page3.required:text').map(function(){return this.name})).map(function() {return $('[name*=' + this + '].required[type=number], [name*=' + this + '].required:text').filter(function() { return $(this).val();}).length > 0}).get().indexOf(false) != -1) || ($.unique($('.page%s.required:not([type=number]):not(:text)').map(function(){return this.name})).map(function() {return $('[name*=' + this + '].required:checked').length > 0}).get().indexOf(false) != -1)){\nreturn false;\n" % (validation, page_number, page_number)

Side note - I'm not actually sure how to debug effectively. I did this by creating the validator in the console with a survey open, moving it to python, adding the appropriate string completions, and testing that the string completion gave me back the working selector. Is there a way to edit expfactory-python such that I generate a webpage using the selector?

@vsoch
Copy link
Member

vsoch commented Apr 5, 2016

As indicated by the circleCI test failing, this change still does not work:

image

Debugging means doing whatever you must do in order to test the functionality. In this case, it's pretty easy to install expfactory:

  sudo python setup.py install

and then try previewing a survey. Please test this properly and then update the PR.

@IanEisenberg
Copy link
Member Author

Ah, of course. I'll try that.

On Tuesday, April 5, 2016, Vanessa Sochat [email protected] wrote:

As indicated by the circleCI test failing, this change still does not work:

[image: image]
https://cloud.githubusercontent.com/assets/814322/14300212/ca4b4f7e-fb43-11e5-9826-518e070bc5c2.png

Debugging means doing whatever you must do in order to test the
functionality. In this case, it's pretty easy to install expfactory:

sudo python setup.py install

and then try previewing a survey. Please test this properly and then
update the PR.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#105 (comment)

Best,
Ian

@vsoch
Copy link
Member

vsoch commented Apr 5, 2016

Great! Thanks.

…ue where each checkbox is recorded separately. Not a big deal, but annoying.
@vsoch vsoch mentioned this pull request Apr 6, 2016
@vsoch vsoch closed this in #108 Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants