Offer users an option to delete card from local if uploading failed

If uploading of a card failed, in the feedback dialog, we will show a string 'invalid' and a trash can button. If the user clicks the button, that card will be deleted from browser and will disappear in the feedback dialog. If all invalid cards are deleted in the dialog, the dialog will be updated to the 'finished' state. If all cards failed, we will not show the "View cards button". Uploaded screenshots in the bug (comment #10 and comment #26). Using TBR because this merge had a conflict and needed to fix manually: TBR=estade@chromium.org,tobiasjs@chromium.org,bsep@chromium.org,mahmadi@chromium.org (cherry picked from commit 26874dc) Bug: 913150 Change-Id: Iae7780027932ac530bcb2f0c643be1ec94d1f334 Reviewed-on: https://chromium-review.googlesource.com/c/1314129 Reviewed-by: Tobias Sargeant <tobiasjs@chromium.org> Reviewed-by: Moe Ahmadi <mahmadi@chromium.org> Reviewed-by: Evan Stade <estade@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Reviewed-by: Jared Saul <jsaul@google.com> Commit-Queue: Siyu An <siyua@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#614193} Reviewed-on: https://chromium-review.googlesource.com/c/1369941 Cr-Commit-Position: refs/branch-heads/3626@{#234} Cr-Branched-From: d897fb1-refs/heads/master@{#612437}
nwjs · Dec 10, 2018 · bbb07a2 · bbb07a2
1 parent 1178b7e
commit bbb07a2
Show file tree

Hide file tree

Showing 27 changed files with 318 additions and 158 deletions.
diff --git a/android_webview/browser/aw_autofill_client.cc b/android_webview/browser/aw_autofill_client.cc
@@ -320,8 +320,8 @@ void AwAutofillClient::ConfirmSaveCreditCardToCloud(
 
 void AwAutofillClient::ShowLocalCardMigrationResults(
     const base::string16& tip_message,
-    const std::vector<autofill::MigratableCreditCard>&
-        migratable_credit_cards) {
+    const std::vector<autofill::MigratableCreditCard>& migratable_credit_cards,
+    MigrationDeleteCardCallback delete_local_card_callback) {
   NOTIMPLEMENTED();
 }
 

diff --git a/android_webview/browser/aw_autofill_client.h b/android_webview/browser/aw_autofill_client.h
@@ -89,7 +89,8 @@ class AwAutofillClient : public autofill::AutofillClient,
   void ShowLocalCardMigrationResults(
       const base::string16& tip_message,
       const std::vector<autofill::MigratableCreditCard>&
-          migratable_credit_cards) override;
+          migratable_credit_cards,
+      MigrationDeleteCardCallback delete_local_card_callback) override;
   void ConfirmSaveAutofillProfile(const autofill::AutofillProfile& profile,
                                   base::OnceClosure callback) override;
   void ConfirmSaveCreditCardLocally(const autofill::CreditCard& card,

diff --git a/chrome/browser/ui/autofill/chrome_autofill_client.cc b/chrome/browser/ui/autofill/chrome_autofill_client.cc
@@ -274,12 +274,14 @@ void ChromeAutofillClient::ConfirmMigrateLocalCardToCloud(
 
 void ChromeAutofillClient::ShowLocalCardMigrationResults(
     const base::string16& tip_message,
-    const std::vector<MigratableCreditCard>& migratable_credit_cards) {
+    const std::vector<MigratableCreditCard>& migratable_credit_cards,
+    MigrationDeleteCardCallback delete_local_card_callback) {
 #if !defined(OS_ANDROID)
   autofill::ManageMigrationUiController::CreateForWebContents(web_contents());
   autofill::ManageMigrationUiController* controller =
       autofill::ManageMigrationUiController::FromWebContents(web_contents());
-  controller->ShowCreditCardIcon(tip_message, migratable_credit_cards);
+  controller->ShowCreditCardIcon(tip_message, migratable_credit_cards,
+                                 delete_local_card_callback);
 #endif
 }
 

diff --git a/chrome/browser/ui/autofill/chrome_autofill_client.h b/chrome/browser/ui/autofill/chrome_autofill_client.h
@@ -74,9 +74,10 @@ class ChromeAutofillClient
       std::unique_ptr<base::DictionaryValue> legal_message,
       const std::vector<MigratableCreditCard>& migratable_credit_cards,
       LocalCardMigrationCallback start_migrating_cards_callback) override;
-  void ShowLocalCardMigrationResults(const base::string16& tip_message,
-                                     const std::vector<MigratableCreditCard>&
-                                         migratable_credit_cards) override;
+  void ShowLocalCardMigrationResults(
+      const base::string16& tip_message,
+      const std::vector<MigratableCreditCard>& migratable_credit_cards,
+      MigrationDeleteCardCallback delete_local_card_callback) override;
   void ConfirmSaveAutofillProfile(const AutofillProfile& profile,
                                   base::OnceClosure callback) override;
   void ConfirmSaveCreditCardLocally(const CreditCard& card,

diff --git a/chrome/browser/ui/autofill/local_card_migration_dialog_controller_impl.cc b/chrome/browser/ui/autofill/local_card_migration_dialog_controller_impl.cc
@@ -75,12 +75,14 @@ void LocalCardMigrationDialogControllerImpl::ShowOfferDialog(
 
 void LocalCardMigrationDialogControllerImpl::ShowCreditCardIcon(
     const base::string16& tip_message,
-    const std::vector<MigratableCreditCard>& migratable_credit_cards) {
+    const std::vector<MigratableCreditCard>& migratable_credit_cards,
+    AutofillClient::MigrationDeleteCardCallback delete_local_card_callback) {
   if (local_card_migration_dialog_)
     local_card_migration_dialog_->CloseDialog();
 
   migratable_credit_cards_ = migratable_credit_cards;
   tip_message_ = tip_message;
+  delete_local_card_callback_ = delete_local_card_callback;
 
   view_state_ = LocalCardMigrationDialogState::kFinished;
   for (const auto& cc : migratable_credit_cards) {
@@ -164,11 +166,45 @@ void LocalCardMigrationDialogControllerImpl::OnLegalMessageLinkClicked(
       AutofillMetrics::LOCAL_CARD_MIGRATION_DIALOG_LEGAL_MESSAGE_CLICKED);
 }
 
+void LocalCardMigrationDialogControllerImpl::DeleteCard(
+    const std::string& deleted_card_guid) {
+  DCHECK(delete_local_card_callback_);
+  delete_local_card_callback_.Run(deleted_card_guid);
+
+  migratable_credit_cards_.erase(
+      std::remove_if(migratable_credit_cards_.begin(),
+                     migratable_credit_cards_.end(),
+                     [&](const auto& card) {
+                       return card.credit_card().guid() == deleted_card_guid;
+                     }),
+      migratable_credit_cards_.end());
+
+  if (!HasFailedCard()) {
+    view_state_ = LocalCardMigrationDialogState::kFinished;
+    delete_local_card_callback_.Reset();
+  }
+}
+
 void LocalCardMigrationDialogControllerImpl::OnDialogClosed() {
   if (local_card_migration_dialog_)
     local_card_migration_dialog_ = nullptr;
 }
 
+bool LocalCardMigrationDialogControllerImpl::AllCardsInvalid() const {
+  // For kOffered state, the migration status of all cards are UNKNOWN,
+  // so this function will return true as well. Need an early exit to avoid
+  // it.
+  if (view_state_ == LocalCardMigrationDialogState::kOffered)
+    return false;
+
+  return std::find_if(
+             migratable_credit_cards_.begin(), migratable_credit_cards_.end(),
+             [](const auto& card) {
+               return card.migration_status() ==
+                      MigratableCreditCard::MigrationStatus::SUCCESS_ON_UPLOAD;
+             }) == migratable_credit_cards_.end();
+}
+
 LocalCardMigrationDialog*
 LocalCardMigrationDialogControllerImpl::local_card_migration_dialog_view()
     const {
@@ -189,4 +225,13 @@ void LocalCardMigrationDialogControllerImpl::UpdateIcon() {
   location_bar->UpdateLocalCardMigrationIcon();
 }
 
+bool LocalCardMigrationDialogControllerImpl::HasFailedCard() const {
+  return std::find_if(
+             migratable_credit_cards_.begin(), migratable_credit_cards_.end(),
+             [](const auto& card) {
+               return card.migration_status() ==
+                      MigratableCreditCard::MigrationStatus::FAILURE_ON_UPLOAD;
+             }) != migratable_credit_cards_.end();
+}
+
 }  // namespace autofill
diff --git a/chrome/browser/ui/autofill/local_card_migration_dialog_controller_impl.h b/chrome/browser/ui/autofill/local_card_migration_dialog_controller_impl.h
@@ -39,7 +39,8 @@ class LocalCardMigrationDialogControllerImpl
   // passes |tip_message|, and |migratable_credit_cards| to controller.
   void ShowCreditCardIcon(
       const base::string16& tip_message,
-      const std::vector<MigratableCreditCard>& migratable_credit_cards);
+      const std::vector<MigratableCreditCard>& migratable_credit_cards,
+      AutofillClient::MigrationDeleteCardCallback delete_local_card_callback);
 
   // If the user clicks on the credit card icon in the omnibox, we show the
   // feedback dialog containing the uploading results of the cards that the
@@ -61,7 +62,9 @@ class LocalCardMigrationDialogControllerImpl
   void OnCancelButtonClicked() override;
   void OnViewCardsButtonClicked() override;
   void OnLegalMessageLinkClicked(const GURL& url) override;
+  void DeleteCard(const std::string& deleted_card_guid) override;
   void OnDialogClosed() override;
+  bool AllCardsInvalid() const override;
 
   // Returns nullptr if no dialog is currently shown.
   LocalCardMigrationDialog* local_card_migration_dialog_view() const;
@@ -78,6 +81,8 @@ class LocalCardMigrationDialogControllerImpl
 
   void UpdateIcon();
 
+  bool HasFailedCard() const;
+
   LocalCardMigrationDialog* local_card_migration_dialog_ = nullptr;
 
   PrefService* pref_service_;
@@ -90,6 +95,11 @@ class LocalCardMigrationDialogControllerImpl
   // GUIDs of cards that the user selected to upload.
   AutofillClient::LocalCardMigrationCallback start_migrating_cards_callback_;
 
+  // Invoked when the trash can button in the action-requied dialog is clicked.
+  // Will pass a string of GUID of the card the user selected to delete from
+  // local storage to LocalCardMigrationManager.
+  AutofillClient::MigrationDeleteCardCallback delete_local_card_callback_;
+
   // Local copy of the MigratableCreditCards vector passed from
   // LocalCardMigrationManager. Used in constructing the
   // LocalCardMigrationDialogView.

diff --git a/chrome/browser/ui/autofill/manage_migration_ui_controller.cc b/chrome/browser/ui/autofill/manage_migration_ui_controller.cc
@@ -45,7 +45,8 @@ void ManageMigrationUiController::ShowOfferDialog(
 
 void ManageMigrationUiController::ShowCreditCardIcon(
     const base::string16& tip_message,
-    const std::vector<MigratableCreditCard>& migratable_credit_cards) {
+    const std::vector<MigratableCreditCard>& migratable_credit_cards,
+    AutofillClient::MigrationDeleteCardCallback delete_local_card_callback) {
   if (!dialog_controller_)
     return;
 
@@ -54,7 +55,8 @@ void ManageMigrationUiController::ShowCreditCardIcon(
   // Show error dialog when the vector is an empty vector, which indicates
   // Payments Rpc failure.
   show_error_dialog_ = migratable_credit_cards.empty();
-  dialog_controller_->ShowCreditCardIcon(tip_message, migratable_credit_cards);
+  dialog_controller_->ShowCreditCardIcon(tip_message, migratable_credit_cards,
+                                         delete_local_card_callback);
 }
 
 void ManageMigrationUiController::OnUserClickedCreditCardIcon() {

diff --git a/chrome/browser/ui/autofill/manage_migration_ui_controller.h b/chrome/browser/ui/autofill/manage_migration_ui_controller.h
@@ -55,7 +55,8 @@ class ManageMigrationUiController
 
   void ShowCreditCardIcon(
       const base::string16& tip_message,
-      const std::vector<MigratableCreditCard>& migratable_credit_cards);
+      const std::vector<MigratableCreditCard>& migratable_credit_cards,
+      AutofillClient::MigrationDeleteCardCallback delete_local_card_callback);
 
   void OnUserClickedCreditCardIcon();
 

diff --git a/chrome/browser/ui/views/autofill/local_card_migration_dialog_view.cc b/chrome/browser/ui/views/autofill/local_card_migration_dialog_view.cc
@@ -33,6 +33,7 @@
 #include "ui/views/border.h"
 #include "ui/views/controls/button/button.h"
 #include "ui/views/controls/button/checkbox.h"
+#include "ui/views/controls/button/image_button.h"
 #include "ui/views/controls/image_view.h"
 #include "ui/views/controls/label.h"
 #include "ui/views/controls/scroll_view.h"
@@ -87,7 +88,10 @@ std::unique_ptr<views::Label> CreateExplanationText(
       message_id = IDS_AUTOFILL_LOCAL_CARD_MIGRATION_DIALOG_MESSAGE_OFFER;
       break;
     case LocalCardMigrationDialogState::kFinished:
-      message_id = IDS_AUTOFILL_LOCAL_CARD_MIGRATION_DIALOG_MESSAGE_DONE;
+      message_id =
+          card_list_size == 0
+              ? IDS_AUTOFILL_LOCAL_CARD_MIGRATION_DIALOG_MESSAGE_INVALID_CARD_REMOVED
+              : IDS_AUTOFILL_LOCAL_CARD_MIGRATION_DIALOG_MESSAGE_DONE;
       break;
     case LocalCardMigrationDialogState::kActionRequired:
       message_id = IDS_AUTOFILL_LOCAL_CARD_MIGRATION_DIALOG_MESSAGE_FIX;
@@ -102,13 +106,13 @@ std::unique_ptr<views::Label> CreateExplanationText(
 }
 
 // Create the scroll view of cards in |migratable_credit_cards|, and each
-// row in the scroll view is a MigratableCardView. |card_list_listener|
+// row in the scroll view is a MigratableCardView. |dialog_view|
 // will be notified whenever the checkbox or the trash can button
 // (if any) in any row is clicked. The content and the layout of the
 // scroll view depends on |should_show_checkbox|.
 std::unique_ptr<views::ScrollView> CreateCardList(
     const std::vector<MigratableCreditCard>& migratable_credit_cards,
-    views::ButtonListener* card_list_listener,
+    LocalCardMigrationDialogView* dialog_view,
     bool should_show_checkbox) {
   ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
   auto* card_list_view = new views::View();
@@ -123,9 +127,8 @@ std::unique_ptr<views::ScrollView> CreateCardList(
   card_list_view_layout->set_main_axis_alignment(
       views::BoxLayout::MAIN_AXIS_ALIGNMENT_START);
   for (size_t index = 0; index < migratable_credit_cards.size(); ++index) {
-    card_list_view->AddChildView(
-        new MigratableCardView(migratable_credit_cards[index],
-                               card_list_listener, should_show_checkbox));
+    card_list_view->AddChildView(new MigratableCardView(
+        migratable_credit_cards[index], dialog_view, should_show_checkbox));
   }
 
   auto card_list_scroll_view = std::make_unique<views::ScrollView>();
@@ -173,7 +176,7 @@ std::unique_ptr<views::View> CreateTip(const base::string16& tip_message) {
 // title, explanation text, card list, and the tip (if present).
 std::unique_ptr<views::View> CreateFeedbackContentView(
     LocalCardMigrationDialogController* controller,
-    views::ButtonListener* card_list_listener) {
+    LocalCardMigrationDialogView* dialog_view) {
   DCHECK(controller->GetViewState() != LocalCardMigrationDialogState::kOffered);
 
   auto feedback_view = std::make_unique<views::View>();
@@ -190,14 +193,18 @@ std::unique_ptr<views::View> CreateFeedbackContentView(
 
   feedback_view->AddChildView(
       CreateExplanationText(view_state, card_list_size).release());
-  feedback_view->AddChildView(
-      CreateCardList(card_list, card_list_listener, false).release());
-  // If there are no more than two cards in the finished dialog, show the tip.
-  constexpr int kShowTipMessageCardNumberLimit = 2;
-  if (view_state == LocalCardMigrationDialogState::kFinished &&
-      card_list_size <= kShowTipMessageCardNumberLimit) {
+
+  if (card_list_size > 0) {
     feedback_view->AddChildView(
-        CreateTip(controller->GetTipMessage()).release());
+        CreateCardList(card_list, dialog_view, false).release());
+
+    // If there are no more than two cards in the finished dialog, show the tip.
+    constexpr int kShowTipMessageCardNumberLimit = 2;
+    if (view_state == LocalCardMigrationDialogState::kFinished &&
+        card_list_size <= kShowTipMessageCardNumberLimit) {
+      feedback_view->AddChildView(
+          CreateTip(controller->GetTipMessage()).release());
+    }
   }
 
   return feedback_view;
@@ -214,7 +221,7 @@ class LocalCardMigrationOfferView : public views::View,
                                     public views::StyledLabelListener {
  public:
   LocalCardMigrationOfferView(LocalCardMigrationDialogController* controller,
-                              views::ButtonListener* card_list_listener)
+                              LocalCardMigrationDialogView* dialog_view)
       : controller_(controller) {
     ChromeLayoutProvider* provider = ChromeLayoutProvider::Get();
     SetLayoutManager(std::make_unique<views::BoxLayout>(
@@ -239,7 +246,7 @@ class LocalCardMigrationOfferView : public views::View,
             .release());
 
     std::unique_ptr<views::ScrollView> scroll_view =
-        CreateCardList(card_list, card_list_listener, card_list_size != 1);
+        CreateCardList(card_list, dialog_view, card_list_size != 1);
     card_list_view_ = scroll_view->contents();
     contents_container->AddChildView(scroll_view.release());
 
@@ -300,7 +307,7 @@ LocalCardMigrationDialogView::LocalCardMigrationDialogView(
 LocalCardMigrationDialogView::~LocalCardMigrationDialogView() {}
 
 void LocalCardMigrationDialogView::ShowDialog() {
-  Init();
+  ConstructView();
   constrained_window::CreateBrowserModalDialogViews(
       this, web_contents_->GetTopLevelNativeWindow())
       ->Show();
@@ -328,6 +335,14 @@ bool LocalCardMigrationDialogView::ShouldShowCloseButton() const {
   return false;
 }
 
+int LocalCardMigrationDialogView::GetDialogButtons() const {
+  // Don't show the "View cards" button if all cards are invalid.
+  if (controller_->AllCardsInvalid())
+    return ui::DIALOG_BUTTON_OK;
+
+  return ui::DIALOG_BUTTON_OK | ui::DIALOG_BUTTON_CANCEL;
+}
+
 base::string16 LocalCardMigrationDialogView::GetDialogButtonLabel(
     ui::DialogButton button) const {
   return button == ui::DIALOG_BUTTON_OK ? GetOkButtonLabel()
@@ -378,18 +393,23 @@ void LocalCardMigrationDialogView::WindowClosing() {
   }
 }
 
-// TODO(crbug/867194): Add button pressed logic for kDeleteCardButtonTag.
-void LocalCardMigrationDialogView::ButtonPressed(views::Button* sender,
-                                                 const ui::Event& event) {
-  // The button clicked is a checkbox. Enable/disable the save
-  // button if needed.
-  DCHECK_EQ(sender->GetClassName(), views::Checkbox::kViewClassName);
-  DialogModelChanged();
+void LocalCardMigrationDialogView::DeleteCard(const std::string& guid) {
+  controller_->DeleteCard(guid);
+  ConstructView();
+  Layout();
+  // Since the dialog does not have anchor view or arrow, cannot use
+  // SizeToContents() for now. TODO(crbug.com/867194): Try to fix the
+  // BubbleDialogDelegateView::GetBubbleBounds() when there is no anchor
+  // view or arrow.
+  GetWidget()->SetSize(GetWidget()->non_client_view()->GetPreferredSize());
 }
 
-void LocalCardMigrationDialogView::Init() {
-  if (has_children())
-    return;
+void LocalCardMigrationDialogView::ConstructView() {
+  DCHECK(controller_->GetViewState() !=
+             LocalCardMigrationDialogState::kOffered ||
+         !has_children());
+
+  RemoveAllChildViews(/*delete_children=*/true);
 
   SetLayoutManager(std::make_unique<views::BoxLayout>(
       views::BoxLayout::kVertical, gfx::Insets(),

diff --git a/chrome/browser/ui/views/autofill/local_card_migration_dialog_view.h b/chrome/browser/ui/views/autofill/local_card_migration_dialog_view.h
@@ -10,7 +10,6 @@
 #include "chrome/browser/ui/views/autofill/dialog_view_ids.h"
 #include "components/autofill/core/browser/ui/local_card_migration_dialog_controller.h"
 #include "ui/views/bubble/bubble_dialog_delegate_view.h"
-#include "ui/views/controls/button/button.h"
 #include "ui/views/view.h"
 #include "ui/views/window/dialog_delegate.h"
 
@@ -24,8 +23,7 @@ enum class LocalCardMigrationDialogState;
 class LocalCardMigrationOfferView;
 
 class LocalCardMigrationDialogView : public LocalCardMigrationDialog,
-                                     public views::BubbleDialogDelegateView,
-                                     public views::ButtonListener {
+                                     public views::BubbleDialogDelegateView {
  public:
   LocalCardMigrationDialogView(LocalCardMigrationDialogController* controller,
                                content::WebContents* web_contents);
@@ -39,17 +37,19 @@ class LocalCardMigrationDialogView : public LocalCardMigrationDialog,
   gfx::Size CalculatePreferredSize() const override;
   ui::ModalType GetModalType() const override;
   bool ShouldShowCloseButton() const override;
+  int GetDialogButtons() const override;
   base::string16 GetDialogButtonLabel(ui::DialogButton button) const override;
   bool IsDialogButtonEnabled(ui::DialogButton button) const override;
   bool Accept() override;
   bool Cancel() override;
-  void Init() override;
   void WindowClosing() override;
 
-  // views::ButtonListener
-  void ButtonPressed(views::Button* sender, const ui::Event& event) override;
+  // Called by MigratableCardView when the user clicks the trash can button.
+  // |guid| is the GUID of the credit card to be deleted.
+  void DeleteCard(const std::string& guid);
 
  private:
+  void ConstructView();
   base::string16 GetOkButtonLabel() const;
   base::string16 GetCancelButtonLabel() const;