Merge branch 'ce-to-ee-2018-03-02' into 'master'

CE upstream - 2018-03-02 21:24 UTC

See merge request gitlab-org/gitlab-ee!4816

app/assets/javascripts/filtered_search/filtered_search_dropdown_manager.js

@@ -16,6 +16,7 @@ export default class FilteredSearchDropdownManager {
     page,
     isGroup,
     isGroupAncestor,
+    isGroupDecendent,
     filteredSearchTokenKeys,
   }) {
     this.container = FilteredSearchContainer.container;
@@ -26,6 +27,7 @@ export default class FilteredSearchDropdownManager {
     this.page = page;
     this.groupsOnly = isGroup;
     this.groupAncestor = isGroupAncestor;
+    this.isGroupDecendent = isGroupDecendent;
 
     this.setupMapping();
 

app/assets/javascripts/filtered_search/filtered_search_manager.js

@@ -22,11 +22,13 @@ export default class FilteredSearchManager {
     page,
     isGroup = false,
     isGroupAncestor = false,
+    isGroupDecendent = false,
     filteredSearchTokenKeys = FilteredSearchTokenKeys,
     stateFiltersSelector = '.issues-state-filters',
   }) {
     this.isGroup = isGroup;
     this.isGroupAncestor = isGroupAncestor;
+    this.isGroupDecendent = isGroupDecendent;
     this.states = ['opened', 'closed', 'merged', 'all'];
 
     this.page = page;

app/assets/javascripts/filtered_search/filtered_search_visual_tokens.js

 import _ from 'underscore';
-import AjaxCache from '../lib/utils/ajax_cache';
+import AjaxCache from '~/lib/utils/ajax_cache';
+import { objectToQueryString } from '~/lib/utils/common_utils';
 import Flash from '../flash';
 import FilteredSearchContainer from './container';
 import UsersCache from '../lib/utils/users_cache';
@@ -16,6 +17,21 @@ export default class FilteredSearchVisualTokens {
     };
   }
 
+  /**
+   * Returns a computed API endpoint
+   * and query string composed of values from endpointQueryParams
+   * @param {String} endpoint
+   * @param {String} endpointQueryParams
+   */
+  static getEndpointWithQueryParams(endpoint, endpointQueryParams) {
+    if (!endpointQueryParams) {
+      return endpoint;
+    }
+
+    const queryString = objectToQueryString(JSON.parse(endpointQueryParams));
+    return `${endpoint}?${queryString}`;
+  }
+
   static unselectTokens() {
     const otherTokens = FilteredSearchContainer.container.querySelectorAll('.js-visual-token .selectable.selected');
     [].forEach.call(otherTokens, t => t.classList.remove('selected'));
@@ -86,7 +102,10 @@ export default class FilteredSearchVisualTokens {
   static updateLabelTokenColor(tokenValueContainer, tokenValue) {
     const filteredSearchInput = FilteredSearchContainer.container.querySelector('.filtered-search');
     const baseEndpoint = filteredSearchInput.dataset.baseEndpoint;
-    const labelsEndpoint = `${baseEndpoint}/labels.json`;
+    const labelsEndpoint = FilteredSearchVisualTokens.getEndpointWithQueryParams(
+      `${baseEndpoint}/labels.json`,
+      filteredSearchInput.dataset.endpointQueryParams,
+    );
 
     return AjaxCache.retrieve(labelsEndpoint)
       .then(FilteredSearchVisualTokens.preprocessLabel.bind(null, labelsEndpoint))

app/assets/javascripts/lib/utils/common_utils.js

@@ -302,6 +302,14 @@ export const parseQueryStringIntoObject = (query = '') => {
     }, {});
 };
 
+/**
+ * Converts object with key-value pairs
+ * into query-param string
+ *
+ * @param {Object} params
+ */
+export const objectToQueryString = (params = {}) => Object.keys(params).map(param => `${param}=${params[param]}`).join('&');
+
 export const buildUrlWithCurrentLocation = param => (param ? `${window.location.pathname}${param}` : window.location.pathname);
 
 /**

app/assets/javascripts/main.js

@@ -39,8 +39,11 @@ import 'ee/main';
 
 import initDispatcher from './dispatcher';
 
-// eslint-disable-next-line global-require, import/no-commonjs
-if (process.env.NODE_ENV !== 'production') require('./test_utils/');
+// inject test utilities if necessary
+if (process.env.NODE_ENV !== 'production' && gon && gon.test_env) {
+  $.fx.off = true;
+  import(/* webpackMode: "eager" */ './test_utils/');
+}
 
 svg4everybody();
 

app/assets/javascripts/pages/search/init_filtered_search.js

@@ -5,6 +5,7 @@ export default ({
   filteredSearchTokenKeys,
   isGroup,
   isGroupAncestor,
+  isGroupDecendent,
   stateFiltersSelector,
 }) => {
   const filteredSearchEnabled = FilteredSearchManager && document.querySelector('.filtered-search');
@@ -13,6 +14,7 @@ export default ({
       page,
       isGroup,
       isGroupAncestor,
+      isGroupDecendent,
       filteredSearchTokenKeys,
       stateFiltersSelector,
     });

app/assets/javascripts/test.js

-$.fx.off = true;

app/finders/snippets_finder.rb

@@ -58,11 +58,37 @@ class SnippetsFinder < UnionFinder
       .public_or_visible_to_user(current_user)
   end
 
+  # Returns a collection of projects that is either public or visible to the
+  # logged in user.
+  #
+  # A caller must pass in a block to modify individual parts of
+  # the query, e.g. to apply .with_feature_available_for_user on top of it.
+  # This is useful for performance as we can stick those additional filters
+  # at the bottom of e.g. the UNION.
+  def projects_for_user
+    return yield(Project.public_to_user) unless current_user
+
+    # If the current_user is allowed to see all projects,
+    # we can shortcut and just return.
+    return yield(Project.all) if current_user.full_private_access?
+
+    authorized_projects = yield(Project.where('EXISTS (?)', current_user.authorizations_for_projects))
+
+    levels = Gitlab::VisibilityLevel.levels_for_user(current_user)
+    visible_projects = yield(Project.where(visibility_level: levels))
+
+    # We use a UNION here instead of OR clauses since this results in better
+    # performance.
+    union = Gitlab::SQL::Union.new([authorized_projects.select('projects.id'), visible_projects.select('projects.id')])
+
+    Project.from("(#{union.to_sql}) AS #{Project.table_name}")
+  end
+
   def feature_available_projects
     # Don't return any project related snippets if the user cannot read cross project
     return table[:id].eq(nil) unless Ability.allowed?(current_user, :read_cross_project)
 
-    projects = Project.public_or_visible_to_user(current_user, use_where_in: false) do |part|
+    projects = projects_for_user do |part|
       part.with_feature_available_for_user(:snippets, current_user)
     end.select(:id)
 

app/models/project.rb

@@ -322,42 +322,13 @@ class Project < ActiveRecord::Base
 
   # Returns a collection of projects that is either public or visible to the
   # logged in user.
-  #
-  # A caller may pass in a block to modify individual parts of
-  # the query, e.g. to apply .with_feature_available_for_user on top of it.
-  # This is useful for performance as we can stick those additional filters
-  # at the bottom of e.g. the UNION.
-  #
-  # Optionally, turning `use_where_in` off leads to returning a
-  # relation using #from instead of #where. This can perform much better
-  # but leads to trouble when used in conjunction with AR's #merge method.
-  def self.public_or_visible_to_user(user = nil, use_where_in: true, &block)
-    # If we don't get a block passed, use identity to avoid if/else repetitions
-    block = ->(part) { part } unless block_given?
-
-    return block.call(public_to_user) unless user
-
-    # If the user is allowed to see all projects,
-    # we can shortcut and just return.
-    return block.call(all) if user.full_private_access?
-
-    authorized = user
-      .project_authorizations
-      .select(1)
-      .where('project_authorizations.project_id = projects.id')
-    authorized_projects = block.call(where('EXISTS (?)', authorized))
-
-    levels = Gitlab::VisibilityLevel.levels_for_user(user)
-    visible_projects = block.call(where(visibility_level: levels))
-
-    # We use a UNION here instead of OR clauses since this results in better
-    # performance.
-    union = Gitlab::SQL::Union.new([authorized_projects.select('projects.id'), visible_projects.select('projects.id')])
-
-    if use_where_in
-      where("projects.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
+  def self.public_or_visible_to_user(user = nil)
+    if user
+      where('EXISTS (?) OR projects.visibility_level IN (?)',
+            user.authorizations_for_projects,
+            Gitlab::VisibilityLevel.levels_for_user(user))
     else
-      from("(#{union.to_sql}) AS #{table_name}")
+      public_to_user
     end
   end
 
@@ -376,14 +347,11 @@ class Project < ActiveRecord::Base
     elsif user
       column = ProjectFeature.quoted_access_level_column(feature)
 
-      authorized = user.project_authorizations.select(1)
-        .where('project_authorizations.project_id = projects.id')
-
       with_project_feature
         .where("#{column} IN (?) OR (#{column} = ? AND EXISTS (?))",
               visible,
               ProjectFeature::PRIVATE,
-              authorized)
+              user.authorizations_for_projects)
     else
       with_feature_access_level(feature, visible)
     end

app/models/project_services/chat_notification_service.rb

@@ -99,7 +99,7 @@ class ChatNotificationService < Service
   def get_message(object_kind, data)
     case object_kind
     when "push", "tag_push"
-      ChatMessage::PushMessage.new(data)
+      ChatMessage::PushMessage.new(data) if notify_for_ref?(data)
     when "issue"
       ChatMessage::IssueMessage.new(data) unless update?(data)
     when "merge_request"
@@ -145,10 +145,16 @@ class ChatNotificationService < Service
   end
 
   def notify_for_ref?(data)
-    return true if data[:object_attributes][:tag]
+    return true if data.dig(:object_attributes, :tag)
     return true unless notify_only_default_branch?
 
-    data[:object_attributes][:ref] == project.default_branch
+    ref = if data[:ref]
+            Gitlab::Git.ref_name(data[:ref])
+          else
+            data.dig(:object_attributes, :ref)
+          end
+
+    ref == project.default_branch
   end
 
   def notify_for_pipeline?(data)

app/models/user.rb

@@ -619,6 +619,15 @@ class User < ActiveRecord::Base
     authorized_projects(min_access_level).exists?({ id: project.id })
   end
 
+  # Typically used in conjunction with projects table to get projects
+  # a user has been given access to.
+  #
+  # Example use:
+  # `Project.where('EXISTS(?)', user.authorizations_for_projects)`
+  def authorizations_for_projects
+    project_authorizations.select(1).where('project_authorizations.project_id = projects.id')
+  end
+
   # Returns the projects this user has reporter (or greater) access to, limited
   # to at most the given projects.
   #

app/views/admin/application_settings/_form.html.haml

@@ -706,15 +706,15 @@
         .checkbox
           = f.label :usage_ping_enabled do
             = f.check_box :usage_ping_enabled, disabled: !can_be_configured
-            Usage ping enabled
-            = link_to icon('question-circle'), help_page_path("user/admin_area/settings/usage_statistics", anchor: "usage-ping")
+            Enable usage ping
           .help-block
             - if can_be_configured
-              Every week GitLab will report license usage back to GitLab, Inc.
-              Disable this option if you do not want this to occur. To see the
-              JSON payload that will be sent, visit the
-              = succeed '.' do
-                = link_to "Cohorts page", admin_cohorts_path(anchor: 'usage-ping')
+              To help improve GitLab and its user experience, GitLab will
+              periodically collect usage information.
+              = link_to 'Learn more', help_page_path("user/admin_area/settings/usage_statistics", anchor: "usage-ping")
+              about what information is shared with GitLab Inc. Visit
+              = link_to 'Cohorts', admin_cohorts_path(anchor: 'usage-ping')
+              to see the JSON payload sent.
             - else
               The usage ping is disabled, and cannot be configured through this
               form. For more information, see the documentation on

app/views/layouts/_head.html.haml

@@ -42,7 +42,6 @@
   = webpack_bundle_tag "common"
   = webpack_bundle_tag "main"
   = webpack_bundle_tag "raven" if Gitlab::CurrentSettings.clientside_sentry_enabled
-  = webpack_bundle_tag "test" if Rails.env.test?
 
   - if content_for?(:page_specific_javascripts)
     = yield :page_specific_javascripts

changelogs/unreleased/33570-slack-notify-default-branch.yml

+---
+title: Fix Slack/Mattermost notifications not respecting `notify_only_default_branch` setting for pushes
+merge_request: 17345
+author:
+type: fixed

changelogs/unreleased/revert-project-visibility-changes.yml

+---
+title: Revert Project.public_or_visible_to_user changes and only apply to snippets
+merge_request:
+author:
+type: fixed

config/webpack.config.js

@@ -54,7 +54,6 @@ function generateEntries() {
     main:                 './main.js',
     ide:                  './ide/index.js',
     raven:                './raven/index.js',
-    test:                 './test.js',
     webpack_runtime:      './webpack.js',
 
     // EE-only

doc/api/merge_requests.md

@@ -312,8 +312,7 @@ Parameters:
     "human_time_estimate": null,
     "human_total_time_spent": null
   },
-  "approvals_before_merge": null
-  },
+  "approvals_before_merge": null,
   "closed_at": "2018-01-19T14:36:11.086Z",
   "latest_build_started_at": null,
   "latest_build_finished_at": null,

spec/javascripts/filtered_search/filtered_search_visual_tokens_spec.js

@@ -128,6 +128,24 @@ describe('Filtered Search Visual Tokens', () => {
     });
   });
 
+  describe('getEndpointWithQueryParams', () => {
+    it('returns `endpoint` string as is when second param `endpointQueryParams` is undefined, null or empty string', () => {
+      const endpoint = 'foo/bar/labels.json';
+      expect(subject.getEndpointWithQueryParams(endpoint)).toBe(endpoint);
+      expect(subject.getEndpointWithQueryParams(endpoint, null)).toBe(endpoint);
+      expect(subject.getEndpointWithQueryParams(endpoint, '')).toBe(endpoint);
+    });
+
+    it('returns `endpoint` string with values of `endpointQueryParams`', () => {
+      const endpoint = 'foo/bar/labels.json';
+      const singleQueryParams = '{"foo":"true"}';
+      const multipleQueryParams = '{"foo":"true","bar":"true"}';
+
+      expect(subject.getEndpointWithQueryParams(endpoint, singleQueryParams)).toBe(`${endpoint}?foo=true`);
+      expect(subject.getEndpointWithQueryParams(endpoint, multipleQueryParams)).toBe(`${endpoint}?foo=true&bar=true`);
+    });
+  });
+
   describe('unselectTokens', () => {
     it('does nothing when there are no tokens', () => {
       const beforeHTML = tokensContainer.innerHTML;

spec/javascripts/lib/utils/common_utils_spec.js

@@ -166,6 +166,21 @@ describe('common_utils', () => {
     });
   });
 
+  describe('objectToQueryString', () => {
+    it('returns empty string when `param` is undefined, null or empty string', () => {
+      expect(commonUtils.objectToQueryString()).toBe('');
+      expect(commonUtils.objectToQueryString('')).toBe('');
+    });
+
+    it('returns query string with values of `params`', () => {
+      const singleQueryParams = { foo: true };
+      const multipleQueryParams = { foo: true, bar: true };
+
+      expect(commonUtils.objectToQueryString(singleQueryParams)).toBe('foo=true');
+      expect(commonUtils.objectToQueryString(multipleQueryParams)).toBe('foo=true&bar=true');
+    });
+  });
+
   describe('buildUrlWithCurrentLocation', () => {
     it('should build an url with current location and given parameters', () => {
       expect(commonUtils.buildUrlWithCurrentLocation()).toEqual(window.location.pathname);

spec/models/user_spec.rb

@@ -1684,6 +1684,32 @@ describe User do
     end
   end
 
+  describe '#authorizations_for_projects' do
+    let!(:user) { create(:user) }
+    subject { Project.where("EXISTS (?)", user.authorizations_for_projects) }
+
+    it 'includes projects that belong to a user, but no other projects' do
+      owned = create(:project, :private, namespace: user.namespace)
+      member = create(:project, :private).tap { |p| p.add_master(user) }
+      other = create(:project)
+
+      expect(subject).to include(owned)
+      expect(subject).to include(member)
+      expect(subject).not_to include(other)
+    end
+
+    it 'includes projects a user has access to, but no other projects' do
+      other_user = create(:user)
+      accessible = create(:project, :private, namespace: other_user.namespace) do |project|
+        project.add_developer(user)
+      end
+      other = create(:project)
+
+      expect(subject).to include(accessible)
+      expect(subject).not_to include(other)
+    end
+  end
+
   describe '#authorized_projects', :delete do
     context 'with a minimum access level' do
       it 'includes projects for which the user is an owner' do

spec/support/slack_mattermost_notifications_shared_examples.rb

@@ -337,6 +337,7 @@ RSpec.shared_examples 'slack or mattermost notifications' do
 
         before do
           chat_service.notify_only_default_branch = true
+          WebMock.stub_request(:post, webhook_url)
         end
 
         it 'does not call the Slack/Mattermost API for pipeline events' do
@@ -345,6 +346,23 @@ RSpec.shared_examples 'slack or mattermost notifications' do
 
           expect(result).to be_falsy
         end
+
+        it 'does not notify push events if they are not for the default branch' do
+          ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}test"
+          push_sample_data = Gitlab::DataBuilder::Push.build(project, user, nil, nil, ref, [])
+
+          chat_service.execute(push_sample_data)
+
+          expect(WebMock).not_to have_requested(:post, webhook_url)
+        end
+
+        it 'notifies about push events for the default branch' do
+          push_sample_data = Gitlab::DataBuilder::Push.build_sample(project, user)
+
+          chat_service.execute(push_sample_data)
+
+          expect(WebMock).to have_requested(:post, webhook_url).once
+        end
       end
 
       context 'when disabled' do