diff options
86 files changed, 1674 insertions, 417 deletions
diff --git a/app/assets/javascripts/broadcast_notification.js b/app/assets/javascripts/broadcast_notification.js index b124502506a..dc5401199dc 100644 --- a/app/assets/javascripts/broadcast_notification.js +++ b/app/assets/javascripts/broadcast_notification.js @@ -6,16 +6,14 @@ const handleOnDismiss = ({ currentTarget }) => { dataset: { id }, } = currentTarget; - Cookies.set(`hide_broadcast_notification_message_${id}`, true); + Cookies.set(`hide_broadcast_message_${id}`, true); const notification = document.querySelector(`.js-broadcast-notification-${id}`); notification.parentNode.removeChild(notification); }; export default () => { - const dismissButton = document.querySelector('.js-dismiss-current-broadcast-notification'); - - if (dismissButton) { - dismissButton.addEventListener('click', handleOnDismiss); - } + document + .querySelectorAll('.js-dismiss-current-broadcast-notification') + .forEach(dismissButton => dismissButton.addEventListener('click', handleOnDismiss)); }; diff --git a/app/assets/javascripts/pages/admin/broadcast_messages/broadcast_message.js b/app/assets/javascripts/pages/admin/broadcast_messages/broadcast_message.js index d191479b1b4..34a024b1b33 100644 --- a/app/assets/javascripts/pages/admin/broadcast_messages/broadcast_message.js +++ b/app/assets/javascripts/pages/admin/broadcast_messages/broadcast_message.js @@ -25,9 +25,11 @@ export default () => { $broadcastMessageType.on('change', () => { const $broadcastMessageColorFormGroup = $('.js-broadcast-message-background-color-form-group'); + const $broadcastMessageDismissableFormGroup = $('.js-broadcast-message-dismissable-form-group'); const $broadcastNotificationMessagePreview = $('.js-broadcast-notification-message-preview'); $broadcastMessageColorFormGroup.toggleClass('hidden'); + $broadcastMessageDismissableFormGroup.toggleClass('hidden'); $broadcastBannerMessagePreview.toggleClass('hidden'); $broadcastNotificationMessagePreview.toggleClass('hidden'); }); diff --git a/app/assets/stylesheets/framework/broadcast_messages.scss b/app/assets/stylesheets/framework/broadcast_messages.scss index 95ea3d90a0e..359f4681938 100644 --- a/app/assets/stylesheets/framework/broadcast_messages.scss +++ b/app/assets/stylesheets/framework/broadcast_messages.scss @@ -17,6 +17,10 @@ @extend .broadcast-message; @extend .alert-warning; text-align: center; + + .broadcast-message-dismiss { + color: inherit; + } } .broadcast-notification-message { @@ -36,6 +40,11 @@ &.preview { position: static; } + + .broadcast-message-dismiss { + height: 100%; + color: $gray-800; + } } .toggle-colors { diff --git a/app/controllers/admin/broadcast_messages_controller.rb b/app/controllers/admin/broadcast_messages_controller.rb index 06ba916fc55..3233c765941 100644 --- a/app/controllers/admin/broadcast_messages_controller.rb +++ b/app/controllers/admin/broadcast_messages_controller.rb @@ -62,6 +62,7 @@ class Admin::BroadcastMessagesController < Admin::ApplicationController starts_at target_path broadcast_type + dismissable )) end end diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index f6f61b6e5fb..d301a5be391 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -24,7 +24,6 @@ class Projects::MilestonesController < Projects::ApplicationController respond_to do |format| format.html do - @project_namespace = @project.namespace.becomes(Namespace) # We need to show group milestones in the JSON response # so that people can filter by and assign group milestones, # but we don't need to show them on the project milestones page itself. @@ -47,8 +46,6 @@ class Projects::MilestonesController < Projects::ApplicationController end def show - @project_namespace = @project.namespace.becomes(Namespace) - respond_to do |format| format.html end diff --git a/app/controllers/repositories/git_http_client_controller.rb b/app/controllers/repositories/git_http_client_controller.rb index 76eb7c67205..d03daa406cf 100644 --- a/app/controllers/repositories/git_http_client_controller.rb +++ b/app/controllers/repositories/git_http_client_controller.rb @@ -6,7 +6,7 @@ module Repositories include KerberosSpnegoHelper include Gitlab::Utils::StrongMemoize - attr_reader :authentication_result, :redirected_path + attr_reader :authentication_result, :redirected_path, :container delegate :actor, :authentication_abilities, to: :authentication_result, allow_nil: true delegate :type, to: :authentication_result, allow_nil: true, prefix: :auth_result @@ -81,7 +81,7 @@ module Repositories end def parse_repo_path - @project, @repo_type, @redirected_path = Gitlab::RepoPath.parse("#{params[:namespace_id]}/#{params[:repository_id]}") + @container, @project, @repo_type, @redirected_path = Gitlab::RepoPath.parse("#{params[:namespace_id]}/#{params[:repository_id]}") end def render_missing_personal_access_token @@ -93,7 +93,7 @@ module Repositories def repository strong_memoize(:repository) do - repo_type.repository_for(project) + repo_type.repository_for(container) end end @@ -117,7 +117,8 @@ module Repositories def http_download_allowed? Gitlab::ProtocolAccess.allowed?('http') && download_request? && - project && Guest.can?(:download_code, project) + container && + Guest.can?(repo_type.guest_read_ability, container) end end end diff --git a/app/controllers/repositories/git_http_controller.rb b/app/controllers/repositories/git_http_controller.rb index 5c2b6089bff..5ce2ed77417 100644 --- a/app/controllers/repositories/git_http_controller.rb +++ b/app/controllers/repositories/git_http_controller.rb @@ -84,10 +84,10 @@ module Repositories end def access - @access ||= access_klass.new(access_actor, project, 'http', + @access ||= access_klass.new(access_actor, container, 'http', authentication_abilities: authentication_abilities, namespace_path: params[:namespace_id], - project_path: project_path, + repository_path: repository_path, redirected_path: redirected_path, auth_result_type: auth_result_type) end @@ -99,15 +99,18 @@ module Repositories def access_check access.check(git_command, Gitlab::GitAccess::ANY) - @project ||= access.project + + if repo_type.project? && !container + @project = @container = access.project + end end def access_klass @access_klass ||= repo_type.access_checker_class end - def project_path - @project_path ||= params[:repository_id].sub(/\.git$/, '') + def repository_path + @repository_path ||= params[:repository_id].sub(/\.git$/, '') end def log_user_activity diff --git a/app/helpers/broadcast_messages_helper.rb b/app/helpers/broadcast_messages_helper.rb index 34e65c322c6..7638710a7c2 100644 --- a/app/helpers/broadcast_messages_helper.rb +++ b/app/helpers/broadcast_messages_helper.rb @@ -2,12 +2,14 @@ module BroadcastMessagesHelper def current_broadcast_banner_messages - BroadcastMessage.current_banner_messages(request.path) + BroadcastMessage.current_banner_messages(request.path).select do |message| + cookies["hide_broadcast_message_#{message.id}"].blank? + end end def current_broadcast_notification_message not_hidden_messages = BroadcastMessage.current_notification_messages(request.path).select do |message| - cookies["hide_broadcast_notification_message_#{message.id}"].blank? + cookies["hide_broadcast_message_#{message.id}"].blank? end not_hidden_messages.last end diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index b12b39073ef..a723269ea0e 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -151,18 +151,20 @@ module MilestonesHelper end def milestone_issues_tooltip_text(milestone) - issues = milestone.count_issues_by_state(current_user) + total = milestone.total_issues_count(current_user) + opened = milestone.opened_issues_count(current_user) + closed = milestone.closed_issues_count(current_user) - return _("Issues") if issues.empty? + return _("Issues") if total.zero? content = [] - if issues["opened"] - content << n_("1 open issue", "%{issues} open issues", issues["opened"]) % { issues: issues["opened"] } + if opened > 0 + content << n_("1 open issue", "%{issues} open issues", opened) % { issues: opened } end - if issues["closed"] - content << n_("1 closed issue", "%{issues} closed issues", issues["closed"]) % { issues: issues["closed"] } + if closed > 0 + content << n_("1 closed issue", "%{issues} closed issues", closed) % { issues: closed } end content.join('<br />').html_safe diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index 88e752e51e7..045e893761a 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -2,13 +2,27 @@ module Milestoneish def total_issues_count(user) - count_issues_by_state(user).values.sum + @total_issues_count ||= + if Feature.enabled?(:cached_milestone_issue_counters) + Milestones::IssuesCountService.new(self).count + else + count_issues_by_state(user).values.sum + end end def closed_issues_count(user) - closed_state_id = Issue.available_states[:closed] + @close_issues_count ||= + if Feature.enabled?(:cached_milestone_issue_counters) + Milestones::ClosedIssuesCountService.new(self).count + else + closed_state_id = Issue.available_states[:closed] + + count_issues_by_state(user)[closed_state_id].to_i + end + end - count_issues_by_state(user)[closed_state_id].to_i + def opened_issues_count(user) + total_issues_count(user) - closed_issues_count(user) end def complete?(user) diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 201cd719ee9..770d9b5205c 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -160,6 +160,10 @@ class Snippet < ApplicationRecord @link_reference_pattern ||= super("snippets", /(?<snippet>\d+)/) end + def self.find_by_id_and_project(id:, project:) + Snippet.find_by(id: id, project: project) + end + def initialize(attributes = {}) # We can't use default_value_for because the database has a default # value of 0 for visibility_level. If someone attempts to create a diff --git a/app/services/issues/base_service.rb b/app/services/issues/base_service.rb index 974f7e598ca..9e72f6dad8d 100644 --- a/app/services/issues/base_service.rb +++ b/app/services/issues/base_service.rb @@ -34,6 +34,18 @@ module Issues def update_project_counter_caches?(issue) super || issue.confidential_changed? end + + def delete_milestone_closed_issue_counter_cache(milestone) + return unless milestone + + Milestones::ClosedIssuesCountService.new(milestone).delete_cache + end + + def delete_milestone_total_issue_counter_cache(milestone) + return unless milestone + + Milestones::IssuesCountService.new(milestone).delete_cache + end end end diff --git a/app/services/issues/close_service.rb b/app/services/issues/close_service.rb index ce955b07648..21e9a2210fb 100644 --- a/app/services/issues/close_service.rb +++ b/app/services/issues/close_service.rb @@ -38,6 +38,8 @@ module Issues issue.update_project_counter_caches store_first_mentioned_in_commit_at(issue, closed_via) if closed_via.is_a?(MergeRequest) + + delete_milestone_closed_issue_counter_cache(issue.milestone) end issue diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index e8879d4df66..7869509aa9c 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -29,6 +29,7 @@ module Issues todo_service.new_issue(issuable, current_user) user_agent_detail_service.create resolve_discussions_with_issue(issuable) + delete_milestone_total_issue_counter_cache(issuable.milestone) super end diff --git a/app/services/issues/reopen_service.rb b/app/services/issues/reopen_service.rb index 56d59b235a7..0ffe33dd317 100644 --- a/app/services/issues/reopen_service.rb +++ b/app/services/issues/reopen_service.rb @@ -12,6 +12,7 @@ module Issues execute_hooks(issue, 'reopen') invalidate_cache_counts(issue, users: issue.assignees) issue.update_project_counter_caches + delete_milestone_closed_issue_counter_cache(issue.milestone) end issue diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 68d1657d881..78ebbd7bff2 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -115,10 +115,26 @@ module Issues end def handle_milestone_change(issue) - return if skip_milestone_email - return unless issue.previous_changes.include?('milestone_id') + invalidate_milestone_issue_counters(issue) + send_milestone_change_notification(issue) + end + + def invalidate_milestone_issue_counters(issue) + issue.previous_changes['milestone_id'].each do |milestone_id| + next unless milestone_id + + milestone = Milestone.find_by_id(milestone_id) + + delete_milestone_closed_issue_counter_cache(milestone) + delete_milestone_total_issue_counter_cache(milestone) + end + end + + def send_milestone_change_notification(issue) + return if skip_milestone_email + if issue.milestone.nil? notification_service.async.removed_milestone_issue(issue, current_user) else diff --git a/app/services/metrics/dashboard/base_embed_service.rb b/app/services/metrics/dashboard/base_embed_service.rb index 8aef9873ac1..4c7fa454460 100644 --- a/app/services/metrics/dashboard/base_embed_service.rb +++ b/app/services/metrics/dashboard/base_embed_service.rb @@ -5,6 +5,10 @@ module Metrics module Dashboard class BaseEmbedService < ::Metrics::Dashboard::BaseService + def self.embedded?(embed_param) + ActiveModel::Type::Boolean.new.cast(embed_param) + end + def cache_key "dynamic_metrics_dashboard_#{identifiers}" end diff --git a/app/services/metrics/dashboard/custom_metric_embed_service.rb b/app/services/metrics/dashboard/custom_metric_embed_service.rb index 9e616f4e379..456074ae6ad 100644 --- a/app/services/metrics/dashboard/custom_metric_embed_service.rb +++ b/app/services/metrics/dashboard/custom_metric_embed_service.rb @@ -18,7 +18,7 @@ module Metrics # custom metrics from the DB. def valid_params?(params) [ - params[:embedded], + embedded?(params[:embedded]), valid_dashboard?(params[:dashboard_path]), valid_group_title?(params[:group]), params[:title].present?, diff --git a/app/services/metrics/dashboard/default_embed_service.rb b/app/services/metrics/dashboard/default_embed_service.rb index 39f7c3943dd..30a8150d6be 100644 --- a/app/services/metrics/dashboard/default_embed_service.rb +++ b/app/services/metrics/dashboard/default_embed_service.rb @@ -22,7 +22,7 @@ module Metrics class << self def valid_params?(params) - params[:embedded].present? + embedded?(params[:embedded]) end end diff --git a/app/services/metrics/dashboard/dynamic_embed_service.rb b/app/services/metrics/dashboard/dynamic_embed_service.rb index db5b7c9e32a..ff540c30579 100644 --- a/app/services/metrics/dashboard/dynamic_embed_service.rb +++ b/app/services/metrics/dashboard/dynamic_embed_service.rb @@ -22,7 +22,7 @@ module Metrics # for additional info on defining custom dashboards. def valid_params?(params) [ - params[:embedded], + embedded?(params[:embedded]), params[:group].present?, params[:title].present?, params[:y_label] diff --git a/app/services/metrics/dashboard/grafana_metric_embed_service.rb b/app/services/metrics/dashboard/grafana_metric_embed_service.rb index 44b58ad9729..3ad3a2c609e 100644 --- a/app/services/metrics/dashboard/grafana_metric_embed_service.rb +++ b/app/services/metrics/dashboard/grafana_metric_embed_service.rb @@ -6,7 +6,7 @@ # Use Gitlab::Metrics::Dashboard::Finder to retrive dashboards. module Metrics module Dashboard - class GrafanaMetricEmbedService < ::Metrics::Dashboard::BaseService + class GrafanaMetricEmbedService < ::Metrics::Dashboard::BaseEmbedService include ReactiveCaching SEQUENCE = [ @@ -24,7 +24,7 @@ module Metrics # to uniquely identify a grafana dashboard. def valid_params?(params) [ - params[:embedded], + embedded?(params[:embedded]), params[:grafana_url] ].all? end diff --git a/app/services/milestones/closed_issues_count_service.rb b/app/services/milestones/closed_issues_count_service.rb new file mode 100644 index 00000000000..80aab235e49 --- /dev/null +++ b/app/services/milestones/closed_issues_count_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Milestones + class ClosedIssuesCountService < BaseCountService + def initialize(milestone) + @milestone = milestone + end + + def cache_key + "milestone_closed_issues_count_#{@milestone.milestoneish_id}" + end + + def relation_for_count + @milestone.issues.closed + end + end +end diff --git a/app/services/milestones/issues_count_service.rb b/app/services/milestones/issues_count_service.rb new file mode 100644 index 00000000000..f8b80fa9aef --- /dev/null +++ b/app/services/milestones/issues_count_service.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Milestones + class IssuesCountService < BaseCountService + def initialize(milestone) + @milestone = milestone + end + + def cache_key + "milestone_total_issues_count_#{@milestone.milestoneish_id}" + end + + def relation_for_count + @milestone.issues + end + end +end diff --git a/app/services/milestones/transfer_service.rb b/app/services/milestones/transfer_service.rb index 1efbfed4853..213c6f8f1dd 100644 --- a/app/services/milestones/transfer_service.rb +++ b/app/services/milestones/transfer_service.rb @@ -22,7 +22,7 @@ module Milestones milestones_to_transfer.find_each do |milestone| new_milestone = find_or_create_milestone(milestone) - update_issues_milestone(milestone.id, new_milestone&.id) + update_issues_milestone(milestone, new_milestone) update_merge_requests_milestone(milestone.id, new_milestone&.id) end end @@ -68,9 +68,12 @@ module Milestones end # rubocop: disable CodeReuse/ActiveRecord - def update_issues_milestone(old_milestone_id, new_milestone_id) - Issue.where(project: project, milestone_id: old_milestone_id) - .update_all(milestone_id: new_milestone_id) + def update_issues_milestone(old_milestone, new_milestone) + Issue.where(project: project, milestone_id: old_milestone.id) + .update_all(milestone_id: new_milestone&.id) + + delete_milestone_issues_caches(old_milestone) + delete_milestone_issues_caches(new_milestone) end # rubocop: enable CodeReuse/ActiveRecord @@ -80,5 +83,12 @@ module Milestones .update_all(milestone_id: new_milestone_id) end # rubocop: enable CodeReuse/ActiveRecord + + def delete_milestone_issues_caches(milestone) + return unless milestone + + Milestones::IssuesCountService.new(milestone).delete_cache + Milestones::ClosedIssuesCountService.new(milestone).delete_cache + end end end diff --git a/app/services/post_receive_service.rb b/app/services/post_receive_service.rb index bc5ec22e77f..f12e45d701a 100644 --- a/app/services/post_receive_service.rb +++ b/app/services/post_receive_service.rb @@ -4,10 +4,11 @@ # # Used for scheduling related jobs after a push action has been performed class PostReceiveService - attr_reader :user, :project, :params + attr_reader :user, :repository, :project, :params - def initialize(user, project, params) + def initialize(user, repository, project, params) @user = user + @repository = repository @project = project @params = params end @@ -24,7 +25,7 @@ class PostReceiveService mr_options = push_options.get(:merge_request) if mr_options.present? - message = process_mr_push_options(mr_options, project, user, params[:changes]) + message = process_mr_push_options(mr_options, params[:changes]) response.add_alert_message(message) end @@ -46,8 +47,13 @@ class PostReceiveService response end - def process_mr_push_options(push_options, project, user, changes) + def process_mr_push_options(push_options, changes) Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-foss/issues/61359') + return unless repository + + unless repository.repo_type.project? + return push_options_warning('Push options are only supported for projects') + end service = ::MergeRequests::PushOptionsHandlerService.new( project, user, changes, push_options @@ -64,6 +70,8 @@ class PostReceiveService end def merge_request_urls + return [] unless repository&.repo_type&.project? + ::MergeRequests::GetUrlsService.new(project).execute(params[:changes]) end end diff --git a/app/views/admin/broadcast_messages/_form.html.haml b/app/views/admin/broadcast_messages/_form.html.haml index 9577a2a79df..8b86a024a6e 100644 --- a/app/views/admin/broadcast_messages/_form.html.haml +++ b/app/views/admin/broadcast_messages/_form.html.haml @@ -46,6 +46,13 @@ = render_suggested_colors + .form-group.row.js-broadcast-message-dismissable-form-group{ class: ('hidden' unless @broadcast_message.banner? ) } + .col-sm-2.col-form-label.pt-0 + = f.label :starts_at, _("Dismissable") + .col-sm-10 + = f.check_box :dismissable + = f.label :dismissable do + = _('Allow users to dismiss the broadcast message') .form-group.row.js-toggle-colors-container.toggle-colors.hide .col-sm-2.col-form-label = f.label :font, "Font Color" diff --git a/app/views/shared/_broadcast_message.html.haml b/app/views/shared/_broadcast_message.html.haml index c058b210688..bc4db672938 100644 --- a/app/views/shared/_broadcast_message.html.haml +++ b/app/views/shared/_broadcast_message.html.haml @@ -1,8 +1,10 @@ %div{ class: "broadcast-#{message.broadcast_type}-message #{opts[:preview] && 'preview'} js-broadcast-notification-#{message.id} d-flex", style: broadcast_message_style(message), dir: 'auto' } - %div + .flex-grow-1.text-right.pr-2 = sprite_icon('bullhorn', size: 16, css_class: 'vertical-align-text-top') + %div{ class: !fluid_layout && 'container-limited' } = render_broadcast_message(message) - - if message.notification? && opts[:preview].blank? - %button.js-dismiss-current-broadcast-notification.btn.btn-link.text-dark.pl-2.pr-2{ 'aria-label' => _('Close'), :type => 'button', data: { id: message.id } } - %i.fa.fa-times + .flex-grow-1.text-right{ style: 'flex-basis: 0' } + - if (message.notification? || message.dismissable?) && opts[:preview].blank? + %button.broadcast-message-dismiss.js-dismiss-current-broadcast-notification.btn.btn-link.pl-2.pr-2{ 'aria-label' => _('Close'), :type => 'button', data: { id: message.id } } + %i.fa.fa-times diff --git a/app/views/shared/milestones/_issuable.html.haml b/app/views/shared/milestones/_issuable.html.haml index 965c72b82ba..0adfe2f0c04 100644 --- a/app/views/shared/milestones/_issuable.html.haml +++ b/app/views/shared/milestones/_issuable.html.haml @@ -1,9 +1,8 @@ -# @project is present when viewing Project's milestone - project = @project || issuable.project -- namespace = @project_namespace || project.namespace.becomes(Namespace) - labels = issuable.labels - assignees = issuable.assignees -- base_url_args = [namespace, project] +- base_url_args = [project] - issuable_type_args = base_url_args + [issuable.class.table_name] - issuable_url_args = base_url_args + [issuable] @@ -17,7 +16,7 @@ = confidential_icon(issuable) = link_to issuable.title, issuable_url_args, title: issuable.title .issuable-detail - = link_to [namespace, project, issuable], class: 'issue-link' do + = link_to issuable_url_args, class: 'issue-link' do %span.issuable-number= issuable.to_reference - labels.each do |label| diff --git a/app/workers/post_receive.rb b/app/workers/post_receive.rb index d0eb188cc42..5178fabb2d8 100644 --- a/app/workers/post_receive.rb +++ b/app/workers/post_receive.rb @@ -9,9 +9,9 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker weight 5 def perform(gl_repository, identifier, changes, push_options = {}) - project, repo_type = Gitlab::GlRepository.parse(gl_repository) + container, project, repo_type = Gitlab::GlRepository.parse(gl_repository) - if project.nil? + if project.nil? && (!repo_type.snippet? || container.is_a?(ProjectSnippet)) log("Triggered hook for non-existing project with gl_repository \"#{gl_repository}\"") return false end @@ -20,12 +20,14 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker # Use Sidekiq.logger so arguments can be correlated with execution # time and thread ID's. Sidekiq.logger.info "changes: #{changes.inspect}" if ENV['SIDEKIQ_LOG_ARGUMENTS'] - post_received = Gitlab::GitPostReceive.new(project, identifier, changes, push_options) + post_received = Gitlab::GitPostReceive.new(container, identifier, changes, push_options) if repo_type.wiki? - process_wiki_changes(post_received) + process_wiki_changes(post_received, container) elsif repo_type.project? - process_project_changes(post_received) + process_project_changes(post_received, container) + elsif repo_type.snippet? + process_snippet_changes(post_received, container) else # Other repos don't have hooks for now end @@ -39,24 +41,50 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker end end - def process_project_changes(post_received) + def process_project_changes(post_received, project) user = identify_user(post_received) return false unless user - project = post_received.project push_options = post_received.push_options changes = post_received.changes # We only need to expire certain caches once per push - expire_caches(post_received, post_received.project.repository) - enqueue_repository_cache_update(post_received) + expire_caches(post_received, project.repository) + enqueue_project_cache_update(post_received, project) process_ref_changes(project, user, push_options: push_options, changes: changes) - update_remote_mirrors(post_received) + update_remote_mirrors(post_received, project) after_project_changes_hooks(project, user, changes.refs, changes.repository_data) end + def process_wiki_changes(post_received, project) + project.touch(:last_activity_at, :last_repository_updated_at) + project.wiki.repository.expire_statistics_caches + ProjectCacheWorker.perform_async(project.id, [], [:wiki_size]) + + user = identify_user(post_received) + return false unless user + + # We only need to expire certain caches once per push + expire_caches(post_received, project.wiki.repository) + + ::Git::WikiPushService.new(project, user, changes: post_received.changes).execute + end + + def process_snippet_changes(post_received, snippet) + user = identify_user(post_received) + + return false unless user + + # At the moment, we only expires the repository caches. + # In the future we might need to call ProjectCacheWorker + # (or the custom class we create) to update the snippet + # repository size or any other key. + # We might also need to update the repository statistics. + expire_caches(post_received, snippet.repository) + end + # Expire the repository status, branch, and tag cache once per push. def expire_caches(post_received, repository) repository.expire_status_cache if repository.empty? @@ -65,12 +93,12 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker end # Schedule an update for the repository size and commit count if necessary. - def enqueue_repository_cache_update(post_received) + def enqueue_project_cache_update(post_received, project) stats_to_invalidate = [:repository_size] stats_to_invalidate << :commit_count if post_received.includes_default_branch? ProjectCacheWorker.perform_async( - post_received.project.id, + project.id, [], stats_to_invalidate, true @@ -83,10 +111,9 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker Git::ProcessRefChangesService.new(project, user, params).execute end - def update_remote_mirrors(post_received) + def update_remote_mirrors(post_received, project) return unless post_received.includes_branches? || post_received.includes_tags? - project = post_received.project return unless project.has_remote_mirror? project.mark_stuck_remote_mirrors_as_failed! @@ -99,20 +126,6 @@ class PostReceive # rubocop:disable Scalability/IdempotentWorker Gitlab::UsageDataCounters::SourceCodeCounter.count(:pushes) end - def process_wiki_changes(post_received) - post_received.project.touch(:last_activity_at, :last_repository_updated_at) - post_received.project.wiki.repository.expire_statistics_caches - ProjectCacheWorker.perform_async(post_received.project.id, [], [:wiki_size]) - - user = identify_user(post_received) - return false unless user - - # We only need to expire certain caches once per push - expire_caches(post_received, post_received.project.wiki.repository) - - ::Git::WikiPushService.new(post_received.project, user, changes: post_received.changes).execute - end - def log(message) Gitlab::GitLogger.error("POST-RECEIVE: #{message}") end diff --git a/changelogs/unreleased/feat-broadcast-message-dimsiss.yml b/changelogs/unreleased/feat-broadcast-message-dimsiss.yml new file mode 100644 index 00000000000..fd03f36d07a --- /dev/null +++ b/changelogs/unreleased/feat-broadcast-message-dimsiss.yml @@ -0,0 +1,5 @@ +--- +title: Add user dismiss option to broadcast messages +merge_request: 20665 +author: Fabio Huser +type: added diff --git a/changelogs/unreleased/fj-39176-create-project-snippet-repository.yml b/changelogs/unreleased/fj-39176-create-project-snippet-repository.yml new file mode 100644 index 00000000000..81645e30b67 --- /dev/null +++ b/changelogs/unreleased/fj-39176-create-project-snippet-repository.yml @@ -0,0 +1,5 @@ +--- +title: Update git workflows and routes to allow snippets +merge_request: 21739 +author: +type: added diff --git a/changelogs/unreleased/group_milestones_n_1.yml b/changelogs/unreleased/group_milestones_n_1.yml new file mode 100644 index 00000000000..154d29784f7 --- /dev/null +++ b/changelogs/unreleased/group_milestones_n_1.yml @@ -0,0 +1,5 @@ +--- +title: Fix N+1 in Group milestone view +merge_request: 26051 +author: +type: performance diff --git a/changelogs/unreleased/use-addressable-for-asset-proxy-badge-render.yml b/changelogs/unreleased/use-addressable-for-asset-proxy-badge-render.yml new file mode 100644 index 00000000000..6a021df8027 --- /dev/null +++ b/changelogs/unreleased/use-addressable-for-asset-proxy-badge-render.yml @@ -0,0 +1,5 @@ +--- +title: Rescue invalid URLs during badge retrieval in asset proxy +merge_request: 26524 +author: +type: fixed diff --git a/config/routes/git_http.rb b/config/routes/git_http.rb index 593f818e434..fb8119904ea 100644 --- a/config/routes/git_http.rb +++ b/config/routes/git_http.rb @@ -32,6 +32,14 @@ concern :lfsable do end end +# Git route for personal and project snippets +scope(path: ':namespace_id/:repository_id', + format: nil, + constraints: { namespace_id: Gitlab::PathRegex.personal_and_project_snippets_path_regex, repository_id: /\d+\.git/ }, + module: :repositories) do + concerns :gitactionable +end + scope(path: '*namespace_id/:repository_id', format: nil, constraints: { namespace_id: Gitlab::PathRegex.full_namespace_route_regex }) do diff --git a/db/migrate/20191123081456_add_dismissable_to_broadcast_messages.rb b/db/migrate/20191123081456_add_dismissable_to_broadcast_messages.rb new file mode 100644 index 00000000000..40235771d80 --- /dev/null +++ b/db/migrate/20191123081456_add_dismissable_to_broadcast_messages.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true + +class AddDismissableToBroadcastMessages < ActiveRecord::Migration[5.2] + DOWNTIME = false + + def change + add_column :broadcast_messages, :dismissable, :boolean + end +end diff --git a/db/schema.rb b/db/schema.rb index 5cf32dc5752..e6f668884d6 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -573,6 +573,7 @@ ActiveRecord::Schema.define(version: 2020_03_03_074328) do t.integer "cached_markdown_version" t.string "target_path", limit: 255 t.integer "broadcast_type", limit: 2, default: 1, null: false + t.boolean "dismissable" t.index ["ends_at", "broadcast_type", "id"], name: "index_broadcast_message_on_ends_at_and_broadcast_type_and_id" end diff --git a/doc/api/broadcast_messages.md b/doc/api/broadcast_messages.md index b9f9621e1f9..1ff40103750 100644 --- a/doc/api/broadcast_messages.md +++ b/doc/api/broadcast_messages.md @@ -36,7 +36,8 @@ Example response: "id":1, "active": false, "target_path": "*/welcome", - "broadcast_type": "banner" + "broadcast_type": "banner", + "dismissable": false } ] ``` @@ -73,7 +74,8 @@ Example response: "id":1, "active":false, "target_path": "*/welcome", - "broadcast_type": "banner" + "broadcast_type": "banner", + "dismissable": false } ``` @@ -87,15 +89,16 @@ POST /broadcast_messages Parameters: -| Attribute | Type | Required | Description | -|:------------|:---------|:---------|:------------------------------------------------------| -| `message` | string | yes | Message to display. | -| `starts_at` | datetime | no | Starting time (defaults to current time). | -| `ends_at` | datetime | no | Ending time (defaults to one hour from current time). | -| `color` | string | no | Background color hex code. | -| `font` | string | no | Foreground color hex code. | -| `target_path`| string | no | Target path of the broadcast message. | -| `broadcast_type`| string | no | Appearance type (defaults to banner) | +| Attribute | Type | Required | Description | +|:----------------|:---------|:---------|:------------------------------------------------------| +| `message` | string | yes | Message to display. | +| `starts_at` | datetime | no | Starting time (defaults to current time). | +| `ends_at` | datetime | no | Ending time (defaults to one hour from current time). | +| `color` | string | no | Background color hex code. | +| `font` | string | no | Foreground color hex code. | +| `target_path` | string | no | Target path of the broadcast message. | +| `broadcast_type`| string | no | Appearance type (defaults to banner) | +| `dismissable` | boolean | no | Can the user dismiss the message? | Example request: @@ -116,6 +119,7 @@ Example response: "active": true, "target_path": "*/welcome", "broadcast_type": "notification", + "dismissable": false } ``` @@ -129,16 +133,17 @@ PUT /broadcast_messages/:id Parameters: -| Attribute | Type | Required | Description | -|:------------|:---------|:---------|:-----------------------------------| -| `id` | integer | yes | ID of broadcast message to update. | -| `message` | string | no | Message to display. | -| `starts_at` | datetime | no | Starting time. | -| `ends_at` | datetime | no | Ending time. | -| `color` | string | no | Background color hex code. | -| `font` | string | no | Foreground color hex code. | -| `target_path`| string | no | Target path of the broadcast message. | -| `broadcast_type`| string | no | Appearance type (defaults to banner) | +| Attribute | Type | Required | Description | +|:----------------|:---------|:---------|:--------------------------------------| +| `id` | integer | yes | ID of broadcast message to update. | +| `message` | string | no | Message to display. | +| `starts_at` | datetime | no | Starting time. | +| `ends_at` | datetime | no | Ending time. | +| `color` | string | no | Background color hex code. | +| `font` | string | no | Foreground color hex code. | +| `target_path` | string | no | Target path of the broadcast message. | +| `broadcast_type`| string | no | Appearance type (defaults to banner) | +| `dismissable` | boolean | no | Can the user dismiss the message? | Example request: @@ -159,6 +164,7 @@ Example response: "active": true, "target_path": "*/welcome", "broadcast_type": "notification", + "dismissable": false } ``` diff --git a/doc/raketasks/backup_restore.md b/doc/raketasks/backup_restore.md index e768c38fd23..54c550f999c 100644 --- a/doc/raketasks/backup_restore.md +++ b/doc/raketasks/backup_restore.md @@ -271,6 +271,31 @@ For installations from source: sudo -u git -H bundle exec rake gitlab:backup:create SKIP=db,uploads RAILS_ENV=production ``` +### Skipping tar creation + +The last part of creating a backup is generation of a `.tar` file containing +all the parts. In some cases (for example, if the backup is picked up by other +backup software) creating a `.tar` file might be wasted effort or even directly +harmful, so you can skip this step by adding `tar` to the `SKIP` environment +variable. + +Adding `tar` to the `SKIP` variable leaves the files and directories containing the +backup in the directory used for the intermediate files. These files will be +overwritten when a new backup is created, so you should make sure they are copied +elsewhere, because you can only have one backup on the system. + +For Omnibus GitLab packages: + +```shell +sudo gitlab-backup create SKIP=tar +``` + +For installations from source: + +```shell +sudo -u git -H bundle exec rake gitlab:backup:create SKIP=tar RAILS_ENV=production +``` + ### Uploading backups to a remote (cloud) storage Starting with GitLab 7.4 you can let the backup script upload the '.tar' file it creates. @@ -658,6 +683,10 @@ lose access to your GitLab server. You may also want to restore any TLS keys, certificates, or [SSH host keys](https://superuser.com/questions/532040/copy-ssh-keys-from-one-server-to-another-server/532079#532079). +Starting with GitLab 12.9 if an untarred backup (like the ones made with +`SKIP=tar`) is found, and no backup is chosen with `BACKUP=<timestamp>`, the +untarred backup is used. + Depending on your case, you might want to run the restore command with one or more of the following options: diff --git a/doc/user/admin_area/broadcast_messages.md b/doc/user/admin_area/broadcast_messages.md index bc51552603d..416bd3bfd00 100644 --- a/doc/user/admin_area/broadcast_messages.md +++ b/doc/user/admin_area/broadcast_messages.md @@ -28,7 +28,7 @@ To add a broadcast message: NOTE: **Note:** Once a broadcast message has expired, it is no longer displayed in the UI but is still listed in the -list of broadcast messages. +list of broadcast messages. User can also dismiss a broadcast message if the option **Dismissable** is set. ## Editing a broadcast message diff --git a/doc/user/project/clusters/serverless/index.md b/doc/user/project/clusters/serverless/index.md index 1460f128eb3..cb149446a19 100644 --- a/doc/user/project/clusters/serverless/index.md +++ b/doc/user/project/clusters/serverless/index.md @@ -311,7 +311,7 @@ Explanation of the fields used above: | Parameter | Description | |-----------|-------------| | `name` | Indicates which provider is used to execute the `serverless.yml` file. In this case, the TriggerMesh middleware. | -| `envs` | Includes the environment variables to be passed as part of function execution for **all** functions in the file, where `FOO` is the variable name and `BAR` are he variable contents. You may replace this with you own variables. | +| `envs` | Includes the environment variables to be passed as part of function execution for **all** functions in the file, where `FOO` is the variable name and `BAR` are the variable contents. You may replace this with your own variables. | | `secrets` | Includes the contents of the Kubernetes secret as environment variables accessible to be passed as part of function execution for **all** functions in the file. The secrets are expected in ini format. | ### `functions` diff --git a/lib/api/broadcast_messages.rb b/lib/api/broadcast_messages.rb index af7c69f857e..42e7dc751f0 100644 --- a/lib/api/broadcast_messages.rb +++ b/lib/api/broadcast_messages.rb @@ -36,6 +36,7 @@ module API optional :font, type: String, desc: 'Foreground color' optional :target_path, type: String, desc: 'Target path' optional :broadcast_type, type: String, values: BroadcastMessage.broadcast_types.keys, desc: 'Broadcast type. Defaults to banner', default: -> { 'banner' } + optional :dismissable, type: Boolean, desc: 'Is dismissable' end post do authenticated_as_admin! @@ -75,6 +76,7 @@ module API optional :font, type: String, desc: 'Foreground color' optional :target_path, type: String, desc: 'Target path' optional :broadcast_type, type: String, values: BroadcastMessage.broadcast_types.keys, desc: 'Broadcast Type' + optional :dismissable, type: Boolean, desc: 'Is dismissable' end put ':id' do authenticated_as_admin! diff --git a/lib/api/entities/broadcast_message.rb b/lib/api/entities/broadcast_message.rb index 403677aa300..e42b110adbe 100644 --- a/lib/api/entities/broadcast_message.rb +++ b/lib/api/entities/broadcast_message.rb @@ -3,7 +3,7 @@ module API module Entities class BroadcastMessage < Grape::Entity - expose :id, :message, :starts_at, :ends_at, :color, :font, :target_path, :broadcast_type + expose :id, :message, :starts_at, :ends_at, :color, :font, :target_path, :broadcast_type, :dismissable expose :active?, as: :active end end diff --git a/lib/api/helpers/internal_helpers.rb b/lib/api/helpers/internal_helpers.rb index ab43096a1de..f7aabc8ce4f 100644 --- a/lib/api/helpers/internal_helpers.rb +++ b/lib/api/helpers/internal_helpers.rb @@ -3,7 +3,7 @@ module API module Helpers module InternalHelpers - attr_reader :redirected_path + attr_reader :redirected_path, :container delegate :wiki?, to: :repo_type @@ -22,10 +22,10 @@ module API end def access_checker_for(actor, protocol) - access_checker_klass.new(actor.key_or_user, project, protocol, + access_checker_klass.new(actor.key_or_user, container, protocol, authentication_abilities: ssh_authentication_abilities, namespace_path: namespace_path, - project_path: project_path, + repository_path: project_path, redirected_path: redirected_path) end @@ -80,7 +80,7 @@ module API # rubocop:disable Gitlab/ModuleWithInstanceVariables def set_project - @project, @repo_type, @redirected_path = + @container, @project, @repo_type, @redirected_path = if params[:gl_repository] Gitlab::GlRepository.parse(params[:gl_repository]) elsif params[:project] @@ -92,17 +92,17 @@ module API # Project id to pass between components that don't share/don't have # access to the same filesystem mounts def gl_repository - repo_type.identifier_for_container(project) + repo_type.identifier_for_container(container) end - def gl_project_path + def gl_repository_path repository.full_path end # Return the repository depending on whether we want the wiki or the # regular repository def repository - @repository ||= repo_type.repository_for(project) + @repository ||= repo_type.repository_for(container) end # Return the Gitaly Address if it is enabled @@ -111,8 +111,8 @@ module API { repository: repository.gitaly_repository, - address: Gitlab::GitalyClient.address(project.repository_storage), - token: Gitlab::GitalyClient.token(project.repository_storage), + address: Gitlab::GitalyClient.address(container.repository_storage), + token: Gitlab::GitalyClient.token(container.repository_storage), features: Feature::Gitaly.server_feature_flags } end diff --git a/lib/api/internal/base.rb b/lib/api/internal/base.rb index ba66404e406..2e1891ac656 100644 --- a/lib/api/internal/base.rb +++ b/lib/api/internal/base.rb @@ -67,7 +67,7 @@ module API when ::Gitlab::GitAccessResult::Success payload = { gl_repository: gl_repository, - gl_project_path: gl_project_path, + gl_project_path: gl_repository_path, gl_id: Gitlab::GlId.gl_id(actor.user), gl_username: actor.username, git_config_options: [], @@ -216,7 +216,7 @@ module API post '/post_receive' do status 200 - response = PostReceiveService.new(actor.user, project, params).execute + response = PostReceiveService.new(actor.user, repository, project, params).execute ee_post_receive_response_hook(response) diff --git a/lib/backup/manager.rb b/lib/backup/manager.rb index 2b6b10cf044..915567f8106 100644 --- a/lib/backup/manager.rb +++ b/lib/backup/manager.rb @@ -12,7 +12,7 @@ module Backup @progress = progress end - def pack + def write_info # Make sure there is a connection ActiveRecord::Base.connection.reconnect! @@ -20,7 +20,11 @@ module Backup File.open("#{backup_path}/backup_information.yml", "w+") do |file| file << backup_information.to_yaml.gsub(/^---\n/, '') end + end + end + def pack + Dir.chdir(backup_path) do # create archive progress.print "Creating backup archive: #{tar_file} ... " # Set file permissions on open to prevent chmod races. @@ -31,8 +35,6 @@ module Backup puts "creating archive #{tar_file} failed".color(:red) raise Backup::Error, 'Backup failed' end - - upload end end @@ -105,8 +107,30 @@ module Backup end end - # rubocop: disable Metrics/AbcSize + def verify_backup_version + Dir.chdir(backup_path) do + # restoring mismatching backups can lead to unexpected problems + if settings[:gitlab_version] != Gitlab::VERSION + progress.puts(<<~HEREDOC.color(:red)) + GitLab version mismatch: + Your current GitLab version (#{Gitlab::VERSION}) differs from the GitLab version in the backup! + Please switch to the following version and try again: + version: #{settings[:gitlab_version]} + HEREDOC + progress.puts + progress.puts "Hint: git checkout v#{settings[:gitlab_version]}" + exit 1 + end + end + end + def unpack + if ENV['BACKUP'].blank? && non_tarred_backup? + progress.puts "Non tarred backup found in #{backup_path}, using that" + + return false + end + Dir.chdir(backup_path) do # check for existing backups in the backup dir if backup_file_list.empty? @@ -141,21 +165,6 @@ module Backup progress.puts 'unpacking backup failed'.color(:red) exit 1 end - - ENV["VERSION"] = "#{settings[:db_version]}" if settings[:db_version].to_i > 0 - - # restoring mismatching backups can lead to unexpected problems - if settings[:gitlab_version] != Gitlab::VERSION - progress.puts(<<~HEREDOC.color(:red)) - GitLab version mismatch: - Your current GitLab version (#{Gitlab::VERSION}) differs from the GitLab version in the backup! - Please switch to the following version and try again: - version: #{settings[:gitlab_version]} - HEREDOC - progress.puts - progress.puts "Hint: git checkout v#{settings[:gitlab_version]}" - exit 1 - end end end @@ -170,6 +179,10 @@ module Backup private + def non_tarred_backup? + File.exist?(File.join(backup_path, 'backup_information.yml')) + end + def backup_path Gitlab.config.backup.path end @@ -252,7 +265,7 @@ module Backup def create_attributes attrs = { key: remote_target, - body: File.open(tar_file), + body: File.open(File.join(backup_path, tar_file)), multipart_chunk_size: Gitlab.config.backup.upload.multipart_chunk_size, encryption: Gitlab.config.backup.upload.encryption, encryption_key: Gitlab.config.backup.upload.encryption_key, diff --git a/lib/gitlab/asset_proxy.rb b/lib/gitlab/asset_proxy.rb index fd7c58ba68f..fb4369e01d8 100644 --- a/lib/gitlab/asset_proxy.rb +++ b/lib/gitlab/asset_proxy.rb @@ -11,12 +11,14 @@ module Gitlab return url if asset_host_whitelisted?(url) "#{Gitlab.config.asset_proxy.url}/#{asset_url_hash(url)}/#{hexencode(url)}" + rescue Addressable::URI::InvalidURIError + url end private def asset_host_whitelisted?(url) - parsed_url = URI.parse(url) + parsed_url = Addressable::URI.parse(url) Gitlab.config.asset_proxy.domain_regexp&.match?(parsed_url.host) end diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb index 45db423187c..7a13a35c096 100644 --- a/lib/gitlab/git_access.rb +++ b/lib/gitlab/git_access.rb @@ -43,15 +43,15 @@ module Gitlab PUSH_COMMANDS = %w{git-receive-pack}.freeze ALL_COMMANDS = DOWNLOAD_COMMANDS + PUSH_COMMANDS - attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :project_path, :redirected_path, :auth_result_type, :changes, :logger + attr_reader :actor, :project, :protocol, :authentication_abilities, :namespace_path, :repository_path, :redirected_path, :auth_result_type, :changes, :logger - def initialize(actor, project, protocol, authentication_abilities:, namespace_path: nil, project_path: nil, redirected_path: nil, auth_result_type: nil) + def initialize(actor, project, protocol, authentication_abilities:, namespace_path: nil, repository_path: nil, redirected_path: nil, auth_result_type: nil) @actor = actor @project = project @protocol = protocol - @authentication_abilities = authentication_abilities + @authentication_abilities = Array(authentication_abilities) @namespace_path = namespace_path || project&.namespace&.full_path - @project_path = project_path || project&.path + @repository_path = repository_path || project&.path @redirected_path = redirected_path @auth_result_type = auth_result_type end @@ -224,7 +224,7 @@ module Gitlab return unless user&.can?(:create_projects, namespace) project_params = { - path: project_path, + path: repository_path, namespace_id: namespace.id, visibility_level: Gitlab::VisibilityLevel::PRIVATE } diff --git a/lib/gitlab/git_post_receive.rb b/lib/gitlab/git_post_receive.rb index 5264bae47a1..13d991cdfbd 100644 --- a/lib/gitlab/git_post_receive.rb +++ b/lib/gitlab/git_post_receive.rb @@ -3,10 +3,10 @@ module Gitlab class GitPostReceive include Gitlab::Identifier - attr_reader :project, :identifier, :changes, :push_options + attr_reader :container, :identifier, :changes, :push_options - def initialize(project, identifier, changes, push_options = {}) - @project = project + def initialize(container, identifier, changes, push_options = {}) + @container = container @identifier = identifier @changes = parse_changes(changes) @push_options = push_options @@ -27,10 +27,10 @@ module Gitlab def includes_default_branch? # If the branch doesn't have a default branch yet, we presume the # first branch pushed will be the default. - return true unless project.default_branch.present? + return true unless container.default_branch.present? changes.branch_changes.any? do |change| - Gitlab::Git.branch_name(change[:ref]) == project.default_branch + Gitlab::Git.branch_name(change[:ref]) == container.default_branch end end diff --git a/lib/gitlab/gl_repository.rb b/lib/gitlab/gl_repository.rb index fcebcb463cd..26440e6f82d 100644 --- a/lib/gitlab/gl_repository.rb +++ b/lib/gitlab/gl_repository.rb @@ -7,19 +7,21 @@ module Gitlab PROJECT = RepoType.new( name: :project, access_checker_class: Gitlab::GitAccess, - repository_resolver: -> (project) { project.repository } + repository_resolver: -> (project) { project&.repository } ).freeze WIKI = RepoType.new( name: :wiki, access_checker_class: Gitlab::GitAccessWiki, - repository_resolver: -> (project) { project.wiki.repository }, + repository_resolver: -> (project) { project&.wiki&.repository }, suffix: :wiki ).freeze SNIPPET = RepoType.new( name: :snippet, access_checker_class: Gitlab::GitAccessSnippet, - repository_resolver: -> (snippet) { snippet.repository }, - container_resolver: -> (id) { Snippet.find_by_id(id) } + repository_resolver: -> (snippet) { snippet&.repository }, + container_resolver: -> (id) { Snippet.find_by_id(id) }, + project_resolver: -> (snippet) { snippet&.project }, + guest_read_ability: :read_snippet ).freeze TYPES = { @@ -42,7 +44,7 @@ module Gitlab container = type.fetch_container!(gl_repository) - [container, type] + [container, type.project_for(container), type] end def self.default_type diff --git a/lib/gitlab/gl_repository/repo_type.rb b/lib/gitlab/gl_repository/repo_type.rb index 9663fd7de8f..052ce578881 100644 --- a/lib/gitlab/gl_repository/repo_type.rb +++ b/lib/gitlab/gl_repository/repo_type.rb @@ -7,6 +7,8 @@ module Gitlab :access_checker_class, :repository_resolver, :container_resolver, + :project_resolver, + :guest_read_ability, :suffix def initialize( @@ -14,11 +16,15 @@ module Gitlab access_checker_class:, repository_resolver:, container_resolver: default_container_resolver, + project_resolver: nil, + guest_read_ability: :download_code, suffix: nil) @name = name @access_checker_class = access_checker_class @repository_resolver = repository_resolver @container_resolver = container_resolver + @project_resolver = project_resolver + @guest_read_ability = guest_read_ability @suffix = suffix end @@ -59,8 +65,18 @@ module Gitlab repository_resolver.call(container) end + def project_for(container) + return container unless project_resolver + + project_resolver.call(container) + end + def valid?(repository_path) - repository_path.end_with?(path_suffix) + repository_path.end_with?(path_suffix) && + ( + !snippet? || + repository_path.match?(Gitlab::PathRegex.full_snippets_repository_path_regex) + ) end private diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb index 9606e3e134c..db094bfe973 100644 --- a/lib/gitlab/path_regex.rb +++ b/lib/gitlab/path_regex.rb @@ -237,8 +237,32 @@ module Gitlab }x end + def full_snippets_repository_path_regex + %r{\A(#{personal_snippet_repository_path_regex}|#{project_snippet_repository_path_regex})\z} + end + + def personal_and_project_snippets_path_regex + %r{#{personal_snippet_path_regex}|#{project_snippet_path_regex}} + end + private + def personal_snippet_path_regex + /snippets/ + end + + def personal_snippet_repository_path_regex + %r{#{personal_snippet_path_regex}/\d+} + end + + def project_snippet_path_regex + %r{#{full_namespace_route_regex}/#{project_route_regex}/snippets} + end + + def project_snippet_repository_path_regex + %r{#{project_snippet_path_regex}/\d+} + end + def single_line_regexp(regex) # Turns a multiline extended regexp into a single line one, # because `rake routes` breaks on multiline regexes. diff --git a/lib/gitlab/repo_path.rb b/lib/gitlab/repo_path.rb index e8c749cac14..da74c38d0ec 100644 --- a/lib/gitlab/repo_path.rb +++ b/lib/gitlab/repo_path.rb @@ -19,22 +19,33 @@ module Gitlab # Removing the suffix (.wiki, .design, ...) from the project path full_path = repo_path.chomp(type.path_suffix) - - project, was_redirected = find_project(full_path) + container, project, was_redirected = find_container(type, full_path) redirected_path = repo_path if was_redirected - # If we found a matching project, then the type was matched, no need to - # continue looking. - return [project, type, redirected_path] if project + return [container, project, type, redirected_path] if container end # When a project did not exist, the parsed repo_type would be empty. # In that case, we want to continue with a regular project repository. As we # could create the project if the user pushing is allowed to do so. - [nil, Gitlab::GlRepository.default_type, nil] + [nil, nil, Gitlab::GlRepository.default_type, nil] + end + + def self.find_container(type, full_path) + if type.snippet? + snippet, was_redirected = find_snippet(full_path) + + [snippet, snippet&.project, was_redirected] + else + project, was_redirected = find_project(full_path) + + [project, project, was_redirected] + end end def self.find_project(project_path) + return [nil, false] if project_path.blank? + project = Project.find_by_full_path(project_path, follow_redirects: true) [project, redirected?(project, project_path)] @@ -43,6 +54,27 @@ module Gitlab def self.redirected?(project, project_path) project && project.full_path.casecmp(project_path) != 0 end + + # Snippet_path can be either: + # - snippets/1 + # - h5bp/html5-boilerplate/snippets/53 + def self.find_snippet(snippet_path) + return [nil, false] if snippet_path.blank? + + snippet_id, project_path = extract_snippet_info(snippet_path) + project, was_redirected = find_project(project_path) + + [Snippet.find_by_id_and_project(id: snippet_id, project: project), was_redirected] + end + + def self.extract_snippet_info(snippet_path) + path_segments = snippet_path.split('/') + snippet_id = path_segments.pop + path_segments.pop # Remove snippets from path + project_path = File.join(path_segments) + + [snippet_id, project_path] + end end end diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 8696e23cbc7..7da20b49d9d 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -24,7 +24,7 @@ module Gitlab attrs = { GL_ID: Gitlab::GlId.gl_id(user), - GL_REPOSITORY: repo_type.identifier_for_container(repository.project), + GL_REPOSITORY: repo_type.identifier_for_container(repository.container), GL_USERNAME: user&.username, ShowAllRefs: show_all_refs, Repository: repository.gitaly_repository.to_h, diff --git a/lib/tasks/gitlab/backup.rake b/lib/tasks/gitlab/backup.rake index 8f34101ea15..e5e2faaa7df 100644 --- a/lib/tasks/gitlab/backup.rake +++ b/lib/tasks/gitlab/backup.rake @@ -17,9 +17,16 @@ namespace :gitlab do Rake::Task['gitlab:backup:registry:create'].invoke backup = Backup::Manager.new(progress) - backup.pack - backup.cleanup - backup.remove_old + backup.write_info + + if ENV['SKIP'] && ENV['SKIP'].include?('tar') + backup.upload + else + backup.pack + backup.upload + backup.cleanup + backup.remove_old + end progress.puts "Warning: Your gitlab.rb and gitlab-secrets.json files contain sensitive data \n" \ "and are not included in this backup. You will need these files to restore a backup.\n" \ @@ -33,7 +40,8 @@ namespace :gitlab do warn_user_is_not_gitlab backup = Backup::Manager.new(progress) - backup.unpack + cleanup_required = backup.unpack + backup.verify_backup_version unless backup.skipped?('db') begin @@ -72,7 +80,10 @@ namespace :gitlab do Rake::Task['gitlab:shell:setup'].invoke Rake::Task['cache:clear'].invoke - backup.cleanup + if cleanup_required + backup.cleanup + end + puts "Warning: Your gitlab.rb and gitlab-secrets.json files contain sensitive data \n" \ "and are not included in this backup. You will need to restore these files manually.".color(:red) puts "Restore task is done." diff --git a/locale/gitlab.pot b/locale/gitlab.pot index c6738dafa23..f8325f680f2 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -1666,6 +1666,9 @@ msgstr "" msgid "Allow this secondary node to replicate content on Object Storage" msgstr "" +msgid "Allow users to dismiss the broadcast message" +msgstr "" + msgid "Allow users to register any application to use GitLab as an OAuth provider" msgstr "" @@ -6910,6 +6913,9 @@ msgstr "" msgid "Dismiss trial promotion" msgstr "" +msgid "Dismissable" +msgstr "" + msgid "Dismissed" msgstr "" diff --git a/spec/controllers/repositories/git_http_controller_spec.rb b/spec/controllers/repositories/git_http_controller_spec.rb index 10a7b72ca89..2573fdf16ff 100644 --- a/spec/controllers/repositories/git_http_controller_spec.rb +++ b/spec/controllers/repositories/git_http_controller_spec.rb @@ -6,16 +6,18 @@ describe Repositories::GitHttpController do include GitHttpHelpers let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:personal_snippet) { create(:personal_snippet, :public, :repository) } + let_it_be(:project_snippet) { create(:project_snippet, :public, :repository, project: project) } let(:namespace_id) { project.namespace.to_param } let(:repository_id) { project.path + '.git' } - let(:project_params) do + let(:container_params) do { namespace_id: namespace_id, repository_id: repository_id } end - let(:params) { project_params } + let(:params) { container_params } describe 'HEAD #info_refs' do it 'returns 403' do @@ -27,7 +29,7 @@ describe Repositories::GitHttpController do shared_examples 'info_refs behavior' do describe 'GET #info_refs' do - let(:params) { project_params.merge(service: 'git-upload-pack') } + let(:params) { container_params.merge(service: 'git-upload-pack') } it 'returns 401 for unauthenticated requests to public repositories when http protocol is disabled' do stub_application_setting(enabled_git_access_protocol: 'ssh') @@ -41,8 +43,6 @@ describe Repositories::GitHttpController do end context 'with authorized user' do - let(:user) { project.owner } - before do request.headers.merge! auth_env(user.username, user.password, nil) end @@ -122,7 +122,7 @@ describe Repositories::GitHttpController do end shared_examples 'access checker class' do - let(:params) { project_params.merge(service: 'git-upload-pack') } + let(:params) { container_params.merge(service: 'git-upload-pack') } it 'calls the right access class checker with the right object' do allow(controller).to receive(:verify_workhorse_api!).and_return(true) @@ -136,11 +136,41 @@ describe Repositories::GitHttpController do end context 'when repository container is a project' do - it_behaves_like 'info_refs behavior' + it_behaves_like 'info_refs behavior' do + let(:user) { project.owner } + end it_behaves_like 'git_upload_pack behavior', true it_behaves_like 'access checker class' do let(:expected_class) { Gitlab::GitAccess } let(:expected_object) { project } end end + + context 'when repository container is a personal snippet' do + let(:namespace_id) { 'snippets' } + let(:repository_id) { personal_snippet.to_param + '.git' } + + it_behaves_like 'info_refs behavior' do + let(:user) { personal_snippet.author } + end + it_behaves_like 'git_upload_pack behavior', false + it_behaves_like 'access checker class' do + let(:expected_class) { Gitlab::GitAccessSnippet } + let(:expected_object) { personal_snippet } + end + end + + context 'when repository container is a project snippet' do + let(:namespace_id) { project.full_path + '/snippets' } + let(:repository_id) { project_snippet.to_param + '.git' } + + it_behaves_like 'info_refs behavior' do + let(:user) { project_snippet.author } + end + it_behaves_like 'git_upload_pack behavior', false + it_behaves_like 'access checker class' do + let(:expected_class) { Gitlab::GitAccessSnippet } + let(:expected_object) { project_snippet } + end + end end diff --git a/spec/features/broadcast_messages_spec.rb b/spec/features/broadcast_messages_spec.rb index 43fbf1010c9..d7c84b8085b 100644 --- a/spec/features/broadcast_messages_spec.rb +++ b/spec/features/broadcast_messages_spec.rb @@ -3,28 +3,58 @@ require 'spec_helper' describe 'Broadcast Messages' do - let!(:broadcast_message) { create(:broadcast_message, broadcast_type: 'notification', message: 'SampleMessage') } + shared_examples 'a Broadcast Messages' do + it 'shows broadcast message' do + visit root_path - it 'shows broadcast message' do - visit root_path + expect(page).to have_content 'SampleMessage' + end + end + + shared_examples 'a dismissable Broadcast Messages' do + it 'hides broadcast message after dismiss', :js do + visit root_path + + find('.js-dismiss-current-broadcast-notification').click + + expect(page).not_to have_content 'SampleMessage' + end + + it 'broadcast message is still hidden after refresh', :js do + visit root_path + + find('.js-dismiss-current-broadcast-notification').click + visit root_path + + expect(page).not_to have_content 'SampleMessage' + end + end + + describe 'banner type' do + let!(:broadcast_message) { create(:broadcast_message, message: 'SampleMessage') } + + it_behaves_like 'a Broadcast Messages' + + it 'shows broadcast message' do + visit root_path - expect(page).to have_content 'SampleMessage' + expect(page).not_to have_selector('.js-dismiss-current-broadcast-notification') + end end - it 'hides broadcast message after dismiss', :js do - visit root_path + describe 'dismissable banner type' do + let!(:broadcast_message) { create(:broadcast_message, dismissable: true, message: 'SampleMessage') } - find('.js-dismiss-current-broadcast-notification').click + it_behaves_like 'a Broadcast Messages' - expect(page).not_to have_content 'SampleMessage' + it_behaves_like 'a dismissable Broadcast Messages' end - it 'broadcast message is still hidden after refresh', :js do - visit root_path + describe 'notification type' do + let!(:broadcast_message) { create(:broadcast_message, broadcast_type: 'notification', message: 'SampleMessage') } - find('.js-dismiss-current-broadcast-notification').click - visit root_path + it_behaves_like 'a Broadcast Messages' - expect(page).not_to have_content 'SampleMessage' + it_behaves_like 'a dismissable Broadcast Messages' end end diff --git a/spec/helpers/broadcast_messages_helper_spec.rb b/spec/helpers/broadcast_messages_helper_spec.rb index 7e181e429d7..69ff5f16ec1 100644 --- a/spec/helpers/broadcast_messages_helper_spec.rb +++ b/spec/helpers/broadcast_messages_helper_spec.rb @@ -14,7 +14,7 @@ describe BroadcastMessagesHelper do context 'when last broadcast message is hidden' do before do - helper.request.cookies["hide_broadcast_notification_message_#{broadcast_message_2.id}"] = 'true' + helper.request.cookies["hide_broadcast_message_#{broadcast_message_2.id}"] = 'true' end it { is_expected.to eq broadcast_message_1 } @@ -29,6 +29,10 @@ describe BroadcastMessagesHelper do describe 'broadcast_message' do let(:current_broadcast_message) { BroadcastMessage.new(message: 'Current Message') } + before do + allow(helper).to receive(:current_user).and_return(create(:user)) + end + it 'returns nil when no current message' do expect(helper.broadcast_message(nil)).to be_nil end diff --git a/spec/lib/backup/manager_spec.rb b/spec/lib/backup/manager_spec.rb index 06ad0557e37..cee299522ce 100644 --- a/spec/lib/backup/manager_spec.rb +++ b/spec/lib/backup/manager_spec.rb @@ -214,6 +214,30 @@ describe Backup::Manager do end end + describe 'verify_backup_version' do + context 'on version mismatch' do + let(:gitlab_version) { Gitlab::VERSION } + + it 'stops the process' do + allow(YAML).to receive(:load_file) + .and_return({ gitlab_version: "not #{gitlab_version}" }) + + expect { subject.verify_backup_version }.to raise_error SystemExit + end + end + + context 'on version match' do + let(:gitlab_version) { Gitlab::VERSION } + + it 'does nothing' do + allow(YAML).to receive(:load_file) + .and_return({ gitlab_version: "#{gitlab_version}" }) + + expect { subject.verify_backup_version }.not_to raise_error + end + end + end + describe '#unpack' do context 'when there are no backup files in the directory' do before do @@ -292,6 +316,23 @@ describe Backup::Manager do expect(progress).to have_received(:puts).with(a_string_matching('done')) end end + + context 'when there is a non-tarred backup in the directory' do + before do + allow(Dir).to receieve(:glob).and_return( + [ + 'backup_information.yml' + ] + ) + + it 'selects the non-tarred backup to restore from' do + expect { subject.unpack }.to output.to_stdout + expect(progress).to have_received(:puts) + .with(a_string_matching('Non tarred backup found ')) + expect(Kernel).not_to receive(:system) + end + end + end end describe '#upload' do @@ -329,9 +370,7 @@ describe Backup::Manager do .with(hash_including(key: backup_filename, public: false)) .and_return(true) - Dir.chdir(Gitlab.config.backup.path) do - subject.upload - end + subject.upload end it 'adds the DIRECTORY environment variable if present' do @@ -341,9 +380,7 @@ describe Backup::Manager do .with(hash_including(key: "daily/#{backup_filename}", public: false)) .and_return(true) - Dir.chdir(Gitlab.config.backup.path) do - subject.upload - end + subject.upload end end @@ -373,9 +410,7 @@ describe Backup::Manager do .with(hash_excluding(public: false)) .and_return(true) - Dir.chdir(Gitlab.config.backup.path) do - subject.upload - end + subject.upload end end end diff --git a/spec/lib/gitlab/asset_proxy_spec.rb b/spec/lib/gitlab/asset_proxy_spec.rb index f5aa1819982..e406917a5a4 100644 --- a/spec/lib/gitlab/asset_proxy_spec.rb +++ b/spec/lib/gitlab/asset_proxy_spec.rb @@ -33,9 +33,15 @@ describe Gitlab::AssetProxy do expect(described_class.proxy_url(url)).to eq(proxied_url) end + it 'returns original URL for invalid domains' do + url = 'foo_bar://' + + expect(described_class.proxy_url(url)).to eq(url) + end + context 'whitelisted domain' do it 'returns original URL for single domain whitelist' do - url = 'http://gitlab.com/test.png' + url = 'http://gitlab.com/${default_branch}/test.png' expect(described_class.proxy_url(url)).to eq(url) end diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index f95349a2125..a29c56c598f 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -240,7 +240,7 @@ describe Gitlab::GitAccess do let(:access) do described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, - project_path: project_path, namespace_path: namespace_path, + repository_path: project_path, namespace_path: namespace_path, redirected_path: redirected_path) end @@ -259,7 +259,7 @@ describe Gitlab::GitAccess do let(:access) do described_class.new(actor, nil, protocol, authentication_abilities: authentication_abilities, - project_path: project_path, namespace_path: namespace_path, + repository_path: project_path, namespace_path: namespace_path, redirected_path: redirected_path) end @@ -453,7 +453,7 @@ describe Gitlab::GitAccess do let(:access) do described_class.new(actor, project, protocol, authentication_abilities: authentication_abilities, - project_path: project_path, namespace_path: namespace_path, + repository_path: project_path, namespace_path: namespace_path, redirected_path: redirected_path) end @@ -598,7 +598,7 @@ describe Gitlab::GitAccess do let(:public_project) { create(:project, :public, :repository) } let(:project_path) { public_project.path } let(:namespace_path) { public_project.namespace.path } - let(:access) { described_class.new(nil, public_project, 'web', authentication_abilities: [:download_code], project_path: project_path, namespace_path: namespace_path) } + let(:access) { described_class.new(nil, public_project, 'web', authentication_abilities: [:download_code], repository_path: project_path, namespace_path: namespace_path) } context 'when repository is enabled' do it 'give access to download code' do @@ -1203,7 +1203,7 @@ describe Gitlab::GitAccess do def access described_class.new(actor, project, protocol, authentication_abilities: authentication_abilities, - namespace_path: namespace_path, project_path: project_path, + namespace_path: namespace_path, repository_path: project_path, redirected_path: redirected_path, auth_result_type: auth_result_type) end diff --git a/spec/lib/gitlab/gl_repository/repo_type_spec.rb b/spec/lib/gitlab/gl_repository/repo_type_spec.rb index 7cf0442fbe1..6185b068d4c 100644 --- a/spec/lib/gitlab/gl_repository/repo_type_spec.rb +++ b/spec/lib/gitlab/gl_repository/repo_type_spec.rb @@ -5,46 +5,62 @@ describe Gitlab::GlRepository::RepoType do let_it_be(:project) { create(:project) } let_it_be(:personal_snippet) { create(:personal_snippet, author: project.owner) } let_it_be(:project_snippet) { create(:project_snippet, project: project, author: project.owner) } + let(:project_path) { project.repository.full_path } + let(:wiki_path) { project.wiki.repository.full_path } + let(:personal_snippet_path) { "snippets/#{personal_snippet.id}" } + let(:project_snippet_path) { "#{project.full_path}/snippets/#{project_snippet.id}" } describe Gitlab::GlRepository::PROJECT do it_behaves_like 'a repo type' do - let(:expected_identifier) { "project-#{project.id}" } let(:expected_id) { project.id.to_s } + let(:expected_identifier) { "project-#{expected_id}" } let(:expected_suffix) { '' } - let(:expected_repository) { project.repository } let(:expected_container) { project } + let(:expected_repository) { expected_container.repository } end it 'knows its type' do - expect(described_class).not_to be_wiki - expect(described_class).to be_project - expect(described_class).not_to be_snippet + aggregate_failures do + expect(described_class).not_to be_wiki + expect(described_class).to be_project + expect(described_class).not_to be_snippet + end end it 'checks if repository path is valid' do - expect(described_class.valid?(project.repository.full_path)).to be_truthy - expect(described_class.valid?(project.wiki.repository.full_path)).to be_truthy + aggregate_failures do + expect(described_class.valid?(project_path)).to be_truthy + expect(described_class.valid?(wiki_path)).to be_truthy + expect(described_class.valid?(personal_snippet_path)).to be_truthy + expect(described_class.valid?(project_snippet_path)).to be_truthy + end end end describe Gitlab::GlRepository::WIKI do it_behaves_like 'a repo type' do - let(:expected_identifier) { "wiki-#{project.id}" } let(:expected_id) { project.id.to_s } + let(:expected_identifier) { "wiki-#{expected_id}" } let(:expected_suffix) { '.wiki' } - let(:expected_repository) { project.wiki.repository } let(:expected_container) { project } + let(:expected_repository) { expected_container.wiki.repository } end it 'knows its type' do - expect(described_class).to be_wiki - expect(described_class).not_to be_project - expect(described_class).not_to be_snippet + aggregate_failures do + expect(described_class).to be_wiki + expect(described_class).not_to be_project + expect(described_class).not_to be_snippet + end end it 'checks if repository path is valid' do - expect(described_class.valid?(project.repository.full_path)).to be_falsey - expect(described_class.valid?(project.wiki.repository.full_path)).to be_truthy + aggregate_failures do + expect(described_class.valid?(project_path)).to be_falsey + expect(described_class.valid?(wiki_path)).to be_truthy + expect(described_class.valid?(personal_snippet_path)).to be_falsey + expect(described_class.valid?(project_snippet_path)).to be_falsey + end end end @@ -59,9 +75,20 @@ describe Gitlab::GlRepository::RepoType do end it 'knows its type' do - expect(described_class).to be_snippet - expect(described_class).not_to be_wiki - expect(described_class).not_to be_project + aggregate_failures do + expect(described_class).to be_snippet + expect(described_class).not_to be_wiki + expect(described_class).not_to be_project + end + end + + it 'checks if repository path is valid' do + aggregate_failures do + expect(described_class.valid?(project_path)).to be_falsey + expect(described_class.valid?(wiki_path)).to be_falsey + expect(described_class.valid?(personal_snippet_path)).to be_truthy + expect(described_class.valid?(project_snippet_path)).to be_truthy + end end end @@ -75,9 +102,20 @@ describe Gitlab::GlRepository::RepoType do end it 'knows its type' do - expect(described_class).to be_snippet - expect(described_class).not_to be_wiki - expect(described_class).not_to be_project + aggregate_failures do + expect(described_class).to be_snippet + expect(described_class).not_to be_wiki + expect(described_class).not_to be_project + end + end + + it 'checks if repository path is valid' do + aggregate_failures do + expect(described_class.valid?(project_path)).to be_falsey + expect(described_class.valid?(wiki_path)).to be_falsey + expect(described_class.valid?(personal_snippet_path)).to be_truthy + expect(described_class.valid?(project_snippet_path)).to be_truthy + end end end end diff --git a/spec/lib/gitlab/gl_repository_spec.rb b/spec/lib/gitlab/gl_repository_spec.rb index 3cfc4c2a132..858f436047e 100644 --- a/spec/lib/gitlab/gl_repository_spec.rb +++ b/spec/lib/gitlab/gl_repository_spec.rb @@ -5,13 +5,18 @@ require 'spec_helper' describe ::Gitlab::GlRepository do describe '.parse' do let_it_be(:project) { create(:project, :repository) } + let_it_be(:snippet) { create(:personal_snippet) } it 'parses a project gl_repository' do - expect(described_class.parse("project-#{project.id}")).to eq([project, Gitlab::GlRepository::PROJECT]) + expect(described_class.parse("project-#{project.id}")).to eq([project, project, Gitlab::GlRepository::PROJECT]) end it 'parses a wiki gl_repository' do - expect(described_class.parse("wiki-#{project.id}")).to eq([project, Gitlab::GlRepository::WIKI]) + expect(described_class.parse("wiki-#{project.id}")).to eq([project, project, Gitlab::GlRepository::WIKI]) + end + + it 'parses a snippet gl_repository' do + expect(described_class.parse("snippet-#{snippet.id}")).to eq([snippet, nil, Gitlab::GlRepository::SNIPPET]) end it 'throws an argument error on an invalid gl_repository type' do diff --git a/spec/lib/gitlab/path_regex_spec.rb b/spec/lib/gitlab/path_regex_spec.rb index 3cbcae4cdeb..8dabe5a756b 100644 --- a/spec/lib/gitlab/path_regex_spec.rb +++ b/spec/lib/gitlab/path_regex_spec.rb @@ -411,4 +411,37 @@ describe Gitlab::PathRegex do it { is_expected.not_to match('git lab') } it { is_expected.not_to match('gitlab.git') } end + + shared_examples 'invalid snippet routes' do + it { is_expected.not_to match('gitlab-org/gitlab/snippets/1.git') } + it { is_expected.not_to match('snippets/1.git') } + it { is_expected.not_to match('gitlab-org/gitlab/snippets/') } + it { is_expected.not_to match('/gitlab-org/gitlab/snippets/1') } + it { is_expected.not_to match('gitlab-org/gitlab/snippets/foo') } + it { is_expected.not_to match('root/snippets/1') } + it { is_expected.not_to match('/snippets/1') } + it { is_expected.not_to match('snippets/') } + it { is_expected.not_to match('snippets/foo') } + end + + describe '.full_snippets_repository_path_regex' do + subject { described_class.full_snippets_repository_path_regex } + + it { is_expected.to match('gitlab-org/gitlab/snippets/1') } + it { is_expected.to match('snippets/1') } + + it_behaves_like 'invalid snippet routes' + end + + describe '.personal_and_project_snippets_path_regex' do + subject { %r{\A#{described_class.personal_and_project_snippets_path_regex}\z} } + + it { is_expected.to match('gitlab-org/gitlab/snippets') } + it { is_expected.to match('snippets') } + + it { is_expected.not_to match('gitlab-org/gitlab/snippets/1') } + it { is_expected.not_to match('snippets/1') } + + it_behaves_like 'invalid snippet routes' + end end diff --git a/spec/lib/gitlab/repo_path_spec.rb b/spec/lib/gitlab/repo_path_spec.rb index 2aeb69db2cb..e72bdc01940 100644 --- a/spec/lib/gitlab/repo_path_spec.rb +++ b/spec/lib/gitlab/repo_path_spec.rb @@ -3,60 +3,72 @@ require 'spec_helper' describe ::Gitlab::RepoPath do - describe '.parse' do - let_it_be(:project) { create(:project, :repository) } + include Gitlab::Routing + + let_it_be(:project) { create(:project, :repository) } + let_it_be(:personal_snippet) { create(:personal_snippet) } + let_it_be(:project_snippet) { create(:project_snippet, project: project) } + let_it_be(:redirect) { project.route.create_redirect('foo/bar/baz') } + describe '.parse' do context 'a repository storage path' do - it 'parses a full repository path' do - expect(described_class.parse(project.repository.full_path)).to eq([project, Gitlab::GlRepository::PROJECT, nil]) + it 'parses a full repository project path' do + expect(described_class.parse(project.repository.full_path)).to eq([project, project, Gitlab::GlRepository::PROJECT, nil]) + end + + it 'parses a full wiki project path' do + expect(described_class.parse(project.wiki.repository.full_path)).to eq([project, project, Gitlab::GlRepository::WIKI, nil]) + end + + it 'parses a personal snippet repository path' do + expect(described_class.parse("snippets/#{personal_snippet.id}")).to eq([personal_snippet, nil, Gitlab::GlRepository::SNIPPET, nil]) end - it 'parses a full wiki path' do - expect(described_class.parse(project.wiki.repository.full_path)).to eq([project, Gitlab::GlRepository::WIKI, nil]) + it 'parses a project snippet repository path' do + expect(described_class.parse("#{project.full_path}/snippets/#{project_snippet.id}")).to eq([project_snippet, project, Gitlab::GlRepository::SNIPPET, nil]) end end context 'a relative path' do it 'parses a relative repository path' do - expect(described_class.parse(project.full_path + '.git')).to eq([project, Gitlab::GlRepository::PROJECT, nil]) + expect(described_class.parse(project.full_path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, nil]) end it 'parses a relative wiki path' do - expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, Gitlab::GlRepository::WIKI, nil]) + expect(described_class.parse(project.full_path + '.wiki.git')).to eq([project, project, Gitlab::GlRepository::WIKI, nil]) end it 'parses a relative path starting with /' do - expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, Gitlab::GlRepository::PROJECT, nil]) + expect(described_class.parse('/' + project.full_path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, nil]) end context 'of a redirected project' do let(:redirect) { project.route.create_redirect('foo/bar') } it 'parses a relative repository path' do - expect(described_class.parse(redirect.path + '.git')).to eq([project, Gitlab::GlRepository::PROJECT, 'foo/bar']) + expect(described_class.parse(redirect.path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, 'foo/bar']) end it 'parses a relative wiki path' do - expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project, Gitlab::GlRepository::WIKI, 'foo/bar.wiki']) + expect(described_class.parse(redirect.path + '.wiki.git')).to eq([project, project, Gitlab::GlRepository::WIKI, 'foo/bar.wiki']) end it 'parses a relative path starting with /' do - expect(described_class.parse('/' + redirect.path + '.git')).to eq([project, Gitlab::GlRepository::PROJECT, 'foo/bar']) + expect(described_class.parse('/' + redirect.path + '.git')).to eq([project, project, Gitlab::GlRepository::PROJECT, 'foo/bar']) + end + + it 'parses a redirected project snippet repository path' do + expect(described_class.parse(redirect.path + "/snippets/#{project_snippet.id}.git")).to eq([project_snippet, project, Gitlab::GlRepository::SNIPPET, "foo/bar/snippets/#{project_snippet.id}"]) end end end - it "returns the default type for non existent paths" do - _project, type, _redirected = described_class.parse("path/non-existent.git") - - expect(type).to eq(Gitlab::GlRepository.default_type) + it 'returns the default type for non existent paths' do + expect(described_class.parse('path/non-existent.git')).to eq([nil, nil, Gitlab::GlRepository.default_type, nil]) end end describe '.find_project' do - let(:project) { create(:project) } - let(:redirect) { project.route.create_redirect('foo/bar/baz') } - context 'when finding a project by its canonical path' do context 'when the cases match' do it 'returns the project and false' do @@ -81,4 +93,34 @@ describe ::Gitlab::RepoPath do end end end + + describe '.find_snippet' do + it 'extracts path and id from personal snippet route' do + expect(described_class.find_snippet("snippets/#{personal_snippet.id}")).to eq([personal_snippet, false]) + end + + it 'extracts path and id from project snippet route' do + expect(described_class.find_snippet("#{project.full_path}/snippets/#{project_snippet.id}")).to eq([project_snippet, false]) + end + + it 'returns nil for invalid snippet paths' do + aggregate_failures do + expect(described_class.find_snippet("snippets/#{project_snippet.id}")).to eq([nil, false]) + expect(described_class.find_snippet("#{project.full_path}/snippets/#{personal_snippet.id}")).to eq([nil, false]) + expect(described_class.find_snippet('')).to eq([nil, false]) + end + end + + it 'returns nil for snippets not associated with the project' do + snippet = create(:project_snippet) + + expect(described_class.find_snippet("#{project.full_path}/snippets/#{snippet.id}")).to eq([nil, false]) + end + + context 'when finding a project snippet via a redirect' do + it 'returns the project and true' do + expect(described_class.find_snippet("#{redirect.path}/snippets/#{project_snippet.id}")).to eq([project_snippet, true]) + end + end + end end diff --git a/spec/lib/gitlab/repository_cache_spec.rb b/spec/lib/gitlab/repository_cache_spec.rb index e787288fc51..be31be761ad 100644 --- a/spec/lib/gitlab/repository_cache_spec.rb +++ b/spec/lib/gitlab/repository_cache_spec.rb @@ -12,19 +12,44 @@ describe Gitlab::RepositoryCache do describe '#cache_key' do subject { cache.cache_key(:foo) } - it 'includes the namespace' do - expect(subject).to eq "foo:#{namespace}" + shared_examples 'cache_key examples' do + it 'includes the namespace' do + expect(subject).to eq "foo:#{namespace}" + end + + context 'with a given namespace' do + let(:extra_namespace) { 'my:data' } + let(:cache) do + described_class.new(repository, extra_namespace: extra_namespace, + backend: backend) + end + + it 'includes the full namespace' do + expect(subject).to eq "foo:#{namespace}:#{extra_namespace}" + end + end end - context 'with a given namespace' do - let(:extra_namespace) { 'my:data' } - let(:cache) do - described_class.new(repository, extra_namespace: extra_namespace, - backend: backend) + describe 'project repository' do + it_behaves_like 'cache_key examples' do + let(:repository) { project.repository } end + end + + describe 'personal snippet repository' do + let_it_be(:personal_snippet) { create(:personal_snippet) } + let(:namespace) { repository.full_path } + + it_behaves_like 'cache_key examples' do + let(:repository) { personal_snippet.repository } + end + end + + describe 'project snippet repository' do + let_it_be(:project_snippet) { create(:project_snippet, project: project) } - it 'includes the full namespace' do - expect(subject).to eq "foo:#{namespace}:#{extra_namespace}" + it_behaves_like 'cache_key examples' do + let(:repository) { project_snippet.repository } end end end diff --git a/spec/lib/gitlab/repository_set_cache_spec.rb b/spec/lib/gitlab/repository_set_cache_spec.rb index de0f3602346..bcf27b338f6 100644 --- a/spec/lib/gitlab/repository_set_cache_spec.rb +++ b/spec/lib/gitlab/repository_set_cache_spec.rb @@ -11,16 +11,41 @@ describe Gitlab::RepositorySetCache, :clean_gitlab_redis_cache do describe '#cache_key' do subject { cache.cache_key(:foo) } - it 'includes the namespace' do - is_expected.to eq("foo:#{namespace}:set") + shared_examples 'cache_key examples' do + it 'includes the namespace' do + is_expected.to eq("foo:#{namespace}:set") + end + + context 'with a given namespace' do + let(:extra_namespace) { 'my:data' } + let(:cache) { described_class.new(repository, extra_namespace: extra_namespace) } + + it 'includes the full namespace' do + is_expected.to eq("foo:#{namespace}:#{extra_namespace}:set") + end + end + end + + describe 'project repository' do + it_behaves_like 'cache_key examples' do + let(:repository) { project.repository } + end + end + + describe 'personal snippet repository' do + let_it_be(:personal_snippet) { create(:personal_snippet) } + let(:namespace) { repository.full_path } + + it_behaves_like 'cache_key examples' do + let(:repository) { personal_snippet.repository } + end end - context 'with a given namespace' do - let(:extra_namespace) { 'my:data' } - let(:cache) { described_class.new(repository, extra_namespace: extra_namespace) } + describe 'project snippet repository' do + let_it_be(:project_snippet) { create(:project_snippet, project: project) } - it 'includes the full namespace' do - is_expected.to eq("foo:#{namespace}:#{extra_namespace}:set") + it_behaves_like 'cache_key examples' do + let(:repository) { project_snippet.repository } end end end diff --git a/spec/models/concerns/milestoneish_spec.rb b/spec/models/concerns/milestoneish_spec.rb index d46c9747845..71a18e2fbec 100644 --- a/spec/models/concerns/milestoneish_spec.rb +++ b/spec/models/concerns/milestoneish_spec.rb @@ -211,20 +211,21 @@ describe Milestone, 'Milestoneish' do end end - describe '#complete?' do + describe '#complete?', :use_clean_rails_memory_store_caching do it 'returns false when has items opened' do expect(milestone.complete?(non_member)).to eq false end it 'returns true when all items are closed' do issue.close - merge_request.close + security_issue_1.close + security_issue_2.close expect(milestone.complete?(non_member)).to eq true end end - describe '#percent_complete' do + describe '#percent_complete', :use_clean_rails_memory_store_caching do context 'division by zero' do let(:new_milestone) { build_stubbed(:milestone) } @@ -233,34 +234,58 @@ describe Milestone, 'Milestoneish' do end describe '#count_issues_by_state' do - it 'does not count confidential issues for non project members' do - expect(milestone.closed_issues_count(non_member)).to eq 2 - expect(milestone.total_issues_count(non_member)).to eq 3 + describe '#total_issues_count', :use_clean_rails_memory_store_caching do + it 'counts all issues including confidential' do + expect(milestone.total_issues_count(guest)).to eq 9 + end end - it 'does not count confidential issues for project members with guest role' do - expect(milestone.closed_issues_count(guest)).to eq 2 - expect(milestone.total_issues_count(guest)).to eq 3 + describe '#opened_issues_count', :use_clean_rails_memory_store_caching do + it 'counts all open issues including confidential' do + expect(milestone.opened_issues_count(guest)).to eq 3 + end end - it 'counts confidential issues for author' do - expect(milestone.closed_issues_count(author)).to eq 4 - expect(milestone.total_issues_count(author)).to eq 6 + describe '#closed_issues_count', :use_clean_rails_memory_store_caching do + it 'counts all closed issues including confidential' do + expect(milestone.closed_issues_count(guest)).to eq 6 + end end - it 'counts confidential issues for assignee' do - expect(milestone.closed_issues_count(assignee)).to eq 4 - expect(milestone.total_issues_count(assignee)).to eq 6 - end + context 'when cached_milestone_issue_counters are disabled' do + before do + stub_feature_flags(cached_milestone_issue_counters: false) + end - it 'counts confidential issues for project members' do - expect(milestone.closed_issues_count(member)).to eq 6 - expect(milestone.total_issues_count(member)).to eq 9 - end + it 'does not count confidential issues for non project members' do + expect(milestone.closed_issues_count(non_member)).to eq 2 + expect(milestone.total_issues_count(non_member)).to eq 3 + end - it 'counts confidential issues for admin' do - expect(milestone.closed_issues_count(admin)).to eq 6 - expect(milestone.total_issues_count(admin)).to eq 9 + it 'does not count confidential issues for project members with guest role' do + expect(milestone.closed_issues_count(guest)).to eq 2 + expect(milestone.total_issues_count(guest)).to eq 3 + end + + it 'counts confidential issues for author' do + expect(milestone.closed_issues_count(author)).to eq 4 + expect(milestone.total_issues_count(author)).to eq 6 + end + + it 'counts confidential issues for assignee' do + expect(milestone.closed_issues_count(assignee)).to eq 4 + expect(milestone.total_issues_count(assignee)).to eq 6 + end + + it 'counts confidential issues for project members' do + expect(milestone.closed_issues_count(member)).to eq 6 + expect(milestone.total_issues_count(member)).to eq 9 + end + + it 'counts confidential issues for admin' do + expect(milestone.closed_issues_count(admin)).to eq 6 + expect(milestone.total_issues_count(admin)).to eq 9 + end end end diff --git a/spec/requests/api/broadcast_messages_spec.rb b/spec/requests/api/broadcast_messages_spec.rb index 17b25d5c7cc..9bfbbe0daab 100644 --- a/spec/requests/api/broadcast_messages_spec.rb +++ b/spec/requests/api/broadcast_messages_spec.rb @@ -17,7 +17,7 @@ describe API::BroadcastMessages do expect(response).to include_pagination_headers expect(json_response).to be_kind_of(Array) expect(json_response.first.keys) - .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type)) + .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type dismissable)) end end @@ -28,7 +28,7 @@ describe API::BroadcastMessages do expect(response).to have_gitlab_http_status(:ok) expect(json_response['id']).to eq message.id expect(json_response.keys) - .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type)) + .to match_array(%w(id message starts_at ends_at color font active target_path broadcast_type dismissable)) end end @@ -111,6 +111,15 @@ describe API::BroadcastMessages do expect(response).to have_gitlab_http_status(:bad_request) end + + it 'accepts an active dismissable value ' do + attrs = { message: 'new message', dismissable: true } + + post api('/broadcast_messages', admin), params: attrs + + expect(response).to have_gitlab_http_status(:created) + expect(json_response['dismissable']).to eq true + end end end @@ -187,6 +196,15 @@ describe API::BroadcastMessages do expect(response).to have_gitlab_http_status(:bad_request) end + + it 'accepts a new dismissable value ' do + attrs = { message: 'new message', dismissable: true } + + put api("/broadcast_messages/#{message.id}", admin), params: attrs + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response['dismissable']).to eq true + end end end diff --git a/spec/requests/api/internal/base_spec.rb b/spec/requests/api/internal/base_spec.rb index fe0a3ffebd3..1f20a088e4f 100644 --- a/spec/requests/api/internal/base_spec.rb +++ b/spec/requests/api/internal/base_spec.rb @@ -10,6 +10,9 @@ describe API::Internal::Base do let(:gl_repository) { "project-#{project.id}" } let(:reference_counter) { double('ReferenceCounter') } + let_it_be(:personal_snippet) { create(:personal_snippet, :repository, author: user) } + let_it_be(:project_snippet) { create(:project_snippet, :repository, author: user, project: project) } + describe "GET /internal/check" do it do expect_any_instance_of(Redis).to receive(:ping).and_return('PONG') @@ -312,6 +315,54 @@ describe API::Internal::Base do end end + context 'git push with personal snippet' do + it 'responds with success' do + push(key, personal_snippet) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response["status"]).to be_truthy + expect(json_response["gl_project_path"]).to eq(personal_snippet.repository.full_path) + expect(json_response["gl_repository"]).to eq("snippet-#{personal_snippet.id}") + expect(user.reload.last_activity_on).to be_nil + end + end + + context 'git pull with personal snippet' do + it 'responds with success' do + pull(key, personal_snippet) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response["status"]).to be_truthy + expect(json_response["gl_project_path"]).to eq(personal_snippet.repository.full_path) + expect(json_response["gl_repository"]).to eq("snippet-#{personal_snippet.id}") + expect(user.reload.last_activity_on).to eql(Date.today) + end + end + + context 'git push with project snippet' do + it 'responds with success' do + push(key, project_snippet) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response["status"]).to be_truthy + expect(json_response["gl_project_path"]).to eq(project_snippet.repository.full_path) + expect(json_response["gl_repository"]).to eq("snippet-#{project_snippet.id}") + expect(user.reload.last_activity_on).to be_nil + end + end + + context 'git pull with project snippet' do + it 'responds with success' do + pull(key, project_snippet) + + expect(response).to have_gitlab_http_status(:ok) + expect(json_response["status"]).to be_truthy + expect(json_response["gl_project_path"]).to eq(project_snippet.repository.full_path) + expect(json_response["gl_repository"]).to eq("snippet-#{project_snippet.id}") + expect(user.reload.last_activity_on).to eql(Date.today) + end + end + context "git pull" do before do allow(Feature).to receive(:persisted_names).and_return(%w[gitaly_mep_mep]) @@ -393,10 +444,28 @@ describe API::Internal::Base do end end - it_behaves_like 'storing arguments in the application context' do - let(:expected_params) { { user: key.user.username, project: project.full_path } } + context 'with Project' do + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: key.user.username, project: project.full_path } } + + subject { push(key, project) } + end + end + + context 'with PersonalSnippet' do + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: key.user.username } } + + subject { push(key, personal_snippet) } + end + end - subject { push(key, project) } + context 'with ProjectSnippet' do + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: key.user.username, project: project_snippet.project.full_path } } + + subject { push(key, project_snippet) } + end end end @@ -450,7 +519,7 @@ describe API::Internal::Base do { authentication_abilities: [:read_project, :download_code, :push_code], namespace_path: project.namespace.path, - project_path: project.path, + repository_path: project.path, redirected_path: nil } ).and_return(access_checker) @@ -835,22 +904,60 @@ describe API::Internal::Base do allow_any_instance_of(Gitlab::Identifier).to receive(:identify).and_return(user) end - it 'executes PostReceiveService' do - message = <<~MESSAGE.strip - To create a merge request for #{branch_name}, visit: - http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name} - MESSAGE + context 'with Project' do + it 'executes PostReceiveService' do + message = <<~MESSAGE.strip + To create a merge request for #{branch_name}, visit: + http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name} + MESSAGE + + subject - subject + expect(json_response).to eq({ + 'messages' => [{ 'message' => message, 'type' => 'basic' }], + 'reference_counter_decreased' => true + }) + end - expect(json_response).to eq({ - 'messages' => [{ 'message' => message, 'type' => 'basic' }], - 'reference_counter_decreased' => true - }) + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: user.username, project: project.full_path } } + end end - it_behaves_like 'storing arguments in the application context' do - let(:expected_params) { { user: user.username, project: project.full_path } } + context 'with PersonalSnippet' do + let(:gl_repository) { "snippet-#{personal_snippet.id}" } + + it 'executes PostReceiveService' do + subject + + expect(json_response).to eq({ + 'messages' => [], + 'reference_counter_decreased' => true + }) + end + + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: key.user.username } } + let(:gl_repository) { "snippet-#{personal_snippet.id}" } + end + end + + context 'with ProjectSnippet' do + let(:gl_repository) { "snippet-#{project_snippet.id}" } + + it 'executes PostReceiveService' do + subject + + expect(json_response).to eq({ + 'messages' => [], + 'reference_counter_decreased' => true + }) + end + + it_behaves_like 'storing arguments in the application context' do + let(:expected_params) { { user: key.user.username, project: project_snippet.project.full_path } } + let(:gl_repository) { "snippet-#{project_snippet.id}" } + end end context 'with an orphaned write deploy key' do @@ -866,16 +973,32 @@ describe API::Internal::Base do end context 'when project is nil' do - let(:gl_repository) { 'project-foo' } + context 'with Project' do + let(:gl_repository) { 'project-foo' } - it 'does not try to notify that project moved' do - allow(Gitlab::GlRepository).to receive(:parse).and_return([nil, Gitlab::GlRepository::PROJECT]) + it 'does not try to notify that project moved' do + allow(Gitlab::GlRepository).to receive(:parse).and_return([nil, nil, Gitlab::GlRepository::PROJECT]) - expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message) + expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message) - subject + subject - expect(response).to have_gitlab_http_status(:ok) + expect(response).to have_gitlab_http_status(:ok) + end + end + + context 'with PersonalSnippet' do + let(:gl_repository) { "snippet-#{personal_snippet.id}" } + + it 'does not try to notify that project moved' do + allow(Gitlab::GlRepository).to receive(:parse).and_return([personal_snippet, nil, Gitlab::GlRepository::PROJECT]) + + expect(Gitlab::Checks::ProjectMoved).not_to receive(:fetch_message) + + subject + + expect(response).to have_gitlab_http_status(:ok) + end end end end @@ -896,24 +1019,37 @@ describe API::Internal::Base do end end - def gl_repository_for(project_or_wiki) - case project_or_wiki + def gl_repository_for(container) + case container when ProjectWiki - Gitlab::GlRepository::WIKI.identifier_for_container(project_or_wiki.project) + Gitlab::GlRepository::WIKI.identifier_for_container(container.project) when Project - Gitlab::GlRepository::PROJECT.identifier_for_container(project_or_wiki) + Gitlab::GlRepository::PROJECT.identifier_for_container(container) + when Snippet + Gitlab::GlRepository::SNIPPET.identifier_for_container(container) else nil end end - def pull(key, project, protocol = 'ssh') + def full_path_for(container) + case container + when PersonalSnippet + "snippets/#{container.id}" + when ProjectSnippet + "#{container.project.full_path}/snippets/#{container.id}" + else + container.full_path + end + end + + def pull(key, container, protocol = 'ssh') post( api("/internal/allowed"), params: { key_id: key.id, - project: project.full_path, - gl_repository: gl_repository_for(project), + project: full_path_for(container), + gl_repository: gl_repository_for(container), action: 'git-upload-pack', secret_token: secret_token, protocol: protocol @@ -921,12 +1057,12 @@ describe API::Internal::Base do ) end - def push(key, project, protocol = 'ssh', env: nil) + def push(key, container, protocol = 'ssh', env: nil) params = { changes: 'd14d6c0abdd253381df51a723d58691b2ee1ab08 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/master', key_id: key.id, - project: project.full_path, - gl_repository: gl_repository_for(project), + project: full_path_for(container), + gl_repository: gl_repository_for(container), action: 'git-receive-pack', secret_token: secret_token, protocol: protocol, @@ -939,14 +1075,14 @@ describe API::Internal::Base do ) end - def archive(key, project) + def archive(key, container) post( api("/internal/allowed"), params: { ref: 'master', key_id: key.id, - project: project.full_path, - gl_repository: gl_repository_for(project), + project: full_path_for(container), + gl_repository: gl_repository_for(container), action: 'git-upload-archive', secret_token: secret_token, protocol: 'ssh' diff --git a/spec/requests/groups/milestones_controller_spec.rb b/spec/requests/groups/milestones_controller_spec.rb index 4d15aa43cd2..1c6743dc678 100644 --- a/spec/requests/groups/milestones_controller_spec.rb +++ b/spec/requests/groups/milestones_controller_spec.rb @@ -12,23 +12,45 @@ describe Groups::MilestonesController do end let!(:private_milestone) { create(:milestone, project: public_project_with_private_issues_and_mrs, title: 'project milestone') } - it 'avoids N+1 database queries' do - public_project = create(:project, :public, :merge_requests_enabled, :issues_enabled, group: public_group) - create(:milestone, project: public_project) + describe 'GET #index' do + it 'avoids N+1 database queries' do + public_project = create(:project, :public, :merge_requests_enabled, :issues_enabled, group: public_group) + create(:milestone, project: public_project) - control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get "/groups/#{public_group.to_param}/-/milestones.json" }.count + control_count = ActiveRecord::QueryRecorder.new(skip_cached: false) { get group_milestones_path(public_group, format: :json) }.count - projects = create_list(:project, 2, :public, :merge_requests_enabled, :issues_enabled, group: public_group) - projects.each do |project| - create(:milestone, project: project) + projects = create_list(:project, 2, :public, :merge_requests_enabled, :issues_enabled, group: public_group) + projects.each do |project| + create(:milestone, project: project) + end + + expect { get group_milestones_path(public_group, format: :json) }.not_to exceed_all_query_limit(control_count) + expect(response).to have_gitlab_http_status(:ok) + milestones = json_response + + expect(milestones.count).to eq(3) + expect(milestones.map {|x| x['title']}).not_to include(private_milestone.title) end + end - expect { get "/groups/#{public_group.to_param}/-/milestones.json" }.not_to exceed_all_query_limit(control_count) - expect(response).to have_gitlab_http_status(:ok) - milestones = json_response + describe 'GET #show' do + let(:milestone) { create(:milestone, group: public_group) } + let(:show_path) { group_milestone_path(public_group, milestone) } - expect(milestones.count).to eq(3) - expect(milestones.map {|x| x['title']}).not_to include(private_milestone.title) + it 'avoids N+1 database queries' do + projects = create_list(:project, 3, :public, :merge_requests_enabled, :issues_enabled, group: public_group) + projects.each do |project| + create_list(:issue, 2, milestone: milestone, project: project) + end + control = ActiveRecord::QueryRecorder.new(skip_cached: false) { get show_path } + + projects = create_list(:project, 3, :public, :merge_requests_enabled, :issues_enabled, group: public_group) + projects.each do |project| + create_list(:issue, 2, milestone: milestone, project: project) + end + + expect { get show_path }.not_to exceed_all_query_limit(control) + end end end end diff --git a/spec/services/issues/close_service_spec.rb b/spec/services/issues/close_service_spec.rb index eb3b1d7c754..141567045a2 100644 --- a/spec/services/issues/close_service_spec.rb +++ b/spec/services/issues/close_service_spec.rb @@ -178,35 +178,55 @@ describe Issues::CloseService do end context "valid params" do - before do + def close_issue perform_enqueued_jobs do described_class.new(project, user).close_issue(issue) end end it 'closes the issue' do + close_issue + expect(issue).to be_valid expect(issue).to be_closed end it 'records closed user' do + close_issue + expect(issue.closed_by_id).to be(user.id) end it 'sends email to user2 about assign of new issue', :sidekiq_might_not_need_inline do + close_issue + email = ActionMailer::Base.deliveries.last expect(email.to.first).to eq(user2.email) expect(email.subject).to include(issue.title) end it 'creates system note about issue reassign' do + close_issue + note = issue.notes.last expect(note.note).to include "closed" end it 'marks todos as done' do + close_issue + expect(todo.reload).to be_done end + + it 'deletes milestone issue counters cache' do + issue.update(milestone: create(:milestone, project: project)) + + expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + close_issue + end end context 'when issue is not confidential' do diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index c9701e5d194..09fff389cec 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -196,6 +196,14 @@ describe Issues::CreateService do end end end + + it 'deletes milestone issues count cache' do + expect_next_instance_of(Milestones::IssuesCountService, milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + issue + end end context 'issue create service' do diff --git a/spec/services/issues/reopen_service_spec.rb b/spec/services/issues/reopen_service_spec.rb index f04029e64aa..ca878ee947a 100644 --- a/spec/services/issues/reopen_service_spec.rb +++ b/spec/services/issues/reopen_service_spec.rb @@ -43,6 +43,16 @@ describe Issues::ReopenService do .to change { project.open_issues_count }.from(0).to(1) end + it 'deletes milestone issue counters cache' do + issue.update(milestone: create(:milestone, project: project)) + + expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + described_class.new(project, user).execute(issue) + end + context 'when issue is not confidential' do it 'executes issue hooks' do expect(project).to receive(:execute_hooks).with(an_instance_of(Hash), :issue_hooks) diff --git a/spec/services/issues/update_service_spec.rb b/spec/services/issues/update_service_spec.rb index 5da77dd914c..69e47d890a5 100644 --- a/spec/services/issues/update_service_spec.rb +++ b/spec/services/issues/update_service_spec.rb @@ -412,9 +412,24 @@ describe Issues::UpdateService, :mailer do should_email(subscriber) should_not_email(non_subscriber) end + + it 'clears milestone issue counters cache' do + issue.milestone = create(:milestone, project: project) + + issue.save + + expect_next_instance_of(Milestones::IssuesCountService, issue.milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::ClosedIssuesCountService, issue.milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + update_issue(milestone_id: "") + end end - context 'when the milestone is changed' do + context 'when the milestone is assigned' do before do stub_feature_flags(track_resource_milestone_change_events: false) end @@ -444,6 +459,43 @@ describe Issues::UpdateService, :mailer do should_email(subscriber) should_not_email(non_subscriber) end + + it 'deletes issue counters cache for the milestone' do + milestone = create(:milestone, project: project) + + expect_next_instance_of(Milestones::IssuesCountService, milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::ClosedIssuesCountService, milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + update_issue(milestone: milestone) + end + end + + context 'when the milestone is changed' do + it 'deletes issue counters cache for both milestones' do + old_milestone = create(:milestone, project: project) + new_milestone = create(:milestone, project: project) + + issue.update!(milestone: old_milestone) + + expect_next_instance_of(Milestones::IssuesCountService, old_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::ClosedIssuesCountService, old_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::IssuesCountService, new_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::ClosedIssuesCountService, new_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + update_issue(milestone: new_milestone) + end end context 'when the labels change' do diff --git a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb index 744693dad15..2f03d18cd1f 100644 --- a/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/custom_metric_embed_service_spec.rb @@ -35,12 +35,18 @@ describe Metrics::Dashboard::CustomMetricEmbedService do it { is_expected.to be_truthy } - context 'not embedded' do + context 'missing embedded' do let(:params) { valid_params.except(:embedded) } it { is_expected.to be_falsey } end + context 'not embedded' do + let(:params) { valid_params.merge(embedded: 'false') } + + it { is_expected.to be_falsey } + end + context 'non-system dashboard' do let(:dashboard_path) { '.gitlab/dashboards/test.yml' } diff --git a/spec/services/metrics/dashboard/default_embed_service_spec.rb b/spec/services/metrics/dashboard/default_embed_service_spec.rb index 1b88276368c..8e32316433d 100644 --- a/spec/services/metrics/dashboard/default_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/default_embed_service_spec.rb @@ -27,7 +27,7 @@ describe Metrics::Dashboard::DefaultEmbedService, :use_clean_rails_memory_store_ end context 'not embedded' do - let(:params) { { embedded: false } } + let(:params) { { embedded: 'false' } } it { is_expected.to be_falsey } end diff --git a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb index c1ce9818f21..ee75284b4ce 100644 --- a/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/dynamic_embed_service_spec.rb @@ -35,12 +35,18 @@ describe Metrics::Dashboard::DynamicEmbedService, :use_clean_rails_memory_store_ it { is_expected.to be_truthy } - context 'not embedded' do + context 'missing embedded' do let(:params) { valid_params.except(:embedded) } it { is_expected.to be_falsey } end + context 'not embedded' do + let(:params) { valid_params.merge(embedded: 'false') } + + it { is_expected.to be_falsey } + end + context 'undefined dashboard' do let(:params) { valid_params.except(:dashboard_path) } diff --git a/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb b/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb index a772b911d8a..034d6aba5d6 100644 --- a/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb +++ b/spec/services/metrics/dashboard/grafana_metric_embed_service_spec.rb @@ -28,12 +28,18 @@ describe Metrics::Dashboard::GrafanaMetricEmbedService do it { is_expected.to be_truthy } - context 'not embedded' do + context 'missing embedded' do let(:params) { valid_params.except(:embedded) } it { is_expected.to be_falsey } end + context 'not embedded' do + let(:params) { valid_params.merge(embedded: 'false') } + + it { is_expected.to be_falsey } + end + context 'undefined grafana_url' do let(:params) { valid_params.except(:grafana_url) } diff --git a/spec/services/milestones/closed_issues_count_service_spec.rb b/spec/services/milestones/closed_issues_count_service_spec.rb new file mode 100644 index 00000000000..b86eede2e22 --- /dev/null +++ b/spec/services/milestones/closed_issues_count_service_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Milestones::ClosedIssuesCountService, :use_clean_rails_memory_store_caching do + let(:project) { create(:project) } + let(:milestone) { create(:milestone, project: project) } + + before do + create(:issue, milestone: milestone, project: project) + create(:issue, :confidential, milestone: milestone, project: project) + + create(:issue, :closed, milestone: milestone, project: project) + create(:issue, :closed, :confidential, milestone: milestone, project: project) + end + + subject { described_class.new(milestone) } + + it_behaves_like 'a counter caching service' + + it 'counts closed issues including confidential' do + expect(subject.count).to eq(2) + end +end diff --git a/spec/services/milestones/issues_count_service_spec.rb b/spec/services/milestones/issues_count_service_spec.rb new file mode 100644 index 00000000000..22aea884424 --- /dev/null +++ b/spec/services/milestones/issues_count_service_spec.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Milestones::IssuesCountService, :use_clean_rails_memory_store_caching do + let(:project) { create(:project) } + let(:milestone) { create(:milestone, project: project) } + + before do + create(:issue, milestone: milestone, project: project) + create(:issue, :confidential, milestone: milestone, project: project) + + create(:issue, :closed, milestone: milestone, project: project) + create(:issue, :closed, milestone: milestone, project: project) + end + + subject { described_class.new(milestone) } + + it_behaves_like 'a counter caching service' + + it 'counts all issues including confidential' do + expect(subject.count).to eq(4) + end +end diff --git a/spec/services/milestones/transfer_service_spec.rb b/spec/services/milestones/transfer_service_spec.rb index 711969ce504..9b087b07cea 100644 --- a/spec/services/milestones/transfer_service_spec.rb +++ b/spec/services/milestones/transfer_service_spec.rb @@ -40,6 +40,25 @@ describe Milestones::TransferService do expect(new_milestone.project_milestone?).to be_truthy end + it 'deletes milestone issue counters cache for both milestones' do + new_milestone = create(:milestone, project: project, title: group_milestone.title) + + expect_next_instance_of(Milestones::IssuesCountService, group_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::ClosedIssuesCountService, group_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::IssuesCountService, new_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + expect_next_instance_of(Milestones::ClosedIssuesCountService, new_milestone) do |service| + expect(service).to receive(:delete_cache).and_call_original + end + + service.execute + end + it 'does not apply new project milestone to issues with project milestone' do service.execute diff --git a/spec/services/post_receive_service_spec.rb b/spec/services/post_receive_service_spec.rb index 64b4a1125e8..f3631ff922f 100644 --- a/spec/services/post_receive_service_spec.rb +++ b/spec/services/post_receive_service_spec.rb @@ -5,8 +5,10 @@ require 'spec_helper' describe PostReceiveService do include Gitlab::Routing - let_it_be(:project) { create(:project, :repository, :wiki_repo) } let_it_be(:user) { create(:user) } + let_it_be(:project) { create(:project, :repository, :wiki_repo, namespace: user.namespace) } + let_it_be(:project_snippet) { create(:project_snippet, :repository, project: project, author: user) } + let_it_be(:personal_snippet) { create(:personal_snippet, :repository, author: user) } let(:identifier) { 'key-123' } let(:gl_repository) { "project-#{project.id}" } @@ -14,6 +16,7 @@ describe PostReceiveService do let(:secret_token) { Gitlab::Shell.secret_token } let(:reference_counter) { double('ReferenceCounter') } let(:push_options) { ['ci.skip', 'another push option'] } + let(:repository) { project.repository } let(:changes) do "#{Gitlab::Git::BLANK_SHA} 570e7b2abdd848b95f2f578043fc23bd6f6fd24d refs/heads/#{branch_name}" @@ -29,104 +32,173 @@ describe PostReceiveService do } end - let(:response) { PostReceiveService.new(user, project, params).execute } + let(:service) { described_class.new(user, repository, project, params) } + let(:response) { service.execute } subject { response.messages.as_json } - it 'enqueues a PostReceive worker job' do - expect(PostReceive).to receive(:perform_async) - .with(gl_repository, identifier, changes, { ci: { skip: true } }) + context 'when project is nil' do + let(:gl_repository) { "snippet-#{personal_snippet.id}" } + let(:project) { nil } + let(:repository) { personal_snippet.repository } - subject + it 'does not return error' do + expect(subject).to be_empty + end end - it 'decreases the reference counter and returns the result' do - expect(Gitlab::ReferenceCounter).to receive(:new).with(gl_repository) - .and_return(reference_counter) - expect(reference_counter).to receive(:decrease).and_return(true) + context 'when repository is nil' do + let(:repository) { nil } - expect(response.reference_counter_decreased).to be(true) + it 'does not return error' do + expect(subject).to be_empty + end end - it 'returns link to create new merge request' do - message = <<~MESSAGE.strip - To create a merge request for #{branch_name}, visit: - http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name} - MESSAGE + context 'when both repository and project are nil' do + let(:gl_repository) { "snippet-#{personal_snippet.id}" } + let(:project) { nil } + let(:repository) { nil } - expect(subject).to include(build_basic_message(message)) + it 'does not return error' do + expect(subject).to be_empty + end end - it 'returns the link to an existing merge request when it exists' do - merge_request = create(:merge_request, source_project: project, source_branch: branch_name, target_branch: 'master') - message = <<~MESSAGE.strip - View merge request for feature: - #{project_merge_request_url(project, merge_request)} - MESSAGE + shared_examples 'post_receive_service actions' do + it 'enqueues a PostReceive worker job' do + expect(PostReceive).to receive(:perform_async) + .with(gl_repository, identifier, changes, { ci: { skip: true } }) - expect(subject).to include(build_basic_message(message)) - end + subject + end - context 'when printing_merge_request_link_enabled is false' do - let(:project) { create(:project, printing_merge_request_link_enabled: false) } + it 'decreases the reference counter and returns the result' do + expect(Gitlab::ReferenceCounter).to receive(:new).with(gl_repository) + .and_return(reference_counter) + expect(reference_counter).to receive(:decrease).and_return(true) - it 'returns no merge request messages' do - expect(subject).to be_blank + expect(response.reference_counter_decreased).to be(true) end end - it 'does not invoke MergeRequests::PushOptionsHandlerService' do - expect(MergeRequests::PushOptionsHandlerService).not_to receive(:new) + context 'with Project' do + it_behaves_like 'post_receive_service actions' - subject - end + it 'returns link to create new merge request' do + message = <<~MESSAGE.strip + To create a merge request for #{branch_name}, visit: + http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/new?merge_request%5Bsource_branch%5D=#{branch_name} + MESSAGE - context 'when there are merge_request push options' do - let(:params) { super().merge(push_options: ['merge_request.create']) } + expect(subject).to include(build_basic_message(message)) + end + + it 'returns the link to an existing merge request when it exists' do + merge_request = create(:merge_request, source_project: project, source_branch: branch_name, target_branch: 'master') + message = <<~MESSAGE.strip + View merge request for feature: + #{project_merge_request_url(project, merge_request)} + MESSAGE - before do - project.add_developer(user) + expect(subject).to include(build_basic_message(message)) end - it 'invokes MergeRequests::PushOptionsHandlerService' do - expect(MergeRequests::PushOptionsHandlerService).to receive(:new).and_call_original + context 'when printing_merge_request_link_enabled is false' do + let(:project) { create(:project, printing_merge_request_link_enabled: false) } - subject + it 'returns no merge request messages' do + expect(subject).to be_blank + end end - it 'creates a new merge request' do - expect { Sidekiq::Testing.fake! { subject } }.to change(MergeRequest, :count).by(1) + it 'does not invoke MergeRequests::PushOptionsHandlerService' do + expect(MergeRequests::PushOptionsHandlerService).not_to receive(:new) + + subject end - it 'links to the newly created merge request' do - message = <<~MESSAGE.strip - View merge request for #{branch_name}: - http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/1 - MESSAGE + context 'when there are merge_request push options' do + let(:params) { super().merge(push_options: ['merge_request.create']) } - expect(subject).to include(build_basic_message(message)) + before do + project.add_developer(user) + end + + it 'invokes MergeRequests::PushOptionsHandlerService' do + expect(MergeRequests::PushOptionsHandlerService).to receive(:new).and_call_original + + subject + end + + it 'creates a new merge request' do + expect { Sidekiq::Testing.fake! { subject } }.to change(MergeRequest, :count).by(1) + end + + it 'links to the newly created merge request' do + message = <<~MESSAGE.strip + View merge request for #{branch_name}: + http://#{Gitlab.config.gitlab.host}/#{project.full_path}/-/merge_requests/1 + MESSAGE + + expect(subject).to include(build_basic_message(message)) + end + + it 'adds errors on the service instance to warnings' do + expect_any_instance_of( + MergeRequests::PushOptionsHandlerService + ).to receive(:errors).at_least(:once).and_return(['my error']) + + message = "WARNINGS:\nError encountered with push options 'merge_request.create': my error" + + expect(subject).to include(build_alert_message(message)) + end + + it 'adds ActiveRecord errors on invalid MergeRequest records to warnings' do + invalid_merge_request = MergeRequest.new + invalid_merge_request.errors.add(:base, 'my error') + message = "WARNINGS:\nError encountered with push options 'merge_request.create': my error" + + expect_any_instance_of( + MergeRequests::CreateService + ).to receive(:execute).and_return(invalid_merge_request) + + expect(subject).to include(build_alert_message(message)) + end end + end + + context 'with PersonalSnippet' do + let(:gl_repository) { "snippet-#{personal_snippet.id}" } + let(:repository) { personal_snippet.repository } - it 'adds errors on the service instance to warnings' do - expect_any_instance_of( - MergeRequests::PushOptionsHandlerService - ).to receive(:errors).at_least(:once).and_return(['my error']) + it_behaves_like 'post_receive_service actions' - message = "WARNINGS:\nError encountered with push options 'merge_request.create': my error" + it 'does not return link to create new merge request' do + expect(subject).to be_empty + end + + it 'does not return the link to an existing merge request when it exists' do + create(:merge_request, source_project: project, source_branch: branch_name, target_branch: 'master') - expect(subject).to include(build_alert_message(message)) + expect(subject).to be_empty end + end - it 'adds ActiveRecord errors on invalid MergeRequest records to warnings' do - invalid_merge_request = MergeRequest.new - invalid_merge_request.errors.add(:base, 'my error') - message = "WARNINGS:\nError encountered with push options 'merge_request.create': my error" + context 'with ProjectSnippet' do + let(:gl_repository) { "snippet-#{project_snippet.id}" } + let(:repository) { project_snippet.repository } - expect_any_instance_of( - MergeRequests::CreateService - ).to receive(:execute).and_return(invalid_merge_request) + it_behaves_like 'post_receive_service actions' - expect(subject).to include(build_alert_message(message)) + it 'does not return link to create new merge request' do + expect(subject).to be_empty + end + + it 'does not return the link to an existing merge request when it exists' do + create(:merge_request, source_project: project, source_branch: branch_name, target_branch: 'master') + + expect(subject).to be_empty end end @@ -178,6 +250,50 @@ describe PostReceiveService do end end + describe '#process_mr_push_options' do + context 'when repository belongs to a snippet' do + context 'with PersonalSnippet' do + let(:repository) { personal_snippet.repository } + + it 'returns an error message' do + result = service.process_mr_push_options(push_options, changes) + + expect(result).to match('Push options are only supported for projects') + end + end + + context 'with ProjectSnippet' do + let(:repository) { project_snippet.repository } + + it 'returns an error message' do + result = service.process_mr_push_options(push_options, changes) + + expect(result).to match('Push options are only supported for projects') + end + end + end + end + + describe '#merge_request_urls' do + context 'when repository belongs to a snippet' do + context 'with PersonalSnippet' do + let(:repository) { personal_snippet.repository } + + it 'returns an empty array' do + expect(service.merge_request_urls).to be_empty + end + end + + context 'with ProjectSnippet' do + let(:repository) { project_snippet.repository } + + it 'returns an empty array' do + expect(service.merge_request_urls).to be_empty + end + end + end + end + def build_alert_message(message) { 'type' => 'alert', 'message' => message } end diff --git a/spec/tasks/gitlab/backup_rake_spec.rb b/spec/tasks/gitlab/backup_rake_spec.rb index e58919c8688..5e15ade492b 100644 --- a/spec/tasks/gitlab/backup_rake_spec.rb +++ b/spec/tasks/gitlab/backup_rake_spec.rb @@ -14,6 +14,14 @@ describe 'gitlab:app namespace rake task' do tars_glob.first end + def backup_files + %w(backup_information.yml artifacts.tar.gz builds.tar.gz lfs.tar.gz pages.tar.gz) + end + + def backup_directories + %w(db repositories) + end + before(:all) do Rake.application.rake_require 'tasks/gitlab/helpers' Rake.application.rake_require 'tasks/gitlab/backup' @@ -28,12 +36,16 @@ describe 'gitlab:app namespace rake task' do before do stub_env('force', 'yes') FileUtils.rm(tars_glob, force: true) + FileUtils.rm(backup_files, force: true) + FileUtils.rm_rf(backup_directories, secure: true) reenable_backup_sub_tasks stub_container_registry_config(enabled: enable_registry) end after do FileUtils.rm(tars_glob, force: true) + FileUtils.rm(backup_files, force: true) + FileUtils.rm_rf(backup_directories, secure: true) end def run_rake_task(task_name) @@ -62,15 +74,6 @@ describe 'gitlab:app namespace rake task' do let(:gitlab_version) { Gitlab::VERSION } - it 'fails on mismatch' do - allow(YAML).to receive(:load_file) - .and_return({ gitlab_version: "not #{gitlab_version}" }) - - expect do - expect { run_rake_task('gitlab:backup:restore') }.to output.to_stdout - end.to raise_error(SystemExit) - end - context 'restore with matching gitlab version' do before do allow(YAML).to receive(:load_file) @@ -241,7 +244,7 @@ describe 'gitlab:app namespace rake task' do ) expect(exit_status).to eq(0) - expect(tar_contents).to match('db/') + expect(tar_contents).to match('db') expect(tar_contents).to match('uploads.tar.gz') expect(tar_contents).to match('repositories/') expect(tar_contents).to match('builds.tar.gz') @@ -379,6 +382,50 @@ describe 'gitlab:app namespace rake task' do end end + describe 'skipping tar archive creation' do + before do + stub_env('SKIP', 'tar') + end + + it 'created files with backup content and no tar archive' do + expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout + + dir_contents = Dir.children(Gitlab.config.backup.path) + + expect(dir_contents).to contain_exactly( + 'backup_information.yml', + 'db', + 'uploads.tar.gz', + 'builds.tar.gz', + 'artifacts.tar.gz', + 'lfs.tar.gz', + 'pages.tar.gz', + 'registry.tar.gz', + 'repositories', + 'tmp' + ) + end + + it 'those component files can be restored from' do + expect { run_rake_task("gitlab:backup:create") }.to output.to_stdout + + allow(Rake::Task['gitlab:shell:setup']) + .to receive(:invoke).and_return(true) + + expect(Rake::Task['gitlab:db:drop_tables']).to receive :invoke + expect(Rake::Task['gitlab:backup:db:restore']).to receive :invoke + expect(Rake::Task['gitlab:backup:repo:restore']).to receive :invoke + expect(Rake::Task['gitlab:backup:uploads:restore']).to receive :invoke + expect(Rake::Task['gitlab:backup:builds:restore']).to receive :invoke + expect(Rake::Task['gitlab:backup:artifacts:restore']).to receive :invoke + expect(Rake::Task['gitlab:backup:pages:restore']).to receive :invoke + expect(Rake::Task['gitlab:backup:lfs:restore']).to receive :invoke + expect(Rake::Task['gitlab:backup:registry:restore']).to receive :invoke + expect(Rake::Task['gitlab:shell:setup']).to receive :invoke + expect { run_rake_task("gitlab:backup:restore") }.to output.to_stdout + end + end + describe "Human Readable Backup Name" do it 'name has human readable time' do expect { run_rake_task('gitlab:backup:create') }.to output.to_stdout diff --git a/spec/views/shared/milestones/_issuable.html.haml_spec.rb b/spec/views/shared/milestones/_issuable.html.haml_spec.rb index 3c2b7c6305a..6e81fec79d4 100644 --- a/spec/views/shared/milestones/_issuable.html.haml_spec.rb +++ b/spec/views/shared/milestones/_issuable.html.haml_spec.rb @@ -3,19 +3,38 @@ require 'spec_helper' describe 'shared/milestones/_issuable.html.haml' do - let(:project) { create(:project) } - let(:user) { create(:user) } - let(:milestone) { create(:milestone, project: project) } - let(:issuable) { create(:issue, project: project, assignees: [user]) } + let_it_be(:project) { create(:project) } + let_it_be(:user) { create(:user) } + let_it_be(:milestone) { create(:milestone, project: project) } before do assign(:project, project) assign(:milestone, milestone) end - it 'avatar links to issues page' do - render 'shared/milestones/issuable', issuable: issuable, show_project_name: true + subject(:rendered) { render 'shared/milestones/issuable', issuable: issuable, show_project_name: true } - expect(rendered).to have_css("a[href='#{project_issues_path(project, milestone_title: milestone.title, assignee_id: user.id, state: 'all')}']") + context 'issue' do + let(:issuable) { create(:issue, project: project, assignees: [user]) } + + it 'links to the page for the issue' do + expect(rendered).to have_css("a[href='#{project_issue_path(project, issuable)}']", class: 'issue-link') + end + + it 'links to issues page for user' do + expect(rendered).to have_css("a[href='#{project_issues_path(project, milestone_title: milestone.title, assignee_id: user.id, state: 'all')}']") + end + end + + context 'merge request' do + let(:issuable) { create(:merge_request, source_project: project, target_project: project, assignees: [user]) } + + it 'links to merge requests page for user' do + expect(rendered).to have_css("a[href='#{project_merge_requests_path(project, milestone_title: milestone.title, assignee_id: user.id, state: 'all')}']") + end + + it 'links to the page for the merge request' do + expect(rendered).to have_css("a[href='#{project_merge_request_path(project, issuable)}']", class: 'issue-link') + end end end diff --git a/spec/workers/post_receive_spec.rb b/spec/workers/post_receive_spec.rb index 34aaa9bb1e9..72e423db611 100644 --- a/spec/workers/post_receive_spec.rb +++ b/spec/workers/post_receive_spec.rb @@ -34,6 +34,31 @@ describe PostReceive do expect(Gitlab::GitLogger).to receive(:error).with("POST-RECEIVE: #{error_message}") expect(perform).to be(false) end + + context 'with PersonalSnippet' do + let(:gl_repository) { "snippet-#{snippet.id}" } + let(:snippet) { create(:personal_snippet, author: project.owner) } + + it 'does not log an error' do + expect(Gitlab::GitLogger).not_to receive(:error) + expect(Gitlab::GitPostReceive).to receive(:new).and_call_original + expect_any_instance_of(described_class) do |instance| + expect(instance).to receive(:process_snippet_changes) + end + + perform + end + end + + context 'with ProjectSnippet' do + let(:gl_repository) { "snippet-#{snippet.id}" } + let(:snippet) { create(:snippet, type: 'ProjectSnippet', project: nil, author: project.owner) } + + it 'returns false and logs an error' do + expect(Gitlab::GitLogger).to receive(:error).with("POST-RECEIVE: #{error_message}") + expect(perform).to be(false) + end + end end describe "#process_project_changes" do @@ -44,7 +69,7 @@ describe PostReceive do before do allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(empty_project.owner) # Need to mock here so we can expect calls on project - allow(Gitlab::GlRepository).to receive(:parse).and_return([empty_project, Gitlab::GlRepository::PROJECT]) + allow(Gitlab::GlRepository).to receive(:parse).and_return([empty_project, empty_project, Gitlab::GlRepository::PROJECT]) end it 'expire the status cache' do @@ -97,7 +122,7 @@ describe PostReceive do before do allow_any_instance_of(Gitlab::GitPostReceive).to receive(:identify).and_return(project.owner) - allow(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::PROJECT]) + allow(Gitlab::GlRepository).to receive(:parse).and_return([project, project, Gitlab::GlRepository::PROJECT]) end shared_examples 'updating remote mirrors' do @@ -176,7 +201,7 @@ describe PostReceive do end before do - expect(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::PROJECT]) + expect(Gitlab::GlRepository).to receive(:parse).and_return([project, project, Gitlab::GlRepository::PROJECT]) end it 'does not expire branches cache' do @@ -256,7 +281,7 @@ describe PostReceive do before do # Need to mock here so we can expect calls on project - allow(Gitlab::GlRepository).to receive(:parse).and_return([project, Gitlab::GlRepository::WIKI]) + allow(Gitlab::GlRepository).to receive(:parse).and_return([project, project, Gitlab::GlRepository::WIKI]) end it 'updates project activity' do @@ -333,4 +358,82 @@ describe PostReceive do perform end end + + describe '#process_snippet_changes' do + let(:gl_repository) { "snippet-#{snippet.id}" } + + before do + # Need to mock here so we can expect calls on project + allow(Gitlab::GlRepository).to receive(:parse).and_return([snippet, snippet.project, Gitlab::GlRepository::SNIPPET]) + end + + shared_examples 'snippet changes actions' do + context 'unidentified user' do + let!(:key_id) { '' } + + it 'returns false' do + expect(perform).to be false + end + end + + context 'with changes' do + context 'branches' do + let(:changes) do + <<~EOF + 123456 789012 refs/heads/tést1 + 123456 789012 refs/heads/tést2 + EOF + end + + it 'expires the branches cache' do + expect(snippet.repository).to receive(:expire_branches_cache).once + + perform + end + + it 'expires the status cache' do + expect(snippet.repository).to receive(:empty?).and_return(true) + expect(snippet.repository).to receive(:expire_status_cache) + + perform + end + end + + context 'tags' do + let(:changes) do + <<~EOF + 654321 210987 refs/tags/tag1 + 654322 210986 refs/tags/tag2 + 654323 210985 refs/tags/tag3 + EOF + end + + it 'does not expire branches cache' do + expect(snippet.repository).not_to receive(:expire_branches_cache) + + perform + end + + it 'only invalidates tags once' do + expect(snippet.repository).to receive(:expire_caches_for_tags).once.and_call_original + expect(snippet.repository).to receive(:expire_tags_cache).once.and_call_original + + perform + end + end + end + end + + context 'with PersonalSnippet' do + let!(:snippet) { create(:personal_snippet, author: project.owner) } + + it_behaves_like 'snippet changes actions' + end + + context 'with ProjectSnippet' do + let!(:snippet) { create(:project_snippet, project: project, author: project.owner) } + + it_behaves_like 'snippet changes actions' + end + end end |