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

feature/7158-Binny-InAppReviewFeatureFlagForDetox #9342

Merged
merged 7 commits into from
Aug 15, 2024

Conversation

dumathane
Copy link
Contributor

Description of Change

Screenshots/Video

Testing

  • Tested on iOS
  • Tested on Android

Reviewer Validations

PR Checklist

Reviewer: Confirm the items below as you review

  • PR is connected to issue(s)
  • Tests are included to cover this change (when possible)
  • No magic strings (All string unions follow the Union -> Constant type pattern)
  • No secrets or API keys are checked in
  • All imports are absolute (no relative imports)
  • New functions and Redux work have proper TSDoc annotations

For QA

Run a build for this branch

alexandec
alexandec previously approved these changes Aug 13, 2024
Copy link
Contributor

@alexandec alexandec left a comment

Choose a reason for hiding this comment

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

Approving with a couple optional suggestions. Thanks for implementing this!

const lastReviewVersion = await AsyncStorage.getItem(STORAGE_LAST_REVIEW_VERSION)

if (days > IN_APP_REVIEW_INTERVAL_DAYS && lastReviewVersion !== versionName) {
if (featureEnabled('inAppReview')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of indenting all this under the conditional, consider doing

if (!featureEnabled('inAppReview')) return

at the top

@@ -402,6 +402,7 @@ export async function verifyNonUSorMilitaryAddresses(addressID: string, addressT
})
}
beforeAll(async () => {
await toggleRemoteConfigFlag(CommonE2eIdConstants.IN_APP_REVIEW_TOGGLE_TEXT)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we toggle this globally for all E2E tests instead of toggling for individual sets of tests? And then remove all the calls to resetInAppReview in all E2E tests? It would really speed up the tests I think

@github-actions github-actions bot added FE-With QA A PR waiting for QA Activity and removed FE-Needs Review labels Aug 13, 2024
TKDickson
TKDickson previously approved these changes Aug 14, 2024
Copy link
Contributor

@TKDickson TKDickson left a comment

Choose a reason for hiding this comment

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

approved to merge

@github-actions github-actions bot added FE-Ready to Merge and removed FE-With QA A PR waiting for QA Activity labels Aug 14, 2024
@TKDickson TKDickson added FE-With QA A PR waiting for QA Activity FE-Needs Review and removed FE-Ready to Merge FE-With QA A PR waiting for QA Activity labels Aug 14, 2024
@github-actions github-actions bot added FE-With QA A PR waiting for QA Activity and removed FE-Needs Review labels Aug 14, 2024
Copy link
Contributor

@TKDickson TKDickson left a comment

Choose a reason for hiding this comment

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

approved to merge to develop

@github-actions github-actions bot added FE-Ready to Merge and removed FE-With QA A PR waiting for QA Activity labels Aug 15, 2024
@dumathane dumathane merged commit c9b4a5e into develop Aug 15, 2024
24 of 25 checks passed
@dumathane dumathane deleted the feature/7158-Binny-InAppReviewFeatureFlagForDetox branch August 15, 2024 16:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants