diff --git a/reviewboard/reviews/builtin_fields.py b/reviewboard/reviews/builtin_fields.py index 424ff46f977c4aa7ed2e506705546a3eae69493c..e99345828a63a318a6d2b56de00402bd861fb724 100644 --- a/reviewboard/reviews/builtin_fields.py +++ b/reviewboard/reviews/builtin_fields.py @@ -73,29 +73,44 @@ class BuiltinTextAreaFieldMixin(BuiltinFieldMixin): return attrs -class BuiltinLocalsFieldMixin(BuiltinFieldMixin): - """Mixin for internal fields needing access to local variables. +class ReviewRequestPageDataMixin(BuiltinFieldMixin): + """Mixin for internal fields needing access to the page data. - These are used by fields that operate on state generated when - creating the review request page. The view handling that page has - a lot of cached variables, which the fields need access to for - performance reasons. + These are used by fields that operate on state generated when creating the + review request page. The view handling that page makes a lot of queries, + and stores the results. This mixin allows access to those results, + preventing additional queries. - This should not be used by any classes outside this file. + The data structure is not meant to be public API, and this mixin should not + be used by any classes outside this file. By default, this will not render or handle any value loading or change entry recording. Subclasses must implement those manually. """ - #: A list of variables needed from the review_detail view's locals(). - locals_vars = [] - def __init__(self, review_request_details, locals_vars={}, - *args, **kwargs): - super(BuiltinLocalsFieldMixin, self).__init__( + def __init__(self, review_request_details, data=None, *args, **kwargs): + """Initialize the mixin. + + Args: + review_request_details (reviewboard.reviews.models.base_review_request_details.BaseReviewRequestDetails): + The review request (or the active draft thereof). In practice + this will either be a + :py:class:`reviewboard.reviews.models.ReviewRequest` or a + :py:class:`reviewboard.reviews.models.ReviewRequestDraft`. + + data (reviewboard.reviews.detail.ReviewRequestPageData): + The data already queried for the review request page. + + *args (tuple): + Additional positional arguments. + + **kwargs (dict): + Additional keyword arguments. + """ + super(ReviewRequestPageDataMixin, self).__init__( review_request_details, *args, **kwargs) - for var in self.locals_vars: - setattr(self, var, locals_vars.get(var, None)) + self.data = data def should_render(self, value): return False @@ -107,7 +122,7 @@ class BuiltinLocalsFieldMixin(BuiltinFieldMixin): return None -class BaseCaptionsField(BuiltinLocalsFieldMixin, BaseReviewRequestField): +class BaseCaptionsField(ReviewRequestPageDataMixin, BaseReviewRequestField): """Base class for rendering captions for attachments. This serves as a base for FileAttachmentCaptionsField and @@ -121,7 +136,7 @@ class BaseCaptionsField(BuiltinLocalsFieldMixin, BaseReviewRequestField): def render_change_entry_html(self, info): render_item = super(BaseCaptionsField, self).render_change_entry_html - obj_map = getattr(self, self.obj_map_attr) + obj_map = getattr(self.data, self.obj_map_attr) s = [''] @@ -459,7 +474,7 @@ class CommitField(BuiltinFieldMixin, BaseReviewRequestField): return escape(commit_id) -class DiffField(BuiltinLocalsFieldMixin, BaseReviewRequestField): +class DiffField(ReviewRequestPageDataMixin, BaseReviewRequestField): """Represents a newly uploaded diff on a review request. This is not shown as an actual displayable field on the review request @@ -468,7 +483,6 @@ class DiffField(BuiltinLocalsFieldMixin, BaseReviewRequestField): """ field_id = 'diff' label = _('Diff') - locals_vars = ['diffsets_by_id'] can_record_change_entry = True @@ -479,7 +493,7 @@ class DiffField(BuiltinLocalsFieldMixin, BaseReviewRequestField): review_request = self.review_request_details.get_review_request() try: - diffset = self.diffsets_by_id[added_diff_info[2]] + diffset = self.data.diffsets_by_id[added_diff_info[2]] except KeyError: # If a published revision of a diff has been deleted from the # database, this will explode. Just return a blank string for this, @@ -633,13 +647,12 @@ class FileAttachmentCaptionsField(BaseCaptionsField): """ field_id = 'file_captions' label = _('File Captions') - obj_map_attr = 'file_attachment_id_map' - locals_vars = [obj_map_attr] + obj_map_attr = 'file_attachments_by_id' model = FileAttachment caption_object_field = 'file_attachment' -class FileAttachmentsField(BuiltinLocalsFieldMixin, BaseCommaEditableField): +class FileAttachmentsField(ReviewRequestPageDataMixin, BaseCommaEditableField): """Renders removed or added file attachments. This is not shown as an actual displayable field on the review request @@ -649,7 +662,6 @@ class FileAttachmentsField(BuiltinLocalsFieldMixin, BaseCommaEditableField): """ field_id = 'files' label = _('Files') - locals_vars = ['file_attachment_id_map'] model = FileAttachment thumbnail_template = 'reviews/changedesc_file_attachment.html' @@ -688,8 +700,8 @@ class FileAttachmentsField(BuiltinLocalsFieldMixin, BaseCommaEditableField): items = [] for caption, filename, pk in values: - if pk in self.file_attachment_id_map: - attachment = self.file_attachment_id_map[pk] + if pk in self.data.file_attachments_by_id: + attachment = self.data.file_attachments_by_id[pk] else: try: attachment = FileAttachment.objects.get(pk=pk) @@ -717,8 +729,7 @@ class ScreenshotCaptionsField(BaseCaptionsField): """ field_id = 'screenshot_captions' label = _('Screenshot Captions') - obj_map_attr = 'screenshot_id_map' - locals_vars = [obj_map_attr] + obj_map_attr = 'screenshots_by_id' model = Screenshot caption_object_field = 'screenshot' diff --git a/reviewboard/reviews/detail.py b/reviewboard/reviews/detail.py index fcc6e59463d942b6d1b93fa1315d6f2db01418f7..6a6e1d9f1bc58768923c539f4378e4a1fceb2a66 100644 --- a/reviewboard/reviews/detail.py +++ b/reviewboard/reviews/detail.py @@ -2,8 +2,346 @@ from __future__ import unicode_literals +from collections import defaultdict +from datetime import datetime + +from django.db.models import Q +from django.utils import six +from django.utils.timezone import utc + +from reviewboard.reviews.builtin_fields import ReviewRequestPageDataMixin from reviewboard.reviews.fields import get_review_request_fieldsets -from reviewboard.reviews.models import BaseComment, ReviewRequest +from reviewboard.reviews.models import (BaseComment, + Comment, + FileAttachmentComment, + GeneralComment, + ReviewRequest, + ScreenshotComment) + + +class ReviewRequestPageData(object): + """Data for the review request page. + + The review request detail page needs a lot of data from the database, and + going through the standard model relations will result in a lot more + queries than necessary. This class bundles all that data together and + handles pre-fetching and re-associating as necessary to limit the required + number of queries. + + All of the attributes within the class may not be available until both + :py:meth:`query_data_pre_etag` and :py:meth:`query_data_post_etag` are + called. + + This object is not meant to be public API, and may change at any time. You + should not use it in extension code. + + Attributes: + body_bottom_replies (dict): + A mapping from a top-level review ID to a list of the + :py:class:`reviewboard.reviews.models.Review` objects which reply + to it. + + body_top_replies (dict): + A mapping from a top-level review ID to a list of the + :py:class:`reviewboard.reviews.models.Review` objects which reply + to it. + + comments (list): + A list of all comments associated with all reviews shown on the + page. + + changedescs (list of reviewboard.changedescs.models.ChangeDescription): + All the change descriptions to be shown on the page. + + diffsets (list of reviewboard.diffviewer.models.DiffSet): + All of the diffsets associated with the review request. + + diffsets_by_id (dict): + A mapping from ID to + :py:class:`reviewboard.diffviewer.models.DiffSet`. + + draft (reviewboard.reviews.models.ReviewRequestDraft): + The active draft of the review request, if any. May be ``None``. + + active file_attachments (list of reviewboard.attachments.models.FileAttachment): + All the active file attachments associated with the review request. + + all_file_attachments (list of reviewboard.attachments.models.FileAttachment): + All the file attachments associated with the review request. + + file_attachments_by_id (dict): + A mapping from ID to + :py:class:`reviewboard.attachments.models.FileAttachment` + + issues (dict): + A dictionary storing counts of the various issue states throughout + the page. + + latest_changedesc_timestamp (datetime.datetime): + The timestamp of the most recent change description on the page. + + latest_review_timestamp (datetime.datetime): + The timestamp of the most recent review on the page. + + latest_timestamps_by_review_id (dict): + A mapping from top-level review ID to the latest timestamp of the + thread. + + review_request (reviewboard.reviews.models.ReviewRequest): + The review request. + + review_request_details (reviewboard.reviews.models.base_review_request_details.BaseReviewRequestDetails): + The review request (or the active draft thereof). In practice this + will either be a + :py:class:`reviewboard.reviews.models.ReviewRequest` or a + :py:class:`reviewboard.reviews.models.ReviewRequestDraft`. + + reviews (list of reviewboard.reviews.models.Review): + All the reviews to be shown on the page. This includes any draft + reviews owned by the requesting user but not drafts owned by + others. + + reviews_by_id (dict): + A mapping from ID to :py:class:`reviewboard.reviews.models.Review`. + + active_screenshots (list of reviewboard.reviews.models.Screenshot): + All the active screenshots associated with the review request. + + all_screenshots (list of reviewboard.reviews.models.Screenshot): + All the screenshots associated with the review request. + + screenshots_by_id (dict): + A mapping from ID to + :py:class:`reviewboard.reviews.models.Screenshot`. + """ # noqa + + def __init__(self, review_request, request): + """Initialize the data object. + + Args: + review_request (reviewboard.reviews.models.ReviewRequest): + The review request. + + request (django.http.HttpRequest): + The HTTP request object. + """ + self.review_request = review_request + self.request = request + + def query_data_pre_etag(self): + """Perform initial queries for the page. + + This method will populate only the data needed to compute the ETag. We + avoid everything else until later so as to do the minimum amount + possible before reporting to the client that they can just use their + cached copy. + """ + # Query for all the reviews that should be shown on the page (either + # ones which are public or draft reviews owned by the current user). + reviews_query = Q(public=True) + + if self.request.user.is_authenticated(): + reviews_query |= Q(user_id=self.request.user.pk) + + self.reviews = list( + self.review_request.reviews + .filter(reviews_query) + .order_by('-timestamp') + .select_related('user') + ) + + if len(self.reviews) == 0: + self.latest_review_timestamp = datetime.fromtimestamp(0, utc) + else: + self.latest_review_timestamp = self.reviews[0].timestamp + + # Get all the public ChangeDescriptions. + self.changedescs = list( + self.review_request.changedescs.filter(public=True)) + + if len(self.changedescs) == 0: + self.latest_changedesc_timestamp = datetime.fromtimestamp(0, utc) + else: + self.latest_changedesc_timestamp = self.changedescs[0].timestamp + + # Get the active draft (if any). + self.draft = self.review_request.get_draft(self.request.user) + + # Get diffsets. + self.diffsets = self.review_request.get_diffsets() + self.diffsets_by_id = self._build_id_map(self.diffsets) + + def query_data_post_etag(self): + """Perform remaining queries for the page. + + This method will populate everything else needed for the display of the + review request page other than that which was required to compute the + ETag. + """ + self.reviews_by_id = self._build_id_map(self.reviews) + + self.body_top_replies = defaultdict(list) + self.body_bottom_replies = defaultdict(list) + self.latest_timestamps_by_review_id = defaultdict(lambda: 0) + + for r in self.reviews: + r._body_top_replies = [] + r._body_bottom_replies = [] + + if r.body_top_reply_to_id is not None: + self.body_top_replies[r.body_top_reply_to_id].append(r) + + if r.body_bottom_reply_to_id is not None: + self.body_bottom_replies[r.body_bottom_reply_to_id].append(r) + + # Find the latest reply timestamp for each top-level review. + parent_id = r.base_reply_to_id + + if parent_id is not None: + self.latest_timestamps_by_review_id[parent_id] = max( + r.timestamp.replace(tzinfo=utc).ctime(), + self.latest_timestamps_by_review_id[parent_id]) + + # Link up all the review body replies. + for reply_id, replies in six.iteritems(self.body_top_replies): + self.reviews_by_id[reply_id]._body_top_replies = reversed(replies) + + for reply_id, replies in six.iteritems(self.body_bottom_replies): + self.reviews_by_id[reply_id]._body_bottom_replies = \ + reversed(replies) + + self.review_request_details = self.draft or self.review_request + + # Get all the file attachments and screenshots. + # + # Note that we fetch both active and inactive file attachments and + # screenshots. We do this because even though they've been removed, + # they still will be rendered in change descriptions. + self.active_file_attachments = \ + list(self.review_request_details.get_file_attachments()) + self.all_file_attachments = ( + self.active_file_attachments + + list(self.review_request_details.get_inactive_file_attachments())) + self.file_attachments_by_id = \ + self._build_id_map(self.all_file_attachments) + + for attachment in self.all_file_attachments: + attachment._comments = [] + + self.active_screenshots = \ + list(self.review_request_details.get_screenshots()) + self.all_screenshots = ( + self.active_screenshots + + list(self.review_request_details.get_inactive_screenshots())) + self.screenshots_by_id = self._build_id_map(self.all_screenshots) + + for screenshot in self.all_screenshots: + screenshot._comments = [] + + # Get all the comments and attach them to the reviews + review_ids = self.reviews_by_id.keys() + + self.comments = [] + self.issues = { + 'total': 0, + 'open': 0, + 'resolved': 0, + 'dropped': 0, + } + + for model, key, ordering in ( + (Comment, 'diff_comments', ('comment__filediff', + 'comment__first_line', + 'comment__timestamp')), + (ScreenshotComment, 'screenshot_comments', None), + (FileAttachmentComment, 'file_attachment_comments', None), + (GeneralComment, 'general_comments', None)): + # Due to mistakes in how we initially made the schema, we have a + # ManyToManyField in between comments and reviews, instead of + # comments having a ForeignKey to the review. This makes it + # difficult to easily go from a comment to a review ID. + # + # The solution to this is to not query the comment objects, but + # rather the through table. This will let us grab the review and + # comment in one go, using select_related. + related_field = model.review.related.field + comment_field_name = related_field.m2m_reverse_field_name() + through = related_field.rel.through + q = through.objects.filter(review__in=review_ids).select_related() + + if ordering: + q = q.order_by(*ordering) + + objs = list(q) + + # We do two passes. One to build a mapping, and one to actually + # process comments. + comment_map = {} + + for obj in objs: + comment = getattr(obj, comment_field_name) + comment._type = key + comment._replies = [] + comment_map[comment.pk] = comment + + for obj in objs: + comment = getattr(obj, comment_field_name) + + self.comments.append(comment) + + # Short-circuit some object fetches for the comment by setting + # some internal state on them. + assert obj.review_id in self.reviews_by_id + review = self.reviews_by_id[obj.review_id] + comment._review = review + comment._review_request = self.review_request + + # If the comment has an associated object (such as a file + # attachment) that we've already fetched, attach it to prevent + # future queries. + if isinstance(comment, FileAttachmentComment): + attachment_id = comment.file_attachment_id + f = self.file_attachments_by_id[attachment_id] + comment.file_attachment = f + f._comments.append(comment) + + diff_against_id = comment.diff_against_file_attachment_id + + if diff_against_id is not None: + f = self.file_attachments_by_id[diff_against_id] + comment.diff_against_file_attachment = f + elif isinstance(comment, ScreenshotComment): + screenshot = self.screenshots_by_id[comment.screenshot_id] + comment.screenshot = screenshot + screenshot._comments.append(comment) + + # We've hit legacy database cases where there were entries that + # weren't a reply, and were just orphaned. Ignore them. + if review.is_reply() and comment.is_reply(): + replied_comment = comment_map[comment.reply_to_id] + replied_comment._replies.append(comment) + + if review.public and comment.issue_opened: + status_key = \ + comment.issue_status_to_string(comment.issue_status) + self.issues[status_key] += 1 + self.issues['total'] += 1 + + def _build_id_map(self, objects): + """Return an ID map from a list of objects. + + Args: + objects (list): + A list of objects queried via django. + + Returns: + dict: + A dictionary mapping each ID to the resulting object. + """ + return { + obj.pk: obj + for obj in objects + } class BaseReviewRequestPageEntry(object): @@ -62,7 +400,7 @@ class ReviewEntry(BaseReviewRequestPageEntry): template_name = 'reviews/boxes/review.html' js_template_name = 'reviews/boxes/review.js' - def __init__(self, request, review_request, review, collapsed): + def __init__(self, request, review_request, review, collapsed, data): """Initialize the entry. Args: @@ -77,6 +415,9 @@ class ReviewEntry(BaseReviewRequestPageEntry): collapsed (bool): Whether the entry is collapsed by default. + + data (ReviewRequestPageData): + Pre-queried data for the review request page. """ super(ReviewEntry, self).__init__(review.timestamp, collapsed) @@ -126,8 +467,7 @@ class ChangeEntry(BaseReviewRequestPageEntry): template_name = 'reviews/boxes/change.html' js_template_name = 'reviews/boxes/change.js' - def __init__(self, request, review_request, changedesc, collapsed, - locals_vars): + def __init__(self, request, review_request, changedesc, collapsed, data): """Initialize the entry. Args: @@ -143,15 +483,8 @@ class ChangeEntry(BaseReviewRequestPageEntry): collapsed (bool): Whether the entry is collapsed by default. - locals_vars (dict): - A dictionary of the local variables inside the review detail - view. This is done because some of the fields in the change - description may make use of some of the maps maintained while - building the page in order to avoid adding additional queries - - .. seealso:: - - :py:data:`~reviewboard.reviews.fields.Field.locals_vars` + data (ReviewRequestPageData): + Pre-queried data for the review request page. """ super(ChangeEntry, self).__init__(changedesc.timestamp, collapsed) @@ -194,9 +527,9 @@ class ChangeEntry(BaseReviewRequestPageEntry): } self.fields_changed_groups.append(cur_field_changed_group) - if hasattr(field_cls, 'locals_vars'): + if issubclass(field_cls, ReviewRequestPageDataMixin): field = field_cls(review_request, request=request, - locals_vars=locals_vars) + data=data) else: field = field_cls(review_request, request=request) diff --git a/reviewboard/reviews/views.py b/reviewboard/reviews/views.py index 6a37fba53e3287f0ff6b23a603370e2e1313c0aa..d8cbd9e34d1b6bbdb08efe982ead31a12ea8e32f 100644 --- a/reviewboard/reviews/views.py +++ b/reviewboard/reviews/views.py @@ -52,15 +52,13 @@ from reviewboard.reviews.context import (comment_counts, has_comments_in_diffsets_excluding, interdiffs_with_comments, make_review_request_context) -from reviewboard.reviews.detail import ChangeEntry, ReviewEntry +from reviewboard.reviews.detail import (ChangeEntry, ReviewEntry, + ReviewRequestPageData) from reviewboard.reviews.markdown_utils import is_rich_text_default_for_user from reviewboard.reviews.models import (Comment, - FileAttachmentComment, - GeneralComment, Review, ReviewRequest, - Screenshot, - ScreenshotComment) + Screenshot) from reviewboard.reviews.ui.base import FileAttachmentReviewUI from reviewboard.scmtools.models import Repository from reviewboard.site.decorators import check_local_site_access @@ -118,19 +116,6 @@ def _find_review_request(request, review_request_id, local_site): return None, _render_permission_denied(request) -def _build_id_map(objects): - """Builds an ID map out of a list of objects. - - The resulting map makes it easy to quickly look up an object from an ID. - """ - id_map = {} - - for obj in objects: - id_map[obj.pk] = obj - - return id_map - - def _query_for_diff(review_request, user, revision, draft): """ Queries for a diff based on several parameters. @@ -329,98 +314,18 @@ def _get_latest_file_attachments(file_attachments): def review_detail(request, review_request_id, local_site=None, - template_name="reviews/review_detail.html"): - """ - Main view for review requests. This covers the review request information - and all the reviews on it. - """ - # If there's a local_site passed in the URL, we want to look up the review - # request based on the local_id instead of the pk. This allows each - # local_site configured to have its own review request ID namespace - # starting from 1. + template_name='reviews/review_detail.html'): + """Render the main review request page.""" review_request, response = _find_review_request( request, review_request_id, local_site) if not review_request: return response - # The review request detail page needs a lot of data from the database, - # and going through standard model relations will result in far too many - # queries. So we'll be optimizing quite a bit by prefetching and - # re-associating data. - # - # We will start by getting the list of reviews. We'll filter this out into - # some other lists, build some ID maps, and later do further processing. - entries = [] - public_reviews = [] - body_top_replies = {} - body_bottom_replies = {} - replies = {} - reply_timestamps = {} - reviews_entry_map = {} - reviews_id_map = {} - review_timestamp = 0 - visited = None + data = ReviewRequestPageData(review_request, request) + data.query_data_pre_etag() - # Start by going through all reviews that point to this review request. - # This includes draft reviews. We'll be separating these into a list of - # public reviews and a mapping of replies. - # - # We'll also compute the latest review timestamp early, for the ETag - # generation below. - # - # The second pass will come after the ETag calculation. - all_reviews = list(review_request.reviews.select_related('user')) - - for review in all_reviews: - review._body_top_replies = [] - review._body_bottom_replies = [] - - if review.public: - # This is a review we'll display on the page. Keep track of it - # for later display and filtering. - public_reviews.append(review) - parent_id = review.base_reply_to_id - - if parent_id is not None: - # This is a reply to a review. We'll store the reply data - # into a map, which associates a review ID with its list of - # replies, and also figures out the timestamps. - # - # Later, we'll use this to associate reviews and replies for - # rendering. - if parent_id not in replies: - replies[parent_id] = [review] - reply_timestamps[parent_id] = review.timestamp - else: - replies[parent_id].append(review) - reply_timestamps[parent_id] = max( - reply_timestamps[parent_id], - review.timestamp) - elif (request.user.is_authenticated() and - review.user_id == request.user.pk and - (review_timestamp == 0 or review.timestamp > review_timestamp)): - # This is the latest draft so far from the current user, so - # we'll use this timestamp in the ETag. - review_timestamp = review.timestamp - - if review.public or (request.user.is_authenticated() and - review.user_id == request.user.pk): - reviews_id_map[review.pk] = review - - # If this review is replying to another review's body_top or - # body_bottom fields, store that data. - for reply_id, reply_list in ( - (review.body_top_reply_to_id, body_top_replies), - (review.body_bottom_reply_to_id, body_bottom_replies)): - if reply_id is not None: - if reply_id not in reply_list: - reply_list[reply_id] = [review] - else: - reply_list[reply_id].append(review) - - pending_review = review_request.get_pending_review(request.user) - review_ids = list(reviews_id_map.keys()) + visited = None last_visited = 0 starred = False @@ -429,7 +334,7 @@ def review_detail(request, visited, visited_is_new = \ ReviewRequestVisit.objects.get_or_create( user=request.user, review_request=review_request) - last_visited = visited.timestamp.replace(tzinfo=utc) + last_visited = visited.timestamp.replace(tzinfo=utc).ctime() except ReviewRequestVisit.DoesNotExist: # Somehow, this visit was seen as created but then not # accessible. We need to log this and then continue on. @@ -453,249 +358,110 @@ def review_detail(request, except Profile.DoesNotExist: pass - draft = review_request.get_draft(request.user) - review_request_details = draft or review_request - - # Map diffset IDs to their object. - diffsets = review_request.get_diffsets() - diffsets_by_id = {} - - for diffset in diffsets: - diffsets_by_id[diffset.pk] = diffset - - # Find out if we can bail early. Generate an ETag for this. last_activity_time, updated_object = \ - review_request.get_last_activity(diffsets, public_reviews) - - if draft: - draft_timestamp = draft.last_updated - else: - draft_timestamp = "" + review_request.get_last_activity(data.diffsets, data.reviews) - if visited: - visibility = visited.visibility + if data.draft: + draft_timestamp = data.draft.last_updated else: - visibility = None + draft_timestamp = '' blocks = review_request.get_blocks() + # Find out if we can bail early. Generate an ETag for this. etag = encode_etag( '%s:%s:%s:%s:%s:%s:%s:%s:%s:%s' % (request.user, last_activity_time, draft_timestamp, - review_timestamp, review_request.last_review_activity_timestamp, + data.latest_review_timestamp, + review_request.last_review_activity_timestamp, is_rich_text_default_for_user(request.user), [r.pk for r in blocks], - starred, visibility, settings.AJAX_SERIAL)) + starred, visited and visited.visibility, settings.AJAX_SERIAL)) if etag_if_none_match(request, etag): return HttpResponseNotModified() - # Get the list of public ChangeDescriptions. - # - # We want to get the latest ChangeDescription along with this. This is - # best done here and not in a separate SQL query. - changedescs = list(review_request.changedescs.filter(public=True)) + data.query_data_post_etag() - if changedescs: - # We sort from newest to oldest, so the latest one is the first. - latest_timestamp = changedescs[0].timestamp - else: - latest_timestamp = None + entries = [] + reviews_entry_map = {} # Now that we have the list of public reviews and all that metadata, # being processing them and adding entries for display in the page. - # - # We do this here and not above because we don't want to build *too* much - # before the ETag check. - for review in public_reviews: - if not review.is_reply(): + for review in data.reviews: + if review.public and not review.is_reply(): # Mark as collapsed if the review is older than the latest # change, assuming there's no reply newer than last_visited. - latest_reply = reply_timestamps.get(review.pk, None) - collapsed = (latest_timestamp and - review.timestamp < latest_timestamp and - not (latest_reply and - last_visited and - last_visited < latest_reply)) - - entry = ReviewEntry(request, review_request, review, collapsed) + latest_reply = data.latest_timestamps_by_review_id.get(review.pk) + collapsed = ( + review.timestamp < data.latest_changedesc_timestamp and + not (latest_reply and + last_visited and + last_visited < latest_reply)) + + entry = ReviewEntry(request, review_request, review, collapsed, + data) reviews_entry_map[review.pk] = entry entries.append(entry) - # Link up all the review body replies. - for key, reply_list in (('_body_top_replies', body_top_replies), - ('_body_bottom_replies', body_bottom_replies)): - for reply_id, replies in six.iteritems(reply_list): - setattr(reviews_id_map[reply_id], key, replies) - - # Get all the file attachments and screenshots and build a couple maps, - # so we can easily associate those objects in comments. - # - # Note that we're fetching inactive file attachments and screenshots. - # is because any file attachments/screenshots created after the initial - # creation of the review request that were later removed will still need - # to be rendered as an added file in a change box. - file_attachments = [] - inactive_file_attachments = [] - screenshots = [] - inactive_screenshots = [] - - for attachment in review_request_details.get_file_attachments(): - attachment._comments = [] - file_attachments.append(attachment) - - for attachment in review_request_details.get_inactive_file_attachments(): - attachment._comments = [] - inactive_file_attachments.append(attachment) - - for screenshot in review_request_details.get_screenshots(): - screenshot._comments = [] - screenshots.append(screenshot) - - for screenshot in review_request_details.get_inactive_screenshots(): - screenshot._comments = [] - inactive_screenshots.append(screenshot) - - file_attachment_id_map = _build_id_map(file_attachments) - file_attachment_id_map.update(_build_id_map(inactive_file_attachments)) - screenshot_id_map = _build_id_map(screenshots) - screenshot_id_map.update(_build_id_map(inactive_screenshots)) - - issues = { - 'total': 0, - 'open': 0, - 'resolved': 0, - 'dropped': 0 - } + # Now that we have entries for all the reviews, go through all the comments + # and add them to those entries. + for comment in data.comments: + review = comment._review + + if review.is_reply(): + # This is a reply to a comment. + base_reply_to_id = comment._review.base_reply_to_id + + assert review.pk not in reviews_entry_map + assert base_reply_to_id in reviews_entry_map - # Get all the comments and attach them to the reviews. - for model, key, ordering in ( - (Comment, 'diff_comments', - ('comment__filediff', 'comment__first_line', 'comment__timestamp')), - (ScreenshotComment, 'screenshot_comments', None), - (FileAttachmentComment, 'file_attachment_comments', None), - (GeneralComment, 'general_comments', None)): - # Due to how we initially made the schema, we have a ManyToManyField - # inbetween comments and reviews, instead of comments having a - # ForeignKey to the review. This makes it difficult to easily go - # from a comment to a review ID. - # - # The solution to this is to not query the comment objects, but rather - # the through table. This will let us grab the review and comment in - # one go, using select_related. - related_field = model.review.related.field - comment_field_name = related_field.m2m_reverse_field_name() - through = related_field.rel.through - q = through.objects.filter(review__in=review_ids).select_related() - - if ordering: - q = q.order_by(*ordering) - - objs = list(q) - - # Two passes. One to build a mapping, and one to actually process - # comments. - comment_map = {} - - for obj in objs: - comment = getattr(obj, comment_field_name) - comment_map[comment.pk] = comment - comment._replies = [] - - for obj in objs: - comment = getattr(obj, comment_field_name) - - # Short-circuit some object fetches for the comment by setting - # some internal state on them. - assert obj.review_id in reviews_id_map - parent_review = reviews_id_map[obj.review_id] - comment._review = parent_review - comment._review_request = review_request - - # If the comment has an associated object that we've already - # queried, attach it to prevent a future lookup. - if isinstance(comment, ScreenshotComment): - if comment.screenshot_id in screenshot_id_map: - screenshot = screenshot_id_map[comment.screenshot_id] - comment.screenshot = screenshot - screenshot._comments.append(comment) - elif isinstance(comment, FileAttachmentComment): - if comment.file_attachment_id in file_attachment_id_map: - file_attachment = \ - file_attachment_id_map[comment.file_attachment_id] - comment.file_attachment = file_attachment - file_attachment._comments.append(comment) - - diff_against_id = comment.diff_against_file_attachment_id - - if diff_against_id in file_attachment_id_map: - file_attachment = file_attachment_id_map[diff_against_id] - comment.diff_against_file_attachment = file_attachment - - if parent_review.is_reply(): - base_reply_to_id = parent_review.base_reply_to_id - - # This is a reply to a comment. Add it to the list of replies. - assert obj.review_id not in reviews_entry_map - assert base_reply_to_id in reviews_entry_map - - # If there's an entry that isn't a reply, then it's - # orphaned. Ignore it. - if comment.is_reply(): - replied_comment = comment_map[comment.reply_to_id] - replied_comment._replies.append(comment) - - if not parent_review.public: - reviews_entry_map[base_reply_to_id].collasped = False - elif parent_review.public: - # This is a comment on a public review we're going to show. - # Add it to the list. - assert obj.review_id in reviews_entry_map - entry = reviews_entry_map[obj.review_id] - entry.add_comment(key, comment) - - if comment.issue_opened: - status_key = \ - comment.issue_status_to_string(comment.issue_status) - issues[status_key] += 1 - issues['total'] += 1 - - # Sort all the reviews and ChangeDescriptions into a single list, for - # display. - for changedesc in changedescs: + # Make sure that any review boxes containing draft replies are + # always expanded. + if comment.is_reply() and not review.public: + reviews_entry_map[base_reply_to_id].collapsed = False + elif review.public: + # This is a comment on a public review. + assert review.id in reviews_entry_map + + entry = reviews_entry_map[review.id] + entry.add_comment(comment._type, comment) + + # Add entries for the change descriptions. + for changedesc in data.changedescs: # Mark as collapsed if the change is older than a newer change - collapsed = (latest_timestamp and - changedesc.timestamp < latest_timestamp) + collapsed = (changedesc.timestamp < data.latest_changedesc_timestamp) - entry = ChangeEntry(request, review_request, changedesc, collapsed, - locals()) - entries.append(entry) + entries.append(ChangeEntry(request, review_request, changedesc, + collapsed, data)) + # Finally, sort all the entries (reviews and change descriptions) by their + # timestamp. entries.sort(key=lambda item: item.timestamp) close_description, close_description_rich_text = \ review_request.get_close_description() - latest_file_attachments = _get_latest_file_attachments(file_attachments) - siteconfig = SiteConfiguration.objects.get_current() + # Time to render the page! context_data = make_review_request_context(request, review_request, { 'blocks': blocks, - 'draft': draft, - 'review_request_details': review_request_details, + 'draft': data.draft, + 'review_request_details': data.review_request_details, 'review_request_visit': visited, 'send_email': siteconfig.get('mail_send_review_mail'), 'entries': entries, 'last_activity_time': last_activity_time, - 'review': pending_review, + 'review': review_request.get_pending_review(request.user), 'request': request, 'close_description': close_description, 'close_description_rich_text': close_description_rich_text, - 'issues': issues, - 'file_attachments': latest_file_attachments, - 'all_file_attachments': file_attachments, - 'screenshots': screenshots, + 'issues': data.issues, + 'file_attachments': _get_latest_file_attachments( + data.active_file_attachments), + 'all_file_attachments': data.all_file_attachments, + 'screenshots': data.active_screenshots, }) response = render_to_response(template_name,