diff --git a/reviewboard/reviews/tests/test_review_ui.py b/reviewboard/reviews/tests/test_review_ui.py index b7cf17bb7e8a501f82bec0b96f24cbb17613bc5d..8f3d409e18de68a06a43bd443fddd11cadd234b3 100644 --- a/reviewboard/reviews/tests/test_review_ui.py +++ b/reviewboard/reviews/tests/test_review_ui.py @@ -28,6 +28,12 @@ class MyReviewUI(ReviewUI): supports_file_attachments = True js_model_class: str = 'RB.Test.ReviewUI' js_view_class: str = 'RB.Test.ReviewUIView' + js_bundle_names = [ + 'admin', + ] + css_bundle_names = [ + 'js-tests', + ] def get_caption(self, draft=None) -> str: return 'Test Caption' @@ -85,6 +91,7 @@ class ReviewUITests(SpyAgency, TestCase): self.review_request) self.assertEqual(context['review'], self.review) self.assertFalse(context['review_ui_inline']) + self.assertFalse(context['skip_static_media']) self.assertFalse(context['draft']) self.assertIs(context['review_ui'], review_ui) self.assertIs(context['obj'], reviewable_obj2) @@ -100,7 +107,7 @@ class ReviewUITests(SpyAgency, TestCase): self.assertEqual(context['comments'][1].text, 'Comment 2') def test_build_render_context_with_inline_true(self) -> None: - """Testing ReviewUI.build_render_context""" + """Testing ReviewUI.build_render_context with inline=1""" reviewable_obj1 = DummyReviewableObject() reviewable_obj2 = DummyReviewableObject() @@ -122,6 +129,49 @@ class ReviewUITests(SpyAgency, TestCase): self.assertEqual(context['review_request_details'], self.review_request) self.assertTrue(context['review_ui_inline']) + self.assertFalse(context['skip_static_media']) + self.assertFalse(context['draft']) + self.assertIs(context['review_ui'], review_ui) + self.assertIs(context['obj'], reviewable_obj2) + self.assertIs(context['diff_against_obj'], reviewable_obj1) + self.assertIn('close_description', context) + self.assertIn('close_description_rich_text', context) + self.assertIn('last_activity_time', context) + self.assertIn('social_page_image_url', context) + self.assertIn('review_ui_uuid', context) + self.assertNotIn('review', context) + self.assertEqual(context['custom_key'], '123') + + self.assertEqual(len(context['comments']), 2) + self.assertEqual(context['comments'][0].text, 'Comment 1') + self.assertEqual(context['comments'][1].text, 'Comment 2') + + def test_build_render_context_with_skip_static_media_true(self) -> None: + """Testing ReviewUI.build_render_context with skip_static_media=1""" + reviewable_obj1 = DummyReviewableObject() + reviewable_obj2 = DummyReviewableObject() + + review_ui = MyReviewUI(review_request=self.review_request, + obj=reviewable_obj2) + review_ui.set_diff_against(reviewable_obj1) + + request = self.create_http_request(data={ + 'skip-static-media': '1', + }) + context = review_ui.build_render_context(request=request, + inline=True) + + self.assertEqual(context['base_template'], + 'reviews/ui/base_inline.html') + self.assertEqual(context['caption'], 'Test Caption') + self.assertEqual(context['social_page_image_url'], '/cover-image.png') + self.assertEqual(context['social_page_title'], + 'Reviewable for Review Request #1: Test Caption') + self.assertEqual(context['review_request'], self.review_request) + self.assertEqual(context['review_request_details'], + self.review_request) + self.assertTrue(context['review_ui_inline']) + self.assertTrue(context['skip_static_media']) self.assertFalse(context['draft']) self.assertIs(context['review_ui'], review_ui) self.assertIs(context['obj'], reviewable_obj2) @@ -245,6 +295,76 @@ class ReviewUITests(SpyAgency, TestCase): self.assertIn('view = new RB.Test.ReviewUIView(', content) self.assertIn('renderedInline: true', content) + def test_render_to_response_with_skip_static_media_false(self) -> None: + """Testing ReviewUI.render_to_response with skip_static_media=0""" + review_ui = MyReviewUI(review_request=self.review_request, + obj=DummyReviewableObject()) + request = self.create_http_request(data={ + 'skip-static-media': '0', + }) + + response = review_ui.render_to_response(request) + self.assertEqual(response.status_code, 200) + self.assertIn( + b'link href="/static/rb/css/js-tests.min.css" ' + b'rel="stylesheet" type="text/css"', + response.content) + self.assertIn( + b'script type="text/javascript" src="/static/rb/js/admin.min.js"', + response.content) + + def test_render_to_response_with_skip_static_media_true(self) -> None: + """Testing ReviewUI.render_to_response with skip_static_media=1""" + review_ui = MyReviewUI(review_request=self.review_request, + obj=DummyReviewableObject()) + request = self.create_http_request(data={ + 'skip-static-media': '1', + }) + + response = review_ui.render_to_response(request) + self.assertEqual(response.status_code, 200) + self.assertNotIn( + b'link href="/static/rb/css/js-tests.min.css" ' + b'rel="stylesheet" type="text/css"', + response.content) + self.assertNotIn( + b'script type="text/javascript" src="/static/rb/js/admin.min.js"', + response.content) + + def test_render_to_string_with_skip_static_media_false(self) -> None: + """Testing ReviewUI.render_to_response with skip_static_media=0""" + review_ui = MyReviewUI(review_request=self.review_request, + obj=DummyReviewableObject()) + request = self.create_http_request(data={ + 'skip-static-media': '0', + }) + + content = review_ui.render_to_string(request) + self.assertIn( + 'link href="/static/rb/css/js-tests.min.css" ' + 'rel="stylesheet" type="text/css"', + content) + self.assertIn( + 'script type="text/javascript" src="/static/rb/js/admin.min.js"', + content) + + def test_render_to_string_with_skip_static_media_true(self) -> None: + """Testing ReviewUI.render_to_response with skip_static_media=1""" + review_ui = MyReviewUI(review_request=self.review_request, + obj=DummyReviewableObject()) + request = self.create_http_request(data={ + 'skip-static-media': '1', + }) + + content = review_ui.render_to_string(request) + self.assertNotIn( + 'link href="/static/rb/css/js-tests.min.css" ' + 'rel="stylesheet" type="text/css"', + content) + self.assertNotIn( + 'script type="text/javascript" src="/static/rb/js/admin.min.js"', + content) + def test_serialize_comments(self) -> None: """Testing ReviewUI.serialize_comments""" # Note that this test is factoring in both the user-owned draft diff --git a/reviewboard/reviews/tests/test_reviews_diff_viewer_view.py b/reviewboard/reviews/tests/test_reviews_diff_viewer_view.py index 1f220c4ae32d045cbe2866c698aeea6258de1340..676a1ff4243eb4551bfb05f7257af76dc85b0484 100644 --- a/reviewboard/reviews/tests/test_reviews_diff_viewer_view.py +++ b/reviewboard/reviews/tests/test_reviews_diff_viewer_view.py @@ -2,11 +2,15 @@ from __future__ import annotations +import kgb + +from reviewboard.attachments.models import FileAttachment +from reviewboard.reviews.ui.base import ReviewUI from reviewboard.site.urlresolvers import local_site_reverse from reviewboard.testing.testcase import BaseFileDiffAncestorTests, TestCase -class ReviewsDiffViewerViewTests(TestCase): +class ReviewsDiffViewerViewTests(kgb.SpyAgency, TestCase): """Unit tests for reviewboard.reviews.views.ReviewsDiffViewerView.""" fixtures = ['test_users', 'test_scmtools'] @@ -288,6 +292,59 @@ class ReviewsDiffViewerViewTests(TestCase): self.assertEqual({file_info['filediff'] for file_info in files}, {filediff1, filediff2, filediff3, filediff4}) + def test_with_static_media(self) -> None: + """Testing ReviewsDiffViewerView with binary files that have static + media defined in their review UI + """ + class DummyReviewUI(ReviewUI): + js_bundle_names = [ + 'js_bundle1', + 'js_bundle2', + ] + + css_bundle_names = [ + 'css_bundle1', + ] + + self.spy_on(ReviewUI.for_object, op=kgb.SpyOpReturn(DummyReviewUI)) + + review_request = self.create_review_request(create_repository=True, + publish=True) + diffset = self.create_diffset(review_request) + filediff1 = self.create_filediff( + diffset=diffset, + source_file='foo.png', + source_revision='123', + dest_file='foo.png', + diff=b'diff1', + binary=True) + FileAttachment.objects.create_from_filediff( + filediff1, + orig_filename='foo.png', + mimetype='image/png') + filediff2 = self.create_filediff(diffset, + source_file='src/main/test.c', + dest_file='src/main/test.cpp') + + response = self.client.get( + local_site_reverse( + 'view-diff-revision', + kwargs={ + 'review_request_id': review_request.display_id, + 'revision': diffset.revision, + })) + self.assertEqual(response.status_code, 200) + + files = response.context['files'] + self.assertEqual({file_info['filediff'] for file_info in files}, + {filediff1, filediff2}) + file1 = files[0] + self.assertEqual(file1['js_media'], {'js_bundle1', 'js_bundle2'}) + self.assertEqual(file1['css_media'], {'css_bundle1'}) + file2 = files[1] + self.assertNotIn('js_media', file2) + self.assertNotIn('cs_media', file2) + class CommitsTests(BaseFileDiffAncestorTests): """Tests for ReviewsDiffViewerView with commit history.""" diff --git a/reviewboard/reviews/ui/base.py b/reviewboard/reviews/ui/base.py index 7891057079c99b85cb3d048f1af7b62914ff5755..3f3dbe24cb00710e718f666643f79553154e41d1 100644 --- a/reviewboard/reviews/ui/base.py +++ b/reviewboard/reviews/ui/base.py @@ -464,6 +464,8 @@ class ReviewUI(Generic[ 'caption': caption, 'comments': self.get_comments(), 'last_activity_time': last_activity_time, + 'skip_static_media': + self.request.GET.get('skip-static-media') == '1', 'review_ui': self, 'review_ui_uuid': str(uuid4()), self.object_key: self.obj, diff --git a/reviewboard/reviews/views/diffviewer.py b/reviewboard/reviews/views/diffviewer.py index a9fa34827e28c8d48fc17e6be6b9d1893f2896ab..591e20acc96268caff3725d523f85bb65715b84f 100644 --- a/reviewboard/reviews/views/diffviewer.py +++ b/reviewboard/reviews/views/diffviewer.py @@ -11,7 +11,8 @@ from django.utils.translation import gettext from typing_extensions import NotRequired, TypeAlias, TypedDict from reviewboard.accounts.mixins import UserProfileRequiredViewMixin -from reviewboard.attachments.models import get_latest_file_attachments +from reviewboard.attachments.models import (FileAttachment, + get_latest_file_attachments) from reviewboard.diffviewer.commit_utils import get_base_and_tip_commits from reviewboard.diffviewer.diffutils import DiffFileExtraContext from reviewboard.diffviewer.models import DiffCommit, DiffSet, FileDiff @@ -31,7 +32,6 @@ if TYPE_CHECKING: from django.http import HttpRequest, HttpResponse - from reviewboard.attachments.models import FileAttachment from reviewboard.reviews.models import Comment, Screenshot from reviewboard.reviews.ui.base import SerializedCommentBlocks from reviewboard.reviews.ui.diff import SerializedDiffComment @@ -204,6 +204,15 @@ class SerializedReviewsDiffFile(TypedDict): #: Whether the file is binary. binary: bool + #: A set of CSS bundle names used to render the file. + #: + #: This will only be set for binary files that are rendered through a + #: :py:class:`~reviewboard.reviews.ui.base.ReviewUI`. + #: + #: Version Added: + #: 7.1 + css_media: NotRequired[set[str]] + #: Whether the file was deleted in the change. deleted: bool @@ -225,6 +234,15 @@ class SerializedReviewsDiffFile(TypedDict): #: Information about the interdiff FileDiff, when present. interfilediff: NotRequired[SerializedDiffFileFileDiff] + #: A set of JavaScript bundle names and URLs used to render the file. + #: + #: This will only be set for binary files that are rendered through a + #: :py:class:`~reviewboard.reviews.ui.base.ReviewUI`. + #: + #: Version Added: + #: 7.1 + js_media: NotRequired[set[str]] + #: Information about the FileDiff. filediff: SerializedDiffFileFileDiff @@ -437,6 +455,14 @@ class ReviewsDiffViewerView(ReviewRequestViewMixin, social_page_image_url = self.get_social_page_image_url( latest_file_attachments) + # Since we have these attachments, we can use them to avoid database + # lookups when fetching review UIs for binary file diffs below. + attachments_by_filediff_id: dict[int, FileAttachment] = {} + + for attachment in file_attachments: + if (f_id := attachment.added_in_filediff_id): + attachments_by_filediff_id[f_id] = attachment + # Build the status information shown below the summary. close_info = review_request.get_close_info() @@ -566,6 +592,8 @@ class ReviewsDiffViewerView(ReviewRequestViewMixin, filediff = f['filediff'] interfilediff = f['interfilediff'] base_filediff = f['base_filediff'] + binary = f['binary'] + filediff_id = filediff.pk interfilediff_id: Optional[int] = None base_filediff_id: Optional[int] = None @@ -576,7 +604,7 @@ class ReviewsDiffViewerView(ReviewRequestViewMixin, if interfilediff: interfilediff_id = interfilediff.pk - key = (filediff.pk, interfilediff_id, base_filediff_id) + key = (filediff_id, interfilediff_id, base_filediff_id) file_comments = comments.get(key, []) @@ -589,13 +617,13 @@ class ReviewsDiffViewerView(ReviewRequestViewMixin, data: SerializedReviewsDiffFile = { 'base_filediff_id': base_filediff_id, - 'binary': f['binary'], + 'binary': binary, 'deleted': f['deleted'], 'extra': f['extra'], - 'id': filediff.pk, + 'id': filediff_id, 'index': f['index'], 'filediff': { - 'id': filediff.pk, + 'id': filediff_id, 'revision': filediff.diffset.revision, }, 'modified_filename': f['modified_filename'], @@ -618,6 +646,22 @@ class ReviewsDiffViewerView(ReviewRequestViewMixin, data['force_interdiff'] = True data['interdiff_revision'] = f['force_interdiff_revision'] + if binary: + # Get the static media bundle names used in rendering the diff + # file. We use this to make sure that static media only gets + # added to the page once per review UI type in the diff viewer. + try: + attachment = attachments_by_filediff_id[filediff_id] + except KeyError: + # This file diff is from the cumulative diff. Need to hit + # the database instead. + attachment = FileAttachment.objects.get_for_filediff(filediff) + + if attachment and (review_ui := attachment.review_ui): + data['js_media'] = set(review_ui.js_bundle_names + + review_ui.js_files) + data['css_media'] = set(review_ui.css_bundle_names) + files.append(data) diff_context['files'] = files diff --git a/reviewboard/static/rb/js/reviews/models/diffFileModel.ts b/reviewboard/static/rb/js/reviews/models/diffFileModel.ts index a4f1e4c61b1863e225bae8c7918d64c5b409ac9d..0155e4367056b77f2f865ac98f9f6a228892ea79 100644 --- a/reviewboard/static/rb/js/reviews/models/diffFileModel.ts +++ b/reviewboard/static/rb/js/reviews/models/diffFileModel.ts @@ -66,6 +66,14 @@ export interface DiffFileAttrs extends ModelAttributes { /** Whether or not this is a binary file. */ binary: boolean; + /** + * A set of CSS bundle names used in rendering the file. + * + * Version Added: + * 7.1 + */ + cssMedia: Set | null; + /** Whether or not the file was deleted. */ deleted: boolean; @@ -92,6 +100,14 @@ export interface DiffFileAttrs extends ModelAttributes { /** Information about the interdiff, if present. */ interfilediff: SerializedFileDiff | null; + /** + * A set of JavaScript bundle names and URLs used in rendering the file. + * + * Version Added: + * 7.1 + */ + jsMedia: Set | null; + /** * The filename for the modified version of the file. * @@ -148,6 +164,7 @@ export interface DiffFileAttrs extends ModelAttributes { export interface DiffFileResourceData { base_filediff_id: number; binary: boolean; + css_media: Array | undefined; deleted: boolean; extra: DiffFileExtraContext; filediff: SerializedFileDiff; @@ -156,6 +173,7 @@ export interface DiffFileResourceData { index: number; interdiff_revision: number; interfilediff: SerializedFileDiff; + js_media: Array | undefined; modified_filename: string; modified_revision: string; newfile: boolean; @@ -174,6 +192,7 @@ export class DiffFile extends BaseModel { static defaults: Result> = { baseFileDiffID: null, binary: false, + cssMedia: null, deleted: false, extra: null, filediff: null, @@ -181,6 +200,7 @@ export class DiffFile extends BaseModel { forceInterdiffRevision: null, index: null, interfilediff: null, + jsMedia: null, modifiedFilename: null, modifiedRevision: null, newfile: false, @@ -207,6 +227,7 @@ export class DiffFile extends BaseModel { return { baseFileDiffID: rsp.base_filediff_id, binary: rsp.binary, + cssMedia: new Set(rsp.css_media), deleted: rsp.deleted, extra: rsp.extra, filediff: rsp.filediff, @@ -215,6 +236,7 @@ export class DiffFile extends BaseModel { id: rsp.id, index: rsp.index, interfilediff: rsp.interfilediff, + jsMedia: new Set(rsp.js_media), modifiedFilename: rsp.modified_filename, modifiedRevision: rsp.modified_revision, newfile: rsp.newfile, diff --git a/reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts b/reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts index c6310a0019a76e4edd9278426f3de69df2cc04ef..8cc815c4fc46ac24a2ae2254538b4ff4f8812486 100644 --- a/reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts +++ b/reviewboard/static/rb/js/reviews/models/diffReviewableModel.ts @@ -123,6 +123,13 @@ export class DiffReviewable * The option arguments that control the behavior of this function. * * Option Args: + * skipStaticMedia (boolean): + * For files that are rendered through a review UI, whether to + * exclude the review UI's static media in the rendered output. + * + * Version Added: + * 7.1 + * * showDeleted (boolean): * Determines whether or not we want to requeue the corresponding * diff in order to show its deleted content. @@ -133,6 +140,7 @@ export class DiffReviewable */ getRenderedDiff( options: { + skipStaticMedia?: boolean; showDeleted?: boolean; } = {}, ): Promise { @@ -141,6 +149,7 @@ export class DiffReviewable url: this._buildRenderedDiffURL({ index: this.get('file').get('index'), showDeleted: options.showDeleted, + skipStaticMedia: options.skipStaticMedia, }), }); } @@ -237,6 +246,13 @@ export class DiffReviewable * linesOfContext (number, optional): * The number of lines of context to load. * + * skipStaticMedia (boolean): + * For files that are rendered through a review UI, whether to + * exclude the review UI's static media in the rendered output. + * + * Version Added: + * 7.1 + * * showDeleted (boolean, optional): * Whether to show deleted content. * @@ -249,6 +265,7 @@ export class DiffReviewable chunkIndex?: number; index?: number; linesOfContext?: number; + skipStaticMedia?: boolean; showDeleted?: boolean; } = {}, ): string { @@ -288,8 +305,12 @@ export class DiffReviewable queryParts.push(`lines-of-context=${options.linesOfContext}`); } + if (options.skipStaticMedia) { + queryParts.push('skip-static-media=1'); + } + if (options.showDeleted) { - queryParts.push(`show-deleted=1`); + queryParts.push('show-deleted=1'); } queryParts.push(`_=${TEMPLATE_SERIAL}`); diff --git a/reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts b/reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts index 3e15ef0853b74280ccd3154aa549d19a16d567a9..fca623539da85bdd9ac5e5c7b572cc845c3557c4 100644 --- a/reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts +++ b/reviewboard/static/rb/js/reviews/models/tests/diffFileModelTests.ts @@ -53,6 +53,7 @@ suite('rb/diffviewer/models/DiffFile', function() { expect(data).not.toBe(undefined); expect(data.baseFileDiffID).toBe(12); expect(data.binary).toBe(false); + expect(data.cssMedia).toEqual(new Set()); expect(data.serializedCommentBlocks).toEqual({ '4-2': [ { @@ -76,6 +77,85 @@ suite('rb/diffviewer/models/DiffFile', function() { expect(data.interfilediff).not.toBe(undefined); expect(data.interfilediff.id).toBe(23); expect(data.interfilediff.revision).toBe(4); + expect(data.jsMedia).toEqual(new Set()); + expect(data.modifiedFilename).toBe('bar'); + expect(data.modifiedRevision).toBe('4'), + expect(data.newfile).toBe(true); + expect(data.origFilename).toBe('foo'); + expect(data.origRevision).toBe('3'); + }); + + it('API payloads with css_media and js_media', () => { + const diffFile = new DiffFile({ + base_filediff_id: 12, + binary: false, + css_media: ['css_bundle1'], + deleted: true, + filediff: { + id: 38, + revision: 2, + }, + id: 28, + index: 3, + interfilediff: { + id: 23, + revision: 4, + }, + js_media: ['js_bundle1', 'js_bundle2'], + modified_filename: 'bar', + modified_revision: '4', + newfile: true, + orig_filename: 'foo', + orig_revision: '3', + serialized_comment_blocks: { + '4-2': [ + { + comment_id: 1, + issue_opened: false, + line: 4, + localdraft: false, + num_lines: 2, + review_id: 1, + text: 'Comment', + user: { name: 'testuser' }, + }, + ], + }, + }, { + parse: true, + }); + + const data = diffFile.attributes; + + expect(data).not.toBe(undefined); + expect(data.baseFileDiffID).toBe(12); + expect(data.binary).toBe(false); + expect(data.cssMedia).toEqual(new Set(['css_bundle1'])); + expect(data.serializedCommentBlocks).toEqual({ + '4-2': [ + { + comment_id: 1, + issue_opened: false, + line: 4, + localdraft: false, + num_lines: 2, + review_id: 1, + text: 'Comment', + user: { name: 'testuser' }, + }, + ], + }); + expect(data.deleted).toBe(true); + expect(data.filediff).not.toBe(undefined); + expect(data.filediff.id).toBe(38); + expect(data.filediff.revision).toBe(2); + expect(data.id).toBe(28); + expect(data.index).toBe(3); + expect(data.interfilediff).not.toBe(undefined); + expect(data.interfilediff.id).toBe(23); + expect(data.interfilediff.revision).toBe(4); + expect(data.jsMedia).toEqual( + new Set(['js_bundle1', 'js_bundle2'])); expect(data.modifiedFilename).toBe('bar'); expect(data.modifiedRevision).toBe('4'), expect(data.newfile).toBe(true); diff --git a/reviewboard/static/rb/js/reviews/models/tests/diffReviewableModelTests.ts b/reviewboard/static/rb/js/reviews/models/tests/diffReviewableModelTests.ts index a20a01792b88f0612e3d60758819a0c3ce8bd7c1..9903c11f50c8cdf0a889ea0d59d52eeca8d9e150 100644 --- a/reviewboard/static/rb/js/reviews/models/tests/diffReviewableModelTests.ts +++ b/reviewboard/static/rb/js/reviews/models/tests/diffReviewableModelTests.ts @@ -186,5 +186,30 @@ suite('rb/diffviewer/models/DiffReviewable', function() { expect($.ajax).toHaveBeenCalled(); expect(html).toEqual('abc'); }); + + it('With skipStaticMedia=true', async () => { + const diffReviewable = new DiffReviewable({ + file: new DiffFile({ + index: 4, + }), + fileDiffID: 3, + reviewRequest: reviewRequest, + revision: 2, + }); + + spyOn($, 'ajax').and.callFake(request => { + expect(request.type).toBe('GET'); + expect(request.url).toBe( + '/r/1/diff/2/fragment/3/?index=4&skip-static-media=1&_=' + TEMPLATE_SERIAL); + + request.complete({ responseText: 'abc' }); + }); + + const html = await diffReviewable.getRenderedDiff({ + skipStaticMedia: true, + }); + expect($.ajax).toHaveBeenCalled(); + expect(html).toEqual('abc'); + }); }); }); diff --git a/reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts b/reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts index c2aaf48780d9a1093635d81aa7b2d2b0c06f0962..dc8facfe02f408e365f4f3f047c210d2f4280dbe 100644 --- a/reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts +++ b/reviewboard/static/rb/js/reviews/views/diffViewerPageView.ts @@ -120,6 +120,24 @@ export class DiffViewerPageView extends ReviewablePageView< _$anchors: JQuery; _commitListView: RB.DiffCommitListView = null; _diffReviewableViews: DiffReviewableView[] = []; + + /** + * A set of CSS bundle names that have been loaded onto the page. + * + * Whenever a file attachment review UI gets rendered onto the page, + * it's CSS bundle names will be added here. This is used to make sure + * we only load CSS static media once per review UI type. + */ + _renderedCSSMedia: Set = null; + + /** + * A set of JS bundle names and URLs that have been loaded onto the page. + * + * Whenever a file attachment review UI gets rendered onto the page, + * it's JS bundle names and URLs will be added here. This is used to make + * sure we only load JS static media once per review UI type. + */ + _renderedJSMedia: Set = null; _selectedAnchorIndex = -1; router: Router; @@ -169,6 +187,13 @@ export class DiffViewerPageView extends ReviewablePageView< this.listenTo(this.model.diffReviewables, 'populated', () => diffQueue.start()); + /* + * Track the static media that has been added to the page + * by rendered DiffReviewables. + */ + this._renderedJSMedia = new Set(); + this._renderedCSSMedia = new Set(); + this.router = new Router(); this.router.route( /^(\d+(?:-\d+)?)\/?(\?[^#]*)?/, @@ -518,11 +543,20 @@ export class DiffViewerPageView extends ReviewablePageView< * showDeleted (boolean, optional): * Determines whether or not we want to requeue the corresponding * diff in order to show its deleted content. + * + * skipStaticMedia (boolean): + * For files that are rendered through a review UI, whether to + * exclude the review UI's static media in the rendered output. + * + * Version Added: + * 7.1 + * */ queueLoadDiff( diffReviewable: DiffReviewable, options: { showDeleted?: boolean, + skipStaticMedia?: boolean, } = {}, ) { $.funcQueue('diff_files').add(async () => { @@ -542,6 +576,32 @@ export class DiffViewerPageView extends ReviewablePageView< const prefix = (options.showDeleted ? '#file' : '#file_container_'); + const diffFile = diffReviewable.get('file'); + + if (diffFile.get('binary')) { + /* + * Check if the reviewable's static media has already been + * added to the page. We want to make sure static media is + * only added once when there are multiple review UIs + * on the page loading the same media. + */ + const renderedJSMedia = this._renderedJSMedia; + const renderedCSSMedia = this._renderedCSSMedia; + const diffFileJSMedia = diffFile.get('jsMedia'); + const diffFileCSSMedia = diffFile.get('cssMedia'); + + if (diffFileJSMedia.size || diffFileCSSMedia.size) { + if (diffFileJSMedia.isSubsetOf(renderedJSMedia) && + diffFileCSSMedia.isSubsetOf(renderedCSSMedia)) { + options.skipStaticMedia = true; + } else { + this._renderedJSMedia = renderedJSMedia.union( + diffFileJSMedia); + this._renderedCSSMedia = renderedCSSMedia.union( + diffFileCSSMedia); + } + } + } const html = await diffReviewable.getRenderedDiff(options); const $container = $(prefix + fileDiffID) diff --git a/reviewboard/static/rb/js/reviews/views/tests/diffViewerPageViewTests.ts b/reviewboard/static/rb/js/reviews/views/tests/diffViewerPageViewTests.ts index e608fedfd551eb3755c17c5ea81727777234fb67..c531e87c99e9768d17b6d7e19e6e89f9ea00e556 100644 --- a/reviewboard/static/rb/js/reviews/views/tests/diffViewerPageViewTests.ts +++ b/reviewboard/static/rb/js/reviews/views/tests/diffViewerPageViewTests.ts @@ -843,6 +843,99 @@ suite('rb/pages/views/DiffViewerPageView', function() { }); }); + describe('queueLoadDiff static media handling', () => { + it('Static media is already rendered', async () => { + const diffFile = new RB.DiffFile({ + binary: true, + cssMedia: new Set(['css_bundle1']), + jsMedia: new Set(['js_bundle1', 'js_bundle2']), + }); + const diffReviewable = new RB.DiffReviewable({ + file: diffFile, + fileDiffID: 42, + id: 1, + reviewRequest: page.get('reviewRequest'), + }); + + spyOn(diffReviewable, 'getRenderedDiff'); + + pageView._renderedJSMedia = new Set( + ['js_bundle1', 'js_bundle2', 'js_bundle3']); + pageView._renderedCSSMedia = new Set( + ['css_bundle1', 'css_bundle2']); + + await pageView.queueLoadDiff(diffReviewable); + + const queue = $.funcQueue('diff_files'); + queue.start(); + + expect(diffReviewable.getRenderedDiff) + .toHaveBeenCalledWith( + { + skipStaticMedia: true, + }); + queue.clear(); + }); + + it('Static media is not rendered', async () => { + const diffFile = new RB.DiffFile({ + binary: true, + cssMedia: new Set(['css_bundle1']), + jsMedia: new Set(['js_bundle1', 'js_bundle2']), + }); + const diffReviewable = new RB.DiffReviewable({ + file: diffFile, + fileDiffID: 42, + id: 1, + reviewRequest: page.get('reviewRequest'), + }); + + spyOn(diffReviewable, 'getRenderedDiff'); + + pageView._renderedJSMedia = new Set( + ['js_bundle3']); + pageView._renderedCSSMedia = new Set([]); + + await pageView.queueLoadDiff(diffReviewable); + + const queue = $.funcQueue('diff_files'); + queue.start(); + + expect(diffReviewable.getRenderedDiff) + .toHaveBeenCalledWith({}); + queue.clear(); + }); + + it('Does not have static media', async () => { + const diffFile = new RB.DiffFile({ + binary: true, + cssMedia: new Set(), + jsMedia: new Set(), + }); + const diffReviewable = new RB.DiffReviewable({ + file: diffFile, + fileDiffID: 42, + id: 1, + reviewRequest: page.get('reviewRequest'), + }); + + spyOn(diffReviewable, 'getRenderedDiff'); + + pageView._renderedJSMedia = new Set( + ['js_bundle3']); + pageView._renderedCSSMedia = new Set([]); + + await pageView.queueLoadDiff(diffReviewable); + + const queue = $.funcQueue('diff_files'); + queue.start(); + + expect(diffReviewable.getRenderedDiff) + .toHaveBeenCalledWith({}); + queue.clear(); + }); + }); + describe('Page view/URL state', function() { let router; diff --git a/reviewboard/templates/reviews/ui/default.html b/reviewboard/templates/reviews/ui/default.html index 9b070ef00dfdd456ff4b079ea812c5d6c149d00f..e835a244b917ea500050fbaaa2923ee66c978ee7 100644 --- a/reviewboard/templates/reviews/ui/default.html +++ b/reviewboard/templates/reviews/ui/default.html @@ -2,8 +2,9 @@ {% load djblets_extensions djblets_js pipeline %} {% block review_ui_css %} -{% for bundle_name in review_ui.css_bundle_names %} -{% comment %} +{% if not skip_static_media %} +{% for bundle_name in review_ui.css_bundle_names %} +{% comment %} NOTE: On Review Board local development servers, rendering review UI static media from installed extension packages is broken when running with Djblets <4.1 and <5.3. Using development setups of extension @@ -14,32 +15,35 @@ NOTE: On Review Board local development servers, rendering review UI static being run with a lower Djblets version, then the `bundle_output` will be empty and we'll fall back on using the `javascript` template tag which will do the right thing on production servers. -{% endcomment %} -{% if review_ui.extension %} -{% ext_css_bundle review_ui.extension bundle_name as bundle_output %} -{% endif %} -{% if bundle_output %} +{% endcomment %} +{% if review_ui.extension %} +{% ext_css_bundle review_ui.extension bundle_name as bundle_output %} +{% endif %} +{% if bundle_output %} {{bundle_output}} -{% else %} -{% stylesheet bundle_name %} -{% endif %} -{% endfor %} +{% else %} +{% stylesheet bundle_name %} +{% endif %} +{% endfor %} +{% endif %} {% endblock %} {% block review_ui_scripts %} -{% for bundle_name in review_ui.js_bundle_names %} -{% if review_ui.extension %} -{% ext_js_bundle review_ui.extension bundle_name as bundle_output %} -{% endif %} -{% if bundle_output %} +{% if not skip_static_media %} +{% for bundle_name in review_ui.js_bundle_names %} +{% if review_ui.extension %} +{% ext_js_bundle review_ui.extension bundle_name as bundle_output %} +{% endif %} +{% if bundle_output %} {{bundle_output}} -{% else %} -{% javascript bundle_name %} -{% endif %} -{% endfor %} -{% for js_file in review_ui.js_files %} +{% else %} +{% javascript bundle_name %} +{% endif %} +{% endfor %} +{% for js_file in review_ui.js_files %} -{% endfor %} +{% endfor %} +{% endif %}