[Autofill Assistant] Small payment request bugfixes and cleanup.

This fixes a bug where the client would not correctly end a PR action for some invalid protos. Note: this is likely *not* b/140008909, but I can't yet confirm.

This also fixes a bug where the client should fail the PR if a login choice was requested but no options are available (e.g., for websites which do not offer guest checkout). In practice, this is unlikely to happen, but the case should be handled regardless.

Bug: 806868
Change-Id: I68174fa84e72fbfe209272cc62d6461970762fe5
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1771604
Commit-Queue: Clemens Arbesser <arbesser@google.com>
Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Cr-Commit-Position: refs/heads/master@{#691097}

components/autofill_assistant/browser/actions/get_payment_information_action.cc

@@ -18,6 +18,7 @@
 #include "components/autofill/core/browser/geo/address_i18n.h"
 #include "components/autofill_assistant/browser/actions/action_delegate.h"
 #include "components/autofill_assistant/browser/client_memory.h"
+#include "components/autofill_assistant/browser/client_status.h"
 #include "components/autofill_assistant/browser/metrics.h"
 #include "components/autofill_assistant/browser/service.pb.h"
 #include "components/autofill_assistant/browser/website_login_fetcher_impl.h"
@@ -31,17 +32,20 @@
 namespace autofill_assistant {
 
 GetPaymentInformationAction::LoginDetails::LoginDetails(
-    bool _hide_if_no_other_choice,
+    bool _choose_automatically_if_no_other_options,
     const std::string& _payload,
     const WebsiteLoginFetcher::Login& _login)
-    : hide_if_no_other_choice(_hide_if_no_other_choice),
+    : choose_automatically_if_no_other_options(
+          _choose_automatically_if_no_other_options),
       payload(_payload),
       login(_login) {}
 
 GetPaymentInformationAction::LoginDetails::LoginDetails(
-    bool _hide_if_no_other_choice,
+    bool _choose_automatically_if_no_other_options,
     const std::string& _payload)
-    : hide_if_no_other_choice(_hide_if_no_other_choice), payload(_payload) {}
+    : choose_automatically_if_no_other_options(
+          _choose_automatically_if_no_other_options),
+      payload(_payload) {}
 
 GetPaymentInformationAction::LoginDetails::~LoginDetails() = default;
 
@@ -69,9 +73,7 @@ void GetPaymentInformationAction::InternalProcessAction(
   callback_ = std::move(callback);
   auto payment_options = CreateOptionsFromProto();
   if (!payment_options) {
-    UpdateProcessedAction(INVALID_ACTION);
-    action_successful_ = false;
-    std::move(callback).Run(std::move(processed_action_proto_));
+    EndAction(ClientStatus(INVALID_ACTION));
     return;
   }
 
@@ -109,6 +111,12 @@ void GetPaymentInformationAction::InternalProcessAction(
   }
 }
 
+void GetPaymentInformationAction::EndAction(const ClientStatus& status) {
+  action_successful_ = status.ok();
+  UpdateProcessedAction(status);
+  std::move(callback_).Run(std::move(processed_action_proto_));
+}
+
 void GetPaymentInformationAction::OnGetLogins(
     const LoginDetailsProto::LoginOptionProto& login_option,
     std::unique_ptr<PaymentRequestOptions> payment_options,
@@ -120,8 +128,9 @@ void GetPaymentInformationAction::OnGetLogins(
     payment_options->login_choices.emplace_back(std::move(choice));
     login_details_map_.emplace(
         choice.identifier,
-        std::make_unique<LoginDetails>(login_option.hide_if_no_other_options(),
-                                       login_option.payload(), login));
+        std::make_unique<LoginDetails>(
+            login_option.choose_automatically_if_no_other_options(),
+            login_option.payload(), login));
   }
   ShowToUser(std::move(payment_options));
 }
@@ -143,8 +152,15 @@ void GetPaymentInformationAction::ShowToUser(
       break;
   }
 
+  if (payment_options->request_login_choice &&
+      payment_options->login_choices.empty()) {
+    EndAction(ClientStatus(PAYMENT_REQUEST_ERROR));
+    return;
+  }
+
   // Special case: if the only available login option has
-  // |hide_if_no_other_options=true|, the section will not be shown.
+  // |choose_automatically_if_no_other_options=true|, the section will not be
+  // shown.
   bool only_login_requested =
       payment_options->request_login_choice &&
       !payment_options->request_payer_name &&
@@ -156,7 +172,7 @@ void GetPaymentInformationAction::ShowToUser(
 
   if (payment_options->login_choices.size() == 1 &&
       login_details_map_.at(payment_options->login_choices.at(0).identifier)
-          ->hide_if_no_other_choice) {
+          ->choose_automatically_if_no_other_options) {
     payment_options->request_login_choice = false;
     payment_information->login_choice_identifier.assign(
         payment_options->login_choices[0].identifier);
@@ -269,30 +285,25 @@ void GetPaymentInformationAction::OnGetPaymentInformation(
         payment_information->payer_email);
   }
 
-  UpdateProcessedAction(succeed ? ACTION_APPLIED : PAYMENT_REQUEST_ERROR);
-  action_successful_ = succeed;
-  std::move(callback_).Run(std::move(processed_action_proto_));
+  EndAction(succeed ? ClientStatus(ACTION_APPLIED)
+                    : ClientStatus(PAYMENT_REQUEST_ERROR));
 }
 
 void GetPaymentInformationAction::OnAdditionalActionTriggered(int index) {
   if (!callback_)
     return;
 
-  UpdateProcessedAction(ACTION_APPLIED);
   processed_action_proto_->mutable_payment_details()
       ->set_additional_action_index(index);
-  action_successful_ = true;
-  std::move(callback_).Run(std::move(processed_action_proto_));
+  EndAction(ClientStatus(ACTION_APPLIED));
 }
 
 void GetPaymentInformationAction::OnTermsAndConditionsLinkClicked(int link) {
   if (!callback_)
     return;
 
-  UpdateProcessedAction(ACTION_APPLIED);
   processed_action_proto_->mutable_payment_details()->set_terms_link(link);
-  action_successful_ = true;
-  std::move(callback_).Run(std::move(processed_action_proto_));
+  EndAction(ClientStatus(ACTION_APPLIED));
 }
 
 std::unique_ptr<PaymentRequestOptions>
@@ -342,10 +353,11 @@ GetPaymentInformationAction::CreateOptionsFromProto() {
                 ? login_option.preselection_priority()
                 : -1};
         payment_options->login_choices.emplace_back(std::move(choice));
-        login_details_map_.emplace(choice.identifier,
-                                   std::make_unique<LoginDetails>(
-                                       login_option.hide_if_no_other_options(),
-                                       login_option.payload()));
+        login_details_map_.emplace(
+            choice.identifier,
+            std::make_unique<LoginDetails>(
+                login_option.choose_automatically_if_no_other_options(),
+                login_option.payload()));
         break;
       }
       case LoginDetailsProto::LoginOptionProto::kPasswordManager: {

components/autofill_assistant/browser/actions/get_payment_information_action.h

@@ -35,18 +35,20 @@ class GetPaymentInformationAction
 
  private:
   struct LoginDetails {
-    LoginDetails(bool hide_if_no_other_choice, const std::string& payload);
-    LoginDetails(bool hide_if_no_other_choice,
+    LoginDetails(bool choose_automatically_if_no_other_options,
+                 const std::string& payload);
+    LoginDetails(bool choose_automatically_if_no_other_options,
                  const std::string& payload,
                  const WebsiteLoginFetcher::Login& login);
     ~LoginDetails();
-    bool hide_if_no_other_choice;
+    bool choose_automatically_if_no_other_options;
     std::string payload;
     // Only for Chrome PWM login details.
     base::Optional<WebsiteLoginFetcher::Login> login;
   };
 
   void InternalProcessAction(ProcessActionCallback callback) override;
+  void EndAction(const ClientStatus& status);
 
   void OnGetPaymentInformation(
       const GetPaymentInformationProto& get_payment_information,

components/autofill_assistant/browser/actions/get_payment_information_action_unittest.cc

@@ -126,7 +126,7 @@ TEST_F(GetPaymentInformationActionTest, SelectLogin) {
   action.ProcessAction(callback_.Get());
 }
 
-TEST_F(GetPaymentInformationActionTest, LoginChoiceHideIfNoOtherOptions) {
+TEST_F(GetPaymentInformationActionTest, LoginChoiceAutomaticIfNoOtherOptions) {
   ActionProto action_proto;
   auto* payment_information = action_proto.mutable_get_payment_information();
   payment_information->set_request_terms_and_conditions(false);
@@ -134,7 +134,7 @@ TEST_F(GetPaymentInformationActionTest, LoginChoiceHideIfNoOtherOptions) {
   auto* guest_login_option = login_details->add_login_options();
   guest_login_option->mutable_custom()->set_label("Guest Checkout");
   guest_login_option->set_payload("guest");
-  guest_login_option->set_hide_if_no_other_options(true);
+  guest_login_option->set_choose_automatically_if_no_other_options(true);
   auto* password_login_option = login_details->add_login_options();
   password_login_option->mutable_password_manager();
   password_login_option->set_payload("password_manager");
@@ -154,6 +154,24 @@ TEST_F(GetPaymentInformationActionTest, LoginChoiceHideIfNoOtherOptions) {
   action.ProcessAction(callback_.Get());
 }
 
+TEST_F(GetPaymentInformationActionTest, SelectLoginFailsIfNoOptionAvailable) {
+  ActionProto action_proto;
+  auto* payment_information = action_proto.mutable_get_payment_information();
+  auto* login_details = payment_information->mutable_login_details();
+  auto* login_option = login_details->add_login_options();
+  login_option->mutable_password_manager();
+  login_option->set_payload("password_manager");
+
+  ON_CALL(mock_website_login_fetcher_, OnGetLoginsForUrl(_, _))
+      .WillByDefault(
+          RunOnceCallback<1>(std::vector<WebsiteLoginFetcher::Login>{}));
+
+  EXPECT_CALL(callback_, Run(Pointee(Property(&ProcessedActionProto::status,
+                                              PAYMENT_REQUEST_ERROR))));
+  GetPaymentInformationAction action(&mock_action_delegate_, action_proto);
+  action.ProcessAction(callback_.Get());
+}
+
 TEST_F(GetPaymentInformationActionTest, SelectContactDetails) {
   ActionProto action_proto;
   auto* payment_information_proto =
@@ -236,5 +254,19 @@ TEST_F(GetPaymentInformationActionTest, SelectPaymentMethod) {
   EXPECT_THAT(client_memory_.selected_card()->Compare(credit_card), Eq(0));
 }
 
+TEST_F(GetPaymentInformationActionTest,
+       MandatoryPostalCodeWithoutErrorMessageFails) {
+  ActionProto action_proto;
+  action_proto.mutable_get_payment_information()->set_ask_for_payment(true);
+  action_proto.mutable_get_payment_information()
+      ->set_require_billing_postal_code(true);
+
+  EXPECT_CALL(
+      callback_,
+      Run(Pointee(Property(&ProcessedActionProto::status, INVALID_ACTION))));
+  GetPaymentInformationAction action(&mock_action_delegate_, action_proto);
+  action.ProcessAction(callback_.Get());
+}
+
 }  // namespace
 }  // namespace autofill_assistant

components/autofill_assistant/browser/service.proto

@@ -1146,9 +1146,9 @@ message LoginDetailsProto {
     // If the option was chosen, this payload will be returned to the server.
     optional bytes payload = 1;
 
-    // Whether the UI should be hidden if this login option is the only
-    // available choice (i.e., this option will be implicitly selected).
-    optional bool hide_if_no_other_options = 2;
+    // Whether the UI should automatically choose this login option if it is the
+    // only available choice.
+    optional bool choose_automatically_if_no_other_options = 2;
 
     // Determines the priority with which to pre-select this login choice.
     // The lower the value, the higher the priority.