-
-
Notifications
You must be signed in to change notification settings - Fork 145
Improve performance when using condition_dict #291
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
base: master
Are you sure you want to change the base?
Conversation
claudep
left a comment
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 for the patch, it looks good to me, even if some added complexity might be a bug source.
I'd like at least one other approval before pushing it.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #291 +/- ##
==========================================
- Coverage 93.80% 93.62% -0.19%
==========================================
Files 11 11
Lines 533 549 +16
Branches 86 90 +4
==========================================
+ Hits 500 514 +14
- Misses 21 22 +1
- Partials 12 13 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@claudep I did run into an issue testing this with real code. It looks like django-two-factor-auth pops from the Let me know if you think it should be fixed in another way. |
|
The coverage issue above should now be fixed, didn't notice it before, sorry! FWIW I've been running this in production for a couple of weeks now and seems to be doing well. |
|
Thanks! Could we have a second approval here? Hopefully we are more than 2 people following the activity on this package 😜 |
Fixes #134
Uses a bit of caching to reduce the number of calls needed when using condition_dict.
For us, this reduces the number of queries in one view (for each step of that view) from nearly 300 to around 17. It's possible it could be improved further but I think this is a dramatic enough of an improvement to be useful on its own.
All the tests pass and I checked my wizards were all working locally. I went for a slightly more realistic test rather than a minimal test, but I can change this if desired.
A simpler solution is to cache the
allproperty. It works quite well, but there are still some extra queries when doing this.