[PM-16807] Update text and behavior for show id/card vault settings

[evan-livefront]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-16807

📔 Objective

• To change the default setting for IDs and Cards in vault suggestions to be turned off
• For ID/Card to added to autofill suggestions when a relevant form is on the page
• For ID/Card to always show when vault setting is enabled

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – 8a5b405f-6060-48cf-9fb1-3d22563f712e

Great job, no security vulnerabilities found in this Pull Request

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 35.19%. Comparing base (c1ebe95) to head (08ad8a5).
Report is 19 commits behind head on main.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #12808      +/-   ##
==========================================
- Coverage   35.20%   35.19%   -0.01%     
==========================================
  Files        2992     2992              
  Lines       90675    90675              
  Branches    16964    16964              
==========================================
- Hits        31918    31909       -9     
- Misses      56293    56300       +7     
- Partials     2464     2466       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[danielleflinn]

@evan-livefront From a design perspective the local build looks good.

One thing that I was hoping we could do–which may not have been clear in the issue description–is migrate existing users using the current settings to this new setting in the following manner:

• If users have the current settings "on" and are seeing cards/ids appear when we detect them to be relevant
  • then the new setting is "on" for this user and they see cards/ids always appear in Autofill Suggestions

The build from this PR doesn't seem to be working this way, however that could be a limitation of my testing set up. If this isn't included in the implementation, is it possible to do while keeping the default setting for new installs "off"?

cc: @jprusik

[evan-livefront]

@evan-livefront From a design perspective the local build looks good.

One thing that I was hoping we could do–which may not have been clear in the issue description–is migrate existing users using the current settings to this new setting in the following manner:

• If users have the current settings "on" and are seeing cards/ids appear when we detect them to be relevant

  • then the new setting is "on" for this user and they see cards/ids always appear in Autofill Suggestions

The build from this PR doesn't seem to be working this way, however that could be a limitation of my testing set up. If this isn't included in the implementation, is it possible to do while keeping the default setting for new installs "off"?

cc: @jprusik

So unless i'm mistaken, this setting should remain on if they currently have it on. Is that different from what you are describing here?

[danielleflinn]

@evan-livefront yes, if it is "on" today it should remain "on"; but I wasn't seeing that behavior when testing locally so I just wanted to confirm that is how you implemented it in the code

[evan-livefront]

@evan-livefront yes, if it is "on" today it should remain "on"; but I wasn't seeing that behavior when testing locally so I just wanted to confirm that is how you implemented it in the code

I'll look into that. It should maintain the setting they currently have and that what I saw when I tested but maybe I was doing something differently. Were you reinstalling the extension from scratch?

[danielleflinn]

@evan-livefront sorry for the back and forth on this-it is working as expected now though, Jon help me figure out what was up with my local testing process. Thanks for your patience!

[jprusik]

I also smoke-tested and it LGTM

To put the changes another way, we're going from:
"Never show cards or identities in suggestions if the relevant option is not enabled"
"Sometimes show cards or identities in suggestions (when relevant field is detected on the page) if the relevant option is enabled"

to:
"Sometimes show cards or identities in suggestions (when relevant field is detected on the page) if the relevant option is not enabled"
"Always show cards or identities in suggestions if the relevant option is enabled"

Additionally, the default is for each settings input to be off, though if the user previously selected it, it will remain selected (and the user will get the updated/changed experience for the setting).

[gbubemismith]

Everything looks good. I just left a question for @djsmith85

[gbubemismith]

@djsmith85 do we need new keys for this or can it just be renamed?

[evan-livefront]

The messages are immutable so a new key is required.

@jprusik did mention that once the old one has been removed it could reused again though so I don't know if it's possible (or desired) to change these, let the change propagate, and then change it back to the original? I had talked with him about this a bit and settled on this current versioning and deleting the old, unused key.

[jprusik]

Yeah, unfortunately we cannot simply update the copy for a given key, as that will mess with our translation automations at this time. We can safely delete keys (automation will remove corresponding translations on merge), or add new keys.

This change is effectively a removal of the old key and addition of a new one.

once the old one has been removed it could reused again

Yes, though I don't think it's worthwhile to bother with for now, and am not eager to test the robustness of the current automations around this. V2 is a good signpost of what has happened here imo.

[danielleflinn]

@evan-livefront Sorry about this last minute change, but we have decided to keep the current default state where these settings are “on”. Can we update the PR to reflect this?