diff options
36 files changed, 432 insertions, 173 deletions
diff --git a/app/assets/javascripts/ide/components/repo_editor.vue b/app/assets/javascripts/ide/components/repo_editor.vue index 99423362924..8a709b31ea0 100644 --- a/app/assets/javascripts/ide/components/repo_editor.vue +++ b/app/assets/javascripts/ide/components/repo_editor.vue @@ -19,7 +19,7 @@ export default { }, }, computed: { - ...mapState(['leftPanelCollapsed', 'rightPanelCollapsed', 'viewer', 'delayViewerUpdated']), + ...mapState(['rightPanelCollapsed', 'viewer', 'delayViewerUpdated', 'panelResizing']), ...mapGetters(['currentMergeRequest']), shouldHideEditor() { return this.file && this.file.binary && !this.file.raw; @@ -42,15 +42,17 @@ export default { this.initMonaco(); } }, - leftPanelCollapsed() { - this.editor.updateDimensions(); - }, rightPanelCollapsed() { this.editor.updateDimensions(); }, viewer() { this.createEditorInstance(); }, + panelResizing() { + if (!this.panelResizing) { + this.editor.updateDimensions(); + } + }, }, beforeDestroy() { this.editor.dispose(); diff --git a/app/assets/javascripts/ide/lib/editor.js b/app/assets/javascripts/ide/lib/editor.js index 6b4ba30e086..001737d6ee8 100644 --- a/app/assets/javascripts/ide/lib/editor.js +++ b/app/assets/javascripts/ide/lib/editor.js @@ -69,6 +69,7 @@ export default class Editor { occurrencesHighlight: false, renderLineHighlight: 'none', hideCursorInOverviewRuler: true, + renderSideBySide: Editor.renderSideBySide(domElement), })), ); @@ -81,7 +82,7 @@ export default class Editor { } attachModel(model) { - if (this.instance.getEditorType() === 'vs.editor.IDiffEditor') { + if (this.isDiffEditorType) { this.instance.setModel({ original: model.getOriginalModel(), modified: model.getModel(), @@ -153,6 +154,7 @@ export default class Editor { updateDimensions() { this.instance.layout(); + this.updateDiffView(); } setPosition({ lineNumber, column }) { @@ -171,4 +173,20 @@ export default class Editor { this.disposable.add(this.instance.onDidChangeCursorPosition(e => cb(this.instance, e))); } + + updateDiffView() { + if (!this.isDiffEditorType) return; + + this.instance.updateOptions({ + renderSideBySide: Editor.renderSideBySide(this.instance.getDomNode()), + }); + } + + get isDiffEditorType() { + return this.instance.getEditorType() === 'vs.editor.IDiffEditor'; + } + + static renderSideBySide(domElement) { + return domElement.offsetWidth >= 700; + } } diff --git a/app/assets/javascripts/ide/lib/editor_options.js b/app/assets/javascripts/ide/lib/editor_options.js index a213862f9b3..9f895d49f2e 100644 --- a/app/assets/javascripts/ide/lib/editor_options.js +++ b/app/assets/javascripts/ide/lib/editor_options.js @@ -6,7 +6,7 @@ export const defaultEditorOptions = { minimap: { enabled: false, }, - wordWrap: 'bounded', + wordWrap: 'on', }; export default [ diff --git a/app/assets/javascripts/notes.js b/app/assets/javascripts/notes.js index b0573510ff9..ac70ddb3ff4 100644 --- a/app/assets/javascripts/notes.js +++ b/app/assets/javascripts/notes.js @@ -1190,12 +1190,12 @@ export default class Notes { addForm = false; let lineTypeSelector = ''; rowCssToAdd = - '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line" colspan="2"></td><td class="notes_content"><div class="content"></div></td></tr>'; + '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line" colspan="2"></td><td class="notes_content"><div class="content discussion-notes"></div></td></tr>'; // In parallel view, look inside the correct left/right pane if (this.isParallelView()) { lineTypeSelector = `.${lineType}`; rowCssToAdd = - '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line old"></td><td class="notes_content parallel old"><div class="content"></div></td><td class="notes_line new"></td><td class="notes_content parallel new"><div class="content"></div></td></tr>'; + '<tr class="notes_holder js-temp-notes-holder"><td class="notes_line old"></td><td class="notes_content parallel old"><div class="content discussion-notes"></div></td><td class="notes_line new"></td><td class="notes_content parallel new"><div class="content discussion-notes"></div></td></tr>'; } const notesContentSelector = `.notes_content${lineTypeSelector} .content`; let notesContent = targetRow.find(notesContentSelector); diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue index e0f883a8e08..476b15aca4a 100644 --- a/app/assets/javascripts/notes/components/noteable_discussion.vue +++ b/app/assets/javascripts/notes/components/noteable_discussion.vue @@ -258,9 +258,7 @@ Please check your network connection and try again.`; :key="note.id" /> </ul> - <div - :class="{ 'is-replying': isReplying }" - class="discussion-reply-holder"> + <div class="discussion-reply-holder"> <template v-if="!isReplying && canReply"> <div class="btn-group-justified discussion-with-resolve-btn" diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss index 7f037582ca0..679f783b1b6 100644 --- a/app/assets/stylesheets/pages/diff.scss +++ b/app/assets/stylesheets/pages/diff.scss @@ -813,6 +813,7 @@ } .discussion-notes { + padding: 0 $gl-padding $gl-padding; min-height: 35px; &:first-child { diff --git a/app/assets/stylesheets/pages/note_form.scss b/app/assets/stylesheets/pages/note_form.scss index 4a528bc2bb1..8720f821ce9 100644 --- a/app/assets/stylesheets/pages/note_form.scss +++ b/app/assets/stylesheets/pages/note_form.scss @@ -173,11 +173,7 @@ } .discussion-form { - background-color: $white-light; -} - -.discussion-form-container { - padding: $gl-padding-top $gl-padding $gl-padding; + padding-top: $gl-padding-top; } .discussion-notes .disabled-comment { @@ -237,12 +233,7 @@ .discussion-body, .diff-file { .discussion-reply-holder { - background-color: $white-light; - padding: 10px 16px; - - &.is-replying { - padding-bottom: $gl-padding; - } + padding-top: $gl-padding; } } diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 81e98f358a8..9d9cbecc958 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -47,7 +47,7 @@ ul.notes { } .timeline-entry-inner { - padding: $gl-padding $gl-btn-padding; + padding: $gl-padding 0; border-bottom: 1px solid $white-normal; } @@ -94,12 +94,6 @@ ul.notes { } } - &.note-discussion { - .timeline-entry-inner { - padding: $gl-padding 10px; - } - } - .editing-spinner { display: none; } @@ -352,6 +346,8 @@ ul.notes { } .discussion-notes { + background-color: $white-light; + &:not(:first-child) { border-top: 1px solid $white-normal; margin-top: 20px; @@ -363,10 +359,6 @@ ul.notes { } } - .notes { - background-color: $white-light; - } - a code { top: 0; margin-right: 0; @@ -647,8 +639,6 @@ ul.notes { border-bottom: 1px solid $white-normal; .timeline-entry-inner { - padding-left: $gl-padding; - padding-right: $gl-padding; border-bottom: 0; } } diff --git a/app/models/project.rb b/app/models/project.rb index ea9df9b10ef..1b29cbf28d2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -637,7 +637,7 @@ class Project < ActiveRecord::Base end def create_or_update_import_data(data: nil, credentials: nil) - return unless import_url.present? && valid_import_url? + return if data.nil? && credentials.nil? project_import_data = import_data || build_import_data if data @@ -1487,6 +1487,7 @@ class Project < ActiveRecord::Base remove_import_jid update_project_counter_caches after_create_default_branch + refresh_markdown_cache! end def update_project_counter_caches diff --git a/app/policies/issuable_policy.rb b/app/policies/issuable_policy.rb index 3f6d7d04667..e86d1c8f98e 100644 --- a/app/policies/issuable_policy.rb +++ b/app/policies/issuable_policy.rb @@ -2,20 +2,6 @@ class IssuablePolicy < BasePolicy delegate { @subject.project } condition(:locked, scope: :subject, score: 0) { @subject.discussion_locked? } - - # We aren't checking `:read_issue` or `:read_merge_request` in this case - # because it could be possible for a user to see an issuable-iid - # (`:read_issue_iid` or `:read_merge_request_iid`) but then wouldn't be allowed - # to read the actual issue after a more expensive `:read_issue` check. - # - # `:read_issue` & `:read_issue_iid` could diverge in gitlab-ee. - condition(:visible_to_user, score: 4) do - Project.where(id: @subject.project) - .public_or_visible_to_user(@user) - .with_feature_available_for_user(@subject, @user) - .any? - end - condition(:is_project_member) { @user && @subject.project && @subject.project.team.member?(@user) } desc "User is the assignee or author" diff --git a/app/policies/issue_policy.rb b/app/policies/issue_policy.rb index ed499511999..263c6e3039c 100644 --- a/app/policies/issue_policy.rb +++ b/app/policies/issue_policy.rb @@ -17,6 +17,4 @@ class IssuePolicy < IssuablePolicy prevent :update_issue prevent :admin_issue end - - rule { can?(:read_issue) | visible_to_user }.enable :read_issue_iid end diff --git a/app/policies/merge_request_policy.rb b/app/policies/merge_request_policy.rb index e003376d219..c3fe857f8a2 100644 --- a/app/policies/merge_request_policy.rb +++ b/app/policies/merge_request_policy.rb @@ -1,3 +1,2 @@ class MergeRequestPolicy < IssuablePolicy - rule { can?(:read_merge_request) | visible_to_user }.enable :read_merge_request_iid end diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb index 57ab0c23dcd..b1ed034cd00 100644 --- a/app/policies/project_policy.rb +++ b/app/policies/project_policy.rb @@ -66,6 +66,22 @@ class ProjectPolicy < BasePolicy project.merge_requests_allowing_push_to_user(user).any? end + # We aren't checking `:read_issue` or `:read_merge_request` in this case + # because it could be possible for a user to see an issuable-iid + # (`:read_issue_iid` or `:read_merge_request_iid`) but then wouldn't be + # allowed to read the actual issue after a more expensive `:read_issue` + # check. These checks are intended to be used alongside + # `:read_project_for_iids`. + # + # `:read_issue` & `:read_issue_iid` could diverge in gitlab-ee. + condition(:issues_visible_to_user, score: 4) do + @subject.feature_available?(:issues, @user) + end + + condition(:merge_requests_visible_to_user, score: 4) do + @subject.feature_available?(:merge_requests, @user) + end + features = %w[ merge_requests issues @@ -81,6 +97,10 @@ class ProjectPolicy < BasePolicy condition(:"#{f}_disabled", score: 32) { !feature_available?(f.to_sym) } end + # `:read_project` may be prevented in EE, but `:read_project_for_iids` should + # not. + rule { guest | admin }.enable :read_project_for_iids + rule { guest }.enable :guest_access rule { reporter }.enable :reporter_access rule { developer }.enable :developer_access @@ -150,6 +170,7 @@ class ProjectPolicy < BasePolicy # where we enable or prevent it based on other coditions. rule { (~anonymous & public_project) | internal_access }.policy do enable :public_user_access + enable :read_project_for_iids end rule { can?(:public_user_access) }.policy do @@ -255,7 +276,11 @@ class ProjectPolicy < BasePolicy end rule { anonymous & ~public_project }.prevent_all - rule { public_project }.enable(:public_access) + + rule { public_project }.policy do + enable :public_access + enable :read_project_for_iids + end rule { can?(:public_access) }.policy do enable :read_project @@ -305,6 +330,14 @@ class ProjectPolicy < BasePolicy enable :update_pipeline end + rule do + (can?(:read_project_for_iids) & issues_visible_to_user) | can?(:read_issue) + end.enable :read_issue_iid + + rule do + (can?(:read_project_for_iids) & merge_requests_visible_to_user) | can?(:read_merge_request) + end.enable :read_merge_request_iid + private def team_member? diff --git a/app/services/projects/gitlab_projects_import_service.rb b/app/services/projects/gitlab_projects_import_service.rb index a68ecb4abe1..fb4afb85588 100644 --- a/app/services/projects/gitlab_projects_import_service.rb +++ b/app/services/projects/gitlab_projects_import_service.rb @@ -5,8 +5,8 @@ module Projects class GitlabProjectsImportService attr_reader :current_user, :params - def initialize(user, params) - @current_user, @params = user, params.dup + def initialize(user, import_params, override_params = nil) + @current_user, @params, @override_params = user, import_params.dup, override_params end def execute @@ -17,6 +17,7 @@ module Projects params[:import_type] = 'gitlab_project' params[:import_source] = import_upload_path + params[:import_data] = { data: { override_params: @override_params } } if @override_params ::Projects::CreateService.new(current_user, params).execute end diff --git a/app/views/shared/notes/_form.html.haml b/app/views/shared/notes/_form.html.haml index 71c0d740bc8..725bf916592 100644 --- a/app/views/shared/notes/_form.html.haml +++ b/app/views/shared/notes/_form.html.haml @@ -24,21 +24,20 @@ -# DiffNote = f.hidden_field :position - .discussion-form-container - = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do - = render 'projects/zen', f: f, - attr: :note, - classes: 'note-textarea js-note-text', - placeholder: "Write a comment or drag your files here...", - supports_quick_actions: supports_quick_actions, - supports_autocomplete: supports_autocomplete - = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions - .error-alert - - .note-form-actions.clearfix - = render partial: 'shared/notes/comment_button' - - = yield(:note_actions) - - %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } } - Discard draft + = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do + = render 'projects/zen', f: f, + attr: :note, + classes: 'note-textarea js-note-text', + placeholder: "Write a comment or drag your files here...", + supports_quick_actions: supports_quick_actions, + supports_autocomplete: supports_autocomplete + = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions + .error-alert + + .note-form-actions.clearfix + = render partial: 'shared/notes/comment_button' + + = yield(:note_actions) + + %a.btn.btn-cancel.js-note-discard{ role: "button", data: {cancel_text: "Cancel" } } + Discard draft diff --git a/changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml b/changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml new file mode 100644 index 00000000000..686258460e0 --- /dev/null +++ b/changelogs/unreleased/43098-controller-projects-issuescontroller-show-executes-more-than-100-sql-queries.yml @@ -0,0 +1,5 @@ +--- +title: Improve performance of loading issues with lots of references to merge requests +merge_request: 17986 +author: +type: performance diff --git a/changelogs/unreleased/44870-remove-extra-space-around-comment-form-on-merge-requests.yml b/changelogs/unreleased/44870-remove-extra-space-around-comment-form-on-merge-requests.yml new file mode 100644 index 00000000000..53e4ebdb996 --- /dev/null +++ b/changelogs/unreleased/44870-remove-extra-space-around-comment-form-on-merge-requests.yml @@ -0,0 +1,5 @@ +--- +title: Refactor and tweak margin for note forms on Issuable +merge_request: 18120 +author: Takuya Noguchi +type: fixed diff --git a/changelogs/unreleased/bvl-override-import-params.yml b/changelogs/unreleased/bvl-override-import-params.yml new file mode 100644 index 00000000000..18cfef873df --- /dev/null +++ b/changelogs/unreleased/bvl-override-import-params.yml @@ -0,0 +1,5 @@ +--- +title: Allow overriding params on project import through API +merge_request: 18086 +author: +type: added diff --git a/doc/api/project_import_export.md b/doc/api/project_import_export.md index 5467187788a..995f10571d0 100644 --- a/doc/api/project_import_export.md +++ b/doc/api/project_import_export.md @@ -111,6 +111,9 @@ POST /projects/import | `namespace` | integer/string | no | The ID or path of the namespace that the project will be imported to. Defaults to the current user's namespace | | `file` | string | yes | The file to be uploaded | | `path` | string | yes | Name and path for new project | +| `override_params` | Hash | no | Supports all fields defined in the [Project API](projects.md)] | + +The override params passed will take precendence over all values defined inside the export file. To upload a file from your filesystem, use the `--form` argument. This causes cURL to post data using the header `Content-Type: multipart/form-data`. diff --git a/lib/api/helpers/projects_helpers.rb b/lib/api/helpers/projects_helpers.rb new file mode 100644 index 00000000000..381d5e8968c --- /dev/null +++ b/lib/api/helpers/projects_helpers.rb @@ -0,0 +1,38 @@ +module API + module Helpers + module ProjectsHelpers + extend ActiveSupport::Concern + + included do + helpers do + params :optional_project_params_ce do + optional :description, type: String, desc: 'The description of the project' + optional :ci_config_path, type: String, desc: 'The path to CI config file. Defaults to `.gitlab-ci.yml`' + optional :issues_enabled, type: Boolean, desc: 'Flag indication if the issue tracker is enabled' + optional :merge_requests_enabled, type: Boolean, desc: 'Flag indication if merge requests are enabled' + optional :wiki_enabled, type: Boolean, desc: 'Flag indication if the wiki is enabled' + optional :jobs_enabled, type: Boolean, desc: 'Flag indication if jobs are enabled' + optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled' + optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project' + optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push' + optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project' + optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project' + optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the project.' + optional :public_builds, type: Boolean, desc: 'Perform public builds' + optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' + optional :only_allow_merge_if_pipeline_succeeds, type: Boolean, desc: 'Only allow to merge if builds succeed' + optional :only_allow_merge_if_all_discussions_are_resolved, type: Boolean, desc: 'Only allow to merge if all discussions are resolved' + optional :tag_list, type: Array[String], desc: 'The list of tags for a project' + optional :avatar, type: File, desc: 'Avatar image for project' + optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line' + optional :merge_method, type: String, values: %w(ff rebase_merge merge), desc: 'The merge method used when merging merge requests' + end + + params :optional_project_params do + use :optional_project_params_ce + end + end + end + end + end +end diff --git a/lib/api/project_import.rb b/lib/api/project_import.rb index a509c1f32c1..303b58a5942 100644 --- a/lib/api/project_import.rb +++ b/lib/api/project_import.rb @@ -1,6 +1,7 @@ module API class ProjectImport < Grape::API include PaginationParams + include Helpers::ProjectsHelpers helpers do def import_params @@ -25,6 +26,11 @@ module API requires :path, type: String, desc: 'The new project path and name' requires :file, type: File, desc: 'The project export file to be imported' optional :namespace, type: String, desc: "The ID or name of the namespace that the project will be imported into. Defaults to the current user's namespace." + optional :override_params, + type: Hash, + desc: 'New project params to override values in the export' do + use :optional_project_params + end end desc 'Create a new project import' do detail 'This feature was introduced in GitLab 10.6.' @@ -47,7 +53,11 @@ module API file: import_params[:file]['tempfile'] } - project = ::Projects::GitlabProjectsImportService.new(current_user, project_params).execute + override_params = import_params.delete(:override_params) + + project = ::Projects::GitlabProjectsImportService.new( + current_user, project_params, override_params + ).execute render_api_error!(project.errors.full_messages&.first, 400) unless project.saved? diff --git a/lib/api/projects.rb b/lib/api/projects.rb index 3d5b3c5a535..d0a4a23e074 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -4,37 +4,11 @@ module API class Projects < Grape::API include PaginationParams include Helpers::CustomAttributes + include Helpers::ProjectsHelpers before { authenticate_non_get! } helpers do - params :optional_params_ce do - optional :description, type: String, desc: 'The description of the project' - optional :ci_config_path, type: String, desc: 'The path to CI config file. Defaults to `.gitlab-ci.yml`' - optional :issues_enabled, type: Boolean, desc: 'Flag indication if the issue tracker is enabled' - optional :merge_requests_enabled, type: Boolean, desc: 'Flag indication if merge requests are enabled' - optional :wiki_enabled, type: Boolean, desc: 'Flag indication if the wiki is enabled' - optional :jobs_enabled, type: Boolean, desc: 'Flag indication if jobs are enabled' - optional :snippets_enabled, type: Boolean, desc: 'Flag indication if snippets are enabled' - optional :shared_runners_enabled, type: Boolean, desc: 'Flag indication if shared runners are enabled for that project' - optional :resolve_outdated_diff_discussions, type: Boolean, desc: 'Automatically resolve merge request diffs discussions on lines changed with a push' - optional :container_registry_enabled, type: Boolean, desc: 'Flag indication if the container registry is enabled for that project' - optional :lfs_enabled, type: Boolean, desc: 'Flag indication if Git LFS is enabled for that project' - optional :visibility, type: String, values: Gitlab::VisibilityLevel.string_values, desc: 'The visibility of the project.' - optional :public_builds, type: Boolean, desc: 'Perform public builds' - optional :request_access_enabled, type: Boolean, desc: 'Allow users to request member access' - optional :only_allow_merge_if_pipeline_succeeds, type: Boolean, desc: 'Only allow to merge if builds succeed' - optional :only_allow_merge_if_all_discussions_are_resolved, type: Boolean, desc: 'Only allow to merge if all discussions are resolved' - optional :tag_list, type: Array[String], desc: 'The list of tags for a project' - optional :avatar, type: File, desc: 'Avatar image for project' - optional :printing_merge_request_link_enabled, type: Boolean, desc: 'Show link to create/view merge request when pushing from the command line' - optional :merge_method, type: String, values: %w(ff rebase_merge merge), desc: 'The merge method used when merging merge requests' - end - - params :optional_params do - use :optional_params_ce - end - params :statistics_params do optional :statistics, type: Boolean, default: false, desc: 'Include project statistics' end @@ -144,7 +118,7 @@ module API optional :name, type: String, desc: 'The name of the project' optional :path, type: String, desc: 'The path of the repository' at_least_one_of :name, :path - use :optional_params + use :optional_project_params use :create_params end post do @@ -172,7 +146,7 @@ module API requires :user_id, type: Integer, desc: 'The ID of a user' optional :path, type: String, desc: 'The path of the repository' optional :default_branch, type: String, desc: 'The default branch of the project' - use :optional_params + use :optional_project_params use :create_params end post "user/:user_id" do @@ -293,7 +267,7 @@ module API optional :default_branch, type: String, desc: 'The default branch of the project' optional :path, type: String, desc: 'The path of the repository' - use :optional_params + use :optional_project_params at_least_one_of(*at_least_one_of_ce) end put ':id' do diff --git a/lib/gitlab/import_export/import_export.yml b/lib/gitlab/import_export/import_export.yml index 4bdd01f5e94..6a0a1b65fe9 100644 --- a/lib/gitlab/import_export/import_export.yml +++ b/lib/gitlab/import_export/import_export.yml @@ -105,6 +105,7 @@ excluded_attributes: - :last_repository_updated_at - :last_repository_check_at - :storage_version + - :description_html snippets: - :expired_at merge_request_diff: @@ -144,8 +145,6 @@ methods: - :diff_head_sha - :source_branch_sha - :target_branch_sha - project: - - :description_html events: - :action push_event_payload: diff --git a/lib/gitlab/import_export/project_tree_restorer.rb b/lib/gitlab/import_export/project_tree_restorer.rb index 8f5bb8f9597..2c315207298 100644 --- a/lib/gitlab/import_export/project_tree_restorer.rb +++ b/lib/gitlab/import_export/project_tree_restorer.rb @@ -77,24 +77,31 @@ module Gitlab end def default_relation_list - Gitlab::ImportExport::Reader.new(shared: @shared).tree.reject do |model| + reader.tree.reject do |model| model.is_a?(Hash) && model[:project_members] end end def restore_project - params = project_params - - if params[:description].present? - params[:description_html] = nil - end - - @project.update_columns(params) + @project.update_columns(project_params) @project end def project_params - @tree_hash.reject do |key, value| + @project_params ||= json_params.merge(override_params) + end + + def override_params + return {} unless params = @project.import_data&.data&.fetch('override_params') + + @override_params ||= params.select do |key, _value| + Project.column_names.include?(key.to_s) && + !reader.project_tree[:except].include?(key.to_sym) + end + end + + def json_params + @json_params ||= @tree_hash.reject do |key, value| # return params that are not 1 to many or 1 to 1 relations value.respond_to?(:each) && !Project.column_names.include?(key) end @@ -181,6 +188,10 @@ module Gitlab relation_hash.merge(params) end + + def reader + @reader ||= Gitlab::ImportExport::Reader.new(shared: @shared) + end end end end diff --git a/spec/javascripts/ide/components/repo_editor_spec.js b/spec/javascripts/ide/components/repo_editor_spec.js index e79b85050b2..63a3d2c6cd5 100644 --- a/spec/javascripts/ide/components/repo_editor_spec.js +++ b/spec/javascripts/ide/components/repo_editor_spec.js @@ -199,47 +199,49 @@ describe('RepoEditor', () => { }); }); - describe('setup editor for merge request viewing', () => { - beforeEach(done => { - // Resetting as the main test setup has already done it - vm.$destroy(); - resetStore(vm.$store); - Editor.editorInstance.modelManager.dispose(); - - const f = { - ...file(), - active: true, - tempFile: true, - html: 'testing', - mrChange: { diff: 'ABC' }, - baseRaw: 'testing', - content: 'test', - }; - const RepoEditor = Vue.extend(repoEditor); - vm = createComponentWithStore(RepoEditor, store, { - file: f, - }); + describe('editor updateDimensions', () => { + beforeEach(() => { + spyOn(vm.editor, 'updateDimensions').and.callThrough(); + spyOn(vm.editor, 'updateDiffView'); + }); - vm.$store.state.openFiles.push(f); - vm.$store.state.entries[f.path] = f; + it('calls updateDimensions when rightPanelCollapsed is changed', done => { + vm.$store.state.rightPanelCollapsed = true; - vm.$store.state.viewer = 'mrdiff'; + vm.$nextTick(() => { + expect(vm.editor.updateDimensions).toHaveBeenCalled(); + expect(vm.editor.updateDiffView).toHaveBeenCalled(); - vm.monaco = true; + done(); + }); + }); - vm.$mount(); + it('calls updateDimensions when panelResizing is false', done => { + vm.$store.state.panelResizing = true; - monacoLoader(['vs/editor/editor.main'], () => { - setTimeout(done, 0); - }); + vm + .$nextTick() + .then(() => { + vm.$store.state.panelResizing = false; + }) + .then(vm.$nextTick) + .then(() => { + expect(vm.editor.updateDimensions).toHaveBeenCalled(); + expect(vm.editor.updateDiffView).toHaveBeenCalled(); + }) + .then(done) + .catch(done.fail); }); - it('attaches merge request model to editor when merge request diff', () => { - spyOn(vm.editor, 'attachMergeRequestModel').and.callThrough(); + it('does not call updateDimensions when panelResizing is true', done => { + vm.$store.state.panelResizing = true; - vm.setupEditor(); + vm.$nextTick(() => { + expect(vm.editor.updateDimensions).not.toHaveBeenCalled(); + expect(vm.editor.updateDiffView).not.toHaveBeenCalled(); - expect(vm.editor.attachMergeRequestModel).toHaveBeenCalledWith(vm.model); + done(); + }); }); }); }); diff --git a/spec/javascripts/ide/lib/editor_spec.js b/spec/javascripts/ide/lib/editor_spec.js index ec56ebc0341..75e6f0f54ec 100644 --- a/spec/javascripts/ide/lib/editor_spec.js +++ b/spec/javascripts/ide/lib/editor_spec.js @@ -76,7 +76,8 @@ describe('Multi-file editor library', () => { occurrencesHighlight: false, renderLineHighlight: 'none', hideCursorInOverviewRuler: true, - wordWrap: 'bounded', + wordWrap: 'on', + renderSideBySide: true, }); }); }); @@ -215,4 +216,56 @@ describe('Multi-file editor library', () => { expect(instance.decorationsController.dispose).not.toHaveBeenCalled(); }); }); + + describe('updateDiffView', () => { + describe('edit mode', () => { + it('does not update options', () => { + instance.createInstance(holder); + + spyOn(instance.instance, 'updateOptions'); + + instance.updateDiffView(); + + expect(instance.instance.updateOptions).not.toHaveBeenCalled(); + }); + }); + + describe('diff mode', () => { + beforeEach(() => { + instance.createDiffInstance(holder); + + spyOn(instance.instance, 'updateOptions').and.callThrough(); + }); + + it('sets renderSideBySide to false if el is less than 700 pixels', () => { + spyOnProperty(instance.instance.getDomNode(), 'offsetWidth').and.returnValue(600); + + expect(instance.instance.updateOptions).not.toHaveBeenCalledWith({ + renderSideBySide: false, + }); + }); + + it('sets renderSideBySide to false if el is more than 700 pixels', () => { + spyOnProperty(instance.instance.getDomNode(), 'offsetWidth').and.returnValue(800); + + expect(instance.instance.updateOptions).not.toHaveBeenCalledWith({ + renderSideBySide: true, + }); + }); + }); + }); + + describe('isDiffEditorType', () => { + it('returns true when diff editor', () => { + instance.createDiffInstance(holder); + + expect(instance.isDiffEditorType).toBe(true); + }); + + it('returns false when not diff editor', () => { + instance.createInstance(holder); + + expect(instance.isDiffEditorType).toBe(false); + }); + }); }); diff --git a/spec/lib/banzai/reference_parser/issue_parser_spec.rb b/spec/lib/banzai/reference_parser/issue_parser_spec.rb index 0a63567ee40..cb7f8b20dda 100644 --- a/spec/lib/banzai/reference_parser/issue_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/issue_parser_spec.rb @@ -117,4 +117,27 @@ describe Banzai::ReferenceParser::IssueParser do expect(subject.records_for_nodes(nodes)).to eq({ link => issue }) end end + + context 'when checking multiple merge requests on another project' do + let(:other_project) { create(:project, :public) } + let(:other_issue) { create(:issue, project: other_project) } + + let(:control_links) do + [issue_link(other_issue)] + end + + let(:actual_links) do + control_links + [issue_link(create(:issue, project: other_project))] + end + + def issue_link(issue) + Nokogiri::HTML.fragment(%Q{<a data-issue="#{issue.id}"></a>}).children[0] + end + + before do + project.add_developer(user) + end + + it_behaves_like 'no N+1 queries' + end end diff --git a/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb index 775749ae3a7..14542342cf6 100644 --- a/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb +++ b/spec/lib/banzai/reference_parser/merge_request_parser_spec.rb @@ -4,14 +4,13 @@ describe Banzai::ReferenceParser::MergeRequestParser do include ReferenceParserHelpers let(:user) { create(:user) } - let(:merge_request) { create(:merge_request) } - subject { described_class.new(merge_request.target_project, user) } + let(:project) { create(:project, :public) } + let(:merge_request) { create(:merge_request, source_project: project) } + subject { described_class.new(project, user) } let(:link) { empty_html_link } describe '#nodes_visible_to_user' do context 'when the link has a data-issue attribute' do - let(:project) { merge_request.target_project } - before do project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) link['data-merge-request'] = merge_request.id.to_s @@ -40,4 +39,27 @@ describe Banzai::ReferenceParser::MergeRequestParser do end end end + + context 'when checking multiple merge requests on another project' do + let(:other_project) { create(:project, :public) } + let(:other_merge_request) { create(:merge_request, source_project: other_project) } + + let(:control_links) do + [merge_request_link(other_merge_request)] + end + + let(:actual_links) do + control_links + [merge_request_link(create(:merge_request, :conflict, source_project: other_project))] + end + + def merge_request_link(merge_request) + Nokogiri::HTML.fragment(%Q{<a data-merge-request="#{merge_request.id}"></a>}).children[0] + end + + before do + project.add_developer(user) + end + + it_behaves_like 'no N+1 queries' + end end diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 4a51777ba9b..8c1bf6f7be8 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -2,7 +2,6 @@ "description": "Nisi et repellendus ut enim quo accusamus vel magnam.", "visibility_level": 10, "archived": false, - "description_html": "description", "labels": [ { "id": 2, diff --git a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb index 8e25cd26c2f..13a8c9adcee 100644 --- a/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_restorer_spec.rb @@ -46,10 +46,6 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do expect(Project.find_by_path('project').description).to eq('Nisi et repellendus ut enim quo accusamus vel magnam.') end - it 'has the project html description' do - expect(Project.find_by_path('project').description_html).to eq('description') - end - it 'has the same label associated to two issues' do expect(ProjectLabel.find_by_title('test2').issues.count).to eq(2) end @@ -317,6 +313,24 @@ describe Gitlab::ImportExport::ProjectTreeRestorer do end end + context 'when the project has overriden params in import data' do + it 'overwrites the params stored in the JSON' do + project.create_import_data(data: { override_params: { description: "Overridden" } }) + + restored_project_json + + expect(project.description).to eq("Overridden") + end + + it 'does not allow setting params that are excluded from import_export settings' do + project.create_import_data(data: { override_params: { lfs_enabled: true } }) + + restored_project_json + + expect(project.lfs_enabled).to be_nil + end + end + context 'with a project that has a group' do let!(:project) do create(:project, diff --git a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb b/spec/lib/gitlab/import_export/project_tree_saver_spec.rb index 0d20a551e2a..2b8a11ce8f9 100644 --- a/spec/lib/gitlab/import_export/project_tree_saver_spec.rb +++ b/spec/lib/gitlab/import_export/project_tree_saver_spec.rb @@ -245,10 +245,6 @@ describe Gitlab::ImportExport::ProjectTreeSaver do end context 'project attributes' do - it 'contains the html description' do - expect(saved_project_json).to include("description_html" => 'description') - end - it 'does not contain the runners token' do expect(saved_project_json).not_to include("runners_token" => 'token') end @@ -274,7 +270,6 @@ describe Gitlab::ImportExport::ProjectTreeSaver do releases: [release], group: group ) - project.update_column(:description_html, 'description') project_label = create(:label, project: project) group_label = create(:group_label, group: group) create(:label_link, label: project_label, target: issue) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 8bd62dcdccb..7007f78e702 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -3243,6 +3243,7 @@ describe Project do expect(project).to receive(:update_project_counter_caches) expect(project).to receive(:remove_import_jid) expect(project).to receive(:after_create_default_branch) + expect(project).to receive(:refresh_markdown_cache!) project.after_import end diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb index ea76e604153..905d82b3bb1 100644 --- a/spec/policies/project_policy_spec.rb +++ b/spec/policies/project_policy_spec.rb @@ -11,10 +11,10 @@ describe ProjectPolicy do let(:base_guest_permissions) do %i[ - read_project read_board read_list read_wiki read_issue read_label - read_milestone read_project_snippet read_project_member - read_note create_project create_issue create_note - upload_file + read_project read_board read_list read_wiki read_issue + read_project_for_iids read_issue_iid read_merge_request_iid read_label + read_milestone read_project_snippet read_project_member read_note + create_project create_issue create_note upload_file ] end @@ -120,7 +120,7 @@ describe ProjectPolicy do project.issues_enabled = false project.save! - expect_disallowed :read_issue, :create_issue, :update_issue, :admin_issue + expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue end end @@ -131,7 +131,7 @@ describe ProjectPolicy do project.issues_enabled = false project.save! - expect_disallowed :read_issue, :create_issue, :update_issue, :admin_issue + expect_disallowed :read_issue, :read_issue_iid, :create_issue, :update_issue, :admin_issue end end end diff --git a/spec/requests/api/project_import_spec.rb b/spec/requests/api/project_import_spec.rb index 987f6e26971..5d13e6de741 100644 --- a/spec/requests/api/project_import_spec.rb +++ b/spec/requests/api/project_import_spec.rb @@ -40,7 +40,7 @@ describe API::ProjectImport do expect(response).to have_gitlab_http_status(201) end - it 'schedules an import at the user namespace level' do + it 'does not shedule an import for a nampespace that does not exist' do expect_any_instance_of(Project).not_to receive(:import_schedule) expect(::Projects::CreateService).not_to receive(:new) @@ -71,6 +71,49 @@ describe API::ProjectImport do expect(json_response['error']).to eq('file is invalid') end + it 'stores params that can be overridden' do + stub_import(namespace) + override_params = { 'description' => 'Hello world' } + + post api('/projects/import', user), + path: 'test-import', + file: fixture_file_upload(file), + namespace: namespace.id, + override_params: override_params + import_project = Project.find(json_response['id']) + + expect(import_project.import_data.data['override_params']).to eq(override_params) + end + + it 'does not store params that are not allowed' do + stub_import(namespace) + override_params = { 'not_allowed' => 'Hello world' } + + post api('/projects/import', user), + path: 'test-import', + file: fixture_file_upload(file), + namespace: namespace.id, + override_params: override_params + import_project = Project.find(json_response['id']) + + expect(import_project.import_data.data['override_params']).to be_empty + end + + it 'correctly overrides params during the import' do + override_params = { 'description' => 'Hello world' } + + Sidekiq::Testing.inline! do + post api('/projects/import', user), + path: 'test-import', + file: fixture_file_upload(file), + namespace: namespace.id, + override_params: override_params + end + import_project = Project.find(json_response['id']) + + expect(import_project.description).to eq('Hello world') + end + def stub_import(namespace) expect_any_instance_of(Project).to receive(:import_schedule) expect(::Projects::CreateService).to receive(:new).with(user, hash_including(namespace_id: namespace.id)).and_call_original diff --git a/spec/services/projects/gitlab_projects_import_service_spec.rb b/spec/services/projects/gitlab_projects_import_service_spec.rb index 6b8f9619bc4..880b2aae66a 100644 --- a/spec/services/projects/gitlab_projects_import_service_spec.rb +++ b/spec/services/projects/gitlab_projects_import_service_spec.rb @@ -2,8 +2,10 @@ require 'spec_helper' describe Projects::GitlabProjectsImportService do set(:namespace) { create(:namespace) } + let(:path) { 'test-path' } let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') } - subject { described_class.new(namespace.owner, { namespace_id: namespace.id, path: path, file: file }) } + let(:import_params) { { namespace_id: namespace.id, path: path, file: file } } + subject { described_class.new(namespace.owner, import_params) } describe '#execute' do context 'with an invalid path' do @@ -18,8 +20,6 @@ describe Projects::GitlabProjectsImportService do end context 'with a valid path' do - let(:path) { 'test-path' } - it 'creates a project' do project = subject.execute @@ -27,5 +27,15 @@ describe Projects::GitlabProjectsImportService do expect(project).to be_valid end end + + context 'override params' do + it 'stores them as import data when passed' do + project = described_class + .new(namespace.owner, import_params, description: 'Hello') + .execute + + expect(project.import_data.data['override_params']['description']).to eq('Hello') + end + end end end diff --git a/spec/support/reference_parser_helpers.rb b/spec/support/reference_parser_helpers.rb index 01689194eac..5d5e80851e6 100644 --- a/spec/support/reference_parser_helpers.rb +++ b/spec/support/reference_parser_helpers.rb @@ -2,4 +2,34 @@ module ReferenceParserHelpers def empty_html_link Nokogiri::HTML.fragment('<a></a>').children[0] end + + shared_examples 'no N+1 queries' do + it 'avoids N+1 queries in #nodes_visible_to_user', :request_store do + record_queries = lambda do |links| + ActiveRecord::QueryRecorder.new do + described_class.new(project, user).nodes_visible_to_user(user, links) + end + end + + control = record_queries.call(control_links) + actual = record_queries.call(actual_links) + + expect(actual.count).to be <= control.count + expect(actual.cached_count).to be <= control.cached_count + end + + it 'avoids N+1 queries in #records_for_nodes', :request_store do + record_queries = lambda do |links| + ActiveRecord::QueryRecorder.new do + described_class.new(project, user).records_for_nodes(links) + end + end + + control = record_queries.call(control_links) + actual = record_queries.call(actual_links) + + expect(actual.count).to be <= control.count + expect(actual.cached_count).to be <= control.cached_count + end + end end |