-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Auth/pm 2996/add auth request data to devices response model #5152
Auth/pm 2996/add auth request data to devices response model #5152
Conversation
…up to do and questions to answer.
…-request-data-to-devices-response-model
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5152 +/- ##
==========================================
+ Coverage 43.34% 43.85% +0.50%
==========================================
Files 1458 1470 +12
Lines 66517 67763 +1246
Branches 6081 6117 +36
==========================================
+ Hits 28835 29720 +885
- Misses 36390 36748 +358
- Partials 1292 1295 +3 ☔ View full report in Codecov by Sentry. |
New Issues
Fixed Issues
|
…est-data-to-devices-response-model
…est-data-to-devices-response-model
d29e69d
to
0562f76
Compare
0562f76
to
647fad8
Compare
test/Infrastructure.EFIntegration.Test/Repositories/DeviceRepositoryTests.cs
Outdated
Show resolved
Hide resolved
bf3949d
to
98434df
Compare
Migrated tests to appropriate place. Fixed up some of the comments discussed with Ike.
98434df
to
97fa55a
Compare
…est-data-to-devices-response-model
Added comment to describe thought process for response model being passed through by the repository.
Tiny fix to remove the settings out of the controller.
…est-data-to-devices-response-model
Tiny fix to remove the settings out of the controller test.
153616a
to
040d0c6
Compare
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.
Thanks boss.
… procedure with recommendations.
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
…est-data-to-devices-response-model
util/Migrator/DbScripts/2024-12-04_00_AddActiveDeviceWithPendingAuth.sql
Outdated
Show resolved
Hide resolved
474bdee
…in build to make it more clear what could be the culprit of the issue.
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.
The workflow change looks fine. I didn't review much else because it seems like that's been covered.
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
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-2996
📔 Objective
This pr adds two new fields to the get devices response endpoint. Now we respond back with the pending auth requests that match a certain criteria when retrieving devices.
A details object was used to separate the response from the controller from the data coming back from the database.
Also updated
.github/workflows/test-database.yml
with a more descriptive error message to assist in debugging.📸 Screenshots
old_response.mov
new_response.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