Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

Commit

Permalink
Merge pull request #4297 from jwhitlock/optimize_history_1269104
Browse files Browse the repository at this point in the history
bug 1269104: Refactor and optimize revision history
  • Loading branch information
escattone committed Jul 7, 2017
2 parents c11c9cc + d6730e8 commit 127f92f
Show file tree
Hide file tree
Showing 9 changed files with 193 additions and 141 deletions.
14 changes: 8 additions & 6 deletions kuma/wiki/jinja2/wiki/list/revisions.html
Expand Up @@ -28,7 +28,7 @@ <h1>{{ _('History of') }} <a href="{{ document.get_absolute_url() }}">{{ documen
</ul>
</div>
<div class="revision-list-contain">
{% set revision_count=revisions|length %}
{% set revision_count=selected_revision_pairs|length %}
{% set user_can_delete=user.has_perm('wiki.delete_revision') %}
{% if revision_count > 0 %}
{% if request.GET.origin == 'translate' %}
Expand All @@ -43,7 +43,9 @@ <h1>{{ _('History of') }} <a href="{{ document.get_absolute_url() }}">{{ documen
<input type="submit" class="link-btn" value="{% if document.parent %}{{ _('Compare Selected Translations') }}{% else %}{{ _('Compare Selected Revisions') }}{% endif %}" />
</div>
{% endif %}
{% for rev in revisions %}
{% for rev_id, prev_id in selected_revision_pairs %}
{% set rev = revisions[rev_id] %}
{% if prev_id %}{% set prev = revisions[prev_id] %}{% else %}{% set prev = none %}{% endif%}
{% if loop.first %}<ul class="revision-list">{% endif %}
<li {%- if document != rev.document %} class="revision-list-en-source"{% endif %}>
<div class="revision-list-cell revision-list-radio">
Expand All @@ -53,8 +55,8 @@ <h1>{{ _('History of') }} <a href="{{ document.get_absolute_url() }}">{{ documen
{% if not loop.last %}<input type="radio" name="to" value="{{ rev.id }}" {% if loop.first %}checked="checked"{% endif %} />{% endif %}
</div>
<div class="revision-list-cell revision-list-prev">
{% if rev.previous_revision %}
<a href="{{ url('wiki.compare_revisions', document.slug) }}?to={{ rev.id }}&amp;from={{ rev.previous_revision.id }}" rel="nofollow, noindex">{{ _('Previous') }}</a>
{% if prev_id %}
<a href="{{ url('wiki.compare_revisions', document.slug) }}?to={{ rev.id }}&amp;from={{ prev_id }}" rel="nofollow, noindex">{{ _('Previous') }}</a>
{% endif %}
</div>
<div class="revision-list-cell revision-list-date">
Expand All @@ -69,9 +71,9 @@ <h1>{{ _('History of') }} <a href="{{ document.get_absolute_url() }}">{{ documen
{% if rev.document.locale != document.locale %}
<em>{{ get_locale_localized(rev.document.locale) }}</em>
{% endif %}
{{ format_comment(rev) }}
{{ format_comment(rev, prev, load_previous=False) }}
</div>
{% if document.current_revision.id != rev.id and document == rev.document and user_can_delete %}
{% if document.current_revision_id != rev_id and document.id == rev.document_id and user_can_delete %}
<div class="revision-list-cell revision-list-revert">
<a href="{{ url('wiki.revert_document', rev.document.slug, rev.id, locale=rev.document.locale) }}" class="button revert-revision" rel="nofollow, noindex">{{ _('Revert to this revision') }}</a>
</div>
Expand Down
7 changes: 0 additions & 7 deletions kuma/wiki/managers.py
Expand Up @@ -9,13 +9,6 @@
from .constants import (ALLOWED_TAGS, ALLOWED_ATTRIBUTES, ALLOWED_PROTOCOLS,
ALLOWED_STYLES)
from .content import parse as parse_content
from .queries import TransformQuerySet


class TransformManager(models.Manager):

def get_queryset(self):
return TransformQuerySet(self.model)


class BaseDocumentManager(models.Manager):
Expand Down
4 changes: 1 addition & 3 deletions kuma/wiki/models.py
Expand Up @@ -43,7 +43,7 @@
from .jobs import DocumentContributorsJob, DocumentNearestZoneJob
from .managers import (DeletedDocumentManager, DocumentAdminManager,
DocumentManager, RevisionIPManager,
TaggedDocumentManager, TransformManager)
TaggedDocumentManager)
from .signals import render_done
from .templatetags.jinja_helpers import absolutify
from .utils import tidy_content, get_doc_components_from_url
Expand Down Expand Up @@ -1692,8 +1692,6 @@ class Revision(models.Model):
is_mindtouch_migration = models.BooleanField(default=False, db_index=True,
help_text="Did this revision come from MindTouch?")

objects = TransformManager()

def get_absolute_url(self):
"""Build the absolute URL to this revision"""
return reverse('wiki.revision',
Expand Down
26 changes: 0 additions & 26 deletions kuma/wiki/queries.py

This file was deleted.

18 changes: 11 additions & 7 deletions kuma/wiki/templatetags/jinja_helpers.py
Expand Up @@ -41,22 +41,26 @@ def bugize_text(content):


@library.global_function
def format_comment(rev, previous_revision=None):
def format_comment(rev, previous_revision=None, load_previous=True):
"""
Massages revision comment content after the fact
Format comment for HTML display, with Bugzilla links and slug changes.
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)
if prev_rev is None:
prev_rev = rev.previous
if previous_revision is None and load_previous:
previous_revision = rev.previous
comment = bugize_text(rev.comment if rev.comment else "")

# If a page move, say so
if prev_rev and prev_rev.slug != rev.slug:
if previous_revision and previous_revision.slug != rev.slug:
comment += jinja2.Markup(
'<span class="slug-change">'
'<span>%s</span>'
' <i class="icon-long-arrow-right" aria-hidden="true"></i> '
'<span>%s</span></span>') % (prev_rev.slug, rev.slug)
'<span>%s</span></span>') % (previous_revision.slug, rev.slug)

return comment

Expand Down
2 changes: 1 addition & 1 deletion kuma/wiki/tests/test_templates.py
Expand Up @@ -1351,7 +1351,7 @@ def one_if(*args):

@pytest.mark.parametrize("elem_num,has_prev,is_english,has_revert", [
(0, True, False, False),
(1, False, False, True),
(1, True, False, True),
(2, False, True, False)],
ids=['current', 'first_trans', 'en_source'])
def test_list_revisions(elem_num, has_prev, is_english, has_revert,
Expand Down
48 changes: 0 additions & 48 deletions kuma/wiki/tests/test_views.py
Expand Up @@ -34,7 +34,6 @@
from kuma.core.templatetags.jinja_helpers import add_utm
from kuma.core.tests import eq_, get_user, ok_
from kuma.core.urlresolvers import reverse
from kuma.core.utils import urlparams
from kuma.spam.akismet import Akismet
from kuma.spam.constants import SPAM_CHECKS_FLAG, SPAM_SUBMISSIONS_FLAG, SPAM_URL, VERIFY_URL
from kuma.users.tests import UserTestCase, user
Expand Down Expand Up @@ -186,53 +185,6 @@ def test_json_view(self):
result_review_tags = sorted([str(x) for x in data['review_tags']])
eq_(expected_review_tags, result_review_tags)

def test_history_view(self):
slug = 'history-view-test-doc'
html = 'history view test doc'

doc = document(title='History view test doc', slug=slug,
html=html, save=True,
locale=settings.WIKI_DEFAULT_LANGUAGE)

for i in xrange(1, 51):
revision(document=doc, content=html,
comment='Revision %s' % i,
is_approved=True, save=True)

url = reverse('wiki.document_revisions', args=(slug,),
locale=settings.WIKI_DEFAULT_LANGUAGE)

resp = self.client.get(url)
eq_(200, resp.status_code)

all_url = urlparams(reverse('wiki.document_revisions', args=(slug,),
locale=settings.WIKI_DEFAULT_LANGUAGE),
limit='all')
resp = self.client.get(all_url)
eq_(403, resp.status_code)

self.client.login(username='testuser', password='testpass')
resp = self.client.get(all_url)
eq_(200, resp.status_code)

def test_translation_history_has_based_on_revision_to_compare(self):
"""In the history of a translation, there should be based on revision
So that its possible to compare a new translation with its based on"""

eng_rev = revision(is_approved=True, save=True)
trans_doc = document(locale='bn-BD', parent=eng_rev.document, save=True)
revision(document=trans_doc, based_on=eng_rev, save=True)

url = reverse('wiki.document_revisions', args=(trans_doc.slug,), locale=trans_doc.locale)
resp = self.client.get(url)
eq_(200, resp.status_code)
data = pq(resp.content)
list_content = data('.revision-list-contain').find('li')
# Check there are 2 revision in the list
eq_(2, len(list_content))
# Check parent_revision id is below to compare from
eq_(str(eng_rev.id), list_content.find('input[name=from]')[1].attrib['value'])

def test_toc_view(self):
slug = 'toc_test_doc'
html = '<h2>Head 2</h2><h3>Head 3</h3>'
Expand Down
123 changes: 123 additions & 0 deletions kuma/wiki/tests/test_views_list.py
@@ -0,0 +1,123 @@
from pyquery import PyQuery as pq

from kuma.core.urlresolvers import reverse
from kuma.core.utils import urlparams
from ..models import Document


def test_revisions(root_doc, client):
"""$history of English doc works."""
url = reverse('wiki.document_revisions', args=(root_doc.slug,),
locale=root_doc.locale)
resp = client.get(url)
assert resp.status_code == 200


def test_revisions_of_translated_document(trans_doc, client):
"""
$history for translated documents includes an English revision.
This is the revision the first translation was based on.
"""
assert trans_doc.revisions.count() == 1
url = reverse('wiki.document_revisions', args=(trans_doc.slug,),
locale=trans_doc.locale)
resp = client.get(url)
assert resp.status_code == 200
page = pq(resp.content)
list_content = page('.revision-list-contain').find('li')
assert len(list_content) == 2 # The translation plus the English revision
eng_rev_id = list_content.find('input[name=from]')[1].attrib['value']
assert str(trans_doc.current_revision.based_on_id) == eng_rev_id


def test_revisions_of_translated_doc_with_no_based_on(trans_revision, client):
"""
$history for trans docs excludes the English revision if no based_on
This can happen for old translated docs, or ones manually associated with
the parent.
"""
assert trans_revision.based_on
trans_revision.based_on = None
trans_revision.save()
trans_doc = trans_revision.document
url = reverse('wiki.document_revisions', args=(trans_doc.slug,),
locale=trans_doc.locale)
resp = client.get(url)
assert resp.status_code == 200
page = pq(resp.content)
list_content = page('.revision-list-contain').find('li')
assert len(list_content) == 1 # The translation alone


def test_revisions_bad_slug_is_not_found(db, client):
"""$history of unknown page returns 404."""
url = reverse('wiki.document_revisions', args=('not_found',),
locale='en-US')
resp = client.get(url)
assert resp.status_code == 404


def test_revisions_doc_without_revisions_is_not_found(db, client):
"""$history of half-created document returns 404."""
doc = Document.objects.create(locale='en-US', slug='half_created')
url = reverse('wiki.document_revisions', args=(doc.slug,),
locale=doc.locale)
resp = client.get(url)
assert resp.status_code == 404


def test_revisions_all_params_as_anon_user_is_forbidden(root_doc, client):
"""Anonymous users are forbidden to request all revisions."""
url = reverse('wiki.document_revisions', args=(root_doc.slug,),
locale=root_doc.locale)
all_url = urlparams(url, limit='all')
resp = client.get(all_url)
assert resp.status_code == 403


def test_revisions_all_params_as_user_is_allowed(root_doc, wiki_user, client):
"""Users are allowed to request all revisions."""
url = reverse('wiki.document_revisions', args=(root_doc.slug,),
locale=root_doc.locale)
all_url = urlparams(url, limit='all')
wiki_user.set_password('password')
wiki_user.save()
client.login(username=wiki_user.username, password='password')
resp = client.get(all_url)
assert resp.status_code == 200


def test_revisions_request_tiny_pages(edit_revision, client):
"""$history will paginate the revisions."""
doc = edit_revision.document
assert doc.revisions.count() > 1
url = reverse('wiki.document_revisions', args=(doc.slug,),
locale=doc.locale)
limit_url = urlparams(url, limit=1)
resp = client.get(limit_url)
assert resp.status_code == 200
page = pq(resp.content)
assert len(page.find('ol.pagination')) == 1


def test_revisions_request_large_pages(root_doc, client):
"""$history?limit=(more than revisions) works, removes pagination."""
rev_count = root_doc.revisions.count()
url = reverse('wiki.document_revisions', args=(root_doc.slug,),
locale=root_doc.locale)
limit_url = urlparams(url, limit=rev_count + 1)
resp = client.get(limit_url)
assert resp.status_code == 200
page = pq(resp.content)
assert len(page.find('ol.pagination')) == 0


def test_revisions_request_invalid_pages(root_doc, client):
"""$history?limit=nonsense uses the default pagination."""
url = reverse('wiki.document_revisions', args=(root_doc.slug,),
locale=root_doc.locale)
limit_url = urlparams(url, limit='nonsense')
resp = client.get(limit_url)
assert resp.status_code == 200

0 comments on commit 127f92f

Please sign in to comment.