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.

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

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

Conversation

issammani
Copy link
Collaborator

@issammani issammani commented Nov 20, 2024

📜 Tickets

Jira ticket
Github issue

💡 Description

This PR:

  • Enables address edit by default on US and CA or if user is enrolled in an experiment that enables it.

⚠️ NOTE1: I am intentionally not removing the nimbus flag because we intend to rollout this feature to other locales soon.
⚠️ NOTE2: This needs to be backported to 133.

📝 Checklist

You have to check all boxes before merging

  • Filled in the above information (tickets numbers and description of your work)
  • Updated the PR name to follow our PR naming guidelines
  • Wrote unit tests and/or ensured the tests suite is passing
  • When working on UI, I checked and implemented accessibility (minimum Dynamic Text and VoiceOver)
  • If needed, I updated documentation / comments for complex code and public methods
  • If needed, added a backport comment (example @Mergifyio backport release/v120)

@issammani issammani requested a review from a team as a code owner November 20, 2024 13:29
@mobiletest-ci-bot
Copy link

Messages
📖 Project coverage: 33.05%
📖 Edited 1 files
📖 Created 0 files

Client.app: Coverage: 31.13

File Coverage
AddressListViewModel.swift 46.08% ⚠️

Generated by 🚫 Danger Swift against 8f82957

@issammani issammani changed the title FXIOS-10362 - Enable address autofill edit by default on US and CA Add FXIOS-10362 - Enable address autofill edit by default on US and CA Nov 20, 2024
Comment on lines +38 to +41
var isEditingFeatureEnabled: Bool {
AddressLocaleFeatureValidator.isValidRegion() ||
featureFlags.isFeatureEnabled(.addressAutofillEdit, checking: .buildOnly)
}
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 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants