[PM-12695] Add hidden field changes to password history #1012
Conversation
|
No New Or Fixed Issues Found |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1012 +/- ##
==========================================
+ Coverage 89.40% 89.47% +0.07%
==========================================
Files 674 675 +1
Lines 42445 42494 +49
==========================================
+ Hits 37948 38022 +74
+ Misses 4497 4472 -25 ☔ View full report in Codecov by Sentry. |
| private func mapFields(_ addEditState: AddEditItemState) -> [FieldView]? { | ||
| addEditState.customFieldsState.customFields.isEmpty ? | ||
| nil : addEditState.customFieldsState.customFields.map { customField in | ||
| FieldView( | ||
| name: customField.name, | ||
| value: customField.value, | ||
| type: .init(fieldType: customField.type), | ||
| linkedId: customField.linkedIdType?.rawValue | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎨 Given that you're modifying this, IMO this goes beyond the responsibilities of CipherView+Update. If we could be more specific on the places where we transform a state into a view it'd be better for maintainability and reusability.
So maybe we could add a FieldView+Extensions file with a init(state: CustomFieldState) which creates the FieldView based on that state so then you can use it here more directly. Additionally, I think it's easier to read with a guard isEmpty instead of the ternary operator.
Moreover, I think we don't need the whole AddEditItemState just the custom fields state array to perform the mapping and the name of the function should be more explicit.
/// Maps the array of `CustomFieldState` into the an array of `FieldView`.
private func mapToCustomFieldViews(from customFields: [CustomFieldState]) -> [FieldView]? {
guard !customFields.isEmpty else {
return nil
}
return customFields.map { FieldView(state: $0) }
}There was a problem hiding this comment.
Yup it makes sense, I'll create the extension for the mapping. Jumping between Android and iOS, I always forget about the Swift guards 🥲
| var newHistory = passwordHistory | ||
| if let oldFields { |
There was a problem hiding this comment.
🎨 You can use a guard here to simplify nesting:
guard let oldFields else {
return passwordHistory
}
BitwardenShared/UI/Vault/VaultItem/ViewItem/ViewLoginItem/Extensions/CipherView+Update.swift
Show resolved
Hide resolved
| if newHistory == nil { | ||
| newHistory = [passHistoryHiddenField] | ||
| } else if !passwordHistory!.contains(passHistoryHiddenField) { | ||
| newHistory!.append(passHistoryHiddenField) | ||
| } |
There was a problem hiding this comment.
passwordHistory is nil this will crash. We almost never use !, it's just better to use ?, if let or guard let to avoid unexpected behaviors/crashes even if we are 100% certain it won't be nil and if that's the case it should be reviewed whether the variable should be optional.
❓ I think it's impossible that the !passwordHistory!.contains(passHistoryHiddenField) is false, I mean passwordHistory won't ever contain the new passHistoryHiddenField because the combination of password + lastUsedDate won't ever be duplicated unless there's a bug with this part or somehow we get duplicated custom fields because of a bug. So I think you could remove the if contains part.
There was a problem hiding this comment.
Sorry this was code that was left behind from one of the iterations I made and the names of the variables tricked my eyes. Thanks for noticing it 👍
| // MARK: - FieldView+Update | ||
|
|
||
| extension BitwardenSdk.FieldView { | ||
| /// initializes a new LoginView with updated properties |
There was a problem hiding this comment.
⛏️
| /// initializes a new LoginView with updated properties | |
| /// initializes a new FieldView with updated properties |
| var newHistory = passwordHistory | ||
| oldFields.filter { field in | ||
| FieldType(fieldType: field.type) == FieldType.hidden && | ||
| !field.name.isEmptyOrNil && | ||
| !field.value.isEmptyOrNil && | ||
| !(newFields?.contains(field) ?? false) | ||
| }.forEach { hiddenField in | ||
| guard !hiddenField.name.isEmptyOrNil else { | ||
| return | ||
| } | ||
|
|
||
| let passHistoryHiddenField = PasswordHistoryView( | ||
| password: "\(hiddenField.name ?? ""): \(hiddenField.value ?? "")", | ||
| lastUsedDate: lastUsedDate | ||
| ) | ||
| if newHistory == nil { | ||
| newHistory = [passHistoryHiddenField] | ||
| } else { | ||
| newHistory?.append(passHistoryHiddenField) | ||
| } | ||
| } | ||
| return newHistory |
There was a problem hiding this comment.
🎨 I was thinking about doing this change to improve the code and avoid having the extra logic to create the array if the newHistory == nil. What do you think about this? Do you feel like it's easier to read or your approach is? I feel like I have pros and cons on both approaches but leaning more towards this one but would love to hear your thoughts on this as well.
| var newHistory = passwordHistory | |
| oldFields.filter { field in | |
| FieldType(fieldType: field.type) == FieldType.hidden && | |
| !field.name.isEmptyOrNil && | |
| !field.value.isEmptyOrNil && | |
| !(newFields?.contains(field) ?? false) | |
| }.forEach { hiddenField in | |
| guard !hiddenField.name.isEmptyOrNil else { | |
| return | |
| } | |
| let passHistoryHiddenField = PasswordHistoryView( | |
| password: "\(hiddenField.name ?? ""): \(hiddenField.value ?? "")", | |
| lastUsedDate: lastUsedDate | |
| ) | |
| if newHistory == nil { | |
| newHistory = [passHistoryHiddenField] | |
| } else { | |
| newHistory?.append(passHistoryHiddenField) | |
| } | |
| } | |
| return newHistory | |
| let newPasswordHistoryFields = oldFields | |
| .filter { field in | |
| FieldType(fieldType: field.type) == FieldType.hidden && | |
| !field.name.isEmptyOrNil && | |
| !field.value.isEmptyOrNil && | |
| !(newFields?.contains(field) ?? false) | |
| }.compactMap { hiddenField in | |
| guard !hiddenField.name.isEmptyOrNil else { | |
| return nil | |
| } | |
| return PasswordHistoryView( | |
| password: "\(hiddenField.name ?? ""): \(hiddenField.value ?? "")", | |
| lastUsedDate: lastUsedDate | |
| ) | |
| } | |
| guard !newPasswordHistoryFields.isEmpty else { | |
| return passwordHistory | |
| } | |
| return passwordHistory?.append(contentsOf: newPasswordHistoryFields) ?? newPasswordHistoryFields |
| let newPasswordHistoryFields: [PasswordHistoryView] = oldFields | ||
| .filter { field in | ||
| FieldType(fieldType: field.type) == FieldType.hidden && | ||
| !field.name.isEmptyOrNil && | ||
| !field.value.isEmptyOrNil && | ||
| !(newFields?.contains(field) ?? false) | ||
| }.compactMap { hiddenField in | ||
| guard !hiddenField.name.isEmptyOrNil else { | ||
| return nil | ||
| } | ||
| return PasswordHistoryView( | ||
| password: "\(hiddenField.name ?? ""): \(hiddenField.value ?? "")", | ||
| lastUsedDate: lastUsedDate | ||
| ) | ||
| } |
There was a problem hiding this comment.
🤔 Sorry I missed this before but given that in the filter you have && !field.name.isEmptyOrNil; is it necessary to check again and add the guard in compactMap?
guard !hiddenField.name.isEmptyOrNil else {
return nil
}🤔 And another point I missed: if you have a Field that now has changed its name then newFields.contains(field) will be false and will be included in the history, adding the PasswordHistoryView but using the old field name instead of the new one. This is not the same behavior as in MAUI where we only added the password history if the names of the old field and the new field would match. Maybe something for Product to make decision on.
There was a problem hiding this comment.
Ran it with product and we will keep consistency with the other clients. So if the name changes it will log old name + old password.
There was a problem hiding this comment.
Screen.Recording.2024-10-24.at.15.37.05.mov
There was a problem hiding this comment.
Removed the nil name check because it was not necessary
🎟️ Tracking
PM-12695
📔 Objective
When an hidden field is updated, the changes should reflect in the password history list.
📸 Screenshots
Screen.Recording.2024-10-04.at.21.04.09.mov
⏰ 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