diff options
| -rw-r--r-- | CHANGELOG | 5 | ||||
| -rw-r--r-- | GITLAB_SHELL_VERSION | 2 | ||||
| -rw-r--r-- | Gemfile | 2 | ||||
| -rw-r--r-- | Gemfile.lock | 4 | ||||
| -rw-r--r-- | app/assets/javascripts/users_select.js | 5 | ||||
| -rw-r--r-- | app/assets/stylesheets/pages/merge_requests.scss | 10 | ||||
| -rw-r--r-- | app/helpers/button_helper.rb | 4 | ||||
| -rw-r--r-- | app/helpers/merge_requests_helper.rb | 4 | ||||
| -rw-r--r-- | app/models/compare.rb | 21 | ||||
| -rw-r--r-- | app/models/merge_request_diff.rb | 7 | ||||
| -rw-r--r-- | app/services/compare_service.rb | 7 | ||||
| -rw-r--r-- | app/views/projects/merge_requests/show/_versions.html.haml | 10 | ||||
| -rw-r--r-- | doc/administration/troubleshooting/debug.md | 4 | ||||
| -rw-r--r-- | doc/development/frontend.md | 11 | ||||
| -rw-r--r-- | spec/models/appearance_spec.rb | 2 | ||||
| -rw-r--r-- | spec/models/deploy_key_spec.rb | 3 | ||||
| -rw-r--r-- | spec/models/event_spec.rb | 110 | ||||
| -rw-r--r-- | spec/models/merge_request_diff_spec.rb | 46 | ||||
| -rw-r--r-- | spec/services/compare_service_spec.rb | 21 |
19 files changed, 185 insertions, 93 deletions
diff --git a/CHANGELOG b/CHANGELOG index 1d10d86212f..e13e0eef87a 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -4,9 +4,10 @@ v 8.13.0 (unreleased) - Improve Merge When Build Succeeds triggers and execute on pipeline success. (!6675) - Respond with 404 Not Found for non-existent tags (Linus Thiel) - Truncate long labels with ellipsis in labels page + - Adding members no longer silently fails when there is extra whitespace - Update runner version only when updating contacted_at - Add link from system note to compare with previous version - - Use gitlab-shell v3.6.2 (GIT TRACE logging) + - Use gitlab-shell v3.6.6 - Add `/projects/visible` API endpoint (Ben Boeckel) - Fix centering of custom header logos (Ashley Dumaine) - ExpireBuildArtifactsWorker query builds table without ordering enqueuing one job per build to cleanup @@ -103,6 +104,7 @@ v 8.13.0 (unreleased) - Allow empty merge requests !6384 (Artem Sidorenko) - Grouped pipeline dropdown is a scrollable container - Cleanup Ci::ApplicationController. !6757 (Takuya Noguchi) + - Fixes padding in all clipboard icons that have .btn class - Fix a typo in doc/api/labels.md - API: all unknown routing will be handled with 404 Not Found - Make guests unable to view MRs on private projects @@ -131,6 +133,7 @@ v 8.12.4 - Fix failed project deletion when feature visibility set to private. !6688 - Prevent claiming associated model IDs via import. - Set GitLab project exported file permissions to owner only + - Improve the way merge request versions are compared with each other v 8.12.3 - Update Gitlab Shell to support low IO priority for storage moves diff --git a/GITLAB_SHELL_VERSION b/GITLAB_SHELL_VERSION index 0f44168a4d5..4f2c1d15f6d 100644 --- a/GITLAB_SHELL_VERSION +++ b/GITLAB_SHELL_VERSION @@ -1 +1 @@ -3.6.4 +3.6.6 @@ -51,7 +51,7 @@ gem 'browser', '~> 2.2' # Extracting information from a git repository # Provide access to Gitlab::Git library -gem 'gitlab_git', '~> 10.6.7' +gem 'gitlab_git', '~> 10.6.8' # LDAP Auth # GitLab fork with several improvements to original library. For full list of changes diff --git a/Gemfile.lock b/Gemfile.lock index 924e3a6781a..067908af3fb 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -282,7 +282,7 @@ GEM diff-lcs (~> 1.1) mime-types (>= 1.16, < 3) posix-spawn (~> 0.3) - gitlab_git (10.6.7) + gitlab_git (10.6.8) activesupport (~> 4.0) charlock_holmes (~> 0.7.3) github-linguist (~> 4.7.0) @@ -866,7 +866,7 @@ DEPENDENCIES github-linguist (~> 4.7.0) github-markup (~> 1.4) gitlab-flowdock-git-hook (~> 1.0.1) - gitlab_git (~> 10.6.7) + gitlab_git (~> 10.6.8) gitlab_omniauth-ldap (~> 1.2.1) gollum-lib (~> 4.2) gollum-rugged_adapter (~> 0.4.2) diff --git a/app/assets/javascripts/users_select.js b/app/assets/javascripts/users_select.js index bcabda3ceb2..d966277a8b2 100644 --- a/app/assets/javascripts/users_select.js +++ b/app/assets/javascripts/users_select.js @@ -261,10 +261,11 @@ } } if (showEmailUser && data.results.length === 0 && query.term.match(/^[^@]+@[^@]+$/)) { + var trimmed = query.term.trim(); emailUser = { name: "Invite \"" + query.term + "\"", - username: query.term, - id: query.term + username: trimmed, + id: trimmed }; data.results.unshift(emailUser); } diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 1006b3e62e8..7cf69c56d15 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -401,8 +401,12 @@ padding: 16px; } + .content-block { + border-top: 1px solid $border-color; + padding: $gl-padding-top $gl-padding; + } + .comments-disabled-notif { - padding: 10px 16px; .btn { margin-left: 5px; } @@ -413,10 +417,6 @@ margin: 0 7px; } - .comments-disabled-notif { - border-top: 1px solid $border-color; - } - .dropdown-title { color: $gl-text-color; } diff --git a/app/helpers/button_helper.rb b/app/helpers/button_helper.rb index a695aceea76..85e1dc33ee8 100644 --- a/app/helpers/button_helper.rb +++ b/app/helpers/button_helper.rb @@ -15,14 +15,14 @@ module ButtonHelper # # See http://clipboardjs.com/#usage def clipboard_button(data = {}) - css_class = data[:class] || 'btn-clipboard' + css_class = data[:class] || 'btn-clipboard btn-transparent' data = { toggle: 'tooltip', placement: 'bottom', container: 'body' }.merge(data) content_tag :button, icon('clipboard'), class: "btn #{css_class}", data: data, type: :button, - title: "Copy to Clipboard" + title: 'Copy to Clipboard' end def http_clone_button(project, placement = 'right', append_link: true) diff --git a/app/helpers/merge_requests_helper.rb b/app/helpers/merge_requests_helper.rb index b0a76765d97..249cb44e9d5 100644 --- a/app/helpers/merge_requests_helper.rb +++ b/app/helpers/merge_requests_helper.rb @@ -123,4 +123,8 @@ module MergeRequestsHelper def version_index(merge_request_diff) @merge_request_diffs.size - @merge_request_diffs.index(merge_request_diff) end + + def different_base?(version1, version2) + version1 && version2 && version1.base_commit_sha != version2.base_commit_sha + end end diff --git a/app/models/compare.rb b/app/models/compare.rb index 4856510f526..3a8bbcb1acd 100644 --- a/app/models/compare.rb +++ b/app/models/compare.rb @@ -11,9 +11,10 @@ class Compare end end - def initialize(compare, project) + def initialize(compare, project, straight: false) @compare = compare @project = project + @straight = straight end def commits @@ -45,6 +46,18 @@ class Compare end end + def start_commit_sha + start_commit.try(:sha) + end + + def base_commit_sha + base_commit.try(:sha) + end + + def head_commit_sha + commit.try(:sha) + end + def raw_diffs(*args) @compare.diffs(*args) end @@ -58,9 +71,9 @@ class Compare def diff_refs Gitlab::Diff::DiffRefs.new( - base_sha: base_commit.try(:sha), - start_sha: start_commit.try(:sha), - head_sha: commit.try(:sha) + base_sha: @straight ? start_commit_sha : base_commit_sha, + start_sha: start_commit_sha, + head_sha: head_commit_sha ) end end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 3f7e96186a1..b8a10b7968e 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -167,8 +167,11 @@ class MergeRequestDiff < ActiveRecord::Base self == merge_request.merge_request_diff end - def compare_with(sha) - CompareService.new.execute(project, head_commit_sha, project, sha) + def compare_with(sha, straight: true) + # When compare merge request versions we want diff A..B instead of A...B + # so we handle cases when user does squash and rebase of the commits between versions. + # For this reason we set straight to true by default. + CompareService.new.execute(project, head_commit_sha, project, sha, straight: straight) end private diff --git a/app/services/compare_service.rb b/app/services/compare_service.rb index 6d6075628af..5e8fafca98c 100644 --- a/app/services/compare_service.rb +++ b/app/services/compare_service.rb @@ -3,7 +3,7 @@ require 'securerandom' # Compare 2 branches for one repo or between repositories # and return Gitlab::Git::Compare object that responds to commits and diffs class CompareService - def execute(source_project, source_branch, target_project, target_branch) + def execute(source_project, source_branch, target_project, target_branch, straight: false) source_commit = source_project.commit(source_branch) return unless source_commit @@ -23,9 +23,10 @@ class CompareService raw_compare = Gitlab::Git::Compare.new( target_project.repository.raw_repository, target_branch, - source_sha + source_sha, + straight ) - Compare.new(raw_compare, target_project) + Compare.new(raw_compare, target_project, straight: straight) end end diff --git a/app/views/projects/merge_requests/show/_versions.html.haml b/app/views/projects/merge_requests/show/_versions.html.haml index 988ac0feae1..eab48b78cb3 100644 --- a/app/views/projects/merge_requests/show/_versions.html.haml +++ b/app/views/projects/merge_requests/show/_versions.html.haml @@ -64,6 +64,16 @@ #{@merge_request.target_branch} (base) .monospace #{short_sha(@merge_request_diff.base_commit_sha)} + - if different_base?(@start_version, @merge_request_diff) + .content-block + = icon('info-circle') + Selected versions have different base commits. + Changes will include + = link_to namespace_project_compare_path(@project.namespace, @project, from: @start_version.base_commit_sha, to: @merge_request_diff.base_commit_sha) do + new commits + from + %code #{@merge_request.target_branch} + - unless @merge_request_diff.latest? && !@start_sha .comments-disabled-notif.content-block = icon('info-circle') diff --git a/doc/administration/troubleshooting/debug.md b/doc/administration/troubleshooting/debug.md index d127d7b85e5..d8dce4388e1 100644 --- a/doc/administration/troubleshooting/debug.md +++ b/doc/administration/troubleshooting/debug.md @@ -144,14 +144,14 @@ separate Rails process to debug the issue: 1. Obtain the private token for your user (Profile Settings -> Account). 1. Bring up the GitLab Rails console. For omnibus users, run: - ```` + ``` sudo gitlab-rails console ``` 1. At the Rails console, run: ```ruby - [1] pry(main)> app.get '<URL FROM STEP 1>/private_token?<TOKEN FROM STEP 2>' + [1] pry(main)> app.get '<URL FROM STEP 2>/?private_token=<TOKEN FROM STEP 3>' ``` For example: diff --git a/doc/development/frontend.md b/doc/development/frontend.md index f879cd57e25..56c8516508e 100644 --- a/doc/development/frontend.md +++ b/doc/development/frontend.md @@ -223,3 +223,14 @@ For our currently-supported browsers, see our [requirements][requirements]. [xss]: https://en.wikipedia.org/wiki/Cross-site_scripting [scss-style-guide]: scss_styleguide.md [requirements]: ../install/requirements.md#supported-web-browsers + +## Common Errors + +### Rspec (Capybara/Poltergeist) chokes on general JavaScript errors + +If you see very generic JavaScript errors (e.g. `jQuery is undefined`) being thrown in tests, but +can't reproduce them manually, you may have included `ES6`-style JavaScript in files that don't +have the `.js.es6` file extension. Either use ES5-friendly JavaScript or rename the file you're +working in (`git mv <file>.js> <file.js.es6>`). + + diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb index c5658bd26e1..0b72a2f979b 100644 --- a/spec/models/appearance_spec.rb +++ b/spec/models/appearance_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' RSpec.describe Appearance, type: :model do - subject { create(:appearance) } + subject { build(:appearance) } it { is_expected.to be_valid } diff --git a/spec/models/deploy_key_spec.rb b/spec/models/deploy_key_spec.rb index 6a90598a629..93623e8e99b 100644 --- a/spec/models/deploy_key_spec.rb +++ b/spec/models/deploy_key_spec.rb @@ -1,9 +1,6 @@ require 'spec_helper' describe DeployKey, models: true do - let(:project) { create(:project) } - let(:deploy_key) { create(:deploy_key, projects: [project]) } - describe "Associations" do it { is_expected.to have_many(:deploy_keys_projects) } it { is_expected.to have_many(:projects) } diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index 06cac929bbf..733b79079ed 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -27,17 +27,17 @@ describe Event, models: true do end describe "Push event" do - before do - project = create(:project) - @user = project.owner - @event = create_event(project, @user) + let(:project) { create(:project) } + let(:user) { project.owner } + let(:event) { create_event(project, user) } + + it do + expect(event.push?).to be_truthy + expect(event.visible_to_user?).to be_truthy + expect(event.tag?).to be_falsey + expect(event.branch_name).to eq("master") + expect(event.author).to eq(user) end - - it { expect(@event.push?).to be_truthy } - it { expect(@event.visible_to_user?).to be_truthy } - it { expect(@event.tag?).to be_falsey } - it { expect(@event.branch_name).to eq("master") } - it { expect(@event.author).to eq(@user) } end describe '#note?' do @@ -59,8 +59,8 @@ describe Event, models: true do describe '#visible_to_user?' do let(:project) { create(:empty_project, :public) } let(:non_member) { create(:user) } - let(:member) { create(:user) } - let(:guest) { create(:user) } + let(:member) { create(:user) } + let(:guest) { create(:user) } let(:author) { create(:author) } let(:assignee) { create(:user) } let(:admin) { create(:admin) } @@ -79,23 +79,27 @@ describe Event, models: true do context 'for non confidential issues' do let(:target) { issue } - it { expect(event.visible_to_user?(non_member)).to eq true } - it { expect(event.visible_to_user?(author)).to eq true } - it { expect(event.visible_to_user?(assignee)).to eq true } - it { expect(event.visible_to_user?(member)).to eq true } - it { expect(event.visible_to_user?(guest)).to eq true } - it { expect(event.visible_to_user?(admin)).to eq true } + it do + expect(event.visible_to_user?(non_member)).to eq true + expect(event.visible_to_user?(author)).to eq true + expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq true + expect(event.visible_to_user?(admin)).to eq true + end end context 'for confidential issues' do let(:target) { confidential_issue } - it { expect(event.visible_to_user?(non_member)).to eq false } - it { expect(event.visible_to_user?(author)).to eq true } - it { expect(event.visible_to_user?(assignee)).to eq true } - it { expect(event.visible_to_user?(member)).to eq true } - it { expect(event.visible_to_user?(guest)).to eq false } - it { expect(event.visible_to_user?(admin)).to eq true } + it do + expect(event.visible_to_user?(non_member)).to eq false + expect(event.visible_to_user?(author)).to eq true + expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq false + expect(event.visible_to_user?(admin)).to eq true + end end end @@ -103,23 +107,27 @@ describe Event, models: true do context 'on non confidential issues' do let(:target) { note_on_issue } - it { expect(event.visible_to_user?(non_member)).to eq true } - it { expect(event.visible_to_user?(author)).to eq true } - it { expect(event.visible_to_user?(assignee)).to eq true } - it { expect(event.visible_to_user?(member)).to eq true } - it { expect(event.visible_to_user?(guest)).to eq true } - it { expect(event.visible_to_user?(admin)).to eq true } + it do + expect(event.visible_to_user?(non_member)).to eq true + expect(event.visible_to_user?(author)).to eq true + expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq true + expect(event.visible_to_user?(admin)).to eq true + end end context 'on confidential issues' do let(:target) { note_on_confidential_issue } - it { expect(event.visible_to_user?(non_member)).to eq false } - it { expect(event.visible_to_user?(author)).to eq true } - it { expect(event.visible_to_user?(assignee)).to eq true } - it { expect(event.visible_to_user?(member)).to eq true } - it { expect(event.visible_to_user?(guest)).to eq false } - it { expect(event.visible_to_user?(admin)).to eq true } + it do + expect(event.visible_to_user?(non_member)).to eq false + expect(event.visible_to_user?(author)).to eq true + expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq false + expect(event.visible_to_user?(admin)).to eq true + end end end @@ -129,22 +137,26 @@ describe Event, models: true do let(:note_on_merge_request) { create(:legacy_diff_note_on_merge_request, noteable: merge_request, project: project) } let(:target) { note_on_merge_request } - it { expect(event.visible_to_user?(non_member)).to eq true } - it { expect(event.visible_to_user?(author)).to eq true } - it { expect(event.visible_to_user?(assignee)).to eq true } - it { expect(event.visible_to_user?(member)).to eq true } - it { expect(event.visible_to_user?(guest)).to eq true } - it { expect(event.visible_to_user?(admin)).to eq true } + it do + expect(event.visible_to_user?(non_member)).to eq true + expect(event.visible_to_user?(author)).to eq true + expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq true + expect(event.visible_to_user?(admin)).to eq true + end context 'private project' do let(:project) { create(:project, :private) } - it { expect(event.visible_to_user?(non_member)).to eq false } - it { expect(event.visible_to_user?(author)).to eq true } - it { expect(event.visible_to_user?(assignee)).to eq true } - it { expect(event.visible_to_user?(member)).to eq true } - it { expect(event.visible_to_user?(guest)).to eq false } - it { expect(event.visible_to_user?(admin)).to eq true } + it do + expect(event.visible_to_user?(non_member)).to eq false + expect(event.visible_to_user?(author)).to eq true + expect(event.visible_to_user?(assignee)).to eq true + expect(event.visible_to_user?(member)).to eq true + expect(event.visible_to_user?(guest)).to eq false + expect(event.visible_to_user?(admin)).to eq true + end end end end @@ -214,6 +226,6 @@ describe Event, models: true do action: Event::PUSHED, data: data, author_id: user.id - }.merge(attrs)) + }.merge!(attrs)) end end diff --git a/spec/models/merge_request_diff_spec.rb b/spec/models/merge_request_diff_spec.rb index f27de0948ee..e5007424041 100644 --- a/spec/models/merge_request_diff_spec.rb +++ b/spec/models/merge_request_diff_spec.rb @@ -74,27 +74,43 @@ describe MergeRequestDiff, models: true do end end end + end - describe '#commits_sha' do - shared_examples 'returning all commits SHA' do - it 'returns all commits SHA' do - commits_sha = subject.commits_sha + describe '#commits_sha' do + shared_examples 'returning all commits SHA' do + it 'returns all commits SHA' do + commits_sha = subject.commits_sha - expect(commits_sha).to eq(subject.commits.map(&:sha)) - end + expect(commits_sha).to eq(subject.commits.map(&:sha)) end + end - context 'when commits were loaded' do - before do - subject.commits - end - - it_behaves_like 'returning all commits SHA' + context 'when commits were loaded' do + before do + subject.commits end - context 'when commits were not loaded' do - it_behaves_like 'returning all commits SHA' - end + it_behaves_like 'returning all commits SHA' + end + + context 'when commits were not loaded' do + it_behaves_like 'returning all commits SHA' + end + end + + describe '#compare_with' do + subject { create(:merge_request, source_branch: 'fix').merge_request_diff } + + it 'delegates compare to the service' do + expect(CompareService).to receive(:new).and_call_original + + subject.compare_with(nil) + end + + it 'uses git diff A..B approach by default' do + diffs = subject.compare_with('0b4bc9a49b562e85de7cc9e834518ea6828729b9').diffs + + expect(diffs.size).to eq(3) end end end diff --git a/spec/services/compare_service_spec.rb b/spec/services/compare_service_spec.rb new file mode 100644 index 00000000000..3760f19aaa2 --- /dev/null +++ b/spec/services/compare_service_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe CompareService, services: true do + let(:project) { create(:project) } + let(:user) { create(:user) } + let(:service) { described_class.new } + + describe '#execute' do + context 'compare with base, like feature...fix' do + subject { service.execute(project, 'feature', project, 'fix', straight: false) } + + it { expect(subject.diffs.size).to eq(1) } + end + + context 'straight compare, like feature..fix' do + subject { service.execute(project, 'feature', project, 'fix', straight: true) } + + it { expect(subject.diffs.size).to eq(3) } + end + end +end |