@acjay: Adds zip code to manual CC tokenization. #526
Conversation
| }() | ||
| lazy var keypaths: Array<Array<String>> = { | ||
| return [["phoneNumber"], ["email"], ["creditCardName", "creditCardType"]] + (self.appSetup.needsZipCode ? [["zipCode"]] : []) | ||
| return [["phoneNumber"], ["email"]] + (self.appSetup.needsZipCode ? [["zipCode"]] : []) + [["creditCardName", "creditCardType"]] |
There was a problem hiding this comment.
logic feels a bit convoluted for a single liner, might be worth making two?
There was a problem hiding this comment.
Good idea, even splitting the same logic into several lines would help readability.
There was a problem hiding this comment.
Looking down below, it looks like we iterate both of these arrays simultaneously with an index i, which is only used to pull data out of the arrays. Could we instead make this a single array of some struct that encapsulates all the relevant info? Then we could just use a forEach. That's a pattern I've had a lot of success with in the past -- using the type system to restrict to exactly the values our logic allows and using HoF's like map/forEach to eliminate any possibility of off-by-one errors.
|
Could probably do with some tests injecting the AppSetup with both true/false for showing the zip code stuff. |
|
Are we being clear that the ZIP code we're requesting should be the same as on the credit card that will be submitted on the next screen? I'm wondering if it would be better to put the ZIP code request with the other credit card details in the manual entry view, since we don't need it / can't use it for physical swipes. |
|
Although, I'm wondering, if we do that, do we need some kind of subsidiary progress indicator for credit card entry? We'd have 4 separate screens for that one item in the panel. |
|
Actually, I can probably do most of this if you think this is a good idea, then we can free you up for the PIN code and tokenization. How's that sound? |
|
Sounds great Alan 💯 |
|
Figured out the snapshot thing, but haven't yet figured out how to fix some of the other tests |
|
Ah, gotcha. I misunderstood earlier what you meant. I don't think having the zip code added in 9429f04 is critical – this is only a backup, if the card reader fails. We can take a look at building a better UI to convey that we need their billing zip code after this release maybe? The zip code is already hidden if the reader is enabled, anyway. Want to remove that commit and merge? |
|
Not sure I understand -- I'm pretty sure we definitely need billing zip, because bids with manually entered cards are being rejected if they don't have the billing ZIP, regardless of whether they're from the kiosk. |
|
Right, and the PR I sent originally contained that info for registration and CC tokenization. I'm just saying we don't need to further change the UI to move the zip collection, in the most recent commit. The manual entry is a backup for the readers, and I think having a subpar UI for it may be an acceptable thing to deprioritize right now. |
|
From the most recent commit I think the UI's fine, since they enter the billing ZIP after the credit card number. Given the reports of the swiper in the case being less than totally reliable, I think having this be unambiguous is the right move. (The subsidiary progress indicator definitely not a priority, though). The only issue is I haven't quite untangled how the tests are set up to figure out exactly why they're failing now and what needs to be changed to fix them. I'm sure I'll figure it out eventually though; I'd rather have you spend your energy on the PIN thing and the tokenization thing. |
|
Roger roger 📡 |
| @@ -30,6 +30,6 @@ class AppSetup { | |||
|
|
|||
| var needsZipCode: Bool { | |||
| // If we're swiping with the card reaer, we don't need to collect a zip code. | |||
| return disableCardReader | |||
| return false | |||
There was a problem hiding this comment.
Was this debugging code left in by accident?
There was a problem hiding this comment.
Well...not exactly. The ZIP code screen is enabled never according to current rules, so this is technically correct. So we can kill this, but probably should kill the original ZIP code screen altogether when we do. I think we can defer that to a lower-priority task.
There was a problem hiding this comment.
OK cool – I'll leave it up to you to open an issue on this repo (mark it as a "chore").
|
Looks great 💯 Gonna wait on input from the above comment to merge. |
@acjay: Adds zip code to manual CC tokenization.
I'm currently getting 401's against the staging API for all requests, so I haven't been able to test this except for the UI. Let me know what you think.
This fixes an issue tracked in Trello at https://trello.com/c/y2CRxXFy/120-kiosk-submit-zip-with-credit-card-registration