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

Fix #2565: Retry ref code lookup on subsequent launches. #2574

Merged
merged 24 commits into from May 27, 2020
Merged

Conversation

@jhreis
Copy link
Contributor

jhreis commented May 22, 2020

Summary of Changes

This is fully dependent on #2535, do not merge this before that completes.

This pull request fixes #2565

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

This impacts user referral promo logic, so should test all initial download ids and referral code retrieval paths. Download Id redemption has not touched, as this only impacts app opening logic, but it may be good to run a quick test or two on that as well.

User launches with clipboard referral and internet

  1. Visit a Super Referrer landing page
  2. Hit the download button
  3. This will redirect you to the App Store, close the App Store
  4. Go to notes app (or some other place with a text field)
  5. Paste the clipboard
  6. Should output F83AB73F-9852-4F01-ABA8-7830B8825993 and then the specific ref code (e.g. ABC123) from the referral
  7. So full out put would be F83AB73F-9852-4F01-ABA8-7830B8825993:ABC123 for example
  8. Download Brave through TF
  9. Open Brave
  10. Verify that correct ref code shows up in settings / on server
  11. Verify assets are shown on the home page
  12. Verify favorites show the referral’s custom favs

User launches with clipboard referral and no internet

  1. Visit a Super Referrer landing page
  2. Hit the download button
  3. This will redirect you to the App Store, close the App Store
  4. Go to notes app (or some other place with a text field)
  5. Paste the clipboard
  6. Should output F83AB73F-9852-4F01-ABA8-7830B8825993 and then the specific ref code (e.g. ABC123) from the referral
  7. So full output would be F83AB73F-9852-4F01-ABA8-7830B8825993:ABC123 for example
  8. Download Brave through TF
  9. Disable internet (this makes sure the fallback is not activated if clipboard fails for some reason)
  10. Open Brave
  11. Since no internet connection, no custom assets / download id, but ref code should still be shown inside the settings page
  12. Close app entirely (swipe up on it)
  13. Re-enable internet connection
  14. Launch Brave again
  15. Check settings page, download id should now be showing
  16. Custom assets should eventually show up (may take a bit)

User launches with no clipboard referral and internet

  1. Referral should still work, even if clipboard approach fails, this tests that condition:
  2. Visit a Super Referrer landing page
  3. Hit the download button
  4. This will redirect you to the App Store, close the App Store
  5. Go to notes app (or some other place with a text field)
  6. Paste the clipboard
  7. Should output F83AB73F-9852-4F01-ABA8-7830B8825993: and then the specific ref code (e.g. ABC123) from the referral
  8. So full output would be F83AB73F-9852-4F01-ABA8-7830B8825993:ABC123 for example
  9. Copy a random string (e.g. “chocolate and coffee”)
  10. Re-paste the clipboard to verify the new string is not the referral string
  11. Download Brave through TF
  12. Open Brave
  13. Verify that correct ref code shows up in settings / on server
  14. Verify assets are shown on the home page
  15. Verify favorites show the referral’s custom favs

User launches with no clipboard referral and no internet

  1. Referral should still work, even if clipboard approach fails, this tests that condition:
  2. Visit a Super Referrer landing page
  3. Hit the download button
  4. This will redirect you to the App Store, close the App Store
  5. Go to notes app (or some other place with a text field)
  6. Paste the clipboard
  7. Should output F83AB73F-9852-4F01-ABA8-7830B8825993: and then the specific ref code (e.g. ABC123) from the referral
  8. So full output would be F83AB73F-9852-4F01-ABA8-7830B8825993:ABC123 for example
  9. Copy a random string (e.g. “chocolate and coffee”)
  10. Re-paste the clipboard to verify the new string is not the referral string
  11. Download Brave through TF
  12. Disable internet (this makes sure the fallback is not activated if clipboard fails for some reason)
  13. Open Brave
  14. Since no internet connection, no custom assets / download id, no ref-code should be shown this time
  15. Close app entirely (swipe up on it)
  16. Re-enable internet connection
  17. Launch Brave again
  18. Check settings page, download id / ref code should now be showing
  19. Custom assets should eventually show up (may take a bit)

Screenshots:

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).
jhreis and others added 19 commits May 11, 2020
@@ -47,6 +47,7 @@ public class UserReferralProgram {

/// Looks for referral and returns its landing page if possible.
public func referralLookup(refCode: String?, completion: @escaping (_ refCode: String?, _ offerUrl: String?) -> Void) {
Preferences.URP.referralLookupOutstanding.value = false

This comment has been minimized.

Copy link
@iccub

iccub May 25, 2020

Contributor

Doesn't make sense to me, shouldn't this flag be toggled after the ref lookup network call?

This comment has been minimized.

Copy link
@jhreis

jhreis May 26, 2020

Author Contributor

Yup 😄 I think I moved this last minute when cleaning things up, but this looks v wrong. Fixing.

@iccub
iccub approved these changes May 26, 2020
@@ -23,6 +23,13 @@ public class UserReferralProgram {
static let prod = "https://laptop-updates.brave.com"
}

// In case of network problems when looking for referrral code
// we retry the call few times while the app is still alive.
private var referralLookupRetryTimer: Timer?

This comment has been minimized.

Copy link
@jhreis

jhreis May 27, 2020

Author Contributor

Probably more clear to just wrap these all in a struct, since all related to the same thing.

struct ReferralLookupRetry {
    private var retryTimer: Timer?
    private var currentCount = 0
    ...
}
@iccub iccub force-pushed the fix-2565 branch from c5eb22a to 78d2f75 May 27, 2020
if refCode != nil {
UrpLog.log("Clipboard ref code found: " + (UIPasteboard.general.string ?? "!Clipboard Empty!"))
UrpLog.log("Clearing clipboard.")
UIPasteboard.general.clearPasteboard()

This comment has been minimized.

Copy link
@jumde

jumde May 27, 2020

Collaborator

If the refcode is cleared here, app won't find the refcode on the next launch if the the 10 network requests are not successful. May be store it in URP preferences?

This comment has been minimized.

Copy link
@jhreis

jhreis May 27, 2020

Author Contributor

Ref code is passed into the look up function, and it is stored internally there:
https://github.com/brave/brave-ios/pull/2574/files#diff-bfe2070e3d8f02013654953e18b60730R280

Here is where it is stored:
https://github.com/brave/brave-ios/pull/2574/files#diff-60f6d7911cf19ceb7bcfcde55f9bc740R126

Then two lines below the network request (and potential retry logic) is executed.

This comment has been minimized.

Copy link
@jumde

jumde May 27, 2020

Collaborator

My concern is that we are still accessing the clipboard on subsequent launches, I think we should just store it in a preference and use it from there: https://github.com/brave/brave-ios/pull/2574/files#diff-bfe2070e3d8f02013654953e18b60730R273 - Let me know if I'm missing something.

This comment has been minimized.

Copy link
@jhreis

jhreis May 27, 2020

Author Contributor

That make sense, we will add a ref code check to prevent reading the clipboard again. Will be in a follow PR though, as it doesn't pertain to what this PR is.

@jhreis
Copy link
Contributor Author

jhreis commented May 27, 2020

New commits from @iccub lgtm, ++
(can't formally review, since originally my PR)

@jhreis
Copy link
Contributor Author

jhreis commented May 27, 2020

Per @jumde's comments, created follow up ticket, which will be required for this feature:
#2584

@jumde
Copy link
Collaborator

jumde commented May 27, 2020

Issue to access clipboard on subsequent launches is being addressed here: https://github.com/brave/brave-ios/pull/2585/files - rest looks good to me

@jumde jumde self-requested a review May 27, 2020
@jumde
jumde approved these changes May 27, 2020
@jhreis jhreis merged commit 0c04ba1 into development May 27, 2020
2 checks passed
2 checks passed
SonarCloud Code Analysis Quality Gate passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@jhreis jhreis deleted the fix-2565 branch May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.