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

Removed unnecessary comment.

  • Loading branch information
jhreis committed May 18, 2020
commit aaa762e381f175fe031574dbff23d508fea2c944
@@ -198,8 +198,6 @@ public class UserReferralProgram {

// +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
let valid = input.range(of: #"\b[A-Z]{3}[0-9]{3}\b"#, options: .regularExpression) != nil // true

This comment has been minimized.

Copy link
@kylehickinson

kylehickinson May 20, 2020

Contributor

Leftover // true comment? 😄

This comment has been minimized.

Copy link
@jhreis

jhreis May 20, 2020

Author Contributor

lol, yup, used for testing in a playground, thanks for pointing out.

// Both conditions must be met
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.