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 #2539: Super Referrer Improvements (Clipboard) #2535

Merged
merged 16 commits into from May 25, 2020
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

Cleaned up code a bit, no functional changes.

  • Loading branch information
jhreis committed May 18, 2020
commit 15bfead65cb6c749f9805723e0abe542cb8a3f59
@@ -82,6 +82,8 @@ public class UserReferralProgram {
}

if let refCode = refCode {
// This is also potentially set after server network request,
// esp important for binaries that require server ref code retrieval.
Preferences.URP.referralCode.value = refCode
This conversation was marked as resolved by jhreis

This comment has been minimized.

Copy link
@kylehickinson

kylehickinson May 11, 2020

Contributor

This pref is not new right? Is this the only place we can set this?

This comment has been minimized.

Copy link
@jhreis

jhreis May 11, 2020

Author Contributor

Not new, and no. This only gets set for ref codes passed in via clipboard.
I'll add comment here too.

It also gets set if the server passes down a ref code.

}

@@ -194,11 +196,11 @@ public class UserReferralProgram {
public class func sanitize(input: String?) -> String? {
guard var input = input, input.hasPrefix(self.clipboardPrefix) else { return nil }

// +1 to strip off `:` that proceeds the defined prefix
input.removeFirst(self.clipboardPrefix.count + 1)
This conversation was marked as resolved by jhreis

This comment has been minimized.

Copy link
@kylehickinson

kylehickinson May 11, 2020

Contributor

Understand this probably set in a spec somewhere, but maybe a comment about why + 1?

This comment has been minimized.

Copy link
@jhreis

jhreis May 11, 2020

Author Contributor

Nah, lol, yeah, will do!

This conversation was marked as resolved by Brandon-T

This comment has been minimized.

Copy link
@Brandon-T

Brandon-T May 20, 2020

Collaborator

If input is exactly the same length as clipboardPrefix, then removing count + 1 is undefined behaviour and could potentially cause a crash.

The number of elements to remove from the collection. k must be greater than or equal to zero and must not exceed the number of elements in the collection.

We can add to the guard statement that input.count > self.clipboardPrefix.count + 1
Is it possible that the ref-code after the prefix could potentially be blank? I'm not sure, but I think some people might try to manipulate it :D

This comment has been minimized.

Copy link
@jhreis

jhreis May 20, 2020

Author Contributor

Good catch, thanks @Brandon-T

This comment has been minimized.

Copy link
@jhreis

jhreis May 20, 2020

Author Contributor

I added a unit test to cover this too, thanks for pointing out.

// Add any other potential validation here, e.g. validating the actual ref code string
This conversation was marked as resolved by jhreis

This comment has been minimized.

This comment has been minimized.

Copy link
@Brandon-T

Brandon-T May 11, 2020

Collaborator

The ref code is guaranteed to be 6 upper-case, 0-9 codes?

This comment has been minimized.

Copy link
@BrendanEich

BrendanEich May 11, 2020

Member

Refcode must be ABC123 format, 3 uppercase alphabetic and 3 decimal digits.

This comment has been minimized.

Copy link
@jhreis

jhreis May 12, 2020

Author Contributor

@jumde Please finalize this on the spec first.

This comment has been minimized.

Copy link
@jhreis

jhreis May 18, 2020

Author Contributor

Updated regex per spec.

if input.isEmpty { return nil }
return input
return input.isEmpty ? nil : input
}

/// Same as `customHeaders` only blocking on result, to gaurantee data is available
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.