diff --git a/app/assets/javascripts/batch_comments/components/review_bar.vue b/app/assets/javascripts/batch_comments/components/review_bar.vue index 3cd1a2525e9..111b670596b 100644 --- a/app/assets/javascripts/batch_comments/components/review_bar.vue +++ b/app/assets/javascripts/batch_comments/components/review_bar.vue @@ -2,10 +2,20 @@ import { mapActions, mapGetters } from 'vuex'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import { REVIEW_BAR_VISIBLE_CLASS_NAME } from '../constants'; +import { PREVENT_LEAVING_PENDING_REVIEW } from '../i18n'; import PreviewDropdown from './preview_dropdown.vue'; import PublishButton from './publish_button.vue'; import SubmitDropdown from './submit_dropdown.vue'; +function closeInterrupt(event) { + event.preventDefault(); + + // This is the correct way to write backwards-compatible beforeunload listeners + // https://developer.chrome.com/blog/page-lifecycle-api/#the-beforeunload-event + /* eslint-disable-next-line no-return-assign, no-param-reassign */ + return (event.returnValue = PREVENT_LEAVING_PENDING_REVIEW); +} + export default { components: { PreviewDropdown, @@ -25,8 +35,26 @@ export default { }, mounted() { document.body.classList.add(REVIEW_BAR_VISIBLE_CLASS_NAME); + /* + * This stuff is a lot trickier than it looks. + * + * Mandatory reading: https://developer.mozilla.org/en-US/docs/Web/API/Window/beforeunload_event + * Some notable sentences: + * - "[...] browsers may not display prompts created in beforeunload event handlers unless the + * page has been interacted with, or may even not display them at all." + * - "Especially on mobile, the beforeunload event is not reliably fired." + * - "The beforeunload event is not compatible with the back/forward cache (bfcache) [...] + * It is recommended that developers listen for beforeunload only in this scenario, and only + * when they actually have unsaved changes, so as to minimize the effect on performance." + * + * Please ensure that this is really not working before you modify it, because there are a LOT + * of scenarios where browser behavior will make it _seem_ like it's not working, but it actually + * is under the right combination of contexts. + */ + window.addEventListener('beforeunload', closeInterrupt, { capture: true }); }, beforeDestroy() { + window.removeEventListener('beforeunload', closeInterrupt, { capture: true }); document.body.classList.remove(REVIEW_BAR_VISIBLE_CLASS_NAME); }, methods: { diff --git a/app/assets/javascripts/batch_comments/i18n.js b/app/assets/javascripts/batch_comments/i18n.js new file mode 100644 index 00000000000..6cdbf00f9ca --- /dev/null +++ b/app/assets/javascripts/batch_comments/i18n.js @@ -0,0 +1,3 @@ +import { __ } from '~/locale'; + +export const PREVENT_LEAVING_PENDING_REVIEW = __('There are unsubmitted review comments.'); diff --git a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js index a44b9827fe9..863d2a99972 100644 --- a/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js +++ b/app/assets/javascripts/batch_comments/stores/modules/batch_comments/actions.js @@ -1,9 +1,12 @@ import { isEmpty } from 'lodash'; + import createFlash from '~/flash'; import { scrollToElement } from '~/lib/utils/common_utils'; import { __ } from '~/locale'; + import { CHANGES_TAB, DISCUSSION_TAB, SHOW_TAB } from '../../../constants'; import service from '../../../services/drafts_service'; + import * as types from './mutation_types'; export const saveDraft = ({ dispatch }, draft) => @@ -15,6 +18,7 @@ export const addDraftToDiscussion = ({ commit }, { endpoint, data }) => .then((res) => res.data) .then((res) => { commit(types.ADD_NEW_DRAFT, res); + return res; }) .catch(() => { @@ -29,6 +33,7 @@ export const createNewDraft = ({ commit }, { endpoint, data }) => .then((res) => res.data) .then((res) => { commit(types.ADD_NEW_DRAFT, res); + return res; }) .catch(() => { diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js index 6c0c9c4e1d0..1cc96ef3d54 100644 --- a/app/assets/javascripts/diffs/constants.js +++ b/app/assets/javascripts/diffs/constants.js @@ -71,12 +71,15 @@ export const DIFF_FILE_MANUAL_COLLAPSE = 'manual'; export const STATE_IDLING = 'idle'; export const STATE_LOADING = 'loading'; export const STATE_ERRORED = 'errored'; +export const STATE_PENDING_REVIEW = 'pending_comments'; // State machine transitions export const TRANSITION_LOAD_START = 'LOAD_START'; export const TRANSITION_LOAD_ERROR = 'LOAD_ERROR'; export const TRANSITION_LOAD_SUCCEED = 'LOAD_SUCCEED'; export const TRANSITION_ACKNOWLEDGE_ERROR = 'ACKNOWLEDGE_ERROR'; +export const TRANSITION_HAS_PENDING_REVIEW = 'PENDING_REVIEW'; +export const TRANSITION_NO_REVIEW = 'NO_REVIEW'; export const RENAMED_DIFF_TRANSITIONS = { [`${STATE_IDLING}:${TRANSITION_LOAD_START}`]: STATE_LOADING, diff --git a/db/post_migrate/20220707075300_reschedule_backfill_imported_issue_search_data.rb b/db/post_migrate/20220707075300_reschedule_backfill_imported_issue_search_data.rb index b5124ce667c..912578d6b7c 100644 --- a/db/post_migrate/20220707075300_reschedule_backfill_imported_issue_search_data.rb +++ b/db/post_migrate/20220707075300_reschedule_backfill_imported_issue_search_data.rb @@ -14,10 +14,6 @@ class RescheduleBackfillImportedIssueSearchData < Gitlab::Database::Migration[2. delete_batched_background_migration(MIGRATION, :issues, :id, []) # reschedule the migration - min_value = Gitlab::Database::BackgroundMigration::BatchedMigration.find_by( - job_class_name: "BackfillIssueSearchData" - )&.max_value || BATCH_MIN_VALUE - queue_batched_background_migration( MIGRATION, :issues, @@ -32,4 +28,21 @@ class RescheduleBackfillImportedIssueSearchData < Gitlab::Database::Migration[2. def down delete_batched_background_migration(MIGRATION, :issues, :id, []) end + + private + + def min_value + start_value = Gitlab::Database::BackgroundMigration::BatchedMigration.find_by( + job_class_name: "BackfillIssueSearchData" + )&.max_value + + return BATCH_MIN_VALUE unless start_value + + max_value = Issue.maximum(:id) + + return BATCH_MIN_VALUE unless max_value + + # Just in case the issue's max ID is now lower than the history in the table + [start_value, max_value].min + end end diff --git a/doc/ci/docker/using_docker_build.md b/doc/ci/docker/using_docker_build.md index 69119cc7cdb..3f09cdb80bf 100644 --- a/doc/ci/docker/using_docker_build.md +++ b/doc/ci/docker/using_docker_build.md @@ -14,6 +14,9 @@ test it, and publish it to a container registry. To run Docker commands in your CI/CD jobs, you must configure GitLab Runner to support `docker` commands. +If you want to build Docker images without enabling privileged mode on the runner, +you can use a [Docker alternative](#docker-alternatives). + ## Enable Docker commands in your CI/CD jobs To enable Docker commands for your CI/CD jobs, you can use: @@ -22,9 +25,6 @@ To enable Docker commands for your CI/CD jobs, you can use: - [Docker-in-Docker](#use-docker-in-docker) - [Docker socket binding](#use-docker-socket-binding) -If you don't want to execute a runner in privileged mode, -but want to use `docker build`, you can also use [`kaniko`](using_kaniko.md) or [`buildah`](https://github.com/containers/buildah). - If you are using shared runners on GitLab.com, [learn more about how these runners are configured](../runners/index.md). @@ -831,6 +831,44 @@ If you're running multiple runners, you have to modify all configuration files. Read more about the [runner configuration](https://docs.gitlab.com/runner/configuration/) and [using the OverlayFS storage driver](https://docs.docker.com/engine/userguide/storagedriver/overlayfs-driver/). +## Docker alternatives + +To build Docker images without enabling privileged mode on the runner, you can +use one of these alternatives: + +- [`kaniko`](using_kaniko.md). +- [`buildah`](https://github.com/containers/buildah). + +For example, with `buildah`: + +```yaml +# Some details from https://major.io/2019/05/24/build-containers-in-gitlab-ci-with-buildah/ + +build: + stage: build + image: quay.io/buildah/stable + variables: + # Use vfs with buildah. Docker offers overlayfs as a default, but buildah + # cannot stack overlayfs on top of another overlayfs filesystem. + STORAGE_DRIVER: vfs + # Write all image metadata in the docker format, not the standard OCI format. + # Newer versions of docker can handle the OCI format, but older versions, like + # the one shipped with Fedora 30, cannot handle the format. + BUILDAH_FORMAT: docker + # You may need this workaround for some errors: https://stackoverflow.com/a/70438141/1233435 + BUILDAH_ISOLATION: chroot + FQ_IMAGE_NAME: "${CI_REGISTRY_IMAGE}/test" + before_script: + # Log in to the GitLab container registry + - export REGISTRY_AUTH_FILE=${HOME}/auth.json + - echo "$CI_REGISTRY_PASSWORD" | buildah login -u "$CI_REGISTRY_USER" --password-stdin $CI_REGISTRY + script: + - buildah images + - buildah build -t $FQ_IMAGE_NAME + - buildah images + - buildah push $FQ_IMAGE_NAME +``` + ## Use the GitLab Container Registry After you've built a Docker image, you can push it up to the built-in diff --git a/locale/gitlab.pot b/locale/gitlab.pot index cc8a045b0b5..5f0058380c0 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -39356,6 +39356,9 @@ msgstr "" msgid "There are several size limits in place." msgstr "" +msgid "There are unsubmitted review comments." +msgstr "" + msgid "There is already a repository with that name on disk" msgstr "" diff --git a/spec/features/issues/incident_issue_spec.rb b/spec/features/issues/incident_issue_spec.rb index d6ec7f1c539..56be1493ed2 100644 --- a/spec/features/issues/incident_issue_spec.rb +++ b/spec/features/issues/incident_issue_spec.rb @@ -30,7 +30,7 @@ RSpec.describe 'Incident Detail', :js do project.add_developer(user) sign_in(user) - visit project_issue_path(project, incident) + visit project_issues_incident_path(project, incident) wait_for_requests end @@ -49,72 +49,32 @@ RSpec.describe 'Incident Detail', :js do expect(incident_tabs).to have_content('Original alert: #1') end + aggregate_failures 'when on summary tab (default tab)' do + hidden_items = find_all('.js-issue-widgets') + + # Linked Issues/MRs and comment box + expect(hidden_items.count).to eq(2) + expect(hidden_items).to all(be_visible) + + edit_button = find_all('[aria-label="Edit title and description"]') + expect(edit_button).to all(be_visible) + end + aggregate_failures 'shows the Alert details tab' do click_link 'Alert details' expect(incident_tabs).to have_content('"title": "Alert title"') expect(incident_tabs).to have_content('"yet.another": 73') - end - end - end - context 'when on summary tab' do - before do - click_link 'Summary' - end - - it 'shows the summary tab with all components' do - page.within('.issuable-details') do - hidden_items = find_all('.js-issue-widgets') - - # Linked Issues/MRs and comment box - expect(hidden_items.count).to eq(2) - - expect(hidden_items).to all(be_visible) - end - end - - it 'shows the edit title and description button' do - edit_button = find_all('[aria-label="Edit title and description"]') - - expect(edit_button).to all(be_visible) - end - end - - context 'when on alert details tab' do - before do - click_link 'Alert details' - end - - it 'does not show the linked issues and notes/comment components' do - page.within('.issuable-details') do + # does not show the linked issues and notes/comment components' do hidden_items = find_all('.js-issue-widgets') # Linked Issues/MRs and comment box are hidden on page expect(hidden_items.count).to eq(0) - end - end - it 'does not show the edit title and description button' do - edit_button = find_all('[aria-label="Edit title and description"]') - - expect(edit_button.count).to eq(0) - end - end - - context 'when on timeline events tab from incident route' do - before do - visit project_issues_incident_path(project, incident) - wait_for_requests - click_link 'Timeline' - end - - it 'does not show the linked issues and notes/comment components' do - page.within('.issuable-details') do - hidden_items = find_all('.js-issue-widgets') - - # Linked Issues/MRs and comment box are hidden on page - expect(hidden_items.count).to eq(0) + # does not show the edit title and description button + edit_button = find_all('[aria-label="Edit title and description"]') + expect(edit_button.count).to eq(0) end end end @@ -126,7 +86,7 @@ RSpec.describe 'Incident Detail', :js do click_link 'Timeline' end - it 'does not show the linked issues and notes/comment commponents' do + it 'does not show the linked issues and notes/comment components' do page.within('.issuable-details') do hidden_items = find_all('.js-issue-widgets') @@ -140,7 +100,7 @@ RSpec.describe 'Incident Detail', :js do before do stub_feature_flags(incident_timeline: false) - visit project_issue_path(project, incident) + visit project_issues_incident_path(project, incident) wait_for_requests end diff --git a/spec/features/merge_request/batch_comments_spec.rb b/spec/features/merge_request/batch_comments_spec.rb index fafaea8ac68..f892b01e624 100644 --- a/spec/features/merge_request/batch_comments_spec.rb +++ b/spec/features/merge_request/batch_comments_spec.rb @@ -101,7 +101,7 @@ RSpec.describe 'Merge request > Batch comments', :js do write_diff_comment - visit_overview + visit_overview_with_pending_comment end it 'can add comment to review' do @@ -232,6 +232,14 @@ RSpec.describe 'Merge request > Batch comments', :js do wait_for_requests end + def visit_overview_with_pending_comment + accept_alert do + visit project_merge_request_path(merge_request.project, merge_request) + end + + wait_for_requests + end + def write_diff_comment(**params) click_diff_line(find_by_scrolling("[id='#{sample_compare.changes[0][:line_code]}']")) diff --git a/spec/frontend/batch_comments/components/review_bar_spec.js b/spec/frontend/batch_comments/components/review_bar_spec.js index f50db6ab210..f98e0a4c64a 100644 --- a/spec/frontend/batch_comments/components/review_bar_spec.js +++ b/spec/frontend/batch_comments/components/review_bar_spec.js @@ -6,6 +6,8 @@ import createStore from '../create_batch_comments_store'; describe('Batch comments review bar component', () => { let store; let wrapper; + let addEventListenerSpy; + let removeEventListenerSpy; const createComponent = (propsData = {}) => { store = createStore(); @@ -18,25 +20,58 @@ describe('Batch comments review bar component', () => { beforeEach(() => { document.body.className = ''; + + addEventListenerSpy = jest.spyOn(window, 'addEventListener'); + removeEventListenerSpy = jest.spyOn(window, 'removeEventListener'); }); afterEach(() => { + addEventListenerSpy.mockRestore(); + removeEventListenerSpy.mockRestore(); wrapper.destroy(); }); - it('it adds review-bar-visible class to body when review bar is mounted', async () => { - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + describe('when mounted', () => { + it('it adds review-bar-visible class to body', async () => { + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); - createComponent(); + createComponent(); - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(true); + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(true); + }); + + it('it adds a blocking handler to the `beforeunload` window event', () => { + expect(addEventListenerSpy).not.toBeCalled(); + + createComponent(); + + expect(addEventListenerSpy).toHaveBeenCalledTimes(1); + expect(addEventListenerSpy).toBeCalledWith('beforeunload', expect.any(Function), { + capture: true, + }); + }); }); - it('it removes review-bar-visible class to body when review bar is destroyed', async () => { - createComponent(); + describe('before destroyed', () => { + it('it removes review-bar-visible class to body', async () => { + createComponent(); - wrapper.destroy(); + wrapper.destroy(); - expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + expect(document.body.classList.contains(REVIEW_BAR_VISIBLE_CLASS_NAME)).toBe(false); + }); + + it('it removes the blocking handler from the `beforeunload` window event', () => { + createComponent(); + + expect(removeEventListenerSpy).not.toBeCalled(); + + wrapper.destroy(); + + expect(removeEventListenerSpy).toHaveBeenCalledTimes(1); + expect(removeEventListenerSpy).toBeCalledWith('beforeunload', expect.any(Function), { + capture: true, + }); + }); }); }); diff --git a/spec/migrations/reschedule_backfill_imported_issue_search_data_spec.rb b/spec/migrations/reschedule_backfill_imported_issue_search_data_spec.rb index 7d1377bbeba..7581c201a59 100644 --- a/spec/migrations/reschedule_backfill_imported_issue_search_data_spec.rb +++ b/spec/migrations/reschedule_backfill_imported_issue_search_data_spec.rb @@ -6,8 +6,22 @@ require_migration! RSpec.describe RescheduleBackfillImportedIssueSearchData do let_it_be(:reschedule_migration) { described_class::MIGRATION } - context 'when BackfillIssueSearchData.max_value is nil' do - it 'schedules a new batched migration with a default value' do + def create_batched_migration(max_value:) + Gitlab::Database::BackgroundMigration::BatchedMigration + .create!( + max_value: max_value, + batch_size: 200, + sub_batch_size: 20, + interval: 120, + job_class_name: 'BackfillIssueSearchData', + table_name: 'issues', + column_name: 'id', + gitlab_schema: 'glschema' + ) + end + + shared_examples 'backfill rescheduler' do + it 'schedules a new batched migration' do reversible_migration do |migration| migration.before -> { expect(reschedule_migration).not_to have_scheduled_batched_migration @@ -17,39 +31,60 @@ RSpec.describe RescheduleBackfillImportedIssueSearchData do table_name: :issues, column_name: :id, interval: described_class::DELAY_INTERVAL, - batch_min_value: described_class::BATCH_MIN_VALUE + batch_min_value: batch_min_value ) } end end end + context 'when BackfillIssueSearchData.max_value is nil' do + let(:batch_min_value) { described_class::BATCH_MIN_VALUE } + + it_behaves_like 'backfill rescheduler' + end + context 'when BackfillIssueSearchData.max_value exists' do + let(:batch_min_value) { described_class::BATCH_MIN_VALUE } + before do - Gitlab::Database::BackgroundMigration::BatchedMigration - .create!( - max_value: 200, - batch_size: 200, - sub_batch_size: 20, - interval: 120, - job_class_name: 'BackfillIssueSearchData', - table_name: 'issues', - column_name: 'id', - gitlab_schema: 'glschema' - ) + create_batched_migration(max_value: 200) end - it 'schedules a new batched migration with a custom max_value' do - reversible_migration do |migration| - migration.after -> { - expect(reschedule_migration).to have_scheduled_batched_migration( - table_name: :issues, - column_name: :id, - interval: described_class::DELAY_INTERVAL, - batch_min_value: 200 - ) - } - end + it_behaves_like 'backfill rescheduler' + end + + context 'when an issue is available' do + let_it_be(:namespaces_table) { table(:namespaces) } + let_it_be(:projects_table) { table(:projects) } + + let(:namespace) { namespaces_table.create!(name: 'gitlab-org', path: 'gitlab-org') } + let(:project) { projects_table.create!(name: 'gitlab', path: 'gitlab-org/gitlab-ce', namespace_id: namespace.id, project_namespace_id: namespace.id) } # rubocop:disable Layout/LineLength + let(:issue) { table(:issues).create!(project_id: project.id, title: 'test title', description: 'test description') } + + before do + create_batched_migration(max_value: max_value) + end + + context 'when BackfillIssueSearchData.max_value = Issue.maximum(:id)' do + let(:max_value) { issue.id } + let(:batch_min_value) { max_value } + + it_behaves_like 'backfill rescheduler' + end + + context 'when BackfillIssueSearchData.max_value > Issue.maximum(:id)' do + let(:max_value) { issue.id + 1 } + let(:batch_min_value) { issue.id } + + it_behaves_like 'backfill rescheduler' + end + + context 'when BackfillIssueSearchData.max_value < Issue.maximum(:id)' do + let(:max_value) { issue.id - 1 } + let(:batch_min_value) { max_value } + + it_behaves_like 'backfill rescheduler' end end end