diff --git a/reviewboard/diffviewer/views.py b/reviewboard/diffviewer/views.py index f1578f1cafebe1fb3296fbe03865b1c1eebd5ea6..0c6e8b9cd35456c05e2758c2b67659f34b582e4b 100644 --- a/reviewboard/diffviewer/views.py +++ b/reviewboard/diffviewer/views.py @@ -23,7 +23,6 @@ from django.urls import NoReverseMatch from django.utils.safestring import mark_safe from django.utils.translation import gettext as _ from django.views.generic.base import TemplateView, View -from djblets.siteconfig.models import SiteConfiguration from djblets.util.http import encode_etag, etag_if_none_match, set_etag from housekeeping import deprecate_non_keyword_only_args from pygments import highlight @@ -31,15 +30,21 @@ from pygments.formatters import HtmlFormatter from pygments.lexers import get_lexer_by_name from typing_extensions import NotRequired, TypedDict -from reviewboard.deprecation import RemovedInReviewBoard80Warning +from reviewboard.deprecation import ( + RemovedInReviewBoard80Warning, + RemovedInReviewBoard90Warning, +) from reviewboard.diffviewer.commit_utils import ( SerializedCommitHistoryDiffEntry, diff_histories) from reviewboard.diffviewer.diffutils import get_diff_files from reviewboard.diffviewer.errors import PatchError, UserVisibleError from reviewboard.diffviewer.models import DiffCommit, DiffSet, FileDiff -from reviewboard.diffviewer.renderers import (get_diff_renderer, - get_diff_renderer_class) +from reviewboard.diffviewer.renderers import ( + DiffRenderer, + get_diff_renderer, + get_diff_renderer_class, +) from reviewboard.diffviewer.settings import DiffSettings from reviewboard.scmtools.errors import FileNotFoundError from reviewboard.site.urlresolvers import local_site_reverse @@ -561,17 +566,17 @@ class DiffFragmentView(View): try: renderer_settings = self._get_renderer_settings(**kwargs) - etag = self.make_etag(renderer_settings, **kwargs) + etag = self.make_etag( + request=request, + renderer_settings=renderer_settings, + **kwargs) if etag_if_none_match(request, etag): return HttpResponseNotModified() - diff_info_or_response = self.process_diffset_info( + diff_info = self.process_diffset_info( base_filediff_id=base_filediff_id, **kwargs) - - if isinstance(diff_info_or_response, HttpResponse): - return diff_info_or_response except Http404: raise except Exception as e: @@ -588,15 +593,16 @@ class DiffFragmentView(View): return exception_traceback(self.request, e, self.error_template_name) - kwargs.update(diff_info_or_response) + kwargs.update(diff_info) try: context = self.get_context_data(**kwargs) renderer = self.create_renderer( + request=request, context=context, renderer_settings=renderer_settings, - *args, **kwargs) + **kwargs) response = renderer.render_to_response(request) except PatchError as e: logger.warning( @@ -674,10 +680,23 @@ class DiffFragmentView(View): return response - def make_etag(self, renderer_settings, filediff_id, - interfilediff_id=None, **kwargs): + @deprecate_non_keyword_only_args(RemovedInReviewBoard90Warning) + def make_etag( + self, + *, + renderer_settings: Mapping[str, Any], + filediff_id: int, + interfilediff_id: (int | None) = None, + request: (HttpRequest | None) = None, + **kwargs, + ) -> str: """Return an ETag identifying this render. + Version Changed: + 7.1.0: + * Deprecated non-keyword arguments. + * Added the ``request`` parameter. + Args: renderer_settings (dict): The settings determining how to render this diff. @@ -697,11 +716,17 @@ class DiffFragmentView(View): :py:class:`~reviewboard.diffviewer.models.filediff.FileDiff` on the other side of the diff revision, if viewing an interdiff. + request (django.http.HttpRequest, optional): + The request from the client. + + Version Added: + 7.1.0 + **kwargs (dict): Additional keyword arguments passed to the function. - Return: - unicode: + Returns: + str: The encoded ETag identifying this render. """ return encode_etag( @@ -715,11 +740,11 @@ class DiffFragmentView(View): def process_diffset_info( self, - diffset_or_id: Union[DiffSet, int], + diffset_or_id: DiffSet | int, filediff_id: int, - interfilediff_id: Optional[int] = None, - interdiffset_or_id: Optional[Union[DiffSet, int]] = None, - base_filediff_id: Optional[int] = None, + interfilediff_id: (int | None) = None, + interdiffset_or_id: (DiffSet | int | None) = None, + base_filediff_id: (int | None) = None, **kwargs, ) -> Mapping[str, Any]: """Process and return information on the desired diff. @@ -728,9 +753,6 @@ class DiffFragmentView(View): converted into DiffSets. A dictionary with the DiffSet and FileDiff information will be returned. - A subclass may instead return a HttpResponse to indicate an error - with the DiffSets. - Args: diffset_or_id (reviewboard.diffviewer.models.diffset.DiffSet or int): @@ -816,8 +838,7 @@ class DiffFragmentView(View): raise UserVisibleError(_( 'The requested FileDiff (ID %s) is not a valid base ' 'FileDiff for FileDiff %s.' - % (base_filediff_id, filediff_id) - )) + ) % (base_filediff_id, filediff_id)) assert diffset is not None @@ -846,9 +867,17 @@ class DiffFragmentView(View): 'diff_file': diff_file, } - def create_renderer(self, context, renderer_settings, diff_file, - *args, **kwargs): - """Creates the renderer for the diff. + @deprecate_non_keyword_only_args(RemovedInReviewBoard90Warning) + def create_renderer( + self, + *, + context: Mapping[str, Any], + renderer_settings: Mapping[str, Any], + diff_file: SerializedDiffFile, + request: HttpRequest, + **kwargs, + ) -> DiffRenderer: + """Create the renderer for the diff. This calculates all the state and data needed for rendering, and constructs a DiffRenderer with that data. That renderer is then @@ -857,6 +886,34 @@ class DiffFragmentView(View): If there's an error in looking up the necessary information, this may raise a UserVisibleError (best case), or some other form of Exception. + + Version Changed: + 7.1.0: + * Deprecated non-keyword arguments. + * Added the ``request`` parameter. + + Args: + context (dict): + The template rendering context. + + renderer_settings (dict): + The diff renderer settings.settings + + diff_file (reviewboard.diffviewer.diffutils.SerializedDiffFile): + The information on the diff file to render. + + request (django.http.HttpRequest): + The request from the client. + + Version Added: + 7.1.0 + + **kwargs (dict): + Keyword arguments, for future expansion. + + Returns: + reviewboard.diffviewer.renderers.DiffRenderer: + The resulting diff renderer. """ return get_diff_renderer( diff_file, @@ -1032,15 +1089,13 @@ class DownloadPatchErrorBundleView(DiffFragmentView): """ try: renderer_settings = self._get_renderer_settings(**kwargs) - etag = self.make_etag(renderer_settings, **kwargs) + etag = self.make_etag(renderer_settings=renderer_settings, + request=request, **kwargs) if etag_if_none_match(request, etag): return HttpResponseNotModified() - diff_info_or_response = self.process_diffset_info(**kwargs) - - if isinstance(diff_info_or_response, HttpResponse): - return diff_info_or_response + diff_info = self.process_diffset_info(**kwargs) except Http404: return HttpResponseNotFound() except Exception as e: @@ -1055,15 +1110,16 @@ class DownloadPatchErrorBundleView(DiffFragmentView): extra={'request': request}) return HttpResponseServerError() - kwargs.update(diff_info_or_response) + kwargs.update(diff_info) try: context = self.get_context_data(**kwargs) renderer = self.create_renderer( + request=request, context=context, renderer_settings=renderer_settings, - *args, **kwargs) + **kwargs) renderer.render_to_response(request) except PatchError as e: patch_error = e diff --git a/reviewboard/reviews/managers.py b/reviewboard/reviews/managers.py index 478dfc66be87c63d48c386df77c13f3521d6f65b..de373602879b9c4d4b1e9a19b98f9e52a0bdb815 100644 --- a/reviewboard/reviews/managers.py +++ b/reviewboard/reviews/managers.py @@ -22,10 +22,16 @@ from reviewboard.scmtools.models import Repository from reviewboard.site.models import LocalSite if TYPE_CHECKING: + from reviewboard.attachments.models import FileAttachment from reviewboard.changedescs.models import ChangeDescription from reviewboard.integrations.base import Integration from reviewboard.integrations.models import IntegrationConfig - from reviewboard.reviews.models import Review, ReviewRequest, StatusUpdate + from reviewboard.reviews.models import ( + FileAttachmentComment, + Review, + ReviewRequest, + StatusUpdate, + ) from reviewboard.site.models import AnyOrAllLocalSites @@ -235,6 +241,60 @@ class CommentManager(ConcurrencyManager): return queryset +class FileAttachmentCommentManager(CommentManager): + """A manager for FileAttachmentComment models. + + Version Added: + 7.1.0 + """ + + def for_file_attachment( + self, + *, + attachment: FileAttachment | None, + diff_against_file_attachment: (FileAttachment | None) = None, + user: (User | AnonymousUser | None) = None, + ) -> QuerySet[FileAttachmentComment]: + """Return a queryset for accessible comments on a given attachment. + + Args: + attachment (reviewboard.attachments.models.FileAttachment): + The file attachment. + + diff_against_file_attachment (reviewboard.attachments.models. + FileAttachment, optional): + The file attachment being diffed against. + + user (django.contrib.auth.models.User, optional): + The user making the request. + + Returns: + django.db.models.QuerySet: + A queryset for the visible comments. + + Raises: + ValueError: + The method was called without a valid file attachment. + """ + if attachment is None and diff_against_file_attachment is None: + raise ValueError( + 'attachment and diff_against_file_attachment cannot both be ' + 'None' + ) + + q = Q(file_attachment=attachment) + + if diff_against_file_attachment is not None: + q &= Q(diff_against_file_attachment=diff_against_file_attachment) + + if user is not None and user.is_authenticated: + q &= Q(review__public=True) | Q(review__user=user) + else: + q &= Q(review__public=True) + + return self.filter(q) + + class DefaultReviewerManager(Manager): """A manager for DefaultReviewer models.""" diff --git a/reviewboard/reviews/models/file_attachment_comment.py b/reviewboard/reviews/models/file_attachment_comment.py index 18ce302a5480bd64eb963ef23d75c053a96ae28a..129fdfe242751715d201bfb3ff216ef2edc4254b 100644 --- a/reviewboard/reviews/models/file_attachment_comment.py +++ b/reviewboard/reviews/models/file_attachment_comment.py @@ -12,9 +12,12 @@ from django.utils.translation import gettext_lazy as _ from typing_extensions import NotRequired, TypedDict from reviewboard.attachments.models import FileAttachment +from reviewboard.reviews.managers import FileAttachmentCommentManager from reviewboard.reviews.models.base_comment import BaseComment if TYPE_CHECKING: + from typing import ClassVar + from django.utils.safestring import SafeText from reviewboard.diffviewer.models import DiffSet, FileDiff @@ -68,22 +71,25 @@ class FileAttachmentCommentRevisionInfo(TypedDict): class FileAttachmentComment(BaseComment): """A comment on a file attachment.""" - anchor_prefix = "fcomment" - comment_type = "file" + anchor_prefix = 'fcomment' + comment_type = 'file' file_attachment = models.ForeignKey( FileAttachment, on_delete=models.CASCADE, verbose_name=_('file attachment'), - related_name="comments") + related_name='comments') diff_against_file_attachment = models.ForeignKey( FileAttachment, on_delete=models.CASCADE, verbose_name=_('diff against file attachment'), - related_name="diffed_against_comments", + related_name='diffed_against_comments', null=True, blank=True) + objects: ClassVar[FileAttachmentCommentManager] = \ + FileAttachmentCommentManager() + @cached_property def review_ui(self) -> Optional[ReviewUI]: """Return a ReviewUI appropriate for this comment. diff --git a/reviewboard/reviews/tests/test_reviews_diff_fragment_view.py b/reviewboard/reviews/tests/test_reviews_diff_fragment_view.py index dabf1f64ed502e5269ed63ede63816d38f7ffc89..51752a0263c8c358c6190b58b6a30ccc44a210a5 100644 --- a/reviewboard/reviews/tests/test_reviews_diff_fragment_view.py +++ b/reviewboard/reviews/tests/test_reviews_diff_fragment_view.py @@ -241,3 +241,111 @@ class ReviewsFileAttachmentDiffFragmentViewTests(BaseFileAttachmentTestCase): orig_attachment) self.assertEqual(response.context['modified_diff_file_attachment'], modified_attachment) + + def test_etag_match(self) -> None: + """Testing ETag generation for binary file fragment with comments""" + user = User.objects.get(username='doc') + review_request = self.create_review_request(submitter=user) + filediff = self.make_filediff( + is_new=False, + diffset_history=review_request.diffset_history) + + # Create file attachments for the binary file. + uploaded_file = self.make_uploaded_file() + orig_attachment = FileAttachment.objects.create_from_filediff( + filediff, + orig_filename='binary-file.png', + file=uploaded_file, + mimetype='image/png', + from_modified=False) + modified_attachment = FileAttachment.objects.create_from_filediff( + filediff, + orig_filename='binary-file.png', + file=uploaded_file, + mimetype='image/png') + review_request.file_attachments.add(orig_attachment) + review_request.file_attachments.add(modified_attachment) + review_request.publish(user) + + # Create a review with file attachment comment. + review = self.create_review( + review_request=review_request, + user=user, + public=True) + + self.create_file_attachment_comment( + review=review, + file_attachment=modified_attachment, + diff_against_file_attachment=orig_attachment) + + # Request the fragment. + url = f'/r/{review_request.pk}/diff/1/fragment/{filediff.pk}/' + + self.client.login(username='doc', password='doc') + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) + + etag = response.headers['ETag'] + + # Fetch the fragment again with the same ETag. We should get a 304. + response = self.client.get(url, headers={'If-None-Match': etag}) + self.assertEqual(response.status_code, 304) + + def test_etag_new_comments(self) -> None: + """Testing ETag updates for binary file fragment with newly-added + comments + """ + user = User.objects.get(username='doc') + review_request = self.create_review_request(submitter=user) + filediff = self.make_filediff( + is_new=False, + diffset_history=review_request.diffset_history) + + # Create file attachments for the binary file. + uploaded_file = self.make_uploaded_file() + orig_attachment = FileAttachment.objects.create_from_filediff( + filediff, + orig_filename='binary-file.png', + file=uploaded_file, + mimetype='image/png', + from_modified=False) + modified_attachment = FileAttachment.objects.create_from_filediff( + filediff, + orig_filename='binary-file.png', + file=uploaded_file, + mimetype='image/png') + review_request.file_attachments.add(orig_attachment) + review_request.file_attachments.add(modified_attachment) + review_request.publish(user) + + # Create a review with file attachment comment. + review = self.create_review( + review_request=review_request, + user=user) + + self.create_file_attachment_comment( + review=review, + file_attachment=modified_attachment, + diff_against_file_attachment=orig_attachment) + + # Request the fragment. + url = f'/r/{review_request.pk}/diff/1/fragment/{filediff.pk}/' + + self.client.login(username='doc', password='doc') + response = self.client.get(url) + + self.assertEqual(response.status_code, 200) + + etag = response.headers['ETag'] + + # Create a new comment. + self.create_file_attachment_comment( + review=review, + file_attachment=modified_attachment, + diff_against_file_attachment=orig_attachment) + + # Fetch the fragment again with the same ETag. We should now get a 200 + # instead of a 304. + response = self.client.get(url, headers={'If-None-Match': etag}) + self.assertEqual(response.status_code, 200) diff --git a/reviewboard/reviews/views/diff_fragments.py b/reviewboard/reviews/views/diff_fragments.py index c5d9904e49cf3706be0c1473bbb2d15a0643bc9b..004b967d81200b227c0a91dd7075c5ed60452c4d 100644 --- a/reviewboard/reviews/views/diff_fragments.py +++ b/reviewboard/reviews/views/diff_fragments.py @@ -6,42 +6,47 @@ import io import logging import os import struct -from typing import Any, Dict, List, Optional, TYPE_CHECKING, Tuple, cast +from typing import Any, Dict, List, Optional, TYPE_CHECKING, Tuple from django.conf import settings from django.contrib.sites.models import Site from django.core.exceptions import MultipleObjectsReturned, ObjectDoesNotExist from django.core.files.base import ContentFile from django.db.models import Q -from django.http import HttpRequest, HttpResponse -from django.shortcuts import get_list_or_404 +from django.http import Http404, HttpRequest, HttpResponse +from django.shortcuts import get_object_or_404 from django.template.loader import render_to_string from django.utils.cache import patch_cache_control from django.utils.safestring import SafeString, mark_safe from django.views.generic.base import ContextMixin, View from djblets.siteconfig.models import SiteConfiguration -from djblets.util.dates import get_latest_timestamp +from djblets.util.http import encode_etag from djblets.views.generic.etag import ETagViewMixin +from housekeeping import deprecate_non_keyword_only_args from typing_extensions import TypedDict from reviewboard.attachments.mimetypes import guess_mimetype from reviewboard.attachments.models import FileAttachment +from reviewboard.deprecation import RemovedInReviewBoard90Warning from reviewboard.diffviewer.diffutils import (get_file_chunks_in_range, get_last_header_before_line, get_last_line_number_in_diff) from reviewboard.diffviewer.models import FileDiff -from reviewboard.diffviewer.renderers import DiffRenderer from reviewboard.diffviewer.settings import DiffSettings from reviewboard.diffviewer.views import (DiffFragmentView, exception_traceback_string) -from reviewboard.reviews.models import Comment +from reviewboard.reviews.models import Comment, FileAttachmentComment from reviewboard.reviews.ui.base import ReviewUI from reviewboard.reviews.views.mixins import ReviewRequestViewMixin from reviewboard.scmtools.core import FileLookupContext from reviewboard.site.urlresolvers import local_site_reverse if TYPE_CHECKING: + from collections.abc import Mapping + + from reviewboard.diffviewer.diffutils import SerializedDiffFile from reviewboard.diffviewer.models import DiffCommit + from reviewboard.diffviewer.renderers import DiffRenderer logger = logging.getLogger(__name__) @@ -297,6 +302,10 @@ class CommentDiffFragmentsView(ReviewRequestViewMixin, ETagViewMixin, Returns: str: The ETag for the page. + + Raises: + django.http.Http404: + The given parameters were not valid. """ q = (Q(pk__in=comment_ids.split(',')) & Q(review__review_request=self.review_request)) @@ -306,15 +315,19 @@ class CommentDiffFragmentsView(ReviewRequestViewMixin, ETagViewMixin, else: q &= Q(review__public=True) - self.comments = get_list_or_404(Comment, q) + comments = list(Comment.objects.filter(q).order_by('pk')) - latest_timestamp = get_latest_timestamp( - comment.timestamp - for comment in self.comments + if not comments: + raise Http404() + + timestamps = ':'.join( + comment.timestamp.isoformat() + for comment in comments ) - return '%s:%s:%s' % (comment_ids, latest_timestamp, - settings.TEMPLATE_SERIAL) + self.comments = comments + + return f'{comment_ids}:{timestamps}:{settings.TEMPLATE_SERIAL}' def get( self, @@ -426,21 +439,36 @@ class ReviewsDiffFragmentView(ReviewRequestViewMixin, DiffFragmentView): accepted query parameters. """ + def __init__( + self, + *args, + **kwargs, + ) -> None: + """Initialize the view. + + Args: + *args (tuple): + Positional arguments to pass through to the parent class. + + **kwargs (dict): + Keyword arguments to pass through to the parent class. + """ + super().__init__(*args, **kwargs) + + self._cached_diffset_info: (Mapping[str, Any] | None) = None + def process_diffset_info( self, revision: int, - interdiff_revision: Optional[int] = None, + interdiff_revision: (int | None) = None, **kwargs, - ) -> dict[str, Any]: + ) -> Mapping[str, Any]: """Process and return information on the desired diff. The diff IDs and other data passed to the view can be processed and converted into DiffSets. A dictionary with the DiffSet and FileDiff information will be returned. - If the review request cannot be accessed by the user, an HttpResponse - will be returned instead. - Args: revision (int): The revision of the diff to view. @@ -455,6 +483,9 @@ class ReviewsDiffFragmentView(ReviewRequestViewMixin, DiffFragmentView): dict: Information on the diff for use in the template and in queries. """ + if self._cached_diffset_info is not None: + return self._cached_diffset_info + draft = self.review_request.get_draft(user=self.request.user) if interdiff_revision is not None: @@ -464,17 +495,103 @@ class ReviewsDiffFragmentView(ReviewRequestViewMixin, DiffFragmentView): diffset = self.get_diff(revision, draft) - return super().process_diffset_info( + info = super().process_diffset_info( diffset_or_id=diffset, interdiffset_or_id=interdiffset, **kwargs) + self._cached_diffset_info = info + + return info + + def make_etag( + self, + *, + request: (HttpRequest | None) = None, + **kwargs, + ) -> str: + """Return an ETag identifying this render. + + Version Added: + 7.1.0 + + Args: + request (django.http.HttpRequest): + The request from the client. + + Version Added: + 7.1.0 + + **kwargs (dict): + Additional keyword arguments passed to the function. + + Returns: + str: + The encoded ETag identifying this render. + + Raises: + django.http.Http404: + The diff for the given parameters was not found. + """ + etag = super().make_etag(request=request, **kwargs) + + filediff_id = kwargs.get('filediff_id') + filediff = get_object_or_404(FileDiff, pk=filediff_id) + + if filediff.binary: + # For binary files, serialized comments get included with the + # Review UI's rendered HTML. We therefore need to include the + # comment timestamps in the ETag or reloading the page can + # show out of date comments. + # + # This isn't an issue for non-binary files because all the comments + # are included along with the main diff viewer page, rather than + # with individual fragments. + assert request is not None + + diff_info = self.process_diffset_info( + base_filediff_id=request.GET.get('base-filediff-id'), + **kwargs) + diff_file = diff_info['diff_file'] + + orig_attachment, modified_attachment = \ + self._get_attachment_objects_for_binary( + request=request, + filediff=diff_file['filediff'], + interfilediff=diff_file['interfilediff'], + base_filediff=diff_file['base_filediff'], + force_interdiff=diff_file['force_interdiff'], + is_new_file=diff_file['is_new_file'], + ) + + if orig_attachment or modified_attachment: + comment_timestamps = ( + FileAttachmentComment.objects.for_file_attachment( + attachment=modified_attachment, + diff_against_file_attachment=orig_attachment, + user=request.user, + ) + .order_by('pk') + .values_list('timestamp', flat=True) + ) + + if comment_timestamps: + timestamps = ':'.join( + timestamp.isoformat() + for timestamp in comment_timestamps + ) + etag = encode_etag(f'{etag}:{timestamps}') + + return etag + + @deprecate_non_keyword_only_args(RemovedInReviewBoard90Warning) def create_renderer( self, - context: dict[str, Any], - renderer_settings: dict[str, Any], - diff_file: dict[str, Any], - *args, + *, + context: Mapping[str, Any], + renderer_settings: Mapping[str, Any], + diff_file: SerializedDiffFile, + request: HttpRequest, **kwargs, ) -> DiffRenderer: """Create the DiffRenderer for this fragment. @@ -482,6 +599,11 @@ class ReviewsDiffFragmentView(ReviewRequestViewMixin, DiffFragmentView): This will augment the renderer for binary files by looking up file attachments, if review UIs are involved, disabling caching. + Version Changed: + 7.1: + * Deprecated non-keyword arguments. + * Added the ``request`` parameter. + Args: context (dict): The current render context. @@ -489,11 +611,11 @@ class ReviewsDiffFragmentView(ReviewRequestViewMixin, DiffFragmentView): renderer_settings (dict): The diff renderer settings. - diff_file (dict): + diff_file (reviewboard.diffviewer.diffutils.SerializedDiffFile): The information on the diff file to render. - *args (tuple): - Additional positional arguments from the parent class. + request (django.http.HttpRequest): + The request from the client. **kwargs (dict): Additional keyword arguments from the parent class. @@ -503,54 +625,33 @@ class ReviewsDiffFragmentView(ReviewRequestViewMixin, DiffFragmentView): The resulting diff renderer. """ renderer = super().create_renderer( + request=request, context=context, renderer_settings=renderer_settings, diff_file=diff_file, - *args, **kwargs) + **kwargs) if diff_file['binary']: # Determine the file attachments to display in the diff viewer, # if any. - filediff = diff_file['filediff'] - interfilediff = diff_file['interfilediff'] + orig_attachment, modified_attachment = \ + self._get_attachment_objects_for_binary( + request=request, + filediff=diff_file['filediff'], + interfilediff=diff_file['interfilediff'], + base_filediff=diff_file['base_filediff'], + force_interdiff=diff_file['force_interdiff'], + is_new_file=diff_file['is_new_file'], + ) - orig_attachment = None - modified_attachment = None - - if diff_file['force_interdiff']: - orig_attachment = self._get_diff_file_attachment( - filediff=filediff) - modified_attachment = self._get_diff_file_attachment( - filediff=interfilediff) - else: - modified_attachment = self._get_diff_file_attachment( - filediff=filediff) - - base_filediff = diff_file['base_filediff'] - - if base_filediff is not None: - orig_attachment = self._get_diff_file_attachment( - filediff=base_filediff) - elif not diff_file['is_new_file']: - orig_attachment = self._get_diff_file_attachment( - filediff=filediff, use_modified=False) - - if (orig_attachment is None and - modified_attachment is not None): - # We only fetch the original version of the file if we - # already have an attachment for the modified version. - # This way we're not cluttering up the DB and - # filesystem with attachments that aren't helpful for - # the review process. - request = cast(HttpRequest, context.get('request')) - orig_attachment = self._create_attachment_for_orig( - request=request, filediff=filediff) - - diff_review_ui_html: Optional[str] = None - orig_review_ui_class: Optional[type[ReviewUI]] = None - orig_review_ui_html: Optional[str] = None - modified_review_ui_class: Optional[type[ReviewUI]] = None - modified_review_ui_html: Optional[str] = None + diff_review_ui_html: (str | None) = None + orig_review_ui_class: (type[ReviewUI[Any, Any, Any]] | None) = None + orig_review_ui_html: (str | None) = None + modified_review_ui_class: ( + type[ReviewUI[Any, Any, Any]] | + None + ) = None + modified_review_ui_html: (str | None) = None review_request = context['review_request'] if orig_attachment: @@ -602,6 +703,90 @@ class ReviewsDiffFragmentView(ReviewRequestViewMixin, DiffFragmentView): return renderer + def _get_attachment_objects_for_binary( + self, + *, + request: HttpRequest, + filediff: FileDiff, + interfilediff: FileDiff | None, + base_filediff: FileDiff | None, + force_interdiff: bool, + is_new_file: bool, + ) -> tuple[FileAttachment | None, FileAttachment | None]: + """Return the file attachments to display in the diff viewer. + + For any binary files which are part of the change, we use file + attachments for storage and review. + + Version Added: + 7.1.0 + + Args: + request (django.http.HttpRequest): + The request from the client. + + filediff (reviewboard.diffviewer.models.FileDiff): + The filediff for the file. + + interfilediff (reviewboard.diffviewer.models.FileDiff): + The filediff for the interdiff, if present. + + base_filediff (reviewboard.diffviewer.models.FileDiff): + The filediff for the base diff, if present. + + force_interdiff (bool): + Whether to force rendering an interdiff. + + This is used to show correct interdiffs for files that were + reverted in later versions. + + is_new_file (bool): + Whether the filediff corresponds to a newly-added file. + + Returns: + tuple: + A 2-tuple of: + + Tuple: + 0 (reviewboard.attachments.models.FileAttachment): + The file attachment for the original version of the file, + if present. + + 1 (reviewboard.attachments.models.FileAttachment): + The file attachment for the modified version of the file, + if present. + """ + orig_attachment: (FileAttachment | None) = None + modified_attachment: (FileAttachment | None) = None + + if force_interdiff: + assert interfilediff is not None + + orig_attachment = self._get_diff_file_attachment(filediff=filediff) + modified_attachment = self._get_diff_file_attachment( + filediff=interfilediff) + else: + modified_attachment = self._get_diff_file_attachment( + filediff=filediff) + + if base_filediff is not None: + orig_attachment = self._get_diff_file_attachment( + filediff=base_filediff) + elif not is_new_file: + orig_attachment = self._get_diff_file_attachment( + filediff=filediff, use_modified=False) + + if (orig_attachment is None and + modified_attachment is not None): + # We only fetch the original version of the file if we + # already have an attachment for the modified version. This + # way we're not cluttering up the DB and filesystem with + # attachments that aren't helpful for the review process. + orig_attachment = self._create_attachment_for_orig( + request=request, filediff=filediff) + + return orig_attachment, modified_attachment + def get_context_data( self, **kwargs,