diff --git a/.flake8 b/.flake8 deleted file mode 100644 index 0fc0cad..0000000 --- a/.flake8 +++ /dev/null @@ -1,8 +0,0 @@ -[flake8] -# E722 - do not use bare 'except' -# W504 - Either W503 (Line break after Operand) or W503 ( -# Line break before operand needs to be ignored for line lengths -# greater than max-line-length. Best practice shows W504 -ignore = E722, W504 -exclude = optimizely/lib/pymmh3.py,*virtualenv*,tests/testapp/application.py -max-line-length = 120 diff --git a/.github/workflows/python.yml b/.github/workflows/python.yml index 24d95e6..20894bb 100644 --- a/.github/workflows/python.yml +++ b/.github/workflows/python.yml @@ -33,17 +33,12 @@ jobs: uses: actions/setup-python@v4 with: python-version: '3.12' - # flake8 version should be same as the version in requirements/test.txt - # to avoid lint errors on CI - - name: pip install flak8 - run: pip install flake8>=4.1.0 - - name: Lint with flake8 + - name: Install dependencies for linting run: | - flake8 - # stop the build if there are Python syntax errors or undefined names - flake8 . --count --select=E9,F63,F7,F82 --show-source --statistics - # exit-zero treats all errors as warnings. The GitHub editor is 127 chars wide - flake8 . --count --exit-zero --max-complexity=10 --max-line-length=127 --statistics + python -m pip install --upgrade pip + pip install -r requirements/test.txt + - name: Lint with Ruff + run: ruff check . integration_tests: uses: optimizely/python-sdk/.github/workflows/integration_test.yml@master diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d14002e..9c14971 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,7 +15,7 @@ Development process 2. Please follow the [commit message guidelines](https://github.com/angular/angular/blob/master/CONTRIBUTING.md#-commit-message-guidelines) for each commit message. 3. Make sure to add tests! -4. Run `flake8` to ensure there are no lint errors. +4. Run `ruff check .` to ensure there are no lint errors. 5. `git push` your changes to GitHub. 6. Open a PR from your fork into the master branch of the original repo. @@ -34,12 +34,12 @@ Pull request acceptance criteria - Tests are located in `/tests` with one file per class. - Please don't change the `__version__`. We'll take care of bumping the version when we next release. -- Lint your code with Flake8 before submitting. +- Lint your code with Ruff before submitting. Style ----- -We enforce Flake8 rules. +We enforce Ruff linting rules (Flake8-equivalent: pycodestyle + Pyflakes). License ------- diff --git a/optimizely/decision_service.py b/optimizely/decision_service.py index 28275ef..d8a6ee3 100644 --- a/optimizely/decision_service.py +++ b/optimizely/decision_service.py @@ -457,7 +457,8 @@ def get_variation( } # Check to see if user has a decision available for the given experiment - if user_profile_tracker is not None and not ignore_user_profile: + # CMAB experiments are excluded from UPS to ensure decisions are always computed dynamically + if user_profile_tracker is not None and not ignore_user_profile and not experiment.cmab: variation = self.get_stored_variation(project_config, experiment, user_profile_tracker.get_user_profile()) if variation: message = f'Returning previously activated variation ID "{variation}" of experiment ' \ @@ -473,6 +474,11 @@ def get_variation( else: self.logger.warning('User profile has invalid format.') + if experiment.cmab and user_profile_tracker is not None and not ignore_user_profile: + message = f'Skipping UPS lookup and save for CMAB experiment "{experiment.key}".' + self.logger.info(message) + decide_reasons.append(message) + # Check audience conditions audience_conditions = experiment.get_audience_conditions_or_ids() user_meets_audience_conditions, reasons_received = audience_helper.does_user_meet_audience_conditions( @@ -529,7 +535,8 @@ def get_variation( self.logger.info(message) decide_reasons.append(message) # Store this new decision and return the variation for the user - if user_profile_tracker is not None and not ignore_user_profile: + # CMAB experiments are excluded from UPS to ensure decisions remain dynamic + if user_profile_tracker is not None and not ignore_user_profile and not experiment.cmab: try: user_profile_tracker.update_user_profile(experiment, variation) except: diff --git a/optimizely/helpers/validator.py b/optimizely/helpers/validator.py index b9e4fcc..5932833 100644 --- a/optimizely/helpers/validator.py +++ b/optimizely/helpers/validator.py @@ -193,7 +193,7 @@ def is_user_profile_valid(user_profile: dict[str, Any]) -> bool: if not user_profile: return False - if not type(user_profile) is dict: + if type(user_profile) is not dict: return False if UserProfile.USER_ID_KEY not in user_profile: @@ -203,7 +203,7 @@ def is_user_profile_valid(user_profile: dict[str, Any]) -> bool: return False experiment_bucket_map = user_profile.get(UserProfile.EXPERIMENT_BUCKET_MAP_KEY) - if not type(experiment_bucket_map) is dict: + if type(experiment_bucket_map) is not dict: return False for decision in experiment_bucket_map.values(): diff --git a/requirements/test.txt b/requirements/test.txt index c2e086c..c54f5bf 100644 --- a/requirements/test.txt +++ b/requirements/test.txt @@ -1,5 +1,5 @@ coverage -flake8 >= 4.0.1 +ruff >= 0.15.0 funcsigs >= 0.4 pytest >= 6.2.0 pytest-cov diff --git a/ruff.toml b/ruff.toml new file mode 100644 index 0000000..5365a7b --- /dev/null +++ b/ruff.toml @@ -0,0 +1,38 @@ +# Ruff configuration +# https://docs.astral.sh/ruff/configuration/ + +line-length = 120 +target-version = "py39" + +exclude = [ + ".git", + ".venv", + "__pycache__", + "build", + "dist", + "*.egg-info", + "*virtualenv*", + "optimizely/lib/pymmh3.py", + "tests/testapp/application.py", +] + +[lint] +# Flake8-equivalent rules: +# E, W = pycodestyle (style errors & warnings) +# F = Pyflakes (logic errors, undefined names, unused imports) +select = ["E", "W", "F"] + +# Match current flake8 ignores +# E722 - do not use bare 'except' +ignore = ["E722"] + +# Allow autofix for all rules +fixable = ["ALL"] +unfixable = [] + +[lint.per-file-ignores] +# Ignore unused imports in __init__ files +"__init__.py" = ["F401"] + +[lint.isort] +known-first-party = ["optimizely"] diff --git a/tests/test_decision_service.py b/tests/test_decision_service.py index dbcb743..ca4e989 100644 --- a/tests/test_decision_service.py +++ b/tests/test_decision_service.py @@ -1074,6 +1074,185 @@ def test_get_variation_cmab_experiment_with_whitelisted_variation(self): mock_bucket.assert_not_called() mock_cmab_decision.assert_not_called() + def test_get_variation_cmab_experiment_skips_ups_lookup(self): + """Test that get_variation skips UPS lookup for CMAB experiments.""" + + user = optimizely_user_context.OptimizelyUserContext( + optimizely_client=None, + logger=None, + user_id="test_user", + user_attributes={} + ) + + # Create a CMAB experiment + cmab_experiment = entities.Experiment( + '111150', + 'cmab_experiment', + 'Running', + '111150', + [], + {}, + [ + entities.Variation('111151', 'variation_1'), + entities.Variation('111152', 'variation_2') + ], + [ + {'entityId': '111151', 'endOfRange': 5000}, + {'entityId': '111152', 'endOfRange': 10000} + ], + cmab={'trafficAllocation': 5000} + ) + + user_profile_service = user_profile.UserProfileService() + user_profile_tracker = user_profile.UserProfileTracker(user.user_id, user_profile_service) + + with mock.patch('optimizely.helpers.experiment.is_experiment_running', return_value=True), \ + mock.patch('optimizely.helpers.audience.does_user_meet_audience_conditions', return_value=[True, []]), \ + mock.patch.object(self.decision_service.bucketer, 'bucket_to_entity_id', + return_value=['$', []]), \ + mock.patch.object(self.decision_service, 'cmab_service') as mock_cmab_service, \ + mock.patch.object(self.project_config, 'get_variation_from_id', + return_value=entities.Variation('111151', 'variation_1')), \ + mock.patch( + 'optimizely.decision_service.DecisionService.get_stored_variation' + ) as mock_get_stored_variation, \ + mock.patch.object(self.decision_service, 'logger') as mock_logger: + + mock_cmab_service.get_decision.return_value = ( + { + 'variation_id': '111151', + 'cmab_uuid': 'test-cmab-uuid-123' + }, + [] + ) + + variation_result = self.decision_service.get_variation( + self.project_config, + cmab_experiment, + user, + user_profile_tracker + ) + variation = variation_result['variation'] + reasons = variation_result['reasons'] + + # Verify variation was returned from CMAB service + self.assertEqual(entities.Variation('111151', 'variation_1'), variation) + + # Verify UPS lookup was NOT called for CMAB experiment + mock_get_stored_variation.assert_not_called() + + # Verify UPS exclusion reason is logged + self.assertIn( + 'Skipping UPS lookup and save for CMAB experiment "cmab_experiment".', + reasons + ) + mock_logger.info.assert_any_call( + 'Skipping UPS lookup and save for CMAB experiment "cmab_experiment".' + ) + + def test_get_variation_cmab_experiment_skips_ups_save(self): + """Test that get_variation skips UPS save for CMAB experiments.""" + + user = optimizely_user_context.OptimizelyUserContext( + optimizely_client=None, + logger=None, + user_id="test_user", + user_attributes={} + ) + + # Create a CMAB experiment + cmab_experiment = entities.Experiment( + '111150', + 'cmab_experiment', + 'Running', + '111150', + [], + {}, + [ + entities.Variation('111151', 'variation_1'), + entities.Variation('111152', 'variation_2') + ], + [ + {'entityId': '111151', 'endOfRange': 5000}, + {'entityId': '111152', 'endOfRange': 10000} + ], + cmab={'trafficAllocation': 5000} + ) + + user_profile_service = user_profile.UserProfileService() + user_profile_tracker = user_profile.UserProfileTracker(user.user_id, user_profile_service) + + with mock.patch('optimizely.helpers.experiment.is_experiment_running', return_value=True), \ + mock.patch('optimizely.helpers.audience.does_user_meet_audience_conditions', return_value=[True, []]), \ + mock.patch.object(self.decision_service.bucketer, 'bucket_to_entity_id', + return_value=['$', []]), \ + mock.patch.object(self.decision_service, 'cmab_service') as mock_cmab_service, \ + mock.patch.object(self.project_config, 'get_variation_from_id', + return_value=entities.Variation('111151', 'variation_1')), \ + mock.patch.object(user_profile_tracker, 'update_user_profile') as mock_update_profile, \ + mock.patch.object(self.decision_service, 'logger'): + + mock_cmab_service.get_decision.return_value = ( + { + 'variation_id': '111151', + 'cmab_uuid': 'test-cmab-uuid-123' + }, + [] + ) + + variation_result = self.decision_service.get_variation( + self.project_config, + cmab_experiment, + user, + user_profile_tracker + ) + variation = variation_result['variation'] + + # Verify variation was returned from CMAB service + self.assertEqual(entities.Variation('111151', 'variation_1'), variation) + + # Verify UPS save was NOT called for CMAB experiment + mock_update_profile.assert_not_called() + + def test_get_variation_non_cmab_experiment_uses_ups(self): + """Test that get_variation still uses UPS for non-CMAB experiments.""" + + user = optimizely_user_context.OptimizelyUserContext( + optimizely_client=None, + logger=None, + user_id="test_user", + user_attributes={} + ) + + experiment = self.project_config.get_experiment_from_key("test_experiment") + user_profile_service = user_profile.UserProfileService() + user_profile_tracker = user_profile.UserProfileTracker(user.user_id, user_profile_service) + + stored_variation = entities.Variation("111129", "variation") + + with mock.patch.object(self.decision_service, "logger"), \ + mock.patch( + "optimizely.decision_service.DecisionService.get_whitelisted_variation", + return_value=[None, []] + ), \ + mock.patch( + "optimizely.decision_service.DecisionService.get_stored_variation", + return_value=stored_variation + ) as mock_get_stored_variation: + + variation_result = self.decision_service.get_variation( + self.project_config, experiment, user, user_profile_tracker + ) + variation = variation_result['variation'] + + # Verify stored variation was returned + self.assertEqual(stored_variation, variation) + + # Verify UPS lookup WAS called for non-CMAB experiment + mock_get_stored_variation.assert_called_once_with( + self.project_config, experiment, user_profile_tracker.get_user_profile() + ) + class FeatureFlagDecisionTests(base.BaseTest): def setUp(self):