-
Notifications
You must be signed in to change notification settings - Fork 412
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
fix: Raise if environment_feature_version
is None
#5028
fix: Raise if environment_feature_version
is None
#5028
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: api/features/models.py
Did you find this useful? React with a 👍 or 👎 |
Docker builds report
|
Uffizzi Preview |
Do we have any idea how we reached the faulty state in the first place? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5028 +/- ##
=======================================
Coverage 97.40% 97.40%
=======================================
Files 1192 1192
Lines 41657 41665 +8
=======================================
+ Hits 40574 40582 +8
Misses 1083 1083 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Kim Gustyr <kim.gustyr@flagsmith.com>
Co-authored-by: Kim Gustyr <kim.gustyr@flagsmith.com>
No but we did somehow. |
assert tuple((user["id"], user["group_admin"]) for user in group_2_users) == ( | ||
(user1.pk, False), | ||
(user2.pk, True), | ||
) | ||
|
||
for user in group_2_users: | ||
if user["id"] == user1.pk: | ||
assert user["group_admin"] is False | ||
continue | ||
if user["id"] == user2.pk: | ||
assert user["group_admin"] is True | ||
continue | ||
# We should only match the two users. | ||
assert False |
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.
nit: I'm pretty sure using sets will solve this in a neater way:
assert set((user["id"], user["group_admin"]) for user in group_2_users) == {
(user1.pk, False),
(user2.pk, True),
}
You won't have to maintain the uncovered assert False
line too.
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.
Oh this is a much better approach. Thank you!!
Changes
Solves this Sentry alert. Elsewhere in this part of the codebase we raise value errors to deal with bad input. Rather than erroring out with a type error that looks like a bug, we should raise an error of our own to deal with it.
How did you test this code?
One new test.