feat: add odp integration w client and user context #408
Conversation
README.md
Outdated
| @@ -24,6 +24,10 @@ documentation](https://docs.developers.optimizely.com/rollouts/docs). | |||
|
|
|||
| ## Getting Started | |||
|
|
|||
| ### Requirements | |||
|
|
|||
| Python 3.7+, PyPy 3.7+ | |||
There was a problem hiding this comment.
4.0+: python 3.7
3.0+: python 2?
There was a problem hiding this comment.
@andrewleap-optimizely yeah, since this note is in README (applicable to all SDK versions ), maybe we add we support Py 2.7 for SDK versions < 4.0
optimizely/optimizely.py
Outdated
| if not self.odp_manager.enabled: | ||
| self.logger.error(enums.Errors.ODP_NOT_ENABLED) | ||
| return | ||
|
|
||
| if self.odp_manager.odp_config.odp_state() == OdpConfigState.NOT_INTEGRATED: | ||
| self.logger.error(enums.Errors.ODP_NOT_INTEGRATED) | ||
| return | ||
|
|
||
| if not are_odp_data_types_valid(data): | ||
| self.logger.error(enums.Errors.ODP_INVALID_DATA) | ||
| return |
There was a problem hiding this comment.
These error handling are all ODP specific. It looks like these can be wrapped in there.
There was a problem hiding this comment.
I'm not sure I follow, are you suggesting we move these into odp_manager.send_event?
optimizely/optimizely.py
Outdated
| def fetch_qualified_segments(self, user_id: str, options: Optional[list[str]] = None) -> Optional[list[str]]: | ||
| return self.odp_manager.fetch_qualified_segments(user_id, options or []) | ||
|
|
||
| def send_odp_event(self, type: str, action: str, identifiers: dict[str, str], data: dict[str, Any]) -> None: |
There was a problem hiding this comment.
Can we support default type (type = "fullstack")?
tests/test_odp_config.py
Outdated
| @@ -3,7 +3,7 @@ | |||
| # you may not use this file except in compliance with the License. | |||
| # You may obtain a copy of the License at | |||
| # | |||
| # http:#www.apache.org/licenses/LICENSE-2.0 | |||
| # http://www.apache.org/licenses/LICENSE-2.0 | |||
There was a problem hiding this comment.
| # http://www.apache.org/licenses/LICENSE-2.0 | |
| # https://www.apache.org/licenses/LICENSE-2.0 |
There was a problem hiding this comment.
@andrewleap-optimizely I can't approve the PR i created lol.
Looks good to me, nothing major stood out, I think well covered (we also went through it a few times).
(Jae may have ats specific requests.)
optimizely-codereview-bot
added
🤖 4 hrs
🤖 8 hrs
🤖 12 hrs
🤖 16 hrs
and removed
🤖 4 hrs
🤖 8 hrs
🤖 12 hrs
labels
andrewleap-optimizely
deleted the
mpirnovar/odp_integration_client_and_user_context
branch
Summary
In optimizely user context:
Optimizely client provides the following new public API: send_odp_event()
Optimizely client provides configurability of ODP services:
Test plan
Unit tests in :
Issues