diff options
21 files changed, 148 insertions, 128 deletions
diff --git a/Gemfile.lock b/Gemfile.lock index d46550a252d..169b30c2c6e 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -434,7 +434,7 @@ GEM multi_json (~> 1.11) os (>= 0.9, < 2.0) signet (~> 0.7) - gpgme (2.0.19) + gpgme (2.0.20) mini_portile2 (~> 2.3) grape (1.1.0) activesupport diff --git a/app/helpers/milestones_helper.rb b/app/helpers/milestones_helper.rb index a723269ea0e..da6a0e38c44 100644 --- a/app/helpers/milestones_helper.rb +++ b/app/helpers/milestones_helper.rb @@ -92,12 +92,12 @@ module MilestonesHelper end def milestone_progress_tooltip_text(milestone) - has_issues = milestone.total_issues_count(current_user) > 0 + has_issues = milestone.total_issues_count > 0 if has_issues [ _('Progress'), - _("%{percent}%% complete") % { percent: milestone.percent_complete(current_user) } + _("%{percent}%% complete") % { percent: milestone.percent_complete } ].join('<br />') else _('Progress') @@ -107,7 +107,7 @@ module MilestonesHelper def milestone_progress_bar(milestone) options = { class: 'progress-bar bg-success', - style: "width: #{milestone.percent_complete(current_user)}%;" + style: "width: #{milestone.percent_complete}%;" } content_tag :div, class: 'progress' do @@ -151,9 +151,9 @@ module MilestonesHelper end def milestone_issues_tooltip_text(milestone) - total = milestone.total_issues_count(current_user) - opened = milestone.opened_issues_count(current_user) - closed = milestone.closed_issues_count(current_user) + total = milestone.total_issues_count + opened = milestone.opened_issues_count + closed = milestone.closed_issues_count return _("Issues") if total.zero? diff --git a/app/models/concerns/milestoneish.rb b/app/models/concerns/milestoneish.rb index 045e893761a..6dbb9649b9f 100644 --- a/app/models/concerns/milestoneish.rb +++ b/app/models/concerns/milestoneish.rb @@ -1,36 +1,24 @@ # frozen_string_literal: true module Milestoneish - def total_issues_count(user) - @total_issues_count ||= - if Feature.enabled?(:cached_milestone_issue_counters) - Milestones::IssuesCountService.new(self).count - else - count_issues_by_state(user).values.sum - end + def total_issues_count + @total_issues_count ||= Milestones::IssuesCountService.new(self).count end - def closed_issues_count(user) - @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 + def closed_issues_count + @close_issues_count ||= Milestones::ClosedIssuesCountService.new(self).count end - def opened_issues_count(user) - total_issues_count(user) - closed_issues_count(user) + def opened_issues_count + total_issues_count - closed_issues_count end - def complete?(user) - total_issues_count(user) > 0 && total_issues_count(user) == closed_issues_count(user) + def complete? + total_issues_count > 0 && total_issues_count == closed_issues_count end - def percent_complete(user) - closed_issues_count(user) * 100 / total_issues_count(user) + def percent_complete + closed_issues_count * 100 / total_issues_count rescue ZeroDivisionError 0 end @@ -135,12 +123,6 @@ module Milestoneish Gitlab::TimeTrackingFormatter.output(total_issue_time_estimate) end - def count_issues_by_state(user) - memoize_per_user(user, :count_issues_by_state) do - issues_visible_to_user(user).reorder(nil).group(:state_id).count - end - end - private def memoize_per_user(user, method_name) diff --git a/app/views/projects/milestones/show.html.haml b/app/views/projects/milestones/show.html.haml index 5f244d3a6c3..b83204c27e3 100644 --- a/app/views/projects/milestones/show.html.haml +++ b/app/views/projects/milestones/show.html.haml @@ -8,10 +8,10 @@ = render_if_exists 'shared/milestones/burndown', milestone: @milestone, project: @project -- if can?(current_user, :read_issue, @project) && @milestone.total_issues_count(current_user).zero? +- if can?(current_user, :read_issue, @project) && @milestone.total_issues_count.zero? .alert.alert-success.prepend-top-default %span= _('Assign some issues to this milestone.') -- elsif @milestone.complete?(current_user) && @milestone.active? +- elsif @milestone.complete? && @milestone.active? .alert.alert-success.prepend-top-default %span= _('All issues for this milestone are closed. You may close this milestone now.') diff --git a/app/views/shared/milestones/_milestone.html.haml b/app/views/shared/milestones/_milestone.html.haml index 6e50b31fd71..451c2c2ba10 100644 --- a/app/views/shared/milestones/_milestone.html.haml +++ b/app/views/shared/milestones/_milestone.html.haml @@ -42,11 +42,11 @@ .col-sm-4.milestone-progress = milestone_progress_bar(milestone) - = link_to pluralize(milestone.total_issues_count(current_user), 'Issue'), issues_path + = link_to pluralize(milestone.total_issues_count, 'Issue'), issues_path - if milestone.merge_requests_enabled? · = link_to pluralize(milestone.merge_requests_visible_to_user(current_user).size, 'Merge Request'), merge_requests_path - .float-lg-right.light #{milestone.percent_complete(current_user)}% complete + .float-lg-right.light #{milestone.percent_complete}% complete .col-sm-2 .milestone-actions.d-flex.justify-content-sm-start.justify-content-md-end - if @project diff --git a/app/views/shared/milestones/_sidebar.html.haml b/app/views/shared/milestones/_sidebar.html.haml index a6fb8e6d4fc..aa9c4be1cc1 100644 --- a/app/views/shared/milestones/_sidebar.html.haml +++ b/app/views/shared/milestones/_sidebar.html.haml @@ -7,7 +7,7 @@ %a.gutter-toggle.float-right.js-sidebar-toggle.has-tooltip{ role: "button", href: "#", "aria-label" => "Toggle sidebar", title: sidebar_gutter_tooltip_text, data: { container: 'body', placement: 'left', boundary: 'viewport' } } = sidebar_gutter_toggle_icon .title.hide-collapsed - %strong.bold== #{milestone.percent_complete(current_user)}% + %strong.bold== #{milestone.percent_complete}% %span.hide-collapsed complete .value.hide-collapsed @@ -15,7 +15,7 @@ .block.milestone-progress.hide-expanded .sidebar-collapsed-icon.has-tooltip{ title: milestone_progress_tooltip_text(milestone), data: { container: 'body', html: 'true', placement: 'left', boundary: 'viewport' } } - %span== #{milestone.percent_complete(current_user)}% + %span== #{milestone.percent_complete}% = milestone_progress_bar(milestone) .block.start_date.hide-collapsed diff --git a/app/views/shared/milestones/_top.html.haml b/app/views/shared/milestones/_top.html.haml index 12575b30a6c..8d911d4247e 100644 --- a/app/views/shared/milestones/_top.html.haml +++ b/app/views/shared/milestones/_top.html.haml @@ -8,7 +8,7 @@ = render 'shared/milestones/deprecation_message' if is_dynamic_milestone = render 'shared/milestones/description', milestone: milestone -- if milestone.complete?(current_user) && milestone.active? +- if milestone.complete? && milestone.active? .alert.alert-success.prepend-top-default %span = _('All issues for this milestone are closed.') diff --git a/changelogs/unreleased/208891-optimize-todos-counters.yml b/changelogs/unreleased/208891-optimize-todos-counters.yml new file mode 100644 index 00000000000..85f1a123f78 --- /dev/null +++ b/changelogs/unreleased/208891-optimize-todos-counters.yml @@ -0,0 +1,5 @@ +--- +title: Optimize todos counters in usage data +merge_request: 26442 +author: +type: performance diff --git a/changelogs/unreleased/31289-show-issue-summary-on-releases-page.yml b/changelogs/unreleased/31289-show-issue-summary-on-releases-page.yml new file mode 100644 index 00000000000..ff17ee0a3cb --- /dev/null +++ b/changelogs/unreleased/31289-show-issue-summary-on-releases-page.yml @@ -0,0 +1,5 @@ +--- +title: Cache milestone issue counters and make them independent of user permissions +merge_request: 21554 +author: +type: performance diff --git a/changelogs/unreleased/replace-undefined-with-unknown.yml b/changelogs/unreleased/replace-undefined-with-unknown.yml new file mode 100644 index 00000000000..38f1ac169f8 --- /dev/null +++ b/changelogs/unreleased/replace-undefined-with-unknown.yml @@ -0,0 +1,5 @@ +--- +title: Replace undefined severity with unknown severity for occurrences +merge_request: 26085 +author: +type: other diff --git a/db/migrate/20200225123228_insert_project_subscriptions_plan_limits.rb b/db/migrate/20200225123228_insert_project_subscriptions_plan_limits.rb index 4a05e2cd779..f04e0c68cf6 100644 --- a/db/migrate/20200225123228_insert_project_subscriptions_plan_limits.rb +++ b/db/migrate/20200225123228_insert_project_subscriptions_plan_limits.rb @@ -6,28 +6,20 @@ class InsertProjectSubscriptionsPlanLimits < ActiveRecord::Migration[6.0] DOWNTIME = false def up - return if Rails.env.test? + return unless Gitlab.com? - if Gitlab.com? - create_or_update_plan_limit('ci_project_subscriptions', 'free', 2) - create_or_update_plan_limit('ci_project_subscriptions', 'bronze', 2) - create_or_update_plan_limit('ci_project_subscriptions', 'silver', 2) - create_or_update_plan_limit('ci_project_subscriptions', 'gold', 2) - else - create_or_update_plan_limit('ci_project_subscriptions', 'default', 2) - end + create_or_update_plan_limit('ci_project_subscriptions', 'free', 2) + create_or_update_plan_limit('ci_project_subscriptions', 'bronze', 2) + create_or_update_plan_limit('ci_project_subscriptions', 'silver', 2) + create_or_update_plan_limit('ci_project_subscriptions', 'gold', 2) end def down - return if Rails.env.test? + return unless Gitlab.com? - if Gitlab.com? - create_or_update_plan_limit('ci_project_subscriptions', 'free', 0) - create_or_update_plan_limit('ci_project_subscriptions', 'bronze', 0) - create_or_update_plan_limit('ci_project_subscriptions', 'silver', 0) - create_or_update_plan_limit('ci_project_subscriptions', 'gold', 0) - else - create_or_update_plan_limit('ci_project_subscriptions', 'default', 0) - end + create_or_update_plan_limit('ci_project_subscriptions', 'free', 0) + create_or_update_plan_limit('ci_project_subscriptions', 'bronze', 0) + create_or_update_plan_limit('ci_project_subscriptions', 'silver', 0) + create_or_update_plan_limit('ci_project_subscriptions', 'gold', 0) end end diff --git a/db/migrate/20200306170531_add_index_on_author_id_and_created_at_to_todos.rb b/db/migrate/20200306170531_add_index_on_author_id_and_created_at_to_todos.rb new file mode 100644 index 00000000000..d0d31ca7c52 --- /dev/null +++ b/db/migrate/20200306170531_add_index_on_author_id_and_created_at_to_todos.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true +class AddIndexOnAuthorIdAndCreatedAtToTodos < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :todos, [:author_id, :created_at] + end + + def down + remove_concurrent_index :todos, [:author_id, :created_at] + end +end diff --git a/db/post_migrate/20200227140242_update_occurrence_severity_column.rb b/db/post_migrate/20200227140242_update_occurrence_severity_column.rb new file mode 100644 index 00000000000..6d250532383 --- /dev/null +++ b/db/post_migrate/20200227140242_update_occurrence_severity_column.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class UpdateOccurrenceSeverityColumn < ActiveRecord::Migration[6.0] + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + BATCH_SIZE = 1_000 + INTERVAL = 5.minutes + + # 23_044 records to be updated on GitLab.com, + def up + # create temporary index for undefined vulnerabilities + add_concurrent_index(:vulnerability_occurrences, :id, where: 'severity = 0', name: 'undefined_vulnerabilities') + + return unless Gitlab.ee? + + migration = Gitlab::BackgroundMigration::RemoveUndefinedOccurrenceSeverityLevel + migration_name = migration.to_s.demodulize + relation = migration::Occurrence.undefined_severity + queue_background_migration_jobs_by_range_at_intervals(relation, + migration_name, + INTERVAL, + batch_size: BATCH_SIZE) + end + + def down + # no-op + # temporary index is to be dropped in a different migration in an upcoming release + remove_concurrent_index(:vulnerability_occurrences, :id, where: 'severity = 0', name: 'undefined_vulnerabilities') + # This migration can not be reversed because we can not know which records had undefined severity + end +end diff --git a/db/schema.rb b/db/schema.rb index 8cfa949548f..b49c68458ca 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2020_03_04_160823) do +ActiveRecord::Schema.define(version: 2020_03_06_170531) do # These are extensions that must be enabled in order to support this database enable_extension "pg_trgm" @@ -4150,6 +4150,7 @@ ActiveRecord::Schema.define(version: 2020_03_04_160823) do t.integer "note_id" t.string "commit_id" t.integer "group_id" + t.index ["author_id", "created_at"], name: "index_todos_on_author_id_and_created_at" t.index ["author_id"], name: "index_todos_on_author_id" t.index ["commit_id"], name: "index_todos_on_commit_id" t.index ["group_id"], name: "index_todos_on_group_id" @@ -4540,6 +4541,7 @@ ActiveRecord::Schema.define(version: 2020_03_04_160823) do t.string "metadata_version", null: false t.text "raw_metadata", null: false t.bigint "vulnerability_id" + t.index ["id"], name: "undefined_vulnerabilities", where: "(severity = 0)" t.index ["primary_identifier_id"], name: "index_vulnerability_occurrences_on_primary_identifier_id" t.index ["project_id", "primary_identifier_id", "location_fingerprint", "scanner_id"], name: "index_vulnerability_occurrences_on_unique_keys", unique: true t.index ["scanner_id"], name: "index_vulnerability_occurrences_on_scanner_id" diff --git a/doc/development/application_limits.md b/doc/development/application_limits.md index 81ccebbd690..c378019c4f6 100644 --- a/doc/development/application_limits.md +++ b/doc/development/application_limits.md @@ -33,18 +33,28 @@ limit values. It's recommended to create separate migration script files. `create_or_update_plan_limit` migration helper, eg: ```ruby - create_or_update_plan_limit('project_hooks', 'free', 10) - create_or_update_plan_limit('project_hooks', 'bronze', 20) - create_or_update_plan_limit('project_hooks', 'silver', 30) - create_or_update_plan_limit('project_hooks', 'gold', 100) + def up + return unless Gitlab.com? + + create_or_update_plan_limit('project_hooks', 'free', 100) + create_or_update_plan_limit('project_hooks', 'bronze', 100) + create_or_update_plan_limit('project_hooks', 'silver', 100) + create_or_update_plan_limit('project_hooks', 'gold', 100) + end + + def down + return unless Gitlab.com? + + create_or_update_plan_limit('project_hooks', 'free', 0) + create_or_update_plan_limit('project_hooks', 'bronze', 0) + create_or_update_plan_limit('project_hooks', 'silver', 0) + create_or_update_plan_limit('project_hooks', 'gold', 0) + end ``` NOTE: **Note:** Some plans exist only on GitLab.com. You can check if the migration is running on GitLab.com with `Gitlab.com?`. -NOTE: **Note:** The test environment doesn't have any plans. You can check if a -migration is running in a test environment with `Rails.env.test?` - ### Plan limits validation #### Get current limit diff --git a/doc/development/experiment_guide/index.md b/doc/development/experiment_guide/index.md index 98086fc63ea..b7f497ecd7a 100644 --- a/doc/development/experiment_guide/index.md +++ b/doc/development/experiment_guide/index.md @@ -2,7 +2,7 @@ Experiments will be conducted by teams from the [Growth Section](https://about.gitlab.com/handbook/engineering/development/growth/) and are not tied to releases, because they will primarily target GitLab.com. -Experiments will be run as an A/B test and will be behind a feature flag to turn the test on or off. Based on the data the experiment generates, the team decides if the experiment had a positive impact and will be the new default or rolled back. +Experiments will be run as an A/B test and will be behind a feature flag to turn the test on or off. Based on the data the experiment generates, the team will decide if the experiment had a positive impact and will be the new default or rolled back. ## Follow-up issue diff --git a/doc/subscriptions/index.md b/doc/subscriptions/index.md index 2089a9dfaf8..68dca96bdb8 100644 --- a/doc/subscriptions/index.md +++ b/doc/subscriptions/index.md @@ -334,7 +334,7 @@ Be aware that: we will calculate a pro-rated charge for your paid plan. That means you may be charged for less than one year since your subscription was previously created with the extra CI minutes. -- Once the extra CI minutes has been assigned to a Group they can't be transferred +- Once the extra CI minutes have been assigned to a Group, they can't be transferred to a different Group. - If you have used more minutes than your default quota, these minutes will be deducted from your Additional Minutes quota immediately after your purchase of additional diff --git a/doc/user/index.md b/doc/user/index.md index ec8a53b842d..fdcc87aca0b 100644 --- a/doc/user/index.md +++ b/doc/user/index.md @@ -14,7 +14,7 @@ and upgrade your GitLab instance. Admin privileges for [GitLab.com](https://gitlab.com/) are restricted to the GitLab team. -For more information on configuring GitLab self-managed instances, see [Administrator documentation](../administration/index.md). +For more information on configuring GitLab self-managed instances, see the [Administrator documentation](../administration/index.md). ## Overview diff --git a/lib/gitlab/background_migration/remove_undefined_occurrence_severity_level.rb b/lib/gitlab/background_migration/remove_undefined_occurrence_severity_level.rb new file mode 100644 index 00000000000..f137e41c728 --- /dev/null +++ b/lib/gitlab/background_migration/remove_undefined_occurrence_severity_level.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +# rubocop:disable Style/Documentation + +module Gitlab + module BackgroundMigration + class RemoveUndefinedOccurrenceSeverityLevel + def perform(start_id, stop_id) + end + end + end +end + +Gitlab::BackgroundMigration::RemoveUndefinedOccurrenceSeverityLevel.prepend_if_ee('EE::Gitlab::BackgroundMigration::RemoveUndefinedOccurrenceSeverityLevel') diff --git a/spec/models/concerns/milestoneish_spec.rb b/spec/models/concerns/milestoneish_spec.rb index 71a18e2fbec..cff607a4731 100644 --- a/spec/models/concerns/milestoneish_spec.rb +++ b/spec/models/concerns/milestoneish_spec.rb @@ -213,7 +213,7 @@ describe Milestone, 'Milestoneish' 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 + expect(milestone.complete?).to eq false end it 'returns true when all items are closed' do @@ -221,7 +221,7 @@ describe Milestone, 'Milestoneish' do security_issue_1.close security_issue_2.close - expect(milestone.complete?(non_member)).to eq true + expect(milestone.complete?).to eq true end end @@ -229,63 +229,19 @@ describe Milestone, 'Milestoneish' do context 'division by zero' do let(:new_milestone) { build_stubbed(:milestone) } - it { expect(new_milestone.percent_complete(admin)).to eq(0) } + it { expect(new_milestone.percent_complete).to eq(0) } end end - describe '#count_issues_by_state' do - 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 - - 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 + describe '#closed_issues_count' do + it 'counts all closed issues including confidential' do + expect(milestone.closed_issues_count).to eq 6 end + end - 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 - - context 'when cached_milestone_issue_counters are disabled' do - before do - stub_feature_flags(cached_milestone_issue_counters: false) - 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 '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 + describe '#total_issues_count' do + it 'counts all issues including confidential' do + expect(milestone.total_issues_count).to eq 9 end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index de79156562d..ee4c35ebddd 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -231,17 +231,17 @@ describe Milestone do describe "#percent_complete" do it "does not count open issues" do milestone.issues << issue - expect(milestone.percent_complete(user)).to eq(0) + expect(milestone.percent_complete).to eq(0) end it "counts closed issues" do issue.close milestone.issues << issue - expect(milestone.percent_complete(user)).to eq(100) + expect(milestone.percent_complete).to eq(100) end it "recovers from dividing by zero" do - expect(milestone.percent_complete(user)).to eq(0) + expect(milestone.percent_complete).to eq(0) end end |