[PM-16807] Update text and behavior for show id/card vault settings #12808
Conversation
|
New Issues
Fixed Issues
|
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## main #12808 +/- ##
=======================================
Coverage 34.13% 34.14%
=======================================
Files 2936 2936
Lines 90433 90433
Branches 16989 16989
=======================================
+ Hits 30869 30874 +5
+ Misses 57107 57100 -7
- Partials 2457 2459 +2 ☔ View full report in Codecov by Sentry. |
|
@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:
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? |
|
@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? |
|
@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! |
|
I also smoke-tested and it LGTM To put the changes another way, we're going from: to: 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). |
There was a problem hiding this comment.
Everything looks good. I just left a question for @djsmith85
| @@ -986,17 +986,17 @@ | |||
| "addLoginNotificationDescAlt": { | |||
| "message": "Ask to add an item if one isn't found in your vault. Applies to all logged in accounts." | |||
| }, | |||
| "showCardsInVaultView": { | |||
| "message": "Show cards as Autofill suggestions on Vault view" | |||
| "showCardsInVaultViewV2": { | |||
There was a problem hiding this comment.
@djsmith85 do we need new keys for this or can it just be renamed?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-16807
📔 Objective
⏰ Reminders before review
🦮 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