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

OAuth: Fix age validation edge case #18912

Merged
merged 2 commits into from
Nov 3, 2017
Merged

Conversation

islemaster
Copy link
Contributor

I believe this will fix this Honeybadger error.

ActiveRecord::RecordInvalid in api # import_google_classroom
Validation failed: Student user age is not included in the list

This happens for both Google Classroom and Clever.

We have two validations for user age: One to see if it's present, another to see if it's in the list of accepted age values.

defer_age = proc {|user| user.provider == 'google_oauth2' || user.provider == 'clever'}
validates :age, presence: true, on: :create, unless: defer_age # only do this on create to avoid problems with existing users
AGE_DROPDOWN_OPTIONS = (4..20).to_a << "21+"
validates :age, presence: false, inclusion: {in: AGE_DROPDOWN_OPTIONS}, allow_blank: true

In #15882 @joshlory correctly set us up to bypass the first age validation for Google Classroom since a birthdate is not guaranteed to be provided via OAuth. In #16632 he did the same for Clever. The second validation still runs, but when an age isn't provided it passes as well.

However, there is an edge case we didn't account for: If a birthdate is provided, but the user is less than four years old, we skip the first validation and fail the second. I verified this with a new set of user model tests in this PR:

screenshot from 2017-11-03 15-22-59

I think a reasonable behavior in this case is to act as if a birthdate wasn't provided at all, so the user will be prompted to provide an age when they log in.

Add a detailed set of age initialization tests for Clever and Google Classroom import, that codify our expectations of age initialization from provided birthdate data and catch the bad edge case of a birthdate indicating the user is under the age of 4.
Copy link
Contributor

@joshlory joshlory left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing!

@islemaster islemaster merged commit 972c90d into staging Nov 3, 2017
@islemaster islemaster deleted the clever-age-edge-case branch November 3, 2017 23:57
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

2 participants