-
Notifications
You must be signed in to change notification settings - Fork 30
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
multiple device refresh improvements #2281
Conversation
Could we add some tests to cover the new functionality/endpoints? |
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.
There are a few important bugs in the new code that need to be fixed before this is ready. At the very least, in the order of importance:
- The captured loop variable will result in the same value stored
len(slice)
times. Plus pointers. - Pointers to UUIDs need to be replaced with values.
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 went ahead and expressed my feedback in #2314. Please read the description and the PR. It addresses the following aspects:
- Expresses the deletion logic so it's testable;
- Added test for the deletion logic so that we can see what's being passed down to the datastore level;
- Fixed types for UUID.
* fix: review * test: add tests
…exceeds threshold updates to multi-device improvements to get active credentials count fixing delete tlv2 test cases to include new check call fixing db field for request_id for tlv2 creds table
2ebbd8e
to
d3ae44e
Compare
…multi-device-improvements
Added some comments, at the minimum can we resolve any marked as |
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.
Waiting on the open comments to be addressed before doing the final review.
[puLL-Merge] - brave-intl/bat-go@2281 DescriptionThis PR makes changes to the ChangesChanges
Security HotspotsN/A |
Summary
updates to delete credentials to only hard delete where request id matches item id
Type of Change
Tested Environments
Before Requesting Review
Manual Test Plan