summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--app/helpers/events_helper.rb9
-rw-r--r--app/models/concerns/each_batch.rb17
-rw-r--r--changelogs/unreleased/gt-use-merge-request-prefix-in-event-feed-title.yml5
-rw-r--r--changelogs/unreleased/sh-fix-issue-52649.yml5
-rw-r--r--db/post_migrate/20181107054254_remove_restricted_todos_again.rb32
-rw-r--r--db/schema.rb2
-rw-r--r--doc/user/markdown.md2
-rw-r--r--lib/gitlab/background_migration/remove_restricted_todos.rb2
-rw-r--r--lib/gitlab/utils.rb5
-rw-r--r--spec/helpers/events_helper_spec.rb46
-rw-r--r--spec/lib/gitlab/utils_spec.rb23
-rw-r--r--spec/models/concerns/each_batch_spec.rb51
12 files changed, 154 insertions, 45 deletions
diff --git a/app/helpers/events_helper.rb b/app/helpers/events_helper.rb
index 2adfc04deb8..3ce2398f1de 100644
--- a/app/helpers/events_helper.rb
+++ b/app/helpers/events_helper.rb
@@ -91,7 +91,14 @@ module EventsHelper
words << "##{event.target_iid}" if event.target_iid
words << "in"
elsif event.target
- words << "##{event.target_iid}:"
+ prefix =
+ if event.merge_request?
+ MergeRequest.reference_prefix
+ else
+ Issue.reference_prefix
+ end
+
+ words << "#{prefix}#{event.target_iid}:" if event.target_iid
words << event.target.title if event.target.respond_to?(:title)
words << "at"
end
diff --git a/app/models/concerns/each_batch.rb b/app/models/concerns/each_batch.rb
index 8cf0b8b154d..6314b46a7e3 100644
--- a/app/models/concerns/each_batch.rb
+++ b/app/models/concerns/each_batch.rb
@@ -39,7 +39,15 @@ module EachBatch
#
# of - The number of rows to retrieve per batch.
# column - The column to use for ordering the batches.
- def each_batch(of: 1000, column: primary_key)
+ # order_hint - An optional column to append to the `ORDER BY id`
+ # clause to help the query planner. PostgreSQL might perform badly
+ # with a LIMIT 1 because the planner is guessing that scanning the
+ # index in ID order will come across the desired row in less time
+ # it will take the planner than using another index. The
+ # order_hint does not affect the search results. For example,
+ # `ORDER BY id ASC, updated_at ASC` means the same thing as `ORDER
+ # BY id ASC`.
+ def each_batch(of: 1000, column: primary_key, order_hint: nil)
unless column
raise ArgumentError,
'the column: argument must be set to a column name to use for ordering rows'
@@ -48,7 +56,9 @@ module EachBatch
start = except(:select)
.select(column)
.reorder(column => :asc)
- .take
+
+ start = start.order(order_hint) if order_hint
+ start = start.take
return unless start
@@ -60,6 +70,9 @@ module EachBatch
.select(column)
.where(arel_table[column].gteq(start_id))
.reorder(column => :asc)
+
+ stop = stop.order(order_hint) if order_hint
+ stop = stop
.offset(of)
.limit(1)
.take
diff --git a/changelogs/unreleased/gt-use-merge-request-prefix-in-event-feed-title.yml b/changelogs/unreleased/gt-use-merge-request-prefix-in-event-feed-title.yml
new file mode 100644
index 00000000000..51af2807a03
--- /dev/null
+++ b/changelogs/unreleased/gt-use-merge-request-prefix-in-event-feed-title.yml
@@ -0,0 +1,5 @@
+---
+title: Use merge request prefix symbol in event feed title
+merge_request: 22449
+author: George Tsiolis
+type: changed
diff --git a/changelogs/unreleased/sh-fix-issue-52649.yml b/changelogs/unreleased/sh-fix-issue-52649.yml
new file mode 100644
index 00000000000..34b7f74a345
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-issue-52649.yml
@@ -0,0 +1,5 @@
+---
+title: Fix statement timeouts in RemoveRestrictedTodos migration
+merge_request: 22795
+author:
+type: other
diff --git a/db/post_migrate/20181107054254_remove_restricted_todos_again.rb b/db/post_migrate/20181107054254_remove_restricted_todos_again.rb
new file mode 100644
index 00000000000..644e0074c46
--- /dev/null
+++ b/db/post_migrate/20181107054254_remove_restricted_todos_again.rb
@@ -0,0 +1,32 @@
+# frozen_string_literal: true
+
+# rescheduling of the revised RemoveRestrictedTodosWithCte background migration
+class RemoveRestrictedTodosAgain < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+ disable_ddl_transaction!
+
+ MIGRATION = 'RemoveRestrictedTodos'.freeze
+ BATCH_SIZE = 1000
+ DELAY_INTERVAL = 5.minutes.to_i
+
+ class Project < ActiveRecord::Base
+ include EachBatch
+
+ self.table_name = 'projects'
+ end
+
+ def up
+ Project.where('EXISTS (SELECT 1 FROM todos WHERE todos.project_id = projects.id)')
+ .each_batch(of: BATCH_SIZE) do |batch, index|
+ range = batch.pluck('MIN(id)', 'MAX(id)').first
+
+ BackgroundMigrationWorker.perform_in(index * DELAY_INTERVAL, MIGRATION, range)
+ end
+ end
+
+ def down
+ # nothing to do
+ end
+end
diff --git a/db/schema.rb b/db/schema.rb
index fbe09cf1611..df3067fae30 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
-ActiveRecord::Schema.define(version: 20181106135939) do
+ActiveRecord::Schema.define(version: 20181107054254) do
# These are extensions that must be enabled in order to support this database
enable_extension "plpgsql"
diff --git a/doc/user/markdown.md b/doc/user/markdown.md
index 96a509c4b21..93aa41e9a98 100644
--- a/doc/user/markdown.md
+++ b/doc/user/markdown.md
@@ -774,7 +774,7 @@ These details <em>will</em> remain <strong>hidden</strong> until expanded.
</details>
</p>
-**Note:** Markdown inside these tags is supported, as long as you have a blank link after the `</summary>` tag and before the `</details>` tag, as shown in the example. _Redcarpet does not support Markdown inside these tags. You can work around this by using HTML, for example you can use `<pre><code>` tags instead of [code fences](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/user/markdown.md#code-and-syntax-highlighting)._
+**Note:** Markdown inside these tags is supported, as long as you have a blank line after the `</summary>` tag and before the `</details>` tag, as shown in the example. _Redcarpet does not support Markdown inside these tags. You can work around this by using HTML, for example you can use `<pre><code>` tags instead of [code fences](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/user/markdown.md#code-and-syntax-highlighting)._
```html
<details>
diff --git a/lib/gitlab/background_migration/remove_restricted_todos.rb b/lib/gitlab/background_migration/remove_restricted_todos.rb
index 9941c2fe6d9..47579d46c1b 100644
--- a/lib/gitlab/background_migration/remove_restricted_todos.rb
+++ b/lib/gitlab/background_migration/remove_restricted_todos.rb
@@ -67,7 +67,7 @@ module Gitlab
.where('access_level >= ?', 20)
confidential_issues = Issue.select(:id, :author_id).where(confidential: true, project_id: project_id)
- confidential_issues.each_batch(of: 100) do |batch|
+ confidential_issues.each_batch(of: 100, order_hint: :confidential) do |batch|
batch.each do |issue|
assigned_users = IssueAssignee.select(:user_id).where(issue_id: issue.id)
diff --git a/lib/gitlab/utils.rb b/lib/gitlab/utils.rb
index 2c92458f777..9e59137a2c0 100644
--- a/lib/gitlab/utils.rb
+++ b/lib/gitlab/utils.rb
@@ -16,6 +16,11 @@ module Gitlab
str.force_encoding(Encoding::UTF_8)
end
+ # Append path to host, making sure there's one single / in between
+ def append_path(host, path)
+ "#{host.to_s.sub(%r{\/+$}, '')}/#{path.to_s.sub(%r{^\/+}, '')}"
+ end
+
# A slugified version of the string, suitable for inclusion in URLs and
# domain names. Rules:
#
diff --git a/spec/helpers/events_helper_spec.rb b/spec/helpers/events_helper_spec.rb
index 466e018d68c..8d0679e5699 100644
--- a/spec/helpers/events_helper_spec.rb
+++ b/spec/helpers/events_helper_spec.rb
@@ -2,18 +2,18 @@ require 'spec_helper'
describe EventsHelper do
describe '#event_commit_title' do
- let(:message) { "foo & bar " + "A" * 70 + "\n" + "B" * 80 }
+ let(:message) { 'foo & bar ' + 'A' * 70 + '\n' + 'B' * 80 }
subject { helper.event_commit_title(message) }
- it "returns the first line, truncated to 70 chars" do
+ it 'returns the first line, truncated to 70 chars' do
is_expected.to eq(message[0..66] + "...")
end
- it "is not html-safe" do
+ it 'is not html-safe' do
is_expected.not_to be_a(ActiveSupport::SafeBuffer)
end
- it "handles empty strings" do
+ it 'handles empty strings' do
expect(helper.event_commit_title("")).to eq("")
end
@@ -22,7 +22,7 @@ describe EventsHelper do
end
it 'does not escape HTML entities' do
- expect(helper.event_commit_title("foo & bar")).to eq("foo & bar")
+ expect(helper.event_commit_title('foo & bar')).to eq('foo & bar')
end
end
@@ -30,38 +30,54 @@ describe EventsHelper do
let(:event) { create(:event) }
let(:project) { create(:project, :public, :repository) }
- it "returns project issue url" do
- event.target = create(:issue)
+ context 'issue' do
+ before do
+ event.target = create(:issue)
+ end
- expect(helper.event_feed_url(event)).to eq(project_issue_url(event.project, event.issue))
+ it 'returns the project issue url' do
+ expect(helper.event_feed_url(event)).to eq(project_issue_url(event.project, event.target))
+ end
+
+ it 'contains the project issue IID link' do
+ expect(helper.event_feed_title(event)).to include("##{event.target.iid}")
+ end
end
- it "returns project merge_request url" do
- event.target = create(:merge_request)
+ context 'merge request' do
+ before do
+ event.target = create(:merge_request)
+ end
+
+ it 'returns the project merge request url' do
+ expect(helper.event_feed_url(event)).to eq(project_merge_request_url(event.project, event.target))
+ end
- expect(helper.event_feed_url(event)).to eq(project_merge_request_url(event.project, event.merge_request))
+ it 'contains the project merge request IID link' do
+ expect(helper.event_feed_title(event)).to include("!#{event.target.iid}")
+ end
end
- it "returns project commit url" do
+ it 'returns project commit url' do
event.target = create(:note_on_commit, project: project)
expect(helper.event_feed_url(event)).to eq(project_commit_url(event.project, event.note_target))
end
- it "returns event note target url" do
+ it 'returns event note target url' do
event.target = create(:note)
expect(helper.event_feed_url(event)).to eq(event_note_target_url(event))
end
- it "returns project url" do
+ it 'returns project url' do
event.project = project
event.action = 1
expect(helper.event_feed_url(event)).to eq(project_url(event.project))
end
- it "returns push event feed url" do
+ it 'returns push event feed url' do
event = create(:push_event)
create(:push_event_payload, event: event, action: :pushed)
diff --git a/spec/lib/gitlab/utils_spec.rb b/spec/lib/gitlab/utils_spec.rb
index 4ba99009855..ad2c9d7f2af 100644
--- a/spec/lib/gitlab/utils_spec.rb
+++ b/spec/lib/gitlab/utils_spec.rb
@@ -2,7 +2,7 @@ require 'spec_helper'
describe Gitlab::Utils do
delegate :to_boolean, :boolean_to_yes_no, :slugify, :random_string, :which, :ensure_array_from_string,
- :bytes_to_megabytes, to: :described_class
+ :bytes_to_megabytes, :append_path, to: :described_class
describe '.slugify' do
{
@@ -106,4 +106,25 @@ describe Gitlab::Utils do
expect(bytes_to_megabytes(bytes)).to eq(1)
end
end
+
+ describe '.append_path' do
+ using RSpec::Parameterized::TableSyntax
+
+ where(:host, :path, :result) do
+ 'http://test/' | '/foo/bar' | 'http://test/foo/bar'
+ 'http://test/' | '//foo/bar' | 'http://test/foo/bar'
+ 'http://test//' | '/foo/bar' | 'http://test/foo/bar'
+ 'http://test' | 'foo/bar' | 'http://test/foo/bar'
+ 'http://test//' | '' | 'http://test/'
+ 'http://test//' | nil | 'http://test/'
+ '' | '/foo/bar' | '/foo/bar'
+ nil | '/foo/bar' | '/foo/bar'
+ end
+
+ with_them do
+ it 'makes sure there is only one slash as path separator' do
+ expect(append_path(host, path)).to eq(result)
+ end
+ end
+ end
end
diff --git a/spec/models/concerns/each_batch_spec.rb b/spec/models/concerns/each_batch_spec.rb
index 951690a217b..17224c09693 100644
--- a/spec/models/concerns/each_batch_spec.rb
+++ b/spec/models/concerns/each_batch_spec.rb
@@ -14,40 +14,45 @@ describe EachBatch do
5.times { create(:user, updated_at: 1.day.ago) }
end
- it 'yields an ActiveRecord::Relation when a block is given' do
- model.each_batch do |relation|
- expect(relation).to be_a_kind_of(ActiveRecord::Relation)
+ shared_examples 'each_batch handling' do |kwargs|
+ it 'yields an ActiveRecord::Relation when a block is given' do
+ model.each_batch(kwargs) do |relation|
+ expect(relation).to be_a_kind_of(ActiveRecord::Relation)
+ end
end
- end
- it 'yields a batch index as the second argument' do
- model.each_batch do |_, index|
- expect(index).to eq(1)
+ it 'yields a batch index as the second argument' do
+ model.each_batch(kwargs) do |_, index|
+ expect(index).to eq(1)
+ end
end
- end
- it 'accepts a custom batch size' do
- amount = 0
+ it 'accepts a custom batch size' do
+ amount = 0
- model.each_batch(of: 1) { amount += 1 }
+ model.each_batch(kwargs.merge({ of: 1 })) { amount += 1 }
- expect(amount).to eq(5)
- end
+ expect(amount).to eq(5)
+ end
- it 'does not include ORDER BYs in the yielded relations' do
- model.each_batch do |relation|
- expect(relation.to_sql).not_to include('ORDER BY')
+ it 'does not include ORDER BYs in the yielded relations' do
+ model.each_batch do |relation|
+ expect(relation.to_sql).not_to include('ORDER BY')
+ end
end
- end
- it 'allows updating of the yielded relations' do
- time = Time.now
+ it 'allows updating of the yielded relations' do
+ time = Time.now
- model.each_batch do |relation|
- relation.update_all(updated_at: time)
- end
+ model.each_batch do |relation|
+ relation.update_all(updated_at: time)
+ end
- expect(model.where(updated_at: time).count).to eq(5)
+ expect(model.where(updated_at: time).count).to eq(5)
+ end
end
+
+ it_behaves_like 'each_batch handling', {}
+ it_behaves_like 'each_batch handling', { order_hint: :updated_at }
end
end
View Lorry Controller Status