From 200c94c6dc58e4b0bda1f3aab24a6c3cde0ba308 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 15 Mar 2023 21:14:11 -0600 Subject: [PATCH 1/7] Show "-" when the size of the MR diffs is 0 There are basically two times when this happens: 1. The MR is actually empty (the diff between the target and the source is nothing) 2. The MR - which generates diffs asynchronously - has not yet finished generating the computed diff Changelog: changed --- app/assets/javascripts/diffs/components/app.vue | 2 +- .../projects/merge_requests_controller.rb | 12 +++++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 9ccba88f7e6598..f54db97eb78d2e 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -453,7 +453,7 @@ export default { const badge = this.diffsTab.querySelector('.gl-badge'); if (this.diffsTab && badge) { - badge.textContent = this.diffFilesLength; + badge.textContent = this.diffFilesLength || '-'; } }, navigateToDiffFileNumber(number) { diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 35cf60a2c2461d..aefc161d117f96 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -437,11 +437,13 @@ def render_html_page end def get_diffs_count - if show_only_context_commits? - @merge_request.context_commits_diff.raw_diffs.size - else - @merge_request.diff_size - end + count = if show_only_context_commits? + @merge_request.context_commits_diff.raw_diffs.size + else + @merge_request.diff_size + end + + String(count) == "0" ? "-" : count end def merge_request_update_params -- GitLab From a62b918cb5a108206420a9bea063bd12793c21ac Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Mar 2023 11:09:15 -0600 Subject: [PATCH 2/7] Move the modified display of the zero count into the HAML --- .../projects/merge_requests_controller.rb | 12 +++++------- app/views/projects/merge_requests/_page.html.haml | 3 ++- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index aefc161d117f96..35cf60a2c2461d 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -437,13 +437,11 @@ def render_html_page end def get_diffs_count - count = if show_only_context_commits? - @merge_request.context_commits_diff.raw_diffs.size - else - @merge_request.diff_size - end - - String(count) == "0" ? "-" : count + if show_only_context_commits? + @merge_request.context_commits_diff.raw_diffs.size + else + @merge_request.diff_size + end end def merge_request_update_params diff --git a/app/views/projects/merge_requests/_page.html.haml b/app/views/projects/merge_requests/_page.html.haml index 4477a30d607cc1..b9b72fe37d9693 100644 --- a/app/views/projects/merge_requests/_page.html.haml +++ b/app/views/projects/merge_requests/_page.html.haml @@ -8,6 +8,7 @@ - page_card_attributes @merge_request.card_attributes - suggest_changes_help_path = help_page_path('user/project/merge_requests/reviews/suggestions.md') - mr_action = j(params[:tab].presence || 'show') +- diffs_count_display = @diffs_count == "0" ? "-" : @diffs_count - add_page_specific_style 'page_bundles/issuable' - add_page_specific_style 'page_bundles/design_management' - add_page_specific_style 'page_bundles/merge_requests' @@ -45,7 +46,7 @@ = render "projects/merge_requests/tabs/tab", name: "diffs", class: "diffs-tab js-diffs-tab", id: "diffs-tab", qa_selector: "diffs_tab" do = tab_link_for @merge_request, :diffs do = _("Changes") - = gl_badge_tag @diffs_count, { size: :sm } + = gl_badge_tag diffs_count_display, { size: :sm } .d-flex.flex-wrap.align-items-center.justify-content-lg-end #js-vue-discussion-counter{ data: { blocks_merge: @project.only_allow_merge_if_all_discussions_are_resolved?.to_s } } - if moved_mr_sidebar_enabled? -- GitLab From 2401a3d51053387a872cd1d3b2e0d01f66527bbc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Mar 2023 12:35:14 -0600 Subject: [PATCH 3/7] Move hyphen to constant Resolves review comment: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/114832/diffs?diff_id=635852604#0adb6eddad49cb6770f782f58cba15591184aa80_457_456 --- app/assets/javascripts/diffs/components/app.vue | 3 ++- app/assets/javascripts/diffs/constants.js | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index f54db97eb78d2e..3852657100474b 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -23,6 +23,7 @@ import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import notesEventHub from '~/notes/event_hub'; import { DynamicScroller, DynamicScrollerItem } from 'vendor/vue-virtual-scroller'; import { + ZERO_CHANGES_ALT_DISPLAY, TREE_LIST_WIDTH_STORAGE_KEY, INITIAL_TREE_WIDTH, MIN_TREE_WIDTH, @@ -453,7 +454,7 @@ export default { const badge = this.diffsTab.querySelector('.gl-badge'); if (this.diffsTab && badge) { - badge.textContent = this.diffFilesLength || '-'; + badge.textContent = this.diffFilesLength || ZERO_CHANGES_ALT_DISPLAY; } }, navigateToDiffFileNumber(number) { diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 873c48196698b8..a459def6b4b9f0 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -112,3 +112,6 @@ export const TRACKING_WHITESPACE_HIDE = 'i_code_review_diff_hide_whitespace'; export const TRACKING_CLICK_SINGLE_FILE_SETTING = 'i_code_review_click_single_file_mode_setting'; export const TRACKING_SINGLE_FILE_MODE = 'i_code_review_diff_single_file'; export const TRACKING_MULTIPLE_FILES_MODE = 'i_code_review_diff_multiple_files'; + +// UI +export const ZERO_CHANGES_ALT_DISPLAY = '-'; -- GitLab From f5e22059b2f62d40fbb75ba3f657bc6e8f36e46b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 22 Mar 2023 13:49:58 -0600 Subject: [PATCH 4/7] Handle variable output type of the count value --- app/views/projects/merge_requests/_page.html.haml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/views/projects/merge_requests/_page.html.haml b/app/views/projects/merge_requests/_page.html.haml index b9b72fe37d9693..f9fe0b33298656 100644 --- a/app/views/projects/merge_requests/_page.html.haml +++ b/app/views/projects/merge_requests/_page.html.haml @@ -8,7 +8,8 @@ - page_card_attributes @merge_request.card_attributes - suggest_changes_help_path = help_page_path('user/project/merge_requests/reviews/suggestions.md') - mr_action = j(params[:tab].presence || 'show') -- diffs_count_display = @diffs_count == "0" ? "-" : @diffs_count +-# @diffs_count is a number when the value is 0, but a string when there are other values +- diffs_count_display = String(@diffs_count) == "0" ? "-" : @diffs_count - add_page_specific_style 'page_bundles/issuable' - add_page_specific_style 'page_bundles/design_management' - add_page_specific_style 'page_bundles/merge_requests' -- GitLab From 5cc525b35c36bd57d55b94270b45412499340e89 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 23 Mar 2023 12:30:38 -0600 Subject: [PATCH 5/7] Use `to_s` instead of explicit typecast --- app/views/projects/merge_requests/_page.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/merge_requests/_page.html.haml b/app/views/projects/merge_requests/_page.html.haml index f9fe0b33298656..858bf2bf4c5557 100644 --- a/app/views/projects/merge_requests/_page.html.haml +++ b/app/views/projects/merge_requests/_page.html.haml @@ -9,7 +9,7 @@ - suggest_changes_help_path = help_page_path('user/project/merge_requests/reviews/suggestions.md') - mr_action = j(params[:tab].presence || 'show') -# @diffs_count is a number when the value is 0, but a string when there are other values -- diffs_count_display = String(@diffs_count) == "0" ? "-" : @diffs_count +- diffs_count_display = @diffs_count.to_s == "0" ? "-" : @diffs_count - add_page_specific_style 'page_bundles/issuable' - add_page_specific_style 'page_bundles/design_management' - add_page_specific_style 'page_bundles/merge_requests' -- GitLab From 37a37d86c40a09be038ce81d0c1283aa150b2ec6 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 27 Mar 2023 21:35:11 -0600 Subject: [PATCH 6/7] Add a utility for modifying the MR changes tab --- .../javascripts/diffs/utils/merge_request.js | 11 ++++ .../diffs/utils/merge_request_spec.js | 56 ++++++++++++++++++- 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/diffs/utils/merge_request.js b/app/assets/javascripts/diffs/utils/merge_request.js index 43e04a814c5ad1..20134b0c0caf24 100644 --- a/app/assets/javascripts/diffs/utils/merge_request.js +++ b/app/assets/javascripts/diffs/utils/merge_request.js @@ -1,3 +1,5 @@ +import { ZERO_CHANGES_ALT_DISPLAY } from '../constants'; + const endpointRE = /^(\/?(.+?)\/(.+?)\/-\/merge_requests\/(\d+)).*$/i; function getVersionInfo({ endpoint } = {}) { @@ -13,6 +15,15 @@ function getVersionInfo({ endpoint } = {}) { }; } +export function updateChangesTabCount({ + count, + badge = document.querySelector('.js-diffs-tab .gl-badge'), +} = {}) { + if (badge) { + badge.textContent = count || ZERO_CHANGES_ALT_DISPLAY; + } +} + export function getDerivedMergeRequestInformation({ endpoint } = {}) { let mrPath; let userOrGroup; diff --git a/spec/frontend/diffs/utils/merge_request_spec.js b/spec/frontend/diffs/utils/merge_request_spec.js index c070e8c004da4c..21599a3be45cc8 100644 --- a/spec/frontend/diffs/utils/merge_request_spec.js +++ b/spec/frontend/diffs/utils/merge_request_spec.js @@ -1,4 +1,8 @@ -import { getDerivedMergeRequestInformation } from '~/diffs/utils/merge_request'; +import { + updateChangesTabCount, + getDerivedMergeRequestInformation, +} from '~/diffs/utils/merge_request'; +import { ZERO_CHANGES_ALT_DISPLAY } from '~/diffs/constants'; import { diffMetadata } from '../mock_data/diff_metadata'; describe('Merge Request utilities', () => { @@ -24,6 +28,56 @@ describe('Merge Request utilities', () => { ...noVersion, }; + describe('updateChangesTabCount', () => { + let dummyTab; + let badge; + + beforeEach(() => { + dummyTab = document.createElement('div'); + dummyTab.classList.add('js-diffs-tab'); + dummyTab.insertAdjacentHTML('afterbegin', 'ERROR'); + badge = dummyTab.querySelector('.gl-badge'); + }); + + afterEach(() => { + dummyTab.remove(); + dummyTab = null; + badge = null; + }); + + it('uses the alt hyphen display when the new changes are falsey', () => { + updateChangesTabCount({ count: 0, badge }); + + expect(dummyTab.textContent).toBe(ZERO_CHANGES_ALT_DISPLAY); + + updateChangesTabCount({ badge }); + + expect(dummyTab.textContent).toBe(ZERO_CHANGES_ALT_DISPLAY); + + updateChangesTabCount({ count: false, badge }); + + expect(dummyTab.textContent).toBe(ZERO_CHANGES_ALT_DISPLAY); + }); + + it('uses the actual value for display when the value is truthy', () => { + updateChangesTabCount({ count: 42, badge }); + + expect(dummyTab.textContent).toBe('42'); + + updateChangesTabCount({ count: '999+', badge }); + + expect(dummyTab.textContent).toBe('999+'); + }); + + it('selects the proper element to modify by default', () => { + document.body.insertAdjacentElement('afterbegin', dummyTab); + + updateChangesTabCount({ count: 42 }); + + expect(dummyTab.textContent).toBe('42'); + }); + }); + describe('getDerivedMergeRequestInformation', () => { let endpoint = `${diffMetadata.latest_version_path}.json?searchParam=irrelevant`; -- GitLab From 8b1c2c2d636db2b2ac2f83bb2ceb27b0b632f530 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 27 Mar 2023 21:37:47 -0600 Subject: [PATCH 7/7] Defer to the MR utility for modifying the Changes tab count The tabs (among other things) are not part of this (Diffs) app. It doesn't really make sense to include the modifications of the rest of the page within this app. So now the tab count is updated by another module's utility (specifically for the overall concept of a Merge Request). --- .../javascripts/diffs/components/app.vue | 18 ++++-------------- .../javascripts/diffs/utils/merge_request.js | 2 ++ 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 3852657100474b..5951912af55537 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -23,7 +23,6 @@ import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import notesEventHub from '~/notes/event_hub'; import { DynamicScroller, DynamicScrollerItem } from 'vendor/vue-virtual-scroller'; import { - ZERO_CHANGES_ALT_DISPLAY, TREE_LIST_WIDTH_STORAGE_KEY, INITIAL_TREE_WIDTH, MIN_TREE_WIDTH, @@ -46,6 +45,7 @@ import { import diffsEventHub from '../event_hub'; import { reviewStatuses } from '../utils/file_reviews'; import { diffsApp } from '../utils/performance'; +import { updateChangesTabCount } from '../utils/merge_request'; import { queueRedisHllEvents } from '../utils/queue_events'; import CollapsedFilesWarning from './collapsed_files_warning.vue'; import CommitWidget from './commit_widget.vue'; @@ -338,8 +338,6 @@ export default { mrReviews: this.rehydratedMrReviews, }); - this.interfaceWithDOM(); - if (this.endpointCodequality) { this.setCodequalityEndpoint(this.endpointCodequality); } @@ -447,16 +445,6 @@ export default { notesEventHub.$off('refetchDiffData', this.refetchDiffData); notesEventHub.$off('fetchDiffData', this.fetchData); }, - interfaceWithDOM() { - this.diffsTab = document.querySelector('.js-diffs-tab'); - }, - updateChangesTabCount() { - const badge = this.diffsTab.querySelector('.gl-badge'); - - if (this.diffsTab && badge) { - badge.textContent = this.diffFilesLength || ZERO_CHANGES_ALT_DISPLAY; - } - }, navigateToDiffFileNumber(number) { this.navigateToDiffFileIndex(number - 1); }, @@ -483,7 +471,9 @@ export default { this.setTreeDisplay(); } - this.updateChangesTabCount(); + updateChangesTabCount({ + count: this.diffFilesLength, + }); }) .catch(() => { createAlert({ diff --git a/app/assets/javascripts/diffs/utils/merge_request.js b/app/assets/javascripts/diffs/utils/merge_request.js index 20134b0c0caf24..6847b8900d2f29 100644 --- a/app/assets/javascripts/diffs/utils/merge_request.js +++ b/app/assets/javascripts/diffs/utils/merge_request.js @@ -20,6 +20,8 @@ export function updateChangesTabCount({ badge = document.querySelector('.js-diffs-tab .gl-badge'), } = {}) { if (badge) { + // The purpose of this function is to assign to this parameter + /* eslint-disable-next-line no-param-reassign */ badge.textContent = count || ZERO_CHANGES_ALT_DISPLAY; } } -- GitLab