Handle expired Autofill credit cards in autofill dialog

Handle expired Autofill credit cards in autofill dialog

BUG=168680, 235270
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=198063

[Dan Beam, 2013-04-24 07:02:17]

stop writing so much code and you wont get all my reviews :P

[Dan Beam, 2013-04-24 07:40:02]

oh yeah, FYI, this only fixes the Autofill case as of now as the wallet version
will be broken either way as we don't have a good edit UI yet

[Evan Stade, 2013-04-24 22:00:17]

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:908: void
AutofillDialogControllerImpl::ShowEditingMode(DialogSection section) {
make functions match header order

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1402: if
(IsASuggestionItemKey(model->GetItemKeyForCheckedItem()) &&
I can't for the life of me figure out what this is doing, so a comment here
would be useful.

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/data_model_wrapper.h:60: virtual bool IsValid() const
= 0;
no need to make this pure virtual

why add this instead of just changing the implementation of GetDisplayText()?

[Dan Beam, 2013-04-26 02:11:32]

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:908: void
AutofillDialogControllerImpl::ShowEditingMode(DialogSection section) {
On 2013/04/24 22:00:17, Evan Stade wrote:
> make functions match header order

Done.

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1402: if
(IsASuggestionItemKey(model->GetItemKeyForCheckedItem()) &&
On 2013/04/24 22:00:17, Evan Stade wrote:
> I can't for the life of me figure out what this is doing, so a comment here
> would be useful.

Done.

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
File chrome/browser/ui/autofill/data_model_wrapper.h (right):

https://codereview.chromium.org/14425010/diff/2001/chrome/browser/ui/autofill...
chrome/browser/ui/autofill/data_model_wrapper.h:60: virtual bool IsValid() const
= 0;
On 2013/04/24 22:00:17, Evan Stade wrote:
> no need to make this pure virtual
> 
> why add this instead of just changing the implementation of GetDisplayText()?

Done.

[Dan Beam, 2013-04-29 21:41:46]

ping

[Evan Stade, 2013-04-29 21:55:40]

lgtm

[Dan Beam, 2013-04-30 04:59:07]

+sky@: for chrome/browser/ui/views

estade@: I found a number of issues with my patch,

1) changing autofill data with a dialog running didn't work as expected
2) invalid wallet suggestions weren't being filled (as they happen a while after
Show())
3) selecting invalid suggestions and then a valid suggestion didn't work because
of stale state
4) some code paths weren't hitting ValidateGroup()

So, basically I just moved some things lower in the code (ValidateGroup() into
UpdateGroupSectionState(), all the SuggestionItemSelected() into
SuggestionsUpdated()).

Sorry that you have to re-review, but the new patch is much better.  

Before you ask, I've started on a browser test here
<https://codereview.chromium.org/14613002/> (it depends on this first:
<https://codereview.chromium.org/14564003/>).

[sky, 2013-04-30 13:45:46]

LGTM

[Evan Stade, 2013-04-30 19:03:10]

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:383: // This
should be done while |view_| is non-NULL.
dislike. The controller should not need to update the view as soon as it's
created. The view should always be able to poll the controller to get all the
current state that's necessary for displaying the UI.

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1847: const size_t
kFirstRealSuggestionIndex = 1;
I don't see the benefit to the way you rewrote this. It actually detracts,
because 

IsASuggestionItemKey(model->GetItemKeyAt(kFirstRealSuggestionIndex))

doesn't make much sense --- on the surface, it should always return true

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1852:
SuggestionItemSelected(model, default_suggestion_index);
this feels wrong to me. For one thing, it screws with the metrics. You should
just fill the inputs and set the editing state for invalid suggestions.

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:443: void
UpdateDetailsGroupState(DetailsGroup* group);
nit: revert this change, instead fix ValidateGroup to take a const ref (all you
have to do is change a couple iterators to const iterators)

[Dan Beam, 2013-05-01 00:43:09]

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:383: // This
should be done while |view_| is non-NULL.
On 2013/04/30 19:03:10, Evan Stade wrote:
> dislike. The controller should not need to update the view as soon as it's
> created. The view should always be able to poll the controller to get all the
> current state that's necessary for displaying the UI.

Reverted.

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1847: const size_t
kFirstRealSuggestionIndex = 1;
On 2013/04/30 19:03:10, Evan Stade wrote:
> I don't see the benefit to the way you rewrote this. It actually detracts,
> because 
> 
> IsASuggestionItemKey(model->GetItemKeyAt(kFirstRealSuggestionIndex))
> 
> doesn't make much sense --- on the surface, it should always return true

Reverted.

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1852:
SuggestionItemSelected(model, default_suggestion_index);
On 2013/04/30 19:03:10, Evan Stade wrote:
> this feels wrong to me. For one thing, it screws with the metrics. You should
> just fill the inputs and set the editing state for invalid suggestions.

Done.

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.h (right):

https://codereview.chromium.org/14425010/diff/36001/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.h:443: void
UpdateDetailsGroupState(DetailsGroup* group);
On 2013/04/30 19:03:10, Evan Stade wrote:
> nit: revert this change, instead fix ValidateGroup to take a const ref (all
you
> have to do is change a couple iterators to const iterators)

Done.

[Dan Beam, 2013-05-01 00:46:22]

https://codereview.chromium.org/14425010/diff/43001/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (left):

https://codereview.chromium.org/14425010/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1580:
ResetManualInputForSection(SECTION_SHIPPING);
these weren't necessary

https://codereview.chromium.org/14425010/diff/43001/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1604:
SuggestionsUpdated();
as this calls PrepareDetailInputsForSection() on all sections

[Dan Beam, 2013-05-01 04:03:51]

note to self: this now depends on https://codereview.chromium.org/14564003/

[Evan Stade, 2013-05-01 21:54:31]

lgtm

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1846: 
^H

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1084: DCHECK(group);
no longer need the dcheck since you deref, err, "indirect", right away

[Dan Beam, 2013-05-02 00:03:31]

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/autofil...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/autofil...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:1846: 
On 2013/05/01 21:54:31, Evan Stade wrote:
> ^H

Done.

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/views/a...
File chrome/browser/ui/views/autofill/autofill_dialog_views.cc (right):

https://codereview.chromium.org/14425010/diff/59003/chrome/browser/ui/views/a...
chrome/browser/ui/views/autofill/autofill_dialog_views.cc:1084: DCHECK(group);
On 2013/05/01 21:54:31, Evan Stade wrote:
> no longer need the dcheck since you deref, err, "indirect", right away

Done.

[Dan Beam, 2013-05-02 05:23:05]

+isherman@ for components/autofill

[Dan Beam, 2013-05-02 05:23:19]

+isherman@ for realz

[Dan Beam, 2013-05-02 05:23:30]

feel free to CQ if LGTY

[Ilya Sherman, 2013-05-02 05:44:10]

lgtm

[commit-bot: I haz the power, 2013-05-02 05:44:19]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14425010/71001

[commit-bot: I haz the power, 2013-05-02 06:29:21]

Retried try job too often on linux_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2013-05-02 18:35:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14425010/95001

[commit-bot: I haz the power, 2013-05-02 20:30:34]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-05-02 23:47:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14425010/95001

[commit-bot: I haz the power, 2013-05-02 23:48:02]

Failed to apply patch for
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
  Hunk #5 succeeded at 1415 (offset 2 lines).
  Hunk #6 succeeded at 1592 (offset 2 lines).
  Hunk #7 FAILED at 1743.
  Hunk #8 FAILED at 1836.
  Hunk #9 succeeded at 2333 (offset 12 lines).
  2 out of 9 hunks FAILED -- saving rejects to file
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc.rej

Patch:       chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
Index: chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
diff --git a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
index
3fde362ba72fc86d5b07879a5fb992d2a1ed9978..9b250c092adb79106cb0add15830b0617be5a44b
100644
--- a/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
+++ b/chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc
@@ -645,12 +645,27 @@ void
AutofillDialogControllerImpl::EnsureLegalDocumentsText() {
   legal_documents_text_ = text;
 }
 
-void AutofillDialogControllerImpl::ResetManualInputForSection(
+void AutofillDialogControllerImpl::PrepareDetailInputsForSection(
     DialogSection section) {
+  // Reset all previously entered data and stop editing |section|.
   DetailInputs* inputs = MutableRequestedFieldsForSection(section);
-  for (size_t i = 0; i < inputs->size(); ++i)
+  for (size_t i = 0; i < inputs->size(); ++i) {
     (*inputs)[i].initial_value.clear();
+  }
   section_editing_state_[section] = false;
+
+  // If the chosen item in |model| yields an empty suggestion text, it is
+  // invalid. In this case, show the editing UI with invalid fields
highlighted.
+  SuggestionsMenuModel* model = SuggestionsMenuModelForSection(section);
+  if (IsASuggestionItemKey(model->GetItemKeyForCheckedItem()) &&
+      SuggestionTextForSection(section).empty()) {
+    scoped_ptr<DataModelWrapper> wrapper = CreateWrapper(section);
+    wrapper->FillInputs(MutableRequestedFieldsForSection(section));
+    section_editing_state_[section] = true;
+  }
+
+  if (view_)
+    view_->UpdateSection(section);
 }
 
 const DetailInputs& AutofillDialogControllerImpl::RequestedFieldsForSection(
@@ -755,8 +770,9 @@ string16
AutofillDialogControllerImpl::SuggestionTextForSection(
   if (!action_text.empty())
     return action_text;
 
-  // When the user has clicked 'edit', don't show a suggestion (even though
-  // there is a profile selected in the model).
+  // When the user has clicked 'edit' or a suggestion is somehow invalid (e.g.
a
+  // user selects a credit card that has expired), don't show a suggestion
(even
+  // though there is a profile selected in the model).
   if (section_editing_state_[section])
     return string16();
 
@@ -906,9 +922,8 @@ bool AutofillDialogControllerImpl::EditEnabledForSection(
 
 void AutofillDialogControllerImpl::EditClickedForSection(
     DialogSection section) {
-  DetailInputs* inputs = MutableRequestedFieldsForSection(section);
   scoped_ptr<DataModelWrapper> model = CreateWrapper(section);
-  model->FillInputs(inputs);
+  model->FillInputs(MutableRequestedFieldsForSection(section));
   section_editing_state_[section] = true;
   view_->UpdateSection(section);
 
@@ -918,8 +933,7 @@ void AutofillDialogControllerImpl::EditClickedForSection(
 
 void AutofillDialogControllerImpl::EditCancelledForSection(
     DialogSection section) {
-  ResetManualInputForSection(section);
-  view_->UpdateSection(section);
+  PrepareDetailInputsForSection(section);
 }
 
 gfx::Image AutofillDialogControllerImpl::IconForField(
@@ -1399,7 +1413,7 @@ void AutofillDialogControllerImpl::SuggestionItemSelected(
   }
 
   model->SetCheckedIndex(index);
-  EditCancelledForSection(SectionForSuggestionsMenuModel(*model));
+  PrepareDetailInputsForSection(SectionForSuggestionsMenuModel(*model));
 
   LogSuggestionItemSelectedMetric(*model);
 }
@@ -1576,13 +1590,6 @@ void
AutofillDialogControllerImpl::OnPersonalDataChanged() {
 // AccountChooserModelDelegate implementation.
 
 void AutofillDialogControllerImpl::AccountChoiceChanged() {
-  // Whenever the user changes the account, all manual inputs should be reset.
-  ResetManualInputForSection(SECTION_EMAIL);
-  ResetManualInputForSection(SECTION_CC);
-  ResetManualInputForSection(SECTION_BILLING);
-  ResetManualInputForSection(SECTION_CC_BILLING);
-  ResetManualInputForSection(SECTION_SHIPPING);
-
   if (is_submitting_)
     GetWalletClient()->CancelRequests();
 
@@ -1736,7 +1743,6 @@ void AutofillDialogControllerImpl::SuggestionsUpdated() {
           base::IntToString(i),
           addresses[i]->DisplayName(),
           addresses[i]->DisplayNameDetail());
-
     }
 
     if (!IsSubmitPausedOn(wallet::VERIFY_CVV)) {
@@ -1829,10 +1835,14 @@ void AutofillDialogControllerImpl::SuggestionsUpdated()
{
   const std::string& first_real_suggestion_item_key =
       suggested_shipping_.GetItemKeyAt(1);
   if (IsASuggestionItemKey(first_real_suggestion_item_key))
-      suggested_shipping_.SetCheckedItem(first_real_suggestion_item_key);
+    suggested_shipping_.SetCheckedItem(first_real_suggestion_item_key);
 
   if (view_)
     view_->ModelChanged();
+
+  for (size_t section = SECTION_MIN; section <= SECTION_MAX; ++section) {
+    PrepareDetailInputsForSection(static_cast<DialogSection>(section));
+  }
 }
 
 bool AutofillDialogControllerImpl::IsCompleteProfile(
@@ -2314,7 +2324,6 @@ void
AutofillDialogControllerImpl::LogSuggestionItemSelectedMetric(
     dialog_ui_event = DialogSectionToUiItemAddedEvent(section);
   } else if (IsASuggestionItemKey(model.GetItemKeyForCheckedItem())) {
     // Selected an existing item.
-    DCHECK(!section_editing_state_[section]);
     dialog_ui_event = DialogSectionToUiSelectionChangedEvent(section);
   } else {
     // TODO(estade): add logging for "Manage items" or "Use billing for

[commit-bot: I haz the power, 2013-05-03 01:06:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dbeam@chromium.org/14425010/115001

[commit-bot: I haz the power, 2013-05-03 04:48:01]

Change committed as 198063