-
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
[pm-5966] Fix Entity Framework query for MySQL #5170
base: main
Are you sure you want to change the base?
Conversation
.Where(x => (DateTime.UtcNow - x.CreationDate).Days == 4 | ||
&& x.VerifiedDate == null) | ||
var unverifiedDomainsInLast72Hours = await dbContext.OrganizationDomains | ||
.Where(x => x.CreationDate.Date >= DateTime.UtcNow.AddDays(-4).Date |
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.
Question: I'm not sure if a unit test would add value here since the behavior can only be tested at runtime.
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.
You can add an integration test on the project Infrastructure.IntegrationTest
using the DatabaseData
attribute which configures the test to run once for each configured DB provider.
Example here https://github.com/bitwarden/server/blob/main/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationRepositoryTests.cs
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5170 +/- ##
=======================================
Coverage 43.35% 43.35%
=======================================
Files 1456 1456
Lines 66468 66468
Branches 6078 6078
=======================================
Hits 28817 28817
Misses 36360 36360
Partials 1291 1291 ☔ View full report in Codecov by Sentry. |
No New Or Fixed Issues Found |
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.
It looks good to me but it would be ideal to have the integration test
.Where(x => (DateTime.UtcNow - x.CreationDate).Days == 4 | ||
&& x.VerifiedDate == null) | ||
var unverifiedDomainsInLast72Hours = await dbContext.OrganizationDomains | ||
.Where(x => x.CreationDate.Date >= DateTime.UtcNow.AddDays(-4).Date |
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.
You can add an integration test on the project Infrastructure.IntegrationTest
using the DatabaseData
attribute which configures the test to run once for each configured DB provider.
Example here https://github.com/bitwarden/server/blob/main/test/Infrastructure.IntegrationTest/AdminConsole/Repositories/OrganizationRepositoryTests.cs
Looks like this should resolve #3722. Funny to see it resolved because of a compilation issue vs. the longstanding pre-existing issue. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-5966
📔 Objective
Problem: The Entity Framework query was causing a compile-time error.
Changes:
📸 Screenshots
Manual testing
Generated SQL query.
With results
⏰ 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