Merge branch '13695-order-contributors-in-api' into 'master'

Adds ordering to projects contributors in API

Closes #13695

See merge request gitlab-org/gitlab-ce!15469

10 files changed, 198 insertions, 6 deletions

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 307e4fcedfe..13c31111134 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -52,6 +52,20 @@ class Commit
       diffs.reduce(0) { |sum, d| sum + Gitlab::Git::Util.count_lines(d.diff) }
     end
 
+    def order_by(collection:, order_by:, sort:)
+      return collection unless %w[email name commits].include?(order_by)
+      return collection unless %w[asc desc].include?(sort)
+
+      collection.sort do |a, b|
+        operands = [a, b].tap { |o| o.reverse! if sort == 'desc' }
+
+        attr1, attr2 = operands.first.public_send(order_by), operands.second.public_send(order_by) # rubocop:disable PublicSend
+
+        # use case insensitive comparison for string values
+        order_by.in?(%w[email name]) ? attr1.casecmp(attr2) : attr1 <=> attr2
+      end
+    end
+
     # Truncate sha to 8 characters
     def truncate_sha(sha)
       sha[0..MIN_SHA_LENGTH]
diff --git a/app/models/repository.rb b/app/models/repository.rb
index c0e31eca8da..28f5fc28b8c 100644
--- a/app/models/repository.rb
+++ b/app/models/repository.rb
@@ -697,10 +697,14 @@ class Repository
     end
   end
 
-  def contributors
+  # Params:
+  #
+  # order_by: name|email|commits
+  # sort: asc|desc default: 'asc'
+  def contributors(order_by: nil, sort: 'asc')
     commits = self.commits(nil, limit: 2000, offset: 0, skip_merges: true)
 
-    commits.group_by(&:author_email).map do |email, commits|
+    commits = commits.group_by(&:author_email).map do |email, commits|
       contributor = Gitlab::Contributor.new
       contributor.email = email
 
@@ -714,6 +718,7 @@ class Repository
 
       contributor
     end
+    Commit.order_by(collection: commits, order_by: order_by, sort: sort)
   end
 
   def refs_contains_sha(ref_type, sha)
diff --git a/changelogs/unreleased/13695-order-contributors-in-api.yml b/changelogs/unreleased/13695-order-contributors-in-api.yml
new file mode 100644
index 00000000000..26bf8650a4a
--- /dev/null
+++ b/changelogs/unreleased/13695-order-contributors-in-api.yml
@@ -0,0 +1,5 @@
+---
+title: Adds ordering to projects contributors in API
+merge_request: 15469
+author: Jacopo Beschi @jacopo-beschi
+type: added
diff --git a/doc/api/repositories.md b/doc/api/repositories.md
index 594babc74be..03b32577872 100644
--- a/doc/api/repositories.md
+++ b/doc/api/repositories.md
@@ -182,6 +182,8 @@ GET /projects/:id/repository/contributors
 Parameters:
 
 - `id` (required) - The ID or [URL-encoded path of the project](README.md#namespaced-path-encoding) owned by the authenticated user
+- `order_by` (optional) - Return contributors ordered by `name`, `email`, or `commits` fields. If not given contributors are ordered by commit date.
+- `sort` (optional) - Return contributors sorted in `asc` or `desc` order. Default is `asc`
 
 Response:
 
diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb
index 7887b886c03..4f36bbd760f 100644
--- a/lib/api/repositories.rb
+++ b/lib/api/repositories.rb
@@ -110,10 +110,12 @@ module API
       end
       params do
         use :pagination
+        optional :order_by, type: String, values: %w[email name commits], default: nil, desc: 'Return contributors ordered by `name` or `email` or `commits`'
+        optional :sort, type: String, values: %w[asc desc], default: nil, desc: 'Sort by asc (ascending) or desc (descending)'
       end
       get ':id/repository/contributors' do
         begin
-          contributors = ::Kaminari.paginate_array(user_project.repository.contributors)
+          contributors = ::Kaminari.paginate_array(user_project.repository.contributors(order_by: params[:order_by], sort: params[:sort]))
           present paginate(contributors), with: Entities::Contributor
         rescue
           not_found!
diff --git a/spec/factories/commits.rb b/spec/factories/commits.rb
index f4f12a095fc..4e2d8e8969e 100644
--- a/spec/factories/commits.rb
+++ b/spec/factories/commits.rb
@@ -2,15 +2,28 @@ require_relative '../support/repo_helpers'
 
 FactoryGirl.define do
   factory :commit do
-    git_commit RepoHelpers.sample_commit
+    transient do
+      author nil
+    end
+
+    git_commit do
+      commit = RepoHelpers.sample_commit
+
+      if author
+        commit.author_email = author.email
+        commit.author_name = author.name
+      end
+
+      commit
+    end
     project
 
     initialize_with do
       new(git_commit, project)
     end
 
-    after(:build) do |commit|
-      allow(commit).to receive(:author).and_return build(:author)
+    after(:build) do |commit, evaluator|
+      allow(commit).to receive(:author).and_return(evaluator.author || build(:author))
     end
 
     trait :without_author do
diff --git a/spec/fixtures/api/schemas/contributor.json b/spec/fixtures/api/schemas/contributor.json
new file mode 100644
index 00000000000..e88470a2363
--- /dev/null
+++ b/spec/fixtures/api/schemas/contributor.json
@@ -0,0 +1,18 @@
+{
+  "type": "object",
+  "required" : [
+    "name",
+    "email",
+    "commits",
+    "additions",
+    "deletions"
+  ],
+  "properties" : {
+    "name": { "type": "string" },
+    "email": { "type": "string" },
+    "commits": { "type": "integer" },
+    "additions": { "type": "integer" },
+    "deletions": { "type": "integer" }
+  },
+  "additionalProperties": false
+}
diff --git a/spec/fixtures/api/schemas/contributors.json b/spec/fixtures/api/schemas/contributors.json
new file mode 100644
index 00000000000..a9f1d1ea64f
--- /dev/null
+++ b/spec/fixtures/api/schemas/contributors.json
@@ -0,0 +1,4 @@
+{
+  "type": "array",
+  "items": { "$ref": "contributor.json" }
+}
diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb
index 129fce74f45..2c0d4db3307 100644
--- a/spec/models/repository_spec.rb
+++ b/spec/models/repository_spec.rb
@@ -2360,4 +2360,111 @@ describe Repository do
       end
     end
   end
+
+  describe '#contributors' do
+    let(:author_a) { build(:author, email: 'tiagonbotelho@hotmail.com', name: 'tiagonbotelho') }
+    let(:author_b) { build(:author, email: 'gitlab@winniehell.de', name: 'Winnie') }
+    let(:author_c) { build(:author, email: 'douwe@gitlab.com', name: 'Douwe Maan') }
+    let(:stubbed_commits) do
+      [build(:commit, author: author_a),
+       build(:commit, author: author_a),
+       build(:commit, author: author_b),
+       build(:commit, author: author_c),
+       build(:commit, author: author_c),
+       build(:commit, author: author_c)]
+    end
+    let(:order_by) { nil }
+    let(:sort) { nil }
+
+    before do
+      allow(repository).to receive(:commits).with(nil, limit: 2000, offset: 0, skip_merges: true).and_return(stubbed_commits)
+    end
+
+    subject { repository.contributors(order_by: order_by, sort: sort) }
+
+    def expect_contributors(*contributors)
+      expect(subject.map(&:email)).to eq(contributors.map(&:email))
+    end
+
+    it 'returns the array of Gitlab::Contributor for the repository' do
+      expect_contributors(author_a, author_b, author_c)
+    end
+
+    context 'order_by email' do
+      let(:order_by) { 'email' }
+
+      context 'asc' do
+        let(:sort) { 'asc' }
+
+        it 'returns all the contributors ordered by email asc case insensitive' do
+          expect_contributors(author_c, author_b, author_a)
+        end
+      end
+
+      context 'desc' do
+        let(:sort) { 'desc' }
+
+        it 'returns all the contributors ordered by email desc case insensitive' do
+          expect_contributors(author_a, author_b, author_c)
+        end
+      end
+    end
+
+    context 'order_by name' do
+      let(:order_by) { 'name' }
+
+      context 'asc' do
+        let(:sort) { 'asc' }
+
+        it 'returns all the contributors ordered by name asc case insensitive' do
+          expect_contributors(author_c, author_a, author_b)
+        end
+      end
+
+      context 'desc' do
+        let(:sort) { 'desc' }
+
+        it 'returns all the contributors ordered by name desc case insensitive' do
+          expect_contributors(author_b, author_a, author_c)
+        end
+      end
+    end
+
+    context 'order_by commits' do
+      let(:order_by) { 'commits' }
+
+      context 'asc' do
+        let(:sort) { 'asc' }
+
+        it 'returns all the contributors ordered by commits asc' do
+          expect_contributors(author_b, author_a, author_c)
+        end
+      end
+
+      context 'desc' do
+        let(:sort) { 'desc' }
+
+        it 'returns all the contributors ordered by commits desc' do
+          expect_contributors(author_c, author_a, author_b)
+        end
+      end
+    end
+
+    context 'invalid ordering' do
+      let(:order_by) { 'unknown' }
+
+      it 'returns the contributors unsorted' do
+        expect_contributors(author_a, author_b, author_c)
+      end
+    end
+
+    context 'invalid sorting' do
+      let(:order_by) { 'name' }
+      let(:sort) { 'unknown' }
+
+      it 'returns the contributors unsorted' do
+        expect_contributors(author_a, author_b, author_c)
+      end
+    end
+  end
 end
diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb
index 9f2ff3b5af6..741800ff61d 100644
--- a/spec/requests/api/repositories_spec.rb
+++ b/spec/requests/api/repositories_spec.rb
@@ -378,6 +378,28 @@ describe API::Repositories do
         expect(first_contributor['additions']).to eq(0)
         expect(first_contributor['deletions']).to eq(0)
       end
+
+      context 'using sorting' do
+        context 'by commits desc' do
+          it 'returns the repository contribuors sorted by commits desc' do
+            get api(route, current_user), { order_by: 'commits', sort: 'desc' }
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(response).to match_response_schema('contributors')
+            expect(json_response.first['commits']).to be > json_response.last['commits']
+          end
+        end
+
+        context 'by name desc' do
+          it 'returns the repository contribuors sorted by name asc case insensitive' do
+            get api(route, current_user), { order_by: 'name', sort: 'asc' }
+
+            expect(response).to have_gitlab_http_status(200)
+            expect(response).to match_response_schema('contributors')
+            expect(json_response.first['name'].downcase).to be < json_response.last['name'].downcase
+          end
+        end
+      end
     end
 
     context 'when unauthenticated', 'and project is public' do