Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add FXIOS-10362 - Enable address autofill edit by default on US and CA #23256

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+4 −1
Open

Add FXIOS-10362 - Enable address autofill edit by default on US and CA #23256

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion firefox-ios/Client/Frontend/Autofill/Address/AddressListViewModel.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,10 @@ final class AddressListViewModel: ObservableObject, FeatureFlaggable {

private let logger: Logger

var isEditingFeatureEnabled: Bool { featureFlags.isFeatureEnabled(.addressAutofillEdit, checking: .buildOnly) }
var isEditingFeatureEnabled: Bool {
AddressLocaleFeatureValidator.isValidRegion() ||
featureFlags.isFeatureEnabled(.addressAutofillEdit, checking: .buildOnly)
}
Comment on lines +38 to +41
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@issammani won't this enable for all even if the reason is valid?

I am not sure if we want the logic to first check feature enabled then validate via region

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I am missing something featureFlags.isFeatureEnabled(.addressAutofillEdit, checking: .buildOnly) should only be true if the user is enrolled in the experiment ? My reasoning to leave it there was because we want to rollout to germany and france at some point so we will need a nimbus flag again. But I can remove it from this check and keep the flag in the nimbus manifest for later use. What do you think ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I understand why we need the || with your original change. Please change it back to the way it was before :)


var addressSelectionCallback: ((UnencryptedAddressFields) -> Void)?
var saveAction: ((@escaping (UpdatableAddressFields) -> Void) -> Void)?
Expand Down