Revert "Revert "Better AuthenticationOption duplicate detection"" by Hamms · Pull Request #36648 · code-dot-org/code-dot-org

Hamms · Sep 8, 2020

Reverts #36647, restoring #36555

Hamms · Sep 8, 2020

  factory :user do
    birthday Time.zone.today - 21.years
-    email {("#{user_type}_#{(User.maximum(:id) || 0) + 1}@code.org")}
+    email {("#{user_type}_#{SecureRandom.uuid}@code.org")}


My first attempt at this PR used a sequence here to attempt to reduce the chance of collision.

I suspect that this is what caused the Validation failed: Email has already been taken error we saw; I was able to repro locally, and verified that that using a UUID rather than a sequential ID here allowed the test to pass on my local machine.

it might help to know what email address was already taken when these tests failed.

teacher_5@code.org, at least when I run just the relevant subset of tests via bundle exec runner.rb -l -f features/plc/pd/* --eyes --html

whoa, any idea how a UI test is depending on a factory method? I thought we only used them for unit tests.

Looks like we use FactoryGirl pretty extensively in the PD UI tests:

code-dot-org/dashboard/test/ui/features/step_definitions/pd.rb

Line 36 in 51706f3

FactoryGirl.create(:program_manager, name: pm_name, email: email, password: password, regional_partner: regional_partner)

code-dot-org/dashboard/test/ui/features/step_definitions/pd.rb

Lines 48 to 50 in 51706f3

FactoryGirl.create(:pd_course_facilitator, course: course, facilitator:

FactoryGirl.create(:facilitator, name: name, email: email, password: password)

)

code-dot-org/dashboard/test/ui/features/step_definitions/pd.rb

Lines 104 to 109 in 51706f3

enrollment = FactoryGirl.create(

:pd_enrollment,

:with_attendance,

:from_user,

user: find_test_user_by_name(test_teacher_name)

)

(and lots more)

mmmm interesting! the first 2 calls look ok because they are using their own mechanism to generate a unique email, so they wouldn't touch the uuid logic you added. However, this one call does not specify an email and would therefore hit your uuid logic:

code-dot-org/dashboard/test/ui/features/step_definitions/pd.rb

Line 421 in 5756d75

FactoryGirl.create :teacher

Can you double-check my reasoning here, and say whether you think it might be worth changing the user factory email back to using sequence and making the line linked above specify an email? I would be willing to open the PR for this.

I'm not seeing the value in that solution; in particular, it seems to me that you shouldn't need to specify an email in order for the factory to work.

Good point. This solution sounds good, now that we know why sequence wasn't working, because it lets UI tests rely on factories for generating unique emails. Thanks for your willingness to investigate!

Thank you for your thoroughness!

Erin007

LGTM! Thanks for the revert and quick follow-up, Elijah!

davidsbailey · Sep 9, 2020

Seems fine to unblock, but any ideas why sequence didn't work?

Hamms · Sep 9, 2020

any ideas why sequence didn't work?

Not so far; I could definitely spend more time digging in, though. In particular, it's not clear to me what exactly it is that sequence actually uses to generate the sequence, particularly in a context in which we are running tests in parallel.

davidsbailey · Sep 9, 2020


  test "age is set exactly for Google OAuth users between ages 4 and 20" do
-    four_year_old = create :user, birthday: (Date.today - 4.years), provider: 'google_oauth2'
+    four_year_old = build :user, birthday: (Date.today - 4.years), provider: 'google_oauth2'


is it a problem that there is no uid here? apologies for my lack of deep understanding here. I am just asking because I see that it is specified by this trait:

code-dot-org/dashboard/test/factories/factories.rb

Lines 319 to 323 in 71a185b

trait :sso_provider do

encrypted_password nil

provider %w(facebook windowslive clever).sample

sequence(:uid) {|n| n}

end

My understanding is that user.uid is the pre-authentication-options way of identifying oauth, and that that information has been migrated to authentication_option.authentication_id

davidsbailey · Sep 9, 2020

@@ -20,13 +20,17 @@ class UserMultiAuthHelperTest < ActiveSupport::TestCase
  test 'oauth_tokens_for_provider returns most recently updated tokens for migrated teacher' do


this class does not appear to be using use_transactional_test_case. any reason why not?

I'm not particularly familiar with this test. Why do you think use_transactional_test_case would be relevant?

Just because we are running into problems with duplicate emails, and wrapping the test in a transaction would cause it not to persist and changes to the DB, which could decrease the changes of conflicting emails. It sounds from your earlier comment that this test isn't failing and isn't even using the same DB as the test that is failing though, because this is a unit test and it is a UI test that is failing. so with that information I don't think this could be causing a UI test to fail.

Ah, that makes sense. Yeah, this test was touched because of the original PR's changes to the way we handle duplicate authentication option detection, not because of the email issue

davidsbailey · Sep 9, 2020

any ideas why sequence didn't work?

Not so far; I could definitely spend more time digging in, though. In particular, it's not clear to me what exactly it is that sequence actually uses to generate the sequence, particularly in a context in which we are running tests in parallel.

I added a few follow-up questions/comments inline related to helping track this down. If none of those or anything you want to try are successful in turning anything up, then I think it could be fine to move on and use uuids.

Hamms added 2 commits September 8, 2020 14:08

Revert "Revert "Better AuthenticationOption duplicate detection""

96bb1e9

use a UUID rather than a sequential ID, to better avoid collision

e9d1173

Hamms commented Sep 8, 2020

View reviewed changes

Hamms marked this pull request as ready for review September 8, 2020 23:19

Hamms requested review from Erin007, davidsbailey and maddiedierker September 8, 2020 23:21

Erin007 approved these changes Sep 9, 2020

View reviewed changes

maddiedierker approved these changes Sep 9, 2020

View reviewed changes

Hamms merged commit aba4f2c into staging Sep 9, 2020

Hamms deleted the revert-36647-revert-36555-better-auth-option-deduplication branch September 9, 2020 17:27

davidsbailey reviewed Sep 9, 2020

View reviewed changes

	FactoryGirl.create(:pd_course_facilitator, course: course, facilitator:
	FactoryGirl.create(:facilitator, name: name, email: email, password: password)
	)

	enrollment = FactoryGirl.create(
	:pd_enrollment,
	:with_attendance,
	:from_user,
	user: find_test_user_by_name(test_teacher_name)
	)

	trait :sso_provider do
	encrypted_password nil
	provider %w(facebook windowslive clever).sample
	sequence(:uid) {\|n\| n}
	end

		@@ -20,13 +20,17 @@ class UserMultiAuthHelperTest < ActiveSupport::TestCase
		test 'oauth_tokens_for_provider returns most recently updated tokens for migrated teacher' do

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

Hamms commented Sep 8, 2020

Uh oh!

Hamms Sep 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Erin007 left a comment

Choose a reason for hiding this comment

Uh oh!

davidsbailey commented Sep 9, 2020

Uh oh!

Hamms commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidsbailey commented Sep 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hamms Sep 8, 2020 •

edited

Loading