Skip to content
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

feat: Add ODP GraphQL Manager and Tests #310

Merged
merged 35 commits into from
Aug 24, 2022
Merged

Conversation

mikechu-optimizely
Copy link
Contributor

Summary

  • Added an GraphQLManager interface and default implementation for querying qualified segments
  • GraphQLManager is also responsible for parsing response from ODP.

Test plan

  • Added specific GraphQL request and response test class

Issues

  • OASIS-8406

Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Some suggestions for error handling.
I see some C# stuffs I cannot review. Let Sohail double check.

OptimizelySDK/Odp/GraphQLManager.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/GraphQLManager.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/GraphQLManager.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Some more suggestions regarding error handling.

OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/Client/OdpClient.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/Client/OdpClient.cs Outdated Show resolved Hide resolved
mikechu-optimizely and others added 5 commits August 8, 2022 13:55
Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>
Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>
Co-authored-by: Jae Kim <45045038+jaeopt@users.noreply.github.com>
Copy link
Contributor

@jaeopt jaeopt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mikechu-optimizely
Copy link
Contributor Author

LGTM

@jaeopt

Should I wait until @msohailhussain reviews?

@jaeopt
Copy link
Contributor

jaeopt commented Aug 8, 2022

LGTM

@jaeopt

Should I wait until @msohailhussain reviews?

@mikechu-optimizely Yes, let's wait for his review.

@mikechu-optimizely
Copy link
Contributor Author

Hopefully, I've not ruined your codebase with this PR @msohailhussain 😁

@mikechu-optimizely
Copy link
Contributor Author

@msohailhussain when you have a chance, this PR also needs your 👍 Thanks!

Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please address.

OptimizelySDK/Odp/Client/IOdpClient.cs Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK.Tests/OdpTests/GraphQLManagerTest.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/GraphQLManager.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/GraphQLManager.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/GraphQLManager.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/GraphQLManager.cs Outdated Show resolved Hide resolved
OptimizelySDK/Odp/IGraphQLManager.cs Show resolved Hide resolved
Copy link
Contributor

@msohailhussain msohailhussain left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

before merge, please discuss GetAwaiter with me.

@mikechu-optimizely mikechu-optimizely merged commit 4e04c3a into master Aug 24, 2022
@mikechu-optimizely mikechu-optimizely deleted the mike/ats-graphql branch August 24, 2022 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants