-
Notifications
You must be signed in to change notification settings - Fork 7
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
[BWA-39] Properly generate codes for search results #119
Conversation
No New Or Fixed Issues Found |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #119 +/- ##
==========================================
+ Coverage 63.37% 65.99% +2.61%
==========================================
Files 202 202
Lines 8685 8698 +13
==========================================
+ Hits 5504 5740 +236
+ Misses 3181 2958 -223 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Left a question about testing one area that may benefit from it, if it makes sense to cover.
errorReporter.log(error: TOTPServiceError | ||
.unableToGenerateCode("Unable to refresh TOTP code for list view item: \(item.id)")) | ||
return item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we test this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not as far as I can tell, unfortunately. In practice I don't think we'll ever actually get here, because we don't have a itemType
other than .totp
at the moment, and if the type is TOTP an ItemListItem
can't be created unless the key is valid. However, because the contained AuthenticatorItemView
has an optional totpKey
and TOTPKeyModel.init()
is a failable initializer, we still have to have code to handle if those, even though in practice the nil/fail cases won't ever happen.
else { | ||
errorReporter.log(error: TOTPServiceError | ||
.unableToGenerateCode("Unable to refresh TOTP code for list view item: \(item.id)")) | ||
return item |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 I know this scenario is highly unlikely to happen, but shouldn't we return nil
here and .compactMap
the result of the asyncMap
? Not quite understanding why it would return the item
if it can't create the TOTPKeyModel
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured that it would be better to keep the item in the list with a stale code rather than have it suddenly disappear, especially because if a new type of item is added at some point, this would still return the item as-is.
🎟️ Tracking
BWA-39
📔 Objective
This fixes a bug where search results weren't generating/refreshing TOTP codes properly. This also backfills some tests.
📸 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