PM-16153: Draw new login action card #1238
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1238 +/- ##
=======================================
Coverage 89.57% 89.57%
=======================================
Files 709 709
Lines 45082 45125 +43
=======================================
+ Hits 40382 40421 +39
- Misses 4700 4704 +4 ☔ View full report in Codecov by Sentry. |
| let isLearnNewLoginCardShown = await services.stateService.getLearnNewLoginActionCardShown() | ||
| let accountCount = try? await services.stateService.getAccounts().count | ||
| let isVaultEmpty = try? await services.vaultRepository.isVaultEmpty() | ||
| let shouldShowLearnNewLoginActionCard = !isLearnNewLoginCardShown && isVaultEmpty == true && accountCount == 1 |
There was a problem hiding this comment.
we want to show this action card for new users only, not returning users, I came up with this checks, if you have better suggestions I am all open for it, even for moving this check to a service, tried to put this in StateService, but can't do it because of CipherService rely on StateService.
|
No New Or Fixed Issues Found |
ezimet-livefront
requested review from
matt-livefront,
phil-livefront,
brant-livefront and
a team
as code owners
BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemView.swift
Outdated
Show resolved
Hide resolved
BitwardenShared/Core/Platform/Models/Enum/LearnNewLoginActionCardStatus.swift
Outdated
Show resolved
Hide resolved
| @@ -608,6 +608,7 @@ class DefaultAuthService: AuthService { // swiftlint:disable:this type_body_leng | |||
| try await stateService.setAccountSetupImportLogins(.incomplete) | |||
| } | |||
| try await stateService.setAccountSetupVaultUnlock(.incomplete) | |||
| await stateService.setLearnNewLoginActionCardStatus(.eligible) | |||
There was a problem hiding this comment.
❓ Should this action card appear only appear for the first new account created on a device or any new account created? If I've already seen/completed the walkthrough and I add a new account, should this overwrite the existing status?
There was a problem hiding this comment.
Additionally, this is only taking into consideration login in with master password. The user could login with other means like SSO, TDE or Device.
There was a problem hiding this comment.
I think we only want to set this when creating a new account as opposed to logging into an existing account, which is why it was done here. I suppose you could use SSO + JIT to create a new account though? Do we know in the app though if signing into SSO created a new account or does that happen seamlessly in the backend?
There was a problem hiding this comment.
❓ Should this action card appear only appear for the first new account created on a device or any new account created? If I've already seen/completed the walkthrough and I add a new account, should this overwrite the existing status?
I think it should not overwrite the existing status.
There was a problem hiding this comment.
🤔 From what I can see on the Jira ticket:
Given I am a first-time user creating a new login item
I understand it as either as I've just created an account on mobile, or I've created it elsewhere and I'm logging in the mobile app and want to start adding logins. Could we ask someone to confirm the scope of this?
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemProcessor.swift
Outdated
Show resolved
Hide resolved
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemState.swift
Outdated
Show resolved
Hide resolved
BitwardenShared/UI/Platform/Application/Support/Localizations/en.lproj/Localizable.strings
Outdated
Show resolved
Hide resolved
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemProcessorTests.swift
Show resolved
Hide resolved
BitwardenShared/UI/Vault/VaultItem/AddEditItem/AddEditItemState.swift
Outdated
Show resolved
Hide resolved
I added this check. |
| if await services.configService.getFeatureFlag(.nativeCreateAccountFlow), | ||
| appExtensionDelegate == nil { | ||
| self.state.isLearnNewLoginActionCardEligible = await services.stateService | ||
| .getLearnNewLoginActionCardStatus() == .incomplete | ||
| } |
There was a problem hiding this comment.
🤔 Shouldn't this be done in perform -> appeared? Additionally the if !state.configuration.isAdding is not meant to be done inside the Task. Can't say for sure that it will match the same behavior as before so I wouldn't change that part.
There was a problem hiding this comment.
if we do it in appeared, it would slide in the action card after the view has appeared.
as far as configuration.isAdding I don't see why it will not match the same behavior, the original code was like this:
if !state.configuration.isAdding {
Task {
await self.services.rehydrationHelper.addRehydratableTarget(self)
}
}
There was a problem hiding this comment.
Have you tested doing it on appear? I think that's what we're doing on other views that show the action card and I don't recall it being an issue.
🎟️ Tracking
PM-16153
📔 Objective
📸 Screenshots
⏰ 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