-
Notifications
You must be signed in to change notification settings - Fork 4
[FSSDK-9029]: Adds Top-level ODP Interface. #48
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
Conversation
2. Adding new unit tests.
jaeopt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! A couple of suggestions.
| /// NOTE: A user context will only be created successfully when the SDK is fully configured using initializeClient. | ||
| /// | ||
| /// Takes [userId] the [String] user ID to be used for bucketing. | ||
| /// Optional [userId] the [String] user ID to be used for bucketing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can add comment about "the device vuid will be used as an userId when userId is not provided."
| /// On failure, **qualifiedSegments** will be nil and an error will be returned. | ||
| /// Optional [options] A set of [OptimizelySegmentOption] for fetching qualified segments. | ||
| /// Returns [FetchQualifiedSegmentsResponse] On success, it returns an array of segment names that the user is qualified for. On failure, ir returns the reason of failure. | ||
| Future<FetchQualifiedSegmentsResponse> fetchQualifiedSegments( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about BaseResponse instead of FetchQualifiedSegmentsResponse? Or boolean in FetchQualifiedSegmentsResponse`, either one that make more sense?
It'll be consistent with other SDKs, where we return success/failure boolean instead of segments. iOS and android cores will return boolean as well.
| /// The segments fetched will be saved in **qualifiedSegments** and can be accessed any time using **getQualifiedSegments**. | ||
| /// On failure, **qualifiedSegments** will be nil and an error will be returned. | ||
| /// Optional [options] A set of [OptimizelySegmentOption] for fetching qualified segments. | ||
| /// Returns [FetchQualifiedSegmentsResponse] On success, it returns an array of segment names that the user is qualified for. On failure, ir returns the reason of failure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We return true, instead of "an array of segment names".
| import 'package:optimizely_flutter_sdk/src/utils/constants.dart'; | ||
|
|
||
| class FetchQualifiedSegmentsResponse extends BaseResponse { | ||
| List<String> qualifiedSegments = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We may not need this list. Also ios/android cores won't return qualifedSegments.
jaeopt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This reverts commit 59e998d.
This reverts commit 59e998d.
Summary
Test plan
Issues