Merge branch 'nfriend-fix-release-milestone-order' into 'master'

Return release milestones in predictable order

See merge request gitlab-org/gitlab!47700

app/graphql/mutations/releases/update.rb

@@ -51,17 +51,17 @@ module Mutations
 
         params = scalars.with_indifferent_access
 
-        release_result = ::Releases::UpdateService.new(project, current_user, params).execute
+        result = ::Releases::UpdateService.new(project, current_user, params).execute
 
-        if release_result[:status] == :success
+        if result[:status] == :success
           {
-            release: release_result[:release],
+            release: result[:release],
             errors: []
           }
         else
           {
             release: nil,
-            errors: [release_result[:message]]
+            errors: [result[:message]]
           }
         end
       end

app/graphql/resolvers/release_milestones_resolver.rb

+# frozen_string_literal: true
+
+module Resolvers
+  class ReleaseMilestonesResolver < BaseResolver
+    type Types::MilestoneType.connection_type, null: true
+
+    alias_method :release, :object
+
+    def resolve(**args)
+      offset_pagination(release.milestones.order_by_dates_and_title)
+    end
+  end
+end

app/graphql/types/release_type.rb

@@ -35,7 +35,8 @@ module Types
     field :links, Types::ReleaseLinksType, null: true, method: :itself,
           description: 'Links of the release'
     field :milestones, Types::MilestoneType.connection_type, null: true,
-          description: 'Milestones associated to the release'
+          description: 'Milestones associated to the release',
+          resolver: ::Resolvers::ReleaseMilestonesResolver
     field :evidences, Types::EvidenceType.connection_type, null: true,
           description: 'Evidence for the release'
 

app/models/milestone.rb

@@ -33,6 +33,7 @@ class Milestone < ApplicationRecord
   scope :order_by_name_asc, -> { order(Arel::Nodes::Ascending.new(arel_table[:title].lower)) }
   scope :reorder_by_due_date_asc, -> { reorder(Gitlab::Database.nulls_last_order('due_date', 'ASC')) }
   scope :with_api_entity_associations, -> { preload(project: [:project_feature, :route, namespace: :route]) }
+  scope :order_by_dates_and_title, -> { order(due_date: :asc, start_date: :asc, title: :asc) }
 
   validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
 

app/models/release.rb

@@ -82,7 +82,7 @@ class Release < ApplicationRecord
   end
 
   def milestone_titles
-    self.milestones.map {|m| m.title }.sort.join(", ")
+    self.milestones.order_by_dates_and_title.map {|m| m.title }.join(', ')
   end
 
   def to_hook_data(action)

changelogs/unreleased/nfriend-fix-release-milestone-order.yml

+---
+title: Return release milestones in predictable order
+merge_request: 47700
+author:
+type: fixed

ee/spec/requests/api/graphql/mutations/releases/create_spec.rb

@@ -66,11 +66,7 @@ RSpec.describe 'Creation of a new release' do
         returned_milestone_titles = mutation_response.dig(:release, :milestones, :nodes)
                                                      .map { |m| m[:title] }
 
-        # Right now the milestones are returned in a non-deterministic order.
-        # This `milestones` test should be moved up into the expect(release)
-        # above (and `.to include` updated to `.to eq`) once
-        # https://gitlab.com/gitlab-org/gitlab/-/issues/259012 is addressed.
-        expect(returned_milestone_titles).to match_array([
+        expect(returned_milestone_titles).to eq([
           milestone_12_3.title,
           milestone_12_4.title,
           group_milestone.title
@@ -88,6 +84,8 @@ RSpec.describe 'Creation of a new release' do
 
         expect(mutation_response[:release]).to be_nil
         expect(mutation_response[:errors].count).to eq(1)
+
+        # Weird error message will be fixed in https://gitlab.com/gitlab-org/gitlab/-/issues/277397
         expect(mutation_response[:errors].first).to match('Validation failed: Milestone releases is invalid, Milestone releases None of the group milestones have the same project as the release,,')
       end
     end

ee/spec/requests/api/graphql/mutations/releases/update_spec.rb

@@ -65,10 +65,7 @@ RSpec.describe 'Updating an existing release' do
         returned_milestone_titles = mutation_response.dig(:release, :milestones, :nodes)
                                                      .map { |m| m[:title] }
 
-        # Right now the milestones are returned in a non-deterministic order.
-        # Once https://gitlab.com/gitlab-org/gitlab/-/issues/259012 is addressed,
-        # this test should be updated to expect a specific order.
-        expect(returned_milestone_titles).to match_array([
+        expect(returned_milestone_titles).to eq([
           milestone_12_3.title,
           milestone_12_4.title,
           group_milestone.title

lib/api/entities/release.rb

@@ -16,7 +16,12 @@ module API
       expose :author, using: Entities::UserBasic, if: -> (release, _) { release.author.present? }
       expose :commit, using: Entities::Commit, if: ->(_, _) { can_download_code? }
       expose :upcoming_release?, as: :upcoming_release
-      expose :milestones, using: Entities::MilestoneWithStats, if: -> (release, _) { release.milestones.present? && can_read_milestone? }
+      expose :milestones,
+             using: Entities::MilestoneWithStats,
+             if: -> (release, _) { release.milestones.present? && can_read_milestone? } do |release, _|
+               release.milestones.order_by_dates_and_title
+             end
+
       expose :commit_path, expose_nil: false
       expose :tag_path, expose_nil: false
 

spec/frontend/releases/__snapshots__/util_spec.js.snap

@@ -88,18 +88,6 @@ Object {
         },
       ],
       "milestones": Array [
-        Object {
-          "description": "The 12.4 milestone",
-          "id": "gid://gitlab/Milestone/124",
-          "issueStats": Object {
-            "closed": 1,
-            "total": 4,
-          },
-          "stats": undefined,
-          "title": "12.4",
-          "webPath": undefined,
-          "webUrl": "/releases-namespace/releases-project/-/milestones/2",
-        },
         Object {
           "description": "The 12.3 milestone",
           "id": "gid://gitlab/Milestone/123",
@@ -112,6 +100,18 @@ Object {
           "webPath": undefined,
           "webUrl": "/releases-namespace/releases-project/-/milestones/1",
         },
+        Object {
+          "description": "The 12.4 milestone",
+          "id": "gid://gitlab/Milestone/124",
+          "issueStats": Object {
+            "closed": 1,
+            "total": 4,
+          },
+          "stats": undefined,
+          "title": "12.4",
+          "webPath": undefined,
+          "webUrl": "/releases-namespace/releases-project/-/milestones/2",
+        },
       ],
       "name": "The first release",
       "releasedAt": "2018-12-10T00:00:00Z",
@@ -216,18 +216,6 @@ Object {
       },
     ],
     "milestones": Array [
-      Object {
-        "description": "The 12.4 milestone",
-        "id": "gid://gitlab/Milestone/124",
-        "issueStats": Object {
-          "closed": 1,
-          "total": 4,
-        },
-        "stats": undefined,
-        "title": "12.4",
-        "webPath": undefined,
-        "webUrl": "/releases-namespace/releases-project/-/milestones/2",
-      },
       Object {
         "description": "The 12.3 milestone",
         "id": "gid://gitlab/Milestone/123",
@@ -240,6 +228,18 @@ Object {
         "webPath": undefined,
         "webUrl": "/releases-namespace/releases-project/-/milestones/1",
       },
+      Object {
+        "description": "The 12.4 milestone",
+        "id": "gid://gitlab/Milestone/124",
+        "issueStats": Object {
+          "closed": 1,
+          "total": 4,
+        },
+        "stats": undefined,
+        "title": "12.4",
+        "webPath": undefined,
+        "webUrl": "/releases-namespace/releases-project/-/milestones/2",
+      },
     ],
     "name": "The first release",
     "releasedAt": "2018-12-10T00:00:00Z",

spec/frontend/releases/components/release_block_milestone_info_spec.js

@@ -54,17 +54,7 @@ describe('Release block milestone info', () => {
     });
 
     it('renders a list of links to all associated milestones', () => {
-      // The API currently returns the milestones in a non-deterministic order,
-      // which causes the frontend fixture used by this test to return the
-      // milestones in one order locally and a different order in the CI pipeline.
-      // This is a bug and is tracked here: https://gitlab.com/gitlab-org/gitlab/-/issues/259012
-      // When this bug is fixed this expectation should be updated to
-      // assert the expected order.
-      const containerText = trimText(milestoneListContainer().text());
-      expect(
-        containerText.includes('Milestones 12.4 • 12.3') ||
-          containerText.includes('Milestones 12.3 • 12.4'),
-      ).toBe(true);
+      expect(milestoneListContainer().text()).toMatchInterpolatedText('Milestones 12.3 • 12.4');
 
       milestones.forEach((m, i) => {
         const milestoneLink = milestoneListContainer().findAll(GlLink).at(i);

spec/graphql/mutations/releases/create_spec.rb

@@ -88,12 +88,9 @@ RSpec.describe Mutations::Releases::Create do
 
       it 'creates the release with the correct milestone associations' do
         expected_milestone_titles = [milestone_12_3.title, milestone_12_4.title]
-        actual_milestone_titles = new_release.milestones.map { |m| m.title }
+        actual_milestone_titles = new_release.milestones.order_by_dates_and_title.map { |m| m.title }
 
-        # Right now the milestones are returned in a non-deterministic order.
-        # `match_array` should be updated to `eq` once
-        # https://gitlab.com/gitlab-org/gitlab/-/issues/259012 is addressed.
-        expect(actual_milestone_titles).to match_array(expected_milestone_titles)
+        expect(actual_milestone_titles).to eq(expected_milestone_titles)
       end
 
       describe 'asset links' do

spec/graphql/mutations/releases/update_spec.rb

@@ -48,12 +48,7 @@ RSpec.describe Mutations::Releases::Update do
       expect(updated_release.name).to eq(name) unless except_for == :name
       expect(updated_release.description).to eq(description) unless except_for == :description
       expect(updated_release.released_at).to eq(released_at) unless except_for == :released_at
-
-      # Right now the milestones are returned in a non-deterministic order.
-      # Because of this, we need to allow for milestones to be returned in any order.
-      # Once https://gitlab.com/gitlab-org/gitlab/-/issues/259012 has been
-      # fixed, this can be updated to expect a specific order.
-      expect(updated_release.milestones).to match_array([milestone_12_3, milestone_12_4]) unless except_for == :milestones
+      expect(updated_release.milestones.order_by_dates_and_title).to eq([milestone_12_3, milestone_12_4]) unless except_for == :milestones
     end
   end
 

spec/graphql/resolvers/release_milestones_resolver_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Resolvers::ReleaseMilestonesResolver do
+  include GraphqlHelpers
+
+  let_it_be(:release) { create(:release, :with_milestones, milestones_count: 2) }
+
+  let(:resolved) do
+    resolve(described_class, obj: release)
+  end
+
+  describe '#resolve' do
+    it "returns an OffsetActiveRecordRelationConnection" do
+      expect(resolved).to be_a(::Gitlab::Graphql::Pagination::OffsetActiveRecordRelationConnection)
+    end
+
+    it "includes the release's milestones in the returned OffsetActiveRecordRelationConnection" do
+      expect(resolved.items).to eq(release.milestones.order_by_dates_and_title)
+    end
+  end
+end

spec/models/release_spec.rb

@@ -3,9 +3,9 @@
 require 'spec_helper'
 
 RSpec.describe Release do
-  let(:user)    { create(:user) }
-  let(:project) { create(:project, :public, :repository) }
-  let(:release) { create(:release, project: project, author: user) }
+  let_it_be(:user)    { create(:user) }
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let_it_be(:release) { create(:release, project: project, author: user) }
 
   it { expect(release).to be_valid }
 
@@ -132,8 +132,10 @@ RSpec.describe Release do
   end
 
   describe '#milestone_titles' do
-    let(:release) { create(:release, :with_milestones) }
+    let_it_be(:milestone_1) { create(:milestone, project: project, title: 'Milestone 1') }
+    let_it_be(:milestone_2) { create(:milestone, project: project, title: 'Milestone 2') }
+    let_it_be(:release) { create(:release, project: project, milestones: [milestone_1, milestone_2]) }
 
-    it { expect(release.milestone_titles).to eq(release.milestones.map {|m| m.title }.sort.join(", "))}
+    it { expect(release.milestone_titles).to eq("#{milestone_1.title}, #{milestone_2.title}")}
   end
 end

spec/requests/api/graphql/mutations/releases/create_spec.rb

@@ -116,11 +116,9 @@ RSpec.describe 'Creation of a new release' do
     context 'when all available mutation arguments are provided' do
       it_behaves_like 'no errors'
 
-      # rubocop: disable CodeReuse/ActiveRecord
       it 'returns the new release data' do
         create_release
 
-        release = mutation_response[:release]
         expected_direct_asset_url = Gitlab::Routing.url_helpers.project_release_url(project, Release.find_by(tag: tag_name)) << "/downloads#{asset_link[:directAssetPath]}"
 
         expected_attributes = {
@@ -139,21 +137,17 @@ RSpec.describe 'Creation of a new release' do
                 directAssetUrl: expected_direct_asset_url
               }]
             }
+          },
+          milestones: {
+            nodes: [
+              { title: '12.3' },
+              { title: '12.4' }
+            ]
           }
-        }
-
-        expect(release).to include(expected_attributes)
+        }.with_indifferent_access
 
-        # Right now the milestones are returned in a non-deterministic order.
-        # This `milestones` test should be moved up into the expect(release)
-        # above (and `.to include` updated to `.to eq`) once
-        # https://gitlab.com/gitlab-org/gitlab/-/issues/259012 is addressed.
-        expect(release['milestones']['nodes']).to match_array([
-          { 'title' => '12.4' },
-          { 'title' => '12.3' }
-        ])
+        expect(mutation_response[:release]).to eq(expected_attributes)
       end
-      # rubocop: enable CodeReuse/ActiveRecord
     end
 
     context 'when only the required mutation arguments are provided' do

spec/requests/api/graphql/mutations/releases/update_spec.rb

@@ -116,15 +116,7 @@ RSpec.describe 'Updating an existing release' do
     it 'updates the correct field and returns the release' do
       update_release
 
-      expect(mutation_response[:release]).to include(expected_attributes.merge(updates).except(:milestones))
-
-      # Right now the milestones are returned in a non-deterministic order.
-      # Because of this, we need to test milestones separately to allow
-      # for them to be returned in any order.
-      # Once https://gitlab.com/gitlab-org/gitlab/-/issues/259012 has been
-      # fixed, this special milestone handling can be removed.
-      expected_milestones = expected_attributes.merge(updates)[:milestones]
-      expect(mutation_response[:release][:milestones][:nodes]).to match_array(expected_milestones[:nodes])
+      expect(mutation_response[:release]).to eq(expected_attributes.merge(updates))
     end
   end
 

spec/requests/api/graphql/project/release_spec.rb

@@ -76,11 +76,11 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
       it 'finds all milestones associated to a release' do
         post_query
 
-        expected = release.milestones.map do |milestone|
+        expected = release.milestones.order_by_dates_and_title.map do |milestone|
           { 'id' => global_id_of(milestone), 'title' => milestone.title }
         end
 
-        expect(data).to match_array(expected)
+        expect(data).to eq(expected)
       end
     end
 
@@ -427,4 +427,33 @@ RSpec.describe 'Query.project(fullPath).release(tagName)' do
       end
     end
   end
+
+  describe 'milestone order' do
+    let(:path) { path_prefix }
+    let(:current_user) { stranger }
+    let_it_be(:project) { create(:project, :public) }
+    let_it_be_with_reload(:release) { create(:release, project: project) }
+
+    let(:release_fields) do
+      query_graphql_field(%{
+        milestones {
+          nodes {
+            title
+          }
+        }
+      })
+    end
+
+    let(:actual_milestone_title_order) do
+      post_query
+
+      data.dig('milestones', 'nodes').map { |m| m['title'] }
+    end
+
+    before do
+      release.update!(milestones: [milestone_2, milestone_1])
+    end
+
+    it_behaves_like 'correct release milestone order'
+  end
 end

spec/requests/api/releases_spec.rb

@@ -16,9 +16,6 @@ RSpec.describe API::Releases do
     project.add_reporter(reporter)
     project.add_guest(guest)
     project.add_developer(developer)
-
-    project.repository.add_tag(maintainer, 'v0.1', commit.id)
-    project.repository.add_tag(maintainer, 'v0.2', commit.id)
   end
 
   describe 'GET /projects/:id/releases' do
@@ -294,6 +291,25 @@ RSpec.describe API::Releases do
         end
       end
 
+      context 'when release is associated to mutiple milestones' do
+        context 'milestones order' do
+          let_it_be(:project) { create(:project, :repository, :public) }
+          let_it_be_with_reload(:release_with_milestones) { create(:release, tag: 'v3.14', project: project) }
+
+          let(:actual_milestone_title_order) do
+            get api("/projects/#{project.id}/releases/#{release_with_milestones.tag}", non_project_member)
+
+            json_response['milestones'].map { |m| m['title'] }
+          end
+
+          before do
+            release_with_milestones.update!(milestones: [milestone_2, milestone_1])
+          end
+
+          it_behaves_like 'correct release milestone order'
+        end
+      end
+
       context 'when release has link asset' do
         let!(:link) do
           create(:release_link,
@@ -461,6 +477,10 @@ RSpec.describe API::Releases do
       }
     end
 
+    before do
+      initialize_tags
+    end
+
     it 'accepts the request' do
       post api("/projects/#{project.id}/releases", maintainer), params: params
 
@@ -858,6 +878,10 @@ RSpec.describe API::Releases do
              description: 'Super nice release')
     end
 
+    before do
+      initialize_tags
+    end
+
     it 'accepts the request' do
       put api("/projects/#{project.id}/releases/v0.1", maintainer), params: params
 
@@ -1108,4 +1132,9 @@ RSpec.describe API::Releases do
       end
     end
   end
+
+  def initialize_tags
+    project.repository.add_tag(maintainer, 'v0.1', commit.id)
+    project.repository.add_tag(maintainer, 'v0.2', commit.id)
+  end
 end

spec/support/shared_examples/requests/releases_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'correct release milestone order' do
+  let_it_be_with_reload(:milestone_1) { create(:milestone, project: project) }
+  let_it_be_with_reload(:milestone_2) { create(:milestone, project: project) }
+
+  shared_examples 'correct sort order' do
+    it 'sorts milestonee_1 before milestone_2' do
+      freeze_time do
+        expect(actual_milestone_title_order).to eq([milestone_1.title, milestone_2.title])
+      end
+    end
+  end
+
+  context 'due_date' do
+    before do
+      milestone_1.update!(due_date: Time.zone.now, start_date: 1.day.ago, title: 'z')
+      milestone_2.update!(due_date: 1.day.from_now, start_date: 2.days.ago, title: 'a')
+    end
+
+    context 'when both milestones have a due_date' do
+      it_behaves_like 'correct sort order'
+    end
+
+    context 'when one milestone does not have a due_date' do
+      before do
+        milestone_2.update!(due_date: nil)
+      end
+
+      it_behaves_like 'correct sort order'
+    end
+  end
+
+  context 'start_date' do
+    before do
+      milestone_1.update!(due_date: 1.day.from_now, start_date: 1.day.ago, title: 'z' )
+      milestone_2.update!(due_date: 1.day.from_now, start_date: milestone_2_start_date, title: 'a' )
+    end
+
+    context 'when both milestones have a start_date' do
+      let(:milestone_2_start_date) { Time.zone.now }
+
+      it_behaves_like 'correct sort order'
+    end
+
+    context 'when one milestone does not have a start_date' do
+      let(:milestone_2_start_date) { nil }
+
+      it_behaves_like 'correct sort order'
+    end
+  end
+
+  context 'title' do
+    before do
+      milestone_1.update!(due_date: 1.day.from_now, start_date: Time.zone.now, title: 'a' )
+      milestone_2.update!(due_date: 1.day.from_now, start_date: Time.zone.now, title: 'z' )
+    end
+
+    it_behaves_like 'correct sort order'
+  end
+end