Merge branch 'ce-to-ee-2018-09-13' into 'master'

CE upstream - 2018-09-13 18:21 UTC

Closes gitlab-ce#51370

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

app/assets/javascripts/diffs/components/diff_file.vue

@@ -44,7 +44,7 @@ export default {
     },
     showExpandMessage() {
       return (
-        !this.isCollapsed &&
+        this.isCollapsed ||
         !this.file.highlightedDiffLines &&
         !this.isLoadingCollapsedDiff &&
         !this.file.tooLarge &&

app/assets/javascripts/notes/stores/mutations.js

@@ -214,12 +214,7 @@ export default {
 
   [types.SET_DISCUSSION_DIFF_LINES](state, { discussionId, diffLines }) {
     const discussion = utils.findNoteObjectById(state.discussions, discussionId);
-    const index = state.discussions.indexOf(discussion);
 
-    const discussionWithDiffLines = Object.assign({}, discussion, {
-      truncated_diff_lines: diffLines,
-    });
-
-    state.discussions.splice(index, 1, discussionWithDiffLines);
+    discussion.truncated_diff_lines = diffLines;
   },
 };

changelogs/unreleased/49990-enable-omniauth-by-default.yml

+---
+title: Enable omniauth by default
+merge_request: 21700
+author:
+type: changed

changelogs/unreleased/7573-show-click-to-expand-on-not-rendered-diffs.yml

+---
+title: Fix absent Click to Expand link on diffs not rendered on first load of Merge
+  Requests Changes tab
+merge_request: 21716
+author:
+type: fixed

changelogs/unreleased/mr-fixed-expanded-state-not-working.yml

+---
+title: Fixed resolved discussions not toggling expanded state on changes tab
+merge_request: 21676
+author:
+type: fixed

config/gitlab.yml.example

@@ -573,7 +573,7 @@ production: &base
   ## OmniAuth settings
   omniauth:
     # Allow login via Twitter, Google, etc. using OmniAuth providers
-    enabled: false
+    # enabled: true
 
     # Uncomment this to automatically sign in with a specific omniauth provider's without
     # showing GitLab's sign-in page (default: show the GitLab sign-in page)
@@ -948,7 +948,7 @@ test:
       project_key: PROJECT
 
   omniauth:
-    enabled: true
+    # enabled: true
     allow_single_sign_on: true
     external_providers: []
 

config/initializers/1_settings.rb

@@ -51,7 +51,7 @@ if Settings.ldap['enabled'] || Rails.env.test?
 end
 
 Settings['omniauth'] ||= Settingslogic.new({})
-Settings.omniauth['enabled'] = false if Settings.omniauth['enabled'].nil?
+Settings.omniauth['enabled'] = true if Settings.omniauth['enabled'].nil?
 Settings.omniauth['auto_sign_in_with_provider'] = false if Settings.omniauth['auto_sign_in_with_provider'].nil?
 Settings.omniauth['allow_single_sign_on'] = false if Settings.omniauth['allow_single_sign_on'].nil?
 Settings.omniauth['external_providers'] = [] if Settings.omniauth['external_providers'].nil?

db/schema.rb

@@ -1315,8 +1315,8 @@ ActiveRecord::Schema.define(version: 20180906101639) do
   add_index "gpg_signatures", ["project_id"], name: "index_gpg_signatures_on_project_id", using: :btree
 
   create_table "group_custom_attributes", force: :cascade do |t|
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.integer "group_id", null: false
     t.string "key", null: false
     t.string "value", null: false
@@ -2062,8 +2062,8 @@ ActiveRecord::Schema.define(version: 20180906101639) do
   add_index "project_ci_cd_settings", ["project_id"], name: "index_project_ci_cd_settings_on_project_id", unique: true, using: :btree
 
   create_table "project_custom_attributes", force: :cascade do |t|
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.integer "project_id", null: false
     t.string "key", null: false
     t.string "value", null: false

doc/development/README.md

@@ -51,6 +51,7 @@ description: 'Learn how to contribute to GitLab.'
 - [Working with Merge Request diffs](diffs.md)
 - [Permissions](permissions.md)
 - [Prometheus metrics](prometheus_metrics.md)
+- [Guidelines for reusing abstractions](reusing_abstractions.md)
 
 ## Performance guides
 

doc/development/reusing_abstractions.md

+# Guidelines for reusing abstractions
+
+As GitLab has grown, different patterns emerged across the codebase. Service
+classes, serializers, and presenters are just a few. These patterns made it easy
+to reuse code, but at the same time make it easy to accidentally reuse the wrong
+abstraction in a particular place.
+
+## Why these guidelines are necessary
+
+Code reuse is good, but sometimes this can lead to shoehorning the wrong
+abstraction into a particular use case. This in turn can have a negative impact
+on maintainability, the ability to easily debug problems, or even performance.
+
+An example would be to use `ProjectsFinder` in `IssuesFinder` to limit issues to
+those belonging to a set of projects. While initially this may seem like a good
+idea, both classes provide a very high level interface with very little control.
+This means that `IssuesFinder` may not be able to produce a better optimised
+database query, as a large portion of the query is controlled by the internals
+of `ProjectsFinder`.
+
+To work around this problem, you would use the same code used by
+`ProjectsFinder`, instead of using `ProjectsFinder` itself directly. This allows
+you to compose your behaviour better, giving you more control over the behaviour
+of the code.
+
+To illustrate, consider the following code from `IssuableFinder#projects`:
+
+```ruby
+return @projects = project if project?
+
+projects =
+  if current_user && params[:authorized_only].presence && !current_user_related?
+    current_user.authorized_projects
+  elsif group
+    finder_options = { include_subgroups: params[:include_subgroups], only_owned: true }
+    GroupProjectsFinder.new(group: group, current_user: current_user, options: finder_options).execute
+  else
+    ProjectsFinder.new(current_user: current_user).execute
+  end
+
+@projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
+```
+
+Here we determine what projects to scope our data to, using three different
+approaches. When a group is specified, we use `GroupProjectsFinder` to retrieve
+all the projects of that group. On the surface this seems harmless: it is easy
+to use, and we only need two lines of code.
+
+In reality, things can get hairy very quickly. For example, the query produced
+by `GroupProjectsFinder` may start out simple. Over time more and more
+functionality is added to this (high level) interface. Instead of _only_
+affecting the cases where this is necessary, it may also start affecting
+`IssuableFinder` in a negative way. For example, the query produced by
+`GroupProjectsFinder` may include unnecessary conditions. Since we're using a
+finder here, we can't easily opt-out of that behaviour. We could add options to
+do so, but then we'd need as many options as we have features. Every option adds
+two code paths, which means that for four features we have to cover 8 different
+code paths.
+
+A much more reliable (and pleasant) way of dealing with this, is to simply use
+the underlying bits that make up `GroupProjectsFinder` directly. This means we
+may need a little bit more code in `IssuableFinder`, but it also gives us much
+more control and certainty. This means we might end up with something like this:
+
+```ruby
+return @projects = project if project?
+
+projects =
+  if current_user && params[:authorized_only].presence && !current_user_related?
+    current_user.authorized_projects
+  elsif group
+    current_user
+      .owned_groups(subgroups: params[:include_subgroups])
+      .projects
+      .any_additional_method_calls
+      .that_might_be_necessary
+  else
+    current_user
+      .projects_visible_to_user
+      .any_additional_method_calls
+      .that_might_be_necessary
+  end
+
+@projects = projects.with_feature_available_for_user(klass, current_user).reorder(nil)
+```
+
+This is just a sketch, but it shows the general idea: we would use whatever the
+`GroupProjectsFinder` and `ProjectsFinder` finders use under the hoods.
+
+## End goal
+
+The guidelines in this document are meant to foster _better_ code reuse, by
+clearly defining what can be reused where, and what to do when you can not reuse
+something. Clearly separating abstractions makes it harder to use the wrong one,
+makes it easier to debug the code, and (hopefully) results in fewer performance
+problems.
+
+## Abstractions
+
+Now let's take a look at the various abstraction levels available, and what they
+can (or cannot) reuse. For this we can use the following table, which defines
+the various abstractions and what they can (not) reuse:
+
+| Abstraction            | Service classes  | Finders  | Presenters  | Serializers   | Model instance method   | Model class methods   | Active Record   | Worker
+|:-----------------------|:-----------------|:---------|:------------|:--------------|:------------------------|:----------------------|:----------------|:--------
+| Controller             | Yes              | Yes      | Yes         | Yes           | Yes                     | No                    | No              | No
+| Service class          | Yes              | Yes      | No          | No            | Yes                     | No                    | No              | Yes
+| Finder                 | No               | No       | No          | No            | Yes                     | Yes                   | No              | No
+| Presenter              | No               | Yes      | No          | No            | Yes                     | Yes                   | No              | No
+| Serializer             | No               | Yes      | No          | No            | Yes                     | Yes                   | No              | No
+| Model class method     | No               | No       | No          | No            | Yes                     | Yes                   | Yes             | No
+| Model instance method  | No               | Yes      | No          | No            | Yes                     | Yes                   | Yes             | Yes
+| Worker                 | Yes              | Yes      | No          | No            | Yes                     | No                    | No              | Yes
+
+### Controllers
+
+Everything in `app/controllers`.
+
+Controllers should not do much work on their own, instead they simply pass input
+to other classes and present the results.
+
+### Grape endpoint
+
+Everything in `lib/api`.
+
+### Service classes
+
+Everything that resides in `app/services`.
+
+### Finders
+
+Everything in `app/finders`, typically used for retrieving data from a database.
+
+Finders can not reuse other finders in an attempt to better control the SQL
+queries they produce.
+
+### Presenters
+
+Everything in `app/presenters`, used for exposing complex data to a Rails view,
+without having to create many instance variables.
+
+### Serializers
+
+Everything in `app/serializers`, used for presenting the response to a request,
+typically in JSON.
+
+### Model class methods
+
+These are class methods defined by _GitLab itself_, including the following
+methods provided by Active Record:
+
+* `find`
+* `find_by_id`
+* `delete_all`
+* `destroy`
+* `destroy_all`
+
+Any other methods such as `find_by(some_column: X)` are not included, and
+instead fall under the "Active Record" abstraction.
+
+### Model instance methods
+
+Instance methods defined on Active Record models by _GitLab itself_. Methods
+provided by Active Record are not included, except for the following methods:
+
+* `save`
+* `update`
+* `destroy`
+* `delete`
+
+### Active Record
+
+The API provided by Active Record itself, such as the `where` method, `save`,
+`delete_all`, etc.
+
+### Worker
+
+Everything in `app/workers`.
+
+The scheduling of Sidekiq jobs using `SomeWorker.perform_async`, `perform_in`,
+etc. Directly invoking a worker using `SomeWorker.new.perform` should be avoided
+at all times in application code, though this is fine to use in tests.

doc/integration/omniauth.md

@@ -39,7 +39,10 @@ contains some settings that are common for all providers.
 Before configuring individual OmniAuth providers there are a few global settings
 that are in common for all providers that we need to consider.
 
-- Omniauth needs to be enabled, see details below for example.
+> **NOTE:**
+> Starting from GitLab 11.4, Omniauth is enabled by default. If you're using an
+> earlier version, you'll need to explicitly enable it.
+
 - `allow_single_sign_on` allows you to specify the providers you want to allow to
   automatically create an account. It defaults to `false`. If `false` users must
   be created manually or they will not be able to sign in via OmniAuth.
@@ -74,7 +77,8 @@ To change these settings:
     and change:
 
     ```ruby
-    gitlab_rails['omniauth_enabled'] = true
+    # Versions prior to 11.4 require this to be set to true
+    # gitlab_rails['omniauth_enabled'] = nil
 
     # CAUTION!
     # This allows users to login without having a user account first. Define the allowed providers
@@ -101,7 +105,8 @@ To change these settings:
      ## OmniAuth settings
       omniauth:
         # Allow login via Twitter, Google, etc. using OmniAuth providers
-        enabled: true
+        # Versions prior to 11.4 require this to be set to true
+        # enabled: true
 
         # CAUTION!
         # This allows users to login without having a user account first. Define the allowed providers
@@ -227,6 +232,27 @@ In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings ->
 ![Enabled OAuth Sign-In sources](img/enabled-oauth-sign-in-sources.png)
 
 
+## Disabling Omniauth
+
+Starting from version 11.4 of GitLab, Omniauth is enabled by default. This only
+has an effect if providers are configured and [enabled](#enable-or-disable-sign-in-with-an-omniauth-provider-without-disabling-import-sources).
+
+If omniauth providers are causing problems even when individually disabled, you
+can disable the entire omniauth subsystem by modifying the configuration file:
+
+**For Omnibus installations**
+
+```ruby
+gitlab_rails['omniauth_enabled'] = false
+```
+
+**For installations from source**
+
+```yaml
+  omniauth:
+    enabled: false
+```
+
 ## Keep OmniAuth user profiles up to date
 
 You can enable profile syncing from selected OmniAuth providers and for all or for specific user information.

lib/api/resource_label_events.rb

@@ -25,6 +25,7 @@ module API
           requires :eventable_id, types: [Integer, String], desc: 'The ID of the eventable'
           use :pagination
         end
+
         # rubocop: disable CodeReuse/ActiveRecord
         get ":id/#{eventables_str}/:eventable_id/resource_label_events" do
           eventable = find_noteable(parent_type, eventables_str, params[:eventable_id])

spec/config/settings_spec.rb

+require 'spec_helper'
+
+describe Settings do
+  describe 'omniauth' do
+    it 'defaults to enabled' do
+      expect(described_class.omniauth.enabled).to be true
+    end
+  end
+end

spec/javascripts/diffs/components/diff_file_spec.js

@@ -63,6 +63,18 @@ describe('DiffFile', () => {
         });
       });
 
+      it('should have collapsed text and link even before rendered', done => {
+        vm.file.renderIt = false;
+        vm.file.collapsed = true;
+
+        vm.$nextTick(() => {
+          expect(vm.$el.innerText).toContain('This diff is collapsed');
+          expect(vm.$el.querySelectorAll('.js-click-to-expand').length).toEqual(1);
+
+          done();
+        });
+      });
+
       it('should have loading icon while loading a collapsed diffs', done => {
         vm.file.collapsed = true;
         vm.isLoadingCollapsedDiff = true;

spec/javascripts/notes/stores/mutation_spec.js

+import Vue from 'vue';
 import mutations from '~/notes/stores/mutations';
 import {
   note,
@@ -333,7 +334,7 @@ describe('Notes Store mutations', () => {
     });
   });
 
-  describe('SET_NOTES_FETCHING_STATE', () => {
+  describe('SET_NOTES_FETCHED_STATE', () => {
     it('should set the given state', () => {
       const state = {
         isNotesFetched: false,
@@ -343,4 +344,37 @@ describe('Notes Store mutations', () => {
       expect(state.isNotesFetched).toEqual(true);
     });
   });
+
+  describe('SET_DISCUSSION_DIFF_LINES', () => {
+    it('sets truncated_diff_lines', () => {
+      const state = {
+        discussions: [
+          {
+            id: 1,
+          },
+        ],
+      };
+
+      mutations.SET_DISCUSSION_DIFF_LINES(state, { discussionId: 1, diffLines: ['test'] });
+
+      expect(state.discussions[0].truncated_diff_lines).toEqual(['test']);
+    });
+
+    it('keeps reactivity of discussion', () => {
+      const state = {};
+      Vue.set(state, 'discussions', [
+        {
+          id: 1,
+          expanded: false,
+        },
+      ]);
+      const discussion = state.discussions[0];
+
+      mutations.SET_DISCUSSION_DIFF_LINES(state, { discussionId: 1, diffLines: ['test'] });
+
+      discussion.expanded = true;
+
+      expect(state.discussions[0].expanded).toBe(true);
+    });
+  });
 });