Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2538543002:
[Payments] Add detailed error messages and title for card unmask prompt. (Closed)
|
Created:
4 years ago by sebsg Modified:
4 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Payments] Add detailed error messages and title for card unmask prompt.
BUG=661279
Committed: https://crrev.com/15e05a11237a95bea52cd48efe20b2c6795bb23a
Cr-Commit-Position: refs/heads/master@{#435598}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed comments #
Total comments: 8
Patch Set 3 : Addressed Mathp's nit #Patch Set 4 : Addressed Rouslan's comments #
Messages
Total messages: 34 (20 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #1 (id:60001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
sebsg@chromium.org changed reviewers: + rouslan@chromium.org
Hi Rouslan, PTAL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looking good! https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:264: setInputError(null, TYPE_BOTH); Looks like you can benefit from TYPE_NONE here https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:271: setInputError(errorMessage, TYPE_BOTH); It's strange to see TYPE_BOTH on its own. Please make the names slightly more verbose, e.g., ERROR_TYPE_BOTH. https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:316: sObserverForTest.onCardUnmaskPromptValidationDone(this); It would make more sense to notify the test after showing the detailed error message (after line 320). https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:421: private void showDetailedErrorMessage(boolean hasValidExpiration, boolean hasValidCvc) { You can reuse ERRROR_TYPE_NONE, ERROR_TYPE_EXPIRATION, ERROR_TYPE_CVC, and ERROR_TYPE_BOTH here. The two booleans represent exactly same information, but using different syntax. https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:422: // Only show an expiration date error if the user has filled both month and year fields. This logic does not show error for a user that typed in 1 digit for year. That input is invalid and should show error. The goal here is to avoid annoying user. When the user has only started typing, UI should not immediately show an error message. The best way to accomplish this goal is to have booleans for each input field named something like: mFinishedTypingMonth mFinishedTypingYear Set mFinishedTypingMonth to true when month field lost focus with some data in there. Set mFinishedTypingYear to true when year field lost focus with some data in there. Then shouldShowExpirationError is !hasValidExpiration && mFinishedTypingMonth && mFinishedTypingYear.
The CQ bit was checked by sebsg@chromium.org to run a CQ dry run
Patchset #2 (id:100001) has been deleted
Thanks! another look? https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:264: setInputError(null, TYPE_BOTH); On 2016/11/29 22:27:36, rouslan wrote: > Looks like you can benefit from TYPE_NONE here As mentioned offline, clearInputError was created instead. https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:271: setInputError(errorMessage, TYPE_BOTH); On 2016/11/29 22:27:36, rouslan wrote: > It's strange to see TYPE_BOTH on its own. Please make the names slightly more > verbose, e.g., ERROR_TYPE_BOTH. Done. https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:316: sObserverForTest.onCardUnmaskPromptValidationDone(this); On 2016/11/29 22:27:36, rouslan wrote: > It would make more sense to notify the test after showing the detailed error > message (after line 320). Done. https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:421: private void showDetailedErrorMessage(boolean hasValidExpiration, boolean hasValidCvc) { On 2016/11/29 22:27:36, rouslan wrote: > You can reuse ERRROR_TYPE_NONE, ERROR_TYPE_EXPIRATION, ERROR_TYPE_CVC, and > ERROR_TYPE_BOTH here. The two booleans represent exactly same information, but > using different syntax. Yeah but I would have to parse the two booleans into the ERROR_TYPE_X in Validate and then use || statements when assigning both shouldShowExpirationError and ...CvcError shouldShowCvcError = !hasValidCvc && (errorType == ERROR_TYPE_CVC || errorType == ERROR_TYPE_BOTH); I think I prefer the use of the booleans for that reason. But I can change it if you feel strongly about the enum. Thanks! https://codereview.chromium.org/2538543002/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:422: // Only show an expiration date error if the user has filled both month and year fields. On 2016/11/29 22:27:36, rouslan wrote: > This logic does not show error for a user that typed in 1 digit for year. That > input is invalid and should show error. > > The goal here is to avoid annoying user. When the user has only started typing, > UI should not immediately show an error message. The best way to accomplish this > goal is to have booleans for each input field named something like: > > mFinishedTypingMonth > mFinishedTypingYear > > Set mFinishedTypingMonth to true when month field lost focus with some data in > there. > Set mFinishedTypingYear to true when year field lost focus with some data in > there. > > Then shouldShowExpirationError is !hasValidExpiration && mFinishedTypingMonth && > mFinishedTypingYear. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
sebsg@chromium.org changed reviewers: + mathp@chromium.org
Hi Mathp, can you please review card_unmask_prompt_controller_impl.cc Thanks!
lgtm with nit https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... File components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc:222: int ids; int id = ShouldRequestExpirationDate() ? IDS_... : IDS_...;
https://codereview.chromium.org/2538543002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2538543002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:203: mFinishedTypingCvc = true; mFinishedTypingCvc should also be flipped to "true" when user types is 3 digits. Users are not very likely to move focus from before they attempt to click "Continue" only to find that button disabled and be puzzled. https://codereview.chromium.org/2538543002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:547: if (message == null) { This path will never be triggered. Remove it. You can even add "assert message != null", if you want. https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... File components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc:222: int ids; On 2016/11/30 15:37:46, Mathieu Perreault wrote: > int id = ShouldRequestExpirationDate() ? IDS_... : IDS_...; Better yet: return l10n_util::GetStringFUTF16(ShouldRequestExpirationDate() ? IDS_... : IDS_..., card_.TypeAndLastFourDigits());
Thanks Math, addressed your comment. Rouslan, another look? :) https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... File components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc:222: int ids; On 2016/11/30 15:37:46, Mathieu Perreault wrote: > int id = ShouldRequestExpirationDate() ? IDS_... : IDS_...; Done.
Thanks! https://codereview.chromium.org/2538543002/diff/120001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java (right): https://codereview.chromium.org/2538543002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:203: mFinishedTypingCvc = true; On 2016/11/30 16:30:29, rouslan wrote: > mFinishedTypingCvc should also be flipped to "true" when user types is 3 digits. > Users are not very likely to move focus from before they attempt to click > "Continue" only to find that button disabled and be puzzled. Good point. Used the value of hasValidCvc since some cards may require 4 digits. https://codereview.chromium.org/2538543002/diff/120001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/autofill/CardUnmaskPrompt.java:547: if (message == null) { On 2016/11/30 16:30:29, rouslan wrote: > This path will never be triggered. Remove it. You can even add "assert message > != null", if you want. Done. https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... File components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc (right): https://codereview.chromium.org/2538543002/diff/120001/components/autofill/co... components/autofill/core/browser/ui/card_unmask_prompt_controller_impl.cc:222: int ids; On 2016/11/30 16:30:29, rouslan wrote: > On 2016/11/30 15:37:46, Mathieu Perreault wrote: > > int id = ShouldRequestExpirationDate() ? IDS_... : IDS_...; > > Better yet: > > return l10n_util::GetStringFUTF16(ShouldRequestExpirationDate() ? IDS_... : > IDS_..., card_.TypeAndLastFourDigits()); Ha! Already did that :)
lgtm
sebsg@chromium.org changed reviewers: + blundell@chromium.org
Hi blundell@ could you please review components/autofill_strings.grdp ? Thanks!
mathp's review is sufficient for OWNERS for that file.
The CQ bit was checked by sebsg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2538543002/#ps60002 (title: "Addressed Rouslan's comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60002, "attempt_start_ts": 1480594728568740,
"parent_rev": "322f84420f970f05ebaf53b06bde22fdb8e78785", "commit_rev":
"2d03315d9083dc9095c2c142becce7d07b5160c2"}
Message was sent while issue was closed.
Committed patchset #4 (id:60002)
Message was sent while issue was closed.
Description was changed from ========== [Payments] Add detailed error messages and title for card unmask prompt. BUG=661279 ========== to ========== [Payments] Add detailed error messages and title for card unmask prompt. BUG=661279 Committed: https://crrev.com/15e05a11237a95bea52cd48efe20b2c6795bb23a Cr-Commit-Position: refs/heads/master@{#435598} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/15e05a11237a95bea52cd48efe20b2c6795bb23a Cr-Commit-Position: refs/heads/master@{#435598} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
