[Autofill Assistant] Fail consistently for empty values

Before this change, an empty value was causing a failure state
only if no other values existed. The empty value was skipped for
filling and later on (maybe) failed in the empty-check. If the
field was filled by outside forces (e.g. cached by website)
the skipped value would not cause a failure state.

This behaviour was causing inconsistent results.

After this change, an empty value will always cause a failure
state immediately and not attempt the filling.

Bug: b/175270472
Change-Id: I939e2069e237d960ebf3a826a799e5ab23ecc4c9
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2582323Reviewed-by: Mathias Carlen <mcarlen@chromium.org>
Reviewed-by: Marian Fechete <marianfe@google.com>
Commit-Queue: Sandro Maggi <sandromaggi@google.com>
Cr-Commit-Position: refs/heads/master@{#836061}

components/autofill_assistant/browser/BUILD.gn

@@ -316,6 +316,7 @@ source_set("unit_tests") {
     "actions/click_action_unittest.cc",
     "actions/collect_user_data_action_unittest.cc",
     "actions/configure_bottom_sheet_action_unittest.cc",
+    "actions/fallback_handler/required_field_unittest.cc",
     "actions/fallback_handler/required_fields_fallback_handler_unittest.cc",
     "actions/generate_password_for_form_field_action_unittest.cc",
     "actions/get_element_status_action_unittest.cc",

components/autofill_assistant/browser/actions/fallback_handler/required_field_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "components/autofill_assistant/browser/actions/fallback_handler/required_field.h"
+
+#include "components/autofill_assistant/browser/selector.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace autofill_assistant {
+namespace {
+
+class RequiredFieldTest : public testing::Test {
+ public:
+  void SetUp() override {}
+};
+
+TEST_F(RequiredFieldTest, ShouldFallbackForNotEmpty) {
+  RequiredField required_field;
+  required_field.status = RequiredField::NOT_EMPTY;
+  required_field.value_expression = "value";
+
+  EXPECT_FALSE(required_field.ShouldFallback(true));
+  EXPECT_FALSE(required_field.ShouldFallback(false));
+}
+
+TEST_F(RequiredFieldTest, ShouldFallbackForNotEmptyToBeCleared) {
+  RequiredField required_field;
+  required_field.status = RequiredField::NOT_EMPTY;
+  required_field.value_expression = std::string();
+
+  EXPECT_TRUE(required_field.ShouldFallback(true));
+  EXPECT_TRUE(required_field.ShouldFallback(false));
+}
+
+TEST_F(RequiredFieldTest, ShouldFallbackForEmpty) {
+  RequiredField required_field;
+  required_field.status = RequiredField::EMPTY;
+  required_field.value_expression = "value";
+
+  EXPECT_TRUE(required_field.ShouldFallback(true));
+  EXPECT_TRUE(required_field.ShouldFallback(false));
+}
+
+TEST_F(RequiredFieldTest, ShouldFallbackForNotEmptyForced) {
+  RequiredField required_field;
+  required_field.forced = true;
+  required_field.status = RequiredField::NOT_EMPTY;
+  required_field.value_expression = "value";
+
+  EXPECT_TRUE(required_field.ShouldFallback(true));
+  EXPECT_FALSE(required_field.ShouldFallback(false));
+}
+
+TEST_F(RequiredFieldTest, ShouldFallbackForEmptyWithClick) {
+  RequiredField required_field;
+  required_field.status = RequiredField::EMPTY;
+  required_field.fallback_click_element = Selector({"#element"});
+
+  EXPECT_TRUE(required_field.ShouldFallback(true));
+  EXPECT_FALSE(required_field.ShouldFallback(false));
+}
+
+}  // namespace
+}  // namespace autofill_assistant

components/autofill_assistant/browser/actions/fallback_handler/required_fields_fallback_handler.cc

@@ -179,6 +179,7 @@ void RequiredFieldsFallbackHandler::OnCheckRequiredFieldsDone(
   // If there are any fallbacks for the empty fields, set them, otherwise fail
   // immediately.
   bool has_fallbacks = false;
+  bool has_empty_value = false;
   for (const RequiredField& required_field : required_fields_) {
     if (!required_field.ShouldFallback(/* apply_fallback= */ true)) {
       continue;
@@ -194,9 +195,10 @@ void RequiredFieldsFallbackHandler::OnCheckRequiredFieldsDone(
       VLOG(3) << "Field has no fallback data: " << required_field.selector
               << " " << required_field.value_expression;
       FillStatusDetailsWithMissingFallbackData(required_field, &client_status_);
+      has_empty_value = true;
     }
   }
-  if (!has_fallbacks) {
+  if (!has_fallbacks || has_empty_value) {
     std::move(status_update_callback_)
         .Run(ClientStatus(AUTOFILL_INCOMPLETE), client_status_);
     return;
@@ -239,12 +241,7 @@ void RequiredFieldsFallbackHandler::SetFallbackFieldValuesSequentially(
 
   auto fallback_value = field_formatter::FormatString(
       required_field.value_expression, fallback_values_);
-  if (!fallback_value.has_value()) {
-    VLOG(3) << "No fallback for " << required_field.selector;
-    // If there is no fallback value, we skip this failed field.
-    SetFallbackFieldValuesSequentially(++required_fields_index);
-    return;
-  }
+  DCHECK(fallback_value.has_value());
 
   if (required_field.fallback_click_element.has_value()) {
     ClickType click_type = required_field.click_type;

components/autofill_assistant/browser/actions/fallback_handler/required_fields_fallback_handler_unittest.cc

@@ -90,8 +90,26 @@ TEST_F(RequiredFieldsFallbackHandlerTest,
 
 TEST_F(RequiredFieldsFallbackHandlerTest,
        AddsMissingOrEmptyFallbackValuesToError) {
-  ON_CALL(mock_web_controller_, OnGetFieldValue(_, _))
-      .WillByDefault(RunOnceCallback<1>(OkClientStatus(), ""));
+  // The checks should only run once (initially). There should not be a
+  // "non-empty" validation because it failed before that.
+  Selector card_name_selector({"#card_name"});
+  Selector card_number_selector({"#card_number"});
+  Selector card_network_selector({"#card_network"});
+  EXPECT_CALL(mock_web_controller_,
+              OnGetFieldValue(EqualsElement(test_util::MockFindElement(
+                                  mock_web_controller_, card_name_selector)),
+                              _))
+      .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()));
+  EXPECT_CALL(mock_web_controller_,
+              OnGetFieldValue(EqualsElement(test_util::MockFindElement(
+                                  mock_web_controller_, card_number_selector)),
+                              _))
+      .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()));
+  EXPECT_CALL(mock_web_controller_,
+              OnGetFieldValue(EqualsElement(test_util::MockFindElement(
+                                  mock_web_controller_, card_network_selector)),
+                              _))
+      .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()));
 
   std::vector<RequiredField> required_fields = {
       CreateRequiredField("${51}", {"#card_name"}),
@@ -104,7 +122,7 @@ TEST_F(RequiredFieldsFallbackHandlerTest,
        "John Doe"},
       {base::NumberToString(
            static_cast<int>(AutofillFormatProto::CREDIT_CARD_NETWORK)),
-       ""}};
+       std::string()}};
 
   RequiredFieldsFallbackHandler fallback_handler(
       required_fields, fallback_values, &mock_action_delegate_);
@@ -122,7 +140,7 @@ TEST_F(RequiredFieldsFallbackHandlerTest,
                           .details()
                           .autofill_error_info()
                           .autofill_field_error_size(),
-                      3);
+                      2);
             EXPECT_EQ(detail_status.value()
                           .details()
                           .autofill_error_info()
@@ -145,17 +163,6 @@ TEST_F(RequiredFieldsFallbackHandlerTest,
                             .autofill_error_info()
                             .autofill_field_error(1)
                             .no_fallback_value());
-            EXPECT_EQ(detail_status.value()
-                          .details()
-                          .autofill_error_info()
-                          .autofill_field_error(2)
-                          .value_expression(),
-                      "${51}");
-            EXPECT_TRUE(detail_status.value()
-                            .details()
-                            .autofill_error_info()
-                            .autofill_field_error(2)
-                            .empty_after_fallback());
           });
 
   fallback_handler.CheckAndFallbackRequiredFields(OkClientStatus(),
@@ -706,5 +713,58 @@ TEST_F(RequiredFieldsFallbackHandlerTest, SkipsForcedFieldCheckOnFirstRun) {
       }));
 }
 
+TEST_F(RequiredFieldsFallbackHandlerTest,
+       EmptyValueDoesNotFailForFieldNotNeedingToBeFilled) {
+  Selector card_name_selector({"#card_name"});
+  Selector card_number_selector({"#card_number"});
+  auto card_name_element =
+      test_util::MockFindElement(mock_web_controller_, card_name_selector, 2);
+  EXPECT_CALL(mock_web_controller_,
+              OnGetFieldValue(EqualsElement(card_name_element), _))
+      .WillOnce(RunOnceCallback<1>(OkClientStatus(), std::string()))
+      .WillOnce(RunOnceCallback<1>(OkClientStatus(), "value"));
+  auto card_number_element =
+      test_util::MockFindElement(mock_web_controller_, card_number_selector, 2);
+  EXPECT_CALL(mock_web_controller_,
+              OnGetFieldValue(EqualsElement(card_number_element), _))
+      .Times(2)
+      .WillRepeatedly(RunOnceCallback<1>(OkClientStatus(), "value"));
+  EXPECT_CALL(mock_action_delegate_,
+              SetValueAttribute(_,
+                                EqualsElement(test_util::MockFindElement(
+                                    mock_action_delegate_, card_name_selector)),
+                                _))
+      .WillOnce(RunOnceCallback<2>(OkClientStatus()));
+
+  std::vector<RequiredField> required_fields = {
+      CreateRequiredField("${51}", {"#card_name"}),
+      CreateRequiredField("${52}", {"#card_number"})};
+
+  std::map<std::string, std::string> fallback_values = {
+      {base::NumberToString(
+           static_cast<int>(autofill::ServerFieldType::CREDIT_CARD_NAME_FULL)),
+       "John Doe"}};
+
+  RequiredFieldsFallbackHandler fallback_handler(
+      required_fields, fallback_values, &mock_action_delegate_);
+
+  base::OnceCallback<void(const ClientStatus&,
+                          const base::Optional<ClientStatus>&)>
+      callback =
+          base::BindOnce([](const ClientStatus& status,
+                            const base::Optional<ClientStatus>& detail_status) {
+            EXPECT_EQ(status.proto_status(), ACTION_APPLIED);
+            ASSERT_TRUE(detail_status.has_value());
+            EXPECT_EQ(detail_status.value()
+                          .details()
+                          .autofill_error_info()
+                          .autofill_field_error_size(),
+                      0);
+          });
+
+  fallback_handler.CheckAndFallbackRequiredFields(OkClientStatus(),
+                                                  std::move(callback));
+}
+
 }  // namespace
 }  // namespace autofill_assistant