-
Notifications
You must be signed in to change notification settings - Fork 11
DRY two factor code handling in pasteboard, fix leading zero loss #561
Conversation
|
Interesting. It isn't the cast after all. It is how .number pattern matching is working with the pasteboard, i.e.: returns 11234 when the pasteboard contains 011234 |
|
LOL. I've been digging into this more today. Finally have a robust sequence for pulling out two factor codes. Will update the PR with it tomorrow: |
bjtitus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor code suggestions. Otherwise, everything looks good!
| } | ||
|
|
||
| /// We need 6 digits. No more, no less. | ||
| if authenticationCode.count > 6 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor thing, but I wonder if the length should be a parameter of this method. If not, it should probably go as a constant at the top. I think I prefer making it a parameter with a default value, though.
public func detectAuthenticatorCode(length: Int = 6, completion: @escaping (Result<String, Error>) -> Void)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in e109be8
| // | ||
| // PasteboardTests.swift | ||
| // WordPressAuthenticatorTests | ||
| // | ||
| // Created by Allen Snook on 1/13/21. | ||
| // Copyright © 2021 Automattic. All rights reserved. | ||
| // |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you remove this generated header?
We generally keep them out of the projects, but there's no good linter rule available to do it. 😞
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in c96f997
|
It's pretty annoying that Apple is stripping the leading zeros and doesn't mention it. |
Co-authored-by: Brandon Titus <b@titus.io>
Agreed. Done. Feedback ID FB8974839 |
|
All feedback addressed. Ready for next round of review. Thank you @bjtitus ! |
bjtitus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me!
Thanks for fixing this @allendav!
Fixes #557
To test, e.g. with WooCommerce iOS