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

Bugfix: enable workshop registrations with manual districts #13848

Merged
merged 2 commits into from Mar 17, 2017

Conversation

ewjordan
Copy link
Contributor

@ewjordan ewjordan commented Mar 17, 2017

Currently, when the form at https://staging-studio.code.org/pd/workshops/1/enroll is submitted with the "Other school district not found above" checkbox checked, server-side validation fails and the user cannot submit the form. This is due to the fact that though the box is checked, the "value" of the item is "false" (checkboxes are quirky).

screen shot 2017-03-17 at 3 20 39 am

This PR syncs up the value with the 'checked' property every time the box is changed so that validation will pass.

Note: this should resolve Zendesk tickets https://codeorg.zendesk.com/agent/tickets/89481 and https://codeorg.zendesk.com/agent/tickets/88510

@ewjordan ewjordan requested review from breville and aoby March 17, 2017 10:26
@@ -293,6 +293,7 @@ window.SchoolInfoManager = function (existingOptions) {
});

$('#school-district-other').change(function () {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that I did not make an analogous change below, at line 313 in the $('#school-other') section - that field seems to work properly as-is for some reason, so I didn't want to mess with it. I do wonder if there might be other forms where we make a similar mistake, though.

@breville
Copy link
Member

Not sure what's happening here, but does the value need to be false at all? Secondly, seems concerning that the issue isn't occurring in a similar situation just below.. is it worth figuring this out?

@aoby
Copy link
Contributor

aoby commented Mar 17, 2017

I agree w @breville. Before merging this, please verify that it isn't needed for the other control. Something seems fishy here. Is that other value not used on the server?

@ewjordan
Copy link
Contributor Author

ewjordan commented Mar 17, 2017

OK, so re: the difference between 'school-district-other' and 'school-other' checkboxes...good call on digging a little deeper.

These both are apparently only sent along with the form if they are checked, they're just left out otherwise, and are filled in with default FALSE at the Ruby level - I assume this is just how HTML forms work?

The difference between the two is actually here,

$("#school-district-other").val(false);
(different branch, but you get the point), looks like we accidentally set the value on that instead of the checked state.

I can fix this by changing that to prop('checked', false) instead of val(false), which matches what we do at

$("#school-other").prop('checked', false);
much better, and clears up the asymmetry. Will update.

@ewjordan ewjordan merged commit c90a4c7 into staging Mar 17, 2017
@ewjordan ewjordan deleted the bugfix-other-district-registration branch March 17, 2017 23:21
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.

None yet

3 participants