bug 1269104: Refactor and optimize revision history #4297
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4297 +/- ##
=========================================
+ Coverage 87.83% 88.3% +0.47%
=========================================
Files 162 163 +1
Lines 10045 10205 +160
Branches 1371 1413 +42
=========================================
+ Hits 8823 9012 +189
+ Misses 993 967 -26
+ Partials 229 226 -3
Continue to review full report at Codecov.
|
jwhitlock
force-pushed
the
optimize_history_1269104
branch
from
bd2be10
to
bee85b8
Compare
Minimize database queries for the document, revisions, and creators, to only request data used in template rendering, and to reduce the total number of DB queries from 15 to 10. This gives a 2.5x speed-up in the local environment, and gets us a step closer to allowing scrapers. New template context also adds "Previous" link for first translation revision, when the English base revision is known, for a consistent UI.
The TransformManager was used for Revisions for the .transform method, which supported adding .previous_revision to the objects in a queryset. This is no longer used, so the standard Django manager can be used again.
jwhitlock
force-pushed
the
optimize_history_1269104
branch
from
bee85b8
to
a8956df
Compare
kuma/wiki/views/list.py
Outdated
| return revisions | ||
| # Load document with only fields for history display | ||
| try: | ||
| document = (Document.objects |
There was a problem hiding this comment.
Is it possible to use get_object_or_404 here?
There was a problem hiding this comment.
Good idea. I switched to get_object_or_404 with a longer queryset, and now Http404 is no longer imported.
|
@jwhitlock Thanks a lot for this PR. It surely needs a closer review! |
There was a problem hiding this comment.
Thanks @jwhitlock, the code is simpler, more readable, faster, and the new and improved tests provide 100% line coverage of the revisions view. What's not to like?!
I have two minor requests for you to consider.
kuma/wiki/views/list.py
Outdated
| # Load document with only fields for history display | ||
| doc_query = (Document.objects | ||
| .only('id', 'locale', 'slug', 'title', | ||
| 'parent__slug', 'parent__locale') |
| Keyword Arguments: | ||
| rev - The revision | ||
| previous_revision - The previous revision (default None) | ||
| load_previous - Try loading previous revision if None (default True) | ||
| """ | ||
| prev_rev = getattr(rev, 'previous_revision', previous_revision) |
There was a problem hiding this comment.
I can't find any other code within Kuma that adds a previous_revision attribute to a revision (and uses format_comment), so it seems safe to change this to prev_rev = previous_revision (or perhaps even better, remove this line and rename the previous_revision argument to prev_rev)?
There was a problem hiding this comment.
Good idea! More code to delete! 🎉
Pre-fetch document.current_revision_id to avoid loading it from the database in wiki/list/revisions.html
Revision.previous_revision was optionally added by the TransformManager, which has been removed. Instead, use previous_revision directly.
Optimize the database queries for the document's revision history page in a few ways:
revision.previous_revisionwork-around.load_previous=Trueparameter to the Jinja helperformat_comment, which can be setFalseto avoid querying the database whenprevious_revisionis None.The effects of the changes are:
TransformManagerandTransformQuerysetforRevision, used to populate theprevious_revisionelement.