diff options
20 files changed, 157 insertions, 50 deletions
diff --git a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue index 5b03eda2eac..cadbd9c28a9 100644 --- a/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue +++ b/app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_missing_branch.vue @@ -1,9 +1,14 @@ <script> -import { GlIcon, GlTooltipDirective } from '@gitlab/ui'; -import { sprintf, s__ } from '~/locale'; +import { GlIcon, GlTooltipDirective, GlSprintf } from '@gitlab/ui'; +import { sprintf } from '~/locale'; import glFeatureFlagMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import mergeRequestQueryVariablesMixin from '../../mixins/merge_request_query_variables'; import missingBranchQuery from '../../queries/states/missing_branch.query.graphql'; +import { + MR_WIDGET_MISSING_BRANCH_WHICH, + MR_WIDGET_MISSING_BRANCH_RESTORE, + MR_WIDGET_MISSING_BRANCH_MANUALCLI, +} from '../../i18n'; import statusIcon from '../mr_widget_status_icon.vue'; export default { @@ -13,6 +18,7 @@ export default { }, components: { GlIcon, + GlSprintf, statusIcon, }, mixins: [glFeatureFlagMixin(), mergeRequestQueryVariablesMixin], @@ -45,26 +51,20 @@ export default { return this.mr.sourceBranchRemoved; }, - missingBranchName() { + type() { return this.sourceBranchRemoved ? 'source' : 'target'; }, - missingBranchNameMessage() { - return sprintf( - s__('mrWidget| Please restore it or use a different %{missingBranchName} branch'), - { - missingBranchName: this.missingBranchName, - }, - ); + name() { + return this.type === 'source' ? this.mr.sourceBranch : this.mr.targetBranch; + }, + warning() { + return sprintf(MR_WIDGET_MISSING_BRANCH_WHICH, { type: this.type, name: this.name }); + }, + restore() { + return sprintf(MR_WIDGET_MISSING_BRANCH_RESTORE, { type: this.type }); }, message() { - return sprintf( - s__( - 'mrWidget|If the %{missingBranchName} branch exists in your local repository, you can merge this merge request manually using the command line', - ), - { - missingBranchName: this.missingBranchName, - }, - ); + return sprintf(MR_WIDGET_MISSING_BRANCH_MANUALCLI, { type: this.type }); }, }, }; @@ -79,9 +79,14 @@ export default { 'gl-ml-0! gl-text-body!': glFeatures.restructuredMrWidget, }" class="bold js-branch-text" + data-testid="widget-content" > - <span class="capitalize" data-testid="missingBranchName"> {{ missingBranchName }} </span> - {{ s__('mrWidget|branch does not exist.') }} {{ missingBranchNameMessage }} + <gl-sprintf :message="warning"> + <template #code="{ content }"> + <code>{{ content }}</code> + </template> + </gl-sprintf> + {{ restore }} <gl-icon v-gl-tooltip :title="message" diff --git a/app/assets/javascripts/vue_merge_request_widget/i18n.js b/app/assets/javascripts/vue_merge_request_widget/i18n.js index c88e795e5f3..5d38a662947 100644 --- a/app/assets/javascripts/vue_merge_request_widget/i18n.js +++ b/app/assets/javascripts/vue_merge_request_widget/i18n.js @@ -1,4 +1,14 @@ -import { __ } from '~/locale'; +import { __, s__ } from '~/locale'; + +export const MR_WIDGET_MISSING_BRANCH_WHICH = s__( + 'mrWidget|The %{type} branch %{codeStart}%{name}%{codeEnd} does not exist.', +); +export const MR_WIDGET_MISSING_BRANCH_RESTORE = s__( + 'mrWidget|Please restore it or use a different %{type} branch.', +); +export const MR_WIDGET_MISSING_BRANCH_MANUALCLI = s__( + 'mrWidget|If the %{type} branch exists in your local repository, you can merge this merge request manually using the command line.', +); export const SQUASH_BEFORE_MERGE = { tooltipTitle: __('Required in this project.'), diff --git a/app/controllers/projects/merge_requests/drafts_controller.rb b/app/controllers/projects/merge_requests/drafts_controller.rb index 645720a0889..686d2c1dc1f 100644 --- a/app/controllers/projects/merge_requests/drafts_controller.rb +++ b/app/controllers/projects/merge_requests/drafts_controller.rb @@ -92,7 +92,8 @@ class Projects::MergeRequests::DraftsController < Projects::MergeRequests::Appli :commit_id, :note, :position, - :resolve_discussion + :resolve_discussion, + :line_code ).tap do |h| # Old FE version will still be sending `draft_note[commit_id]` as 'undefined'. # That can result to having a note linked to a commit with 'undefined' ID diff --git a/app/models/draft_note.rb b/app/models/draft_note.rb index febede9beba..9f7977fce68 100644 --- a/app/models/draft_note.rb +++ b/app/models/draft_note.rb @@ -25,6 +25,7 @@ class DraftNote < ApplicationRecord validates :merge_request_id, presence: true validates :author_id, presence: true, uniqueness: { scope: [:merge_request_id, :discussion_id] }, if: :discussion_id? validates :discussion_id, allow_nil: true, format: { with: /\A\h{40}\z/ } + validates :line_code, length: { maximum: 255 }, allow_nil: true scope :authored_by, ->(u) { where(author_id: u.id) } @@ -89,7 +90,11 @@ class DraftNote < ApplicationRecord end def line_code - @line_code ||= diff_file&.line_code_for_position(original_position) + super.presence || find_line_code + end + + def find_line_code + write_attribute(:line_code, diff_file&.line_code_for_position(original_position)) end def publish_params diff --git a/db/migrate/20211122103051_add_line_code_to_draft_notes.rb b/db/migrate/20211122103051_add_line_code_to_draft_notes.rb new file mode 100644 index 00000000000..c78b75f3a33 --- /dev/null +++ b/db/migrate/20211122103051_add_line_code_to_draft_notes.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddLineCodeToDraftNotes < Gitlab::Database::Migration[1.0] + # rubocop:disable Migration/AddLimitToTextColumns + # limit is added in db/migrate/20211124095704_add_draft_notes_line_code_text_limit.rb + def change + add_column :draft_notes, :line_code, :text + end + # rubocop:enable Migration/AddLimitToTextColumns +end diff --git a/db/migrate/20211124095704_add_draft_notes_line_code_text_limit.rb b/db/migrate/20211124095704_add_draft_notes_line_code_text_limit.rb new file mode 100644 index 00000000000..b3567d6b873 --- /dev/null +++ b/db/migrate/20211124095704_add_draft_notes_line_code_text_limit.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class AddDraftNotesLineCodeTextLimit < Gitlab::Database::Migration[1.0] + disable_ddl_transaction! + + def up + add_text_limit :draft_notes, :line_code, 255 + end + + def down + remove_text_limit :draft_notes, :line_code + end +end diff --git a/db/post_migrate/20220120094340_drop_position_from_security_findings.rb b/db/post_migrate/20220120094340_drop_position_from_security_findings.rb new file mode 100644 index 00000000000..56af346ad1b --- /dev/null +++ b/db/post_migrate/20220120094340_drop_position_from_security_findings.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class DropPositionFromSecurityFindings < Gitlab::Database::Migration[1.0] + enable_lock_retries! + + def change + remove_column :security_findings, :position, :integer + end +end diff --git a/db/schema_migrations/20211122103051 b/db/schema_migrations/20211122103051 new file mode 100644 index 00000000000..eacca224329 --- /dev/null +++ b/db/schema_migrations/20211122103051 @@ -0,0 +1 @@ +674a44e70291d6ed04318a5f6b639d216f2c26c43d15cb00e59b06cc6f6cc401
\ No newline at end of file diff --git a/db/schema_migrations/20211124095704 b/db/schema_migrations/20211124095704 new file mode 100644 index 00000000000..db18301ecd9 --- /dev/null +++ b/db/schema_migrations/20211124095704 @@ -0,0 +1 @@ +1f5ed9e7af3f56d0e11d1a2bb78a7430ce05af49c8102d1c75c8ff84ae4e1c6d
\ No newline at end of file diff --git a/db/schema_migrations/20220120094340 b/db/schema_migrations/20220120094340 new file mode 100644 index 00000000000..423adf5cc94 --- /dev/null +++ b/db/schema_migrations/20220120094340 @@ -0,0 +1 @@ +69d8283501ba6a4946a60e3812fe2cf3031fc4a915d6e7c6fac3bf272423f2f2
\ No newline at end of file diff --git a/db/structure.sql b/db/structure.sql index 47b5c7c76dd..c867071e5c5 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -13688,7 +13688,9 @@ CREATE TABLE draft_notes ( "position" text, original_position text, change_position text, - commit_id bytea + commit_id bytea, + line_code text, + CONSTRAINT check_c497a94a0e CHECK ((char_length(line_code) <= 255)) ); CREATE SEQUENCE draft_notes_id_seq @@ -19397,7 +19399,6 @@ CREATE TABLE security_findings ( confidence smallint NOT NULL, project_fingerprint text, deduplicated boolean DEFAULT false NOT NULL, - "position" integer, uuid uuid, overridden_uuid uuid, CONSTRAINT check_b9508c6df8 CHECK ((char_length(project_fingerprint) <= 40)) @@ -27560,8 +27561,6 @@ CREATE INDEX index_security_findings_on_project_fingerprint ON security_findings CREATE INDEX index_security_findings_on_scan_id_and_deduplicated ON security_findings USING btree (scan_id, deduplicated); -CREATE UNIQUE INDEX index_security_findings_on_scan_id_and_position ON security_findings USING btree (scan_id, "position"); - CREATE INDEX index_security_findings_on_scanner_id ON security_findings USING btree (scanner_id); CREATE INDEX index_security_findings_on_severity ON security_findings USING btree (severity); diff --git a/doc/user/group/saml_sso/index.md b/doc/user/group/saml_sso/index.md index 20ff4a201f5..e3e88ff51cb 100644 --- a/doc/user/group/saml_sso/index.md +++ b/doc/user/group/saml_sso/index.md @@ -21,7 +21,7 @@ SAML SSO is only configurable at the top-level group. If required, you can find [a glossary of common terms](../../../integration/saml.md#glossary-of-common-terms). -## Configuring your identity provider +## Configure your identity provider 1. On the top bar, select **Menu > Groups** and find your group. 1. On the left sidebar, select **Settings > SAML SSO**. @@ -32,7 +32,7 @@ If required, you can find [a glossary of common terms](../../../integration/saml 1. Configure the required [user attributes](#user-attributes), ensuring you include the user's email address. 1. While the default is enabled for most SAML providers, please ensure the app is set to have service provider initiated calls in order to link existing GitLab accounts. -1. Once the identity provider is set up, move on to [configuring GitLab](#configuring-gitlab). +1. Once the identity provider is set up, move on to [configuring GitLab](#configure-gitlab).  @@ -82,7 +82,7 @@ GitLab provides metadata XML that can be used to configure your identity provide 1. Copy the provided **GitLab metadata URL**. 1. Follow your identity provider's documentation and paste the metadata URL when it's requested. -## Configuring GitLab +## Configure GitLab After you set up your identity provider to work with GitLab, you must configure GitLab to use it for authentication: @@ -139,7 +139,7 @@ When SSO is enforced, users are not immediately revoked. If the user: The SAML standard means that you can use a wide range of identity providers with GitLab. Your identity provider might have relevant documentation. It can be generic SAML documentation or specifically targeted for GitLab. -When [configuring your identity provider](#configuring-your-identity-provider), please consider the notes below for specific providers to help avoid common issues and as a guide for terminology used. +When [configuring your identity provider](#configure-your-identity-provider), please consider the notes below for specific providers to help avoid common issues and as a guide for terminology used. For providers not listed below, you can refer to the [instance SAML notes on configuring an identity provider](../../../integration/saml.md#notes-on-configuring-your-identity-provider) for additional guidance on information your identity provider may require. @@ -293,12 +293,16 @@ convert the information to XML. An example SAML response is shown here. ### Role -Starting from [GitLab 13.3](https://gitlab.com/gitlab-org/gitlab/-/issues/214523), group owners can set a 'Default membership role' other than 'Guest'. To do so, [configure the SAML SSO for the group](#configuring-gitlab). That role becomes the starting access level of all users added to the group. +Starting from [GitLab 13.3](https://gitlab.com/gitlab-org/gitlab/-/issues/214523), group owners can set a +"Default membership role" other than Guest. To do so, [configure the SAML SSO for the group](#configure-gitlab). +That role becomes the starting access level of all users added to the group. Existing members with appropriate privileges can promote or demote users, as needed. If a user is already a member of the group, linking the SAML identity does not change their role. +Users given a "minimal access" role have [specific restrictions](../../permissions.md#users-with-minimal-access). + ### Blocking access To rescind a user's access to the group when only SAML SSO is configured, either: @@ -533,7 +537,7 @@ This can then be compared to the [NameID](#nameid) being sent by the identity pr If you receive a `404` during setup when using "verify configuration", make sure you have used the correct [SHA-1 generated fingerprint](../../../integration/saml.md#notes-on-configuring-your-identity-provider). -If a user is trying to sign in for the first time and the GitLab single sign-on URL has not [been configured](#configuring-your-identity-provider), they may see a 404. +If a user is trying to sign in for the first time and the GitLab single sign-on URL has not [been configured](#configure-your-identity-provider), they may see a 404. As outlined in the [user access section](#linking-saml-to-your-existing-gitlabcom-account), a group Owner needs to provide the URL to users. ### Message: "SAML authentication failed: Extern UID has already been taken" diff --git a/doc/user/permissions.md b/doc/user/permissions.md index ea79a026aa9..4ab9f9d6c74 100644 --- a/doc/user/permissions.md +++ b/doc/user/permissions.md @@ -473,11 +473,16 @@ with the permissions described on the documentation on [auditor users permission > [Introduced](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40942) in [GitLab Premium](https://about.gitlab.com/pricing/) 13.4. -Owners can add members with a "minimal access" role to a parent group. Such users don't -automatically have access to projects and subgroups underneath. To support such access, owners must explicitly add these "minimal access" users to the specific subgroups/projects. +Owners can add members with a "minimal access" role to a parent group. Such users don't automatically have access to +projects and subgroups underneath. Owners must explicitly add these "minimal access" users to the specific subgroups and +projects. -Users with minimal access can list the group in the UI and through the API. However, they cannot see -details such as projects or subgroups. They do not have access to the group's page or list any of its subgroups or projects. +Because of an [outstanding issue](https://gitlab.com/gitlab-org/gitlab/-/issues/267996), when minimal access users: + +- Sign in with standard web authentication, they receive a `404` error when accessing the parent group. +- Sign in with Group SSO, they receive a `404` error immediately because they are redirected to the parent group page. + +To work around the issue, give these users the Guest role or higher to any project or subgroup within the parent group. ### Minimal access users take license seats diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3bc963c7510..4adb76122de 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -42646,9 +42646,6 @@ msgstr "" msgid "mrWidgetNothingToMerge|Use merge requests to propose changes to your project and discuss them with your team. To make changes, push a commit or edit this merge request to use a different branch. With %{linkStart}CI/CD%{linkEnd}, automatically test your changes before merging." msgstr "" -msgid "mrWidget| Please restore it or use a different %{missingBranchName} branch" -msgstr "" - msgid "mrWidget|%{boldHeaderStart}Looks like there's no pipeline here.%{boldHeaderEnd}" msgstr "" @@ -42771,7 +42768,7 @@ msgstr "" msgid "mrWidget|Hide %{widget} details" msgstr "" -msgid "mrWidget|If the %{missingBranchName} branch exists in your local repository, you can merge this merge request manually using the command line" +msgid "mrWidget|If the %{type} branch exists in your local repository, you can merge this merge request manually using the command line." msgstr "" msgid "mrWidget|If the last pipeline ran in the fork project, it may be inaccurate. Before merge, we advise running a pipeline in this project." @@ -42875,6 +42872,9 @@ msgstr "" msgid "mrWidget|Plain diff" msgstr "" +msgid "mrWidget|Please restore it or use a different %{type} branch." +msgstr "" + msgid "mrWidget|Ready to be merged automatically. Ask someone with write access to this repository to merge this request" msgstr "" @@ -42920,6 +42920,9 @@ msgstr "" msgid "mrWidget|Show %{widget} details" msgstr "" +msgid "mrWidget|The %{type} branch %{codeStart}%{name}%{codeEnd} does not exist." +msgstr "" + msgid "mrWidget|The changes were merged into" msgstr "" @@ -42965,9 +42968,6 @@ msgstr "" msgid "mrWidget|Your password" msgstr "" -msgid "mrWidget|branch does not exist." -msgstr "" - msgid "mrWidget|into" msgstr "" diff --git a/spec/features/merge_request/user_sees_deleted_target_branch_spec.rb b/spec/features/merge_request/user_sees_deleted_target_branch_spec.rb index 7c93952ee99..dc50c3bc8db 100644 --- a/spec/features/merge_request/user_sees_deleted_target_branch_spec.rb +++ b/spec/features/merge_request/user_sees_deleted_target_branch_spec.rb @@ -15,7 +15,7 @@ RSpec.describe 'Merge request > User sees deleted target branch', :js do end it 'shows a message about missing target branch' do - expect(page).to have_content('Target branch does not exist') + expect(page).to have_content('The target branch feature does not exist') end it 'does not show link to target branch' do diff --git a/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb b/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb index e997fb3e853..39bba3f2f73 100644 --- a/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb +++ b/spec/features/merge_request/user_sees_mr_with_deleted_source_branch_spec.rb @@ -17,7 +17,7 @@ RSpec.describe 'Merge request > User sees MR with deleted source branch', :js do end it 'shows a message about missing source branch' do - expect(page).to have_content('Source branch does not exist.') + expect(page).to have_content('The source branch this-branch-does-not-exist does not exist.') end it 'still contains Discussion, Commits and Changes tabs' do @@ -27,7 +27,7 @@ RSpec.describe 'Merge request > User sees MR with deleted source branch', :js do expect(page).to have_content('Changes') end - expect(page).to have_content('Source branch does not exist.') + expect(page).to have_content('The source branch this-branch-does-not-exist does not exist.') click_on 'Changes' wait_for_requests diff --git a/spec/frontend/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js b/spec/frontend/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js index 936d673768c..ec5ec989d0f 100644 --- a/spec/frontend/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js +++ b/spec/frontend/vue_mr_widget/components/states/mr_widget_missing_branch_spec.js @@ -40,7 +40,7 @@ describe('MRWidgetMissingBranch', () => { async ({ sourceBranchRemoved, branchName }) => { await factory(sourceBranchRemoved, mergeRequestWidgetGraphql); - expect(wrapper.find('[data-testid="missingBranchName"]').text()).toContain(branchName); + expect(wrapper.find('[data-testid="widget-content"]').text()).toContain(branchName); }, ); }); diff --git a/spec/migrations/20220120094340_drop_position_from_security_findings_spec.rb b/spec/migrations/20220120094340_drop_position_from_security_findings_spec.rb new file mode 100644 index 00000000000..2ad9a8220c3 --- /dev/null +++ b/spec/migrations/20220120094340_drop_position_from_security_findings_spec.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_migration!('drop_position_from_security_findings') + +RSpec.describe DropPositionFromSecurityFindings do + let(:events) { table(:security_findings) } + + it 'correctly migrates up and down' do + reversible_migration do |migration| + migration.before -> { + expect(events.column_names).to include('position') + } + + migration.after -> { + events.reset_column_information + expect(events.column_names).not_to include('position') + } + end + end +end diff --git a/spec/models/draft_note_spec.rb b/spec/models/draft_note_spec.rb index 580a588ae1d..0f85871fd9e 100644 --- a/spec/models/draft_note_spec.rb +++ b/spec/models/draft_note_spec.rb @@ -20,6 +20,28 @@ RSpec.describe DraftNote do it { is_expected.to delegate_method(:file_identifier_hash).to(:diff_file).allow_nil } end + describe '#line_code' do + describe 'stored line_code' do + let(:draft_note) { build(:draft_note, merge_request: merge_request, line_code: '1234567890') } + + it 'returns stored line_code' do + expect(draft_note.line_code).to eq('1234567890') + end + end + + describe 'none stored line_code' do + let(:draft_note) { build(:draft_note, merge_request: merge_request) } + + before do + allow(draft_note).to receive(:find_line_code).and_return('none stored line_code') + end + + it 'returns found line_code' do + expect(draft_note.line_code).to eq('none stored line_code') + end + end + end + describe '#diff_file' do let(:draft_note) { build(:draft_note, merge_request: merge_request) } diff --git a/spec/services/draft_notes/create_service_spec.rb b/spec/services/draft_notes/create_service_spec.rb index 9e084dbed1c..528c8717525 100644 --- a/spec/services/draft_notes/create_service_spec.rb +++ b/spec/services/draft_notes/create_service_spec.rb @@ -92,7 +92,7 @@ RSpec.describe DraftNotes::CreateService do expect(merge_request).to receive_message_chain(:diffs, :clear_cache) - create_draft(note: 'This is a test') + create_draft(note: 'This is a test', line_code: '123') end end @@ -104,7 +104,7 @@ RSpec.describe DraftNotes::CreateService do expect(merge_request).not_to receive(:diffs) - create_draft(note: 'This is a test') + create_draft(note: 'This is a test', line_code: '123') end end end |