From 2e5f2f2be3b0fa9fe1fb9455849dd0c97ae21251 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Mon, 16 Mar 2020 16:09:41 -0400 Subject: [PATCH] Fix error in experiment tracking If the user is not logged in, the ExperimentWaffleFlag code was raising an exception when trying to send an event to segment. This is a quick fix to ignore anonymous users. --- lms/djangoapps/courseware/tests/test_views.py | 5 ----- lms/djangoapps/experiments/flags.py | 13 +++++++++---- lms/djangoapps/experiments/tests/test_flags.py | 7 +++++-- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index 898c91aac9..1262030044 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -3174,7 +3174,6 @@ class DatesTabTestCase(ModuleStoreTestCase): def setUp(self): super(DatesTabTestCase, self).setUp() - self.user = UserFactory.create() now = datetime.now(utc) self.course = CourseFactory.create(start=now + timedelta(days=-1)) @@ -3197,10 +3196,6 @@ class DatesTabTestCase(ModuleStoreTestCase): @RELATIVE_DATES_FLAG.override(active=True) def test_defaults(self): - request = RequestFactory().request() - request.user = self.user - self.addCleanup(crum.set_current_request, None) - crum.set_current_request(request) enrollment = CourseEnrollmentFactory(course_id=self.course.id, user=self.user, mode=CourseMode.VERIFIED) now = datetime.now(utc) with self.store.bulk_operations(self.course.id): diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index efb04f550c..3d0afa8e7d 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -70,6 +70,10 @@ class ExperimentWaffleFlag(CourseWaffleFlag): if not request: return 0 + if not request.user.id: + # We need username for stable bucketing and id for tracking, so just skip anonymous (not-logged-in) users + return 0 + # Use course key in experiment name to separate caches and segment calls per-course-run experiment_name = self.namespaced_flag_name + ('.{}'.format(course_key) if course_key else '') @@ -136,7 +140,8 @@ class ExperimentWaffleFlag(CourseWaffleFlag): @contextmanager def override(self, active=True, bucket=1): # pylint: disable=arguments-differ - from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag - with override_waffle_flag(self, active): - with override_waffle_flag(self.bucket_flags[bucket], True): - yield + from mock import patch + if not active: + bucket = 0 + with patch.object(self, 'get_bucket', return_value=bucket): + yield diff --git a/lms/djangoapps/experiments/tests/test_flags.py b/lms/djangoapps/experiments/tests/test_flags.py index f12ca2eae3..a30598d81b 100644 --- a/lms/djangoapps/experiments/tests/test_flags.py +++ b/lms/djangoapps/experiments/tests/test_flags.py @@ -16,6 +16,7 @@ from experiments.factories import ExperimentKeyValueFactory from experiments.flags import ExperimentWaffleFlag from openedx.core.djangoapps.site_configuration.tests.factories import SiteFactory from openedx.core.djangoapps.waffle_utils import CourseWaffleFlag +from openedx.core.djangoapps.waffle_utils.testutils import override_waffle_flag from student.tests.factories import CourseEnrollmentFactory, UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase @@ -45,8 +46,10 @@ class ExperimentWaffleFlagTests(SharedModuleStoreTestCase): self.addCleanup(RequestCache.clear_all_namespaces) def get_bucket(self, track=False, active=True): - with self.flag.override(active=active): - return self.flag.get_bucket(course_key=self.key, track=track) + # Does not use ExperimentWaffleFlag.override, since that shortcuts get_bucket and we want to test internals + with override_waffle_flag(self.flag, active): + with override_waffle_flag(self.flag.bucket_flags[1], True): + return self.flag.get_bucket(course_key=self.key, track=track) def test_basic_happy_path(self): self.assertEqual(self.get_bucket(), 1)