Commit 179a6bfd authored by Elizabeth Popova's avatar Elizabeth Popova Committed by Commit Bot

[Android][Autofill] Make optional required fields check in AddressEditor

Prior to this change, it was impossible to save an Autofill profile
until every required field is filled. Now it depends on the use
case (purpose argument):
- if opened from Autofill Settings (purpose = AUTOFILL_SETTINGS), no
fields are marked as required and empty values are acceptable;
- if opened from Payment Request or Autofill Assistant
(purpose = PAYMENT_REQUEST), the behavior is the same as before: an
error message is shown when the required field is not filled.

Note, that phone number validation is still enabled for both cases.
However, an empty value is allowed only for the first case.

Change the test accordingly and add another test for phone number
validation. Also explicilty focus the first text field in the
testKeyboardShownOnDpadCenter test. Previously it was focused because it
was the first required field and after the change it didn't get focused
by default.

Change-Id: Ib476af34b2d755236cf88a29bcf3a2ba4977ccc5
Bug: 1149568
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2537670
Commit-Queue: Elizabeth Popova <lizapopova@google.com>
Reviewed-by: default avatarRouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: default avatarMatthias Körber <koerber@google.com>
Reviewed-by: default avatarChristoph Schwering <schwering@google.com>
Cr-Commit-Position: refs/heads/master@{#827904}
parent bdc94970
...@@ -52,11 +52,12 @@ public class AddressEditor ...@@ -52,11 +52,12 @@ public class AddressEditor
private final Handler mHandler = new Handler(); private final Handler mHandler = new Handler();
private final Map<Integer, EditorFieldModel> mAddressFields = new HashMap<>(); private final Map<Integer, EditorFieldModel> mAddressFields = new HashMap<>();
private final Set<CharSequence> mPhoneNumbers = new HashSet<>(); private final Set<CharSequence> mPhoneNumbers = new HashSet<>();
private final PhoneNumberUtil.CountryAwareFormatTextWatcher mPhoneFormatter;
private final CountryAwarePhoneNumberValidator mPhoneValidator;
@Purpose @Purpose
private final int mPurpose; private final int mPurpose;
private final boolean mCheckRequiredFields;
private final boolean mSaveToDisk; private final boolean mSaveToDisk;
private final PhoneNumberUtil.CountryAwareFormatTextWatcher mPhoneFormatter;
private final CountryAwarePhoneNumberValidator mPhoneValidator;
@Nullable @Nullable
private AutofillProfileBridge mAutofillProfileBridge; private AutofillProfileBridge mAutofillProfileBridge;
@Nullable @Nullable
...@@ -84,10 +85,11 @@ public class AddressEditor ...@@ -84,10 +85,11 @@ public class AddressEditor
* @param saveToDisk Whether to save changes to disk after editing. * @param saveToDisk Whether to save changes to disk after editing.
*/ */
public AddressEditor(@Purpose int purpose, boolean saveToDisk) { public AddressEditor(@Purpose int purpose, boolean saveToDisk) {
mPhoneFormatter = new PhoneNumberUtil.CountryAwareFormatTextWatcher();
mPhoneValidator = new CountryAwarePhoneNumberValidator();
mPurpose = purpose; mPurpose = purpose;
mCheckRequiredFields = mPurpose != Purpose.AUTOFILL_SETTINGS;
mSaveToDisk = saveToDisk; mSaveToDisk = saveToDisk;
mPhoneFormatter = new PhoneNumberUtil.CountryAwareFormatTextWatcher();
mPhoneValidator = new CountryAwarePhoneNumberValidator(!mCheckRequiredFields);
} }
/** /**
...@@ -155,7 +157,7 @@ public class AddressEditor ...@@ -155,7 +157,7 @@ public class AddressEditor
* ... <-- field order, presence, required, and labels depend on country. * ... <-- field order, presence, required, and labels depend on country.
* [ an address field ] / * [ an address field ] /
* [ an address field ] / * [ an address field ] /
* [ phone number field ] <----- phone is always present and required. * [ phone number field ] <----- phone is always present.
* [ email address field ] <----- only present if purpose is Purpose.AUTOFILL_SETTINGS. * [ email address field ] <----- only present if purpose is Purpose.AUTOFILL_SETTINGS.
*/ */
@Override @Override
...@@ -248,13 +250,16 @@ public class AddressEditor ...@@ -248,13 +250,16 @@ public class AddressEditor
EditorFieldModel.INPUT_TYPE_HINT_PERSON_NAME)); EditorFieldModel.INPUT_TYPE_HINT_PERSON_NAME));
} }
// Phone number is present for all countries.
// Phone number is present and required for all countries.
if (mPhoneField == null) { if (mPhoneField == null) {
String requiredErrorMessage = mCheckRequiredFields
? mContext.getString(
R.string.pref_edit_dialog_field_required_validation_message)
: null;
mPhoneField = EditorFieldModel.createTextInput(EditorFieldModel.INPUT_TYPE_HINT_PHONE, mPhoneField = EditorFieldModel.createTextInput(EditorFieldModel.INPUT_TYPE_HINT_PHONE,
mContext.getString(R.string.autofill_profile_editor_phone_number), mContext.getString(R.string.autofill_profile_editor_phone_number),
mPhoneNumbers, mPhoneFormatter, mPhoneValidator, null /* valueIconGenerator */, mPhoneNumbers, mPhoneFormatter, mPhoneValidator, null /* valueIconGenerator */,
mContext.getString(R.string.pref_edit_dialog_field_required_validation_message), requiredErrorMessage,
mContext.getString(R.string.payments_phone_invalid_validation_message), mContext.getString(R.string.payments_phone_invalid_validation_message),
null /* value */); null /* value */);
} }
...@@ -288,13 +293,19 @@ public class AddressEditor ...@@ -288,13 +293,19 @@ public class AddressEditor
cancelCallback.onResult(toEdit); cancelCallback.onResult(toEdit);
}); });
// If the user clicks [Done], save changes on disk, mark the address "complete," and send it // If the user clicks [Done], save changes on disk, mark the address "complete" if possible,
// back to the caller. // and send it back to the caller.
mEditor.setDoneCallback(() -> { mEditor.setDoneCallback(() -> {
mAdminAreasLoaded = true; mAdminAreasLoaded = true;
PersonalDataManager.getInstance().cancelPendingGetSubKeys(); PersonalDataManager.getInstance().cancelPendingGetSubKeys();
commitChanges(mProfile); commitChanges(mProfile);
address.completeAddress(mProfile); if (mCheckRequiredFields) {
address.completeAddress(mProfile);
} else {
// The address cannot be marked "complete" because it has not been checked
// for all required fields.
address.updateAddress(mProfile);
}
doneCallback.onResult(address); doneCallback.onResult(address);
}); });
...@@ -506,12 +517,12 @@ public class AddressEditor ...@@ -506,12 +517,12 @@ public class AddressEditor
// Libaddressinput formats do not always require the full name (RECIPIENT), but // Libaddressinput formats do not always require the full name (RECIPIENT), but
// PaymentRequest does. // PaymentRequest does.
if (component.isRequired || component.id == AddressField.RECIPIENT) { field.setRequiredErrorMessage(mCheckRequiredFields
field.setRequiredErrorMessage(mContext.getString( && (component.isRequired
R.string.pref_edit_dialog_field_required_validation_message)); || component.id == AddressField.RECIPIENT)
} else { ? mContext.getString(
field.setRequiredErrorMessage(null); R.string.pref_edit_dialog_field_required_validation_message)
} : null);
field.setCustomErrorMessage(getAddressError(component.id)); field.setCustomErrorMessage(getAddressError(component.id));
mEditor.addField(field); mEditor.addField(field);
...@@ -526,6 +537,16 @@ public class AddressEditor ...@@ -526,6 +537,16 @@ public class AddressEditor
private static class CountryAwarePhoneNumberValidator implements EditorFieldValidator { private static class CountryAwarePhoneNumberValidator implements EditorFieldValidator {
@Nullable @Nullable
private String mCountryCode; private String mCountryCode;
private boolean mAllowEmptyValue;
/**
* Builds a country based phone number validator.
*
* @param allowEmptyValue whether null or 0-length string is considered valid.
*/
CountryAwarePhoneNumberValidator(boolean allowEmptyValue) {
mAllowEmptyValue = allowEmptyValue;
}
/** /**
* Sets the country code used to validate the phone number. * Sets the country code used to validate the phone number.
...@@ -542,8 +563,9 @@ public class AddressEditor ...@@ -542,8 +563,9 @@ public class AddressEditor
// invalid, crbug.com/736387. // invalid, crbug.com/736387.
// Note that isPossibleNumber is used since the metadata in libphonenumber has to be // Note that isPossibleNumber is used since the metadata in libphonenumber has to be
// updated frequently (daily) to do more strict validation. // updated frequently (daily) to do more strict validation.
return value != null return TextUtils.isEmpty(value)
&& PhoneNumberUtil.isPossibleNumber(value.toString(), mCountryCode); ? mAllowEmptyValue
: PhoneNumberUtil.isPossibleNumber(value.toString(), mCountryCode);
} }
@Override @Override
......
...@@ -93,12 +93,25 @@ public class AutofillAddress extends EditableOption { ...@@ -93,12 +93,25 @@ public class AutofillAddress extends EditableOption {
} }
/** /**
* Updates the address and marks it "complete." Called after the user has edited this address. * Updates the address and marks it "complete".
* Called after the user has edited this address.
* Updates the identifier and labels. * Updates the identifier and labels.
* *
* @param profile The new profile to use. * @param profile The new profile to use.
*/ */
public void completeAddress(AutofillProfile profile) { public void completeAddress(AutofillProfile profile) {
updateAddress(profile);
assert mIsComplete;
}
/**
* Updates the address and its completeness if needed.
* Called after the user has edited this address.
* Updates the identifier and labels.
*
* @param profile The new profile to use.
*/
public void updateAddress(AutofillProfile profile) {
// Since the profile changed, our cached labels are now out of date. Set them to null so the // Since the profile changed, our cached labels are now out of date. Set them to null so the
// labels are recomputed next time they are needed. // labels are recomputed next time they are needed.
mShippingLabelWithCountry = null; mShippingLabelWithCountry = null;
...@@ -109,7 +122,6 @@ public class AutofillAddress extends EditableOption { ...@@ -109,7 +122,6 @@ public class AutofillAddress extends EditableOption {
updateIdentifierAndLabels(mProfile.getGUID(), mProfile.getFullName(), mProfile.getLabel(), updateIdentifierAndLabels(mProfile.getGUID(), mProfile.getFullName(), mProfile.getLabel(),
mProfile.getPhoneNumber()); mProfile.getPhoneNumber());
checkAndUpdateAddressCompleteness(); checkAndUpdateAddressCompleteness();
assert mIsComplete;
} }
/** /**
......
...@@ -112,9 +112,39 @@ public class AutofillProfilesFragmentTest { ...@@ -112,9 +112,39 @@ public class AutofillProfilesFragmentTest {
AutofillProfilesFragment.PREF_NEW_PROFILE); AutofillProfilesFragment.PREF_NEW_PROFILE);
Assert.assertNotNull(addProfile); Assert.assertNotNull(addProfile);
// Try to add an incomplete profile. // Add an incomplete profile.
updatePreferencesAndWait(autofillProfileFragment, addProfile, new String[] {"Mike Doe"}, updatePreferencesAndWait(autofillProfileFragment, addProfile, new String[] {"Mike Doe"},
R.id.editor_dialog_done_button, true); R.id.editor_dialog_done_button, false);
// Incomplete profile should still be added.
Assert.assertEquals(7 /* One toggle + one add button + five profiles. */,
autofillProfileFragment.getPreferenceScreen().getPreferenceCount());
AutofillProfileEditorPreference addedProfile =
(AutofillProfileEditorPreference) fragment.findPreference("Mike Doe");
Assert.assertNotNull(addedProfile);
activity.finish();
}
@Test
@MediumTest
@Feature({"Preferences"})
public void testAddProfileWithInvalidPhone() throws Exception {
SettingsActivity activity = mSettingsActivityTestRule.startSettingsActivity();
AutofillProfilesFragment autofillProfileFragment =
(AutofillProfilesFragment) activity.getMainFragment();
// Check the preferences on the initial screen.
Assert.assertEquals(6 /* One toggle + one add button + four profiles. */,
autofillProfileFragment.getPreferenceScreen().getPreferenceCount());
PreferenceFragmentCompat fragment = (PreferenceFragmentCompat) activity.getMainFragment();
AutofillProfileEditorPreference addProfile =
(AutofillProfileEditorPreference) fragment.findPreference(
AutofillProfilesFragment.PREF_NEW_PROFILE);
Assert.assertNotNull(addProfile);
// Try to add a profile with invalid phone.
updatePreferencesAndWait(autofillProfileFragment, addProfile,
new String[] {"", "", "", "", "", "", "123"}, R.id.editor_dialog_done_button, true);
activity.finish(); activity.finish();
} }
...@@ -254,10 +284,12 @@ public class AutofillProfilesFragmentTest { ...@@ -254,10 +284,12 @@ public class AutofillProfilesFragmentTest {
// The keyboard is shown as soon as AutofillProfileEditorPreference comes into view. // The keyboard is shown as soon as AutofillProfileEditorPreference comes into view.
waitForKeyboardStatus(true, activity); waitForKeyboardStatus(true, activity);
final List<EditText> fields =
fragment.getEditorDialogForTest().getEditableTextFieldsForTest();
// Ensure the first text field is focused.
TestThreadUtils.runOnUiThreadBlocking(() -> { fields.get(0).requestFocus(); });
// Hide the keyboard. // Hide the keyboard.
TestThreadUtils.runOnUiThreadBlocking(() -> { TestThreadUtils.runOnUiThreadBlocking(() -> {
List<EditText> fields =
fragment.getEditorDialogForTest().getEditableTextFieldsForTest();
KeyboardVisibilityDelegate.getInstance().hideKeyboard(fields.get(0)); KeyboardVisibilityDelegate.getInstance().hideKeyboard(fields.get(0));
}); });
// Check that the keyboard is hidden. // Check that the keyboard is hidden.
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment