From 91cd5ae71cbede8f4a05a61d98c4e252cdbfe8c7 Mon Sep 17 00:00:00 2001 From: Chris Peressini Date: Thu, 11 Aug 2016 10:11:17 +0000 Subject: Add reference to product map. Remove line that says the GitLab logo and user picture are in the sidebar. --- doc/development/ui_guide.md | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/doc/development/ui_guide.md b/doc/development/ui_guide.md index 3a8c823e026..2d1d504202c 100644 --- a/doc/development/ui_guide.md +++ b/doc/development/ui_guide.md @@ -15,11 +15,14 @@ repository and maintained by GitLab UX designers. ## Navigation GitLab's layout contains 2 sections: the left sidebar and the content. The left sidebar contains a static navigation menu. -This menu will be visible regardless of what page you visit. The left sidebar also contains the GitLab logo -and the current user's profile picture. The content section contains a header and the content itself. -The header describes the current GitLab page and what navigation is -available to user in this area. Depending on the area (project, group, profile setting) the header name and navigation may change. For example when user visits one of the -project pages the header will contain a project name and navigation for that project. When the user visits a group page it will contain a group name and navigation related to this group. +This menu will be visible regardless of what page you visit. +The content section contains a header and the content itself. The header describes the current GitLab page and what navigation is +available to the user in this area. Depending on the area (project, group, profile setting) the header name and navigation may change. For example, when the user visits one of the +project pages the header will contain the project's name and navigation for that project. When the user visits a group page it will contain the group's name and navigation related to this group. + +You can see a visual representation of the navigation in GitLab in the GitLab Product Map, which is located in the [Design Repository](gitlab-map-graffle) +along with [PDF](gitlab-map-pdf) and [PNG](gitlab-map-png) exports. + ### Adding new tab to header navigation @@ -99,3 +102,6 @@ Do not use both green and blue button in one form. display counts in the UI. [number_with_delimiter]: http://api.rubyonrails.org/classes/ActionView/Helpers/NumberHelper.html#method-i-number_with_delimiter +[gitlab-map-graffle]: https://gitlab.com/gitlab-org/gitlab-design/blob/master/production/resources/gitlab-map.graffle +[gitlab-map-pdf]: https://gitlab.com/gitlab-org/gitlab-design/raw/master/production/resources/gitlab-map.pdf +[gitlab-map-png]: https://gitlab.com/gitlab-org/gitlab-design/raw/master/production/resources/gitlab-map.png \ No newline at end of file -- cgit v1.2.1 From efab1677a7a2959a28a09393a2b6519072005534 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Aug 2016 11:03:26 +0200 Subject: Fix attribute inclusion in import/export config ignored in some cases --- CHANGELOG | 3 +++ lib/gitlab/import_export/json_hash_builder.rb | 9 +++------ spec/lib/gitlab/import_export/reader_spec.rb | 3 ++- spec/support/import_export/import_export.yml | 4 ++++ 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 5aa5cbec279..b8f06f4c445 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -90,6 +90,9 @@ v 8.11.0 (unreleased) - Fix importing GitLab projects with an invalid MR source project - Sort folders with submodules in Files view !5521 +v 8.10.6 (unreleased) + - Fix import/export configuration missing some included attributes + v 8.10.5 - Add a data migration to fix some missing timestamps in the members table. !5670 - Revert the "Defend against 'Host' header injection" change in the source NGINX templates. !5706 diff --git a/lib/gitlab/import_export/json_hash_builder.rb b/lib/gitlab/import_export/json_hash_builder.rb index 008300bde45..b7db0e7a06a 100644 --- a/lib/gitlab/import_export/json_hash_builder.rb +++ b/lib/gitlab/import_export/json_hash_builder.rb @@ -57,10 +57,7 @@ module Gitlab # +value+ existing model to be included in the hash # +json_config_hash+ the original hash containing the root model def create_model_value(current_key, value, json_config_hash) - parsed_hash = { include: value } - parse_hash(value, parsed_hash) - - json_config_hash[current_key] = parsed_hash + json_config_hash[current_key] = parse_hash(value, { include: value }) end # Calls attributes finder to parse the hash and add any attributes to it @@ -69,8 +66,8 @@ module Gitlab # +parsed_hash+ the original hash def parse_hash(value, parsed_hash) @attributes_finder.parse(value) do |hash| - parsed_hash = { include: hash_or_merge(value, hash) } - end + { include: hash_or_merge(value, hash) } + end || parsed_hash end # Adds new model configuration to an existing hash with key +current_key+ diff --git a/spec/lib/gitlab/import_export/reader_spec.rb b/spec/lib/gitlab/import_export/reader_spec.rb index b76e14deca1..b6dec41d218 100644 --- a/spec/lib/gitlab/import_export/reader_spec.rb +++ b/spec/lib/gitlab/import_export/reader_spec.rb @@ -12,7 +12,8 @@ describe Gitlab::ImportExport::Reader, lib: true do except: [:iid], include: [:merge_request_diff, :merge_request_test] } }, - { commit_statuses: { include: :commit } }] + { commit_statuses: { include: :commit } }, + { project_members: { include: { user: { only: [:email] } } } }] } end diff --git a/spec/support/import_export/import_export.yml b/spec/support/import_export/import_export.yml index 3ceec506401..17136dee000 100644 --- a/spec/support/import_export/import_export.yml +++ b/spec/support/import_export/import_export.yml @@ -7,6 +7,8 @@ project_tree: - :merge_request_test - commit_statuses: - :commit + - project_members: + - :user included_attributes: project: @@ -14,6 +16,8 @@ included_attributes: - :path merge_requests: - :id + user: + - :email excluded_attributes: merge_requests: -- cgit v1.2.1 From 5f7394070f92acb2b7858b792034b30102fe1d9b Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Thu, 11 Aug 2016 14:22:21 +0200 Subject: Added documentation on adding database indexes --- doc/development/README.md | 4 + doc/development/adding_database_indexes.md | 123 +++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 doc/development/adding_database_indexes.md diff --git a/doc/development/README.md b/doc/development/README.md index bf67b5d8dff..57f37da6f80 100644 --- a/doc/development/README.md +++ b/doc/development/README.md @@ -30,7 +30,11 @@ - [Rake tasks](rake_tasks.md) for development - [Shell commands](shell_commands.md) in the GitLab codebase - [Sidekiq debugging](sidekiq_debugging.md) + +## Databases + - [What requires downtime?](what_requires_downtime.md) +- [Adding database indexes](adding_database_indexes.md) ## Compliance diff --git a/doc/development/adding_database_indexes.md b/doc/development/adding_database_indexes.md new file mode 100644 index 00000000000..ea6f14da3b9 --- /dev/null +++ b/doc/development/adding_database_indexes.md @@ -0,0 +1,123 @@ +# Adding Database Indexes + +Indexes can be used to speed up database queries, but when should you add a new +index? Traditionally the answer to this question has been to add an index for +every column used for filtering or joining data. For example, consider the +following query: + +```sql +SELECT * +FROM projects +WHERE user_id = 2; +``` + +Here we are filtering by the `user_id` column and as such a developer may decide +to index this column. + +While in certain cases indexing columns using the above approach may make sense +it can actually have a negative impact. Whenever you write data to a table any +existing indexes need to be updated. The more indexes there are the slower this +can potentially become. Indexes can also take up quite some disk space depending +on the amount of data indexed and the index type. For example, PostgreSQL offers +"GIN" indexes which can be used to index certain data types that can not be +indexed by regular btree indexes. These indexes however generally take up more +data and are slower to update compared to btree indexes. + +Because of all this one should not blindly add a new index for every column used +to filter data by. Instead one should ask themselves the following questions: + +1. Can I write my query in such a way that it re-uses as many existing indexes + as possible? +2. Is the data going to be large enough that using an index will actually be + faster than just iterating over the rows in the table? +3. Is the overhead of maintaining the index worth the reduction in query + timings? + +We'll explore every question in detail below. + +## Re-using Queries + +The first step is to make sure your query re-uses as many existing indexes as +possible. For example, consider the following query: + +```sql +SELECT * +FROM todos +WHERE user_id = 123 +AND state = 'open'; +``` + +Now imagine we already have an index on the `user_id` column but not on the +`state` column. One may think this query will perform badly due to `state` being +unindexed. In reality the query may perform just fine given the index on +`user_id` can filter out enough rows. + +The best way to determine if indexes are re-used is to run your query using +`EXPLAIN ANALYZE`. Depending on any extra tables that may be joined and +other columns being used for filtering you may find an extra index is not going +to make much (if any) difference. On the other hand you may determine that the +index _may_ make a difference. + +In short: + +1. Try to write your query in such a way that it re-uses as many existing + indexes as possible. +2. Run the query using `EXPLAIN ANALYZE` and study the output to find the most + ideal query. + +## Data Size + +A database may decide not to use an index despite it existing in case a regular +sequence scan (= simply iterating over all existing rows) is faster. This is +especially the case for small tables. + +If a table is expected to grow in size and you expect your query has to filter +out a lot of rows you may want to consider adding an index. If the table size is +very small (e.g. only a handful of rows) or any existing indexes filter out +enough rows you may _not_ want to add a new index. + +## Maintenance Overhead + +Indexes have to be updated on every table write. In case of PostgreSQL _all_ +existing indexes will be updated whenever data is written to a table. As a +result of this having many indexes on the same table will slow down writes. + +Because of this one should ask themselves: is the reduction in query performance +worth the overhead of maintaining an extra index? + +If adding an index reduces SELECT timings by 5 milliseconds but increases +INSERT/UPDATE/DELETE timings by 10 milliseconds then the index may not be worth +it. On the other hand, if SELECT timings are reduced but INSERT/UPDATE/DELETE +timings are not affected you may want to add the index after all. + +## Finding Unused Indexes + +To see which indexes are unused you can run the following query: + +```sql +SELECT relname as table_name, indexrelname as index_name, idx_scan, idx_tup_read, idx_tup_fetch, pg_size_pretty(pg_relation_size(indexrelname::regclass)) +FROM pg_stat_all_indexes +WHERE schemaname = 'public' +AND idx_scan = 0 +AND idx_tup_read = 0 +AND idx_tup_fetch = 0 +ORDER BY pg_relation_size(indexrelname::regclass) desc; +``` + +This query outputs a list containing all indexes that are never used and sorts +them by indexes sizes in descending order. This query can be useful to +determine if any previously indexes are useful after all. More information on +the meaning of the various columns can be found at +. + +Because the output of this query relies on the actual usage of your database it +may be affected by factors such as (but not limited to): + +* Certain queries never being executed, thus not being able to use certain + indexes. +* Certain tables having little data, resulting in PostgreSQL using sequence + scans instead of index scans. + +In other words, this data is only reliable for a frequently used database with +plenty of data and with as many GitLab features enabled (and being used) as +possible. -- cgit v1.2.1 From 30f9596c612abc19dd060fa3a8e8ae3d92001d45 Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Aug 2016 16:59:37 +0200 Subject: Fix permissions check in controller, added relevant spec and updated docs --- .../import/gitlab_projects_controller.rb | 5 ++ app/views/projects/new.html.haml | 2 +- doc/user/project/settings/import_export.md | 3 +- features/dashboard/new_project.feature | 2 +- features/steps/dashboard/new_project.rb | 3 +- .../projects/import_export/import_file_spec.rb | 98 +++++++++++++--------- 6 files changed, 69 insertions(+), 44 deletions(-) diff --git a/app/controllers/import/gitlab_projects_controller.rb b/app/controllers/import/gitlab_projects_controller.rb index 3ec173abcdb..7d0eff37635 100644 --- a/app/controllers/import/gitlab_projects_controller.rb +++ b/app/controllers/import/gitlab_projects_controller.rb @@ -1,5 +1,6 @@ class Import::GitlabProjectsController < Import::BaseController before_action :verify_gitlab_project_import_enabled + before_action :authenticate_admin! def new @namespace_id = project_params[:namespace_id] @@ -47,4 +48,8 @@ class Import::GitlabProjectsController < Import::BaseController :path, :namespace_id, :file ) end + + def authenticate_admin! + render_404 unless current_user.is_admin? + end end diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml index adcc984f506..ea4898f2107 100644 --- a/app/views/projects/new.html.haml +++ b/app/views/projects/new.html.haml @@ -77,7 +77,7 @@ = link_to "#", class: 'btn js-toggle-button import_git' do = icon('git', text: 'Repo by URL') %div{ class: 'import_gitlab_project' } - - if gitlab_project_import_enabled? + - if gitlab_project_import_enabled? && current_user.is_admin? = link_to new_import_gitlab_project_path, class: 'btn btn_import_gitlab_project project-submit' do = icon('gitlab', text: 'GitLab export') diff --git a/doc/user/project/settings/import_export.md b/doc/user/project/settings/import_export.md index 2513def49a4..08ff89ce6ae 100644 --- a/doc/user/project/settings/import_export.md +++ b/doc/user/project/settings/import_export.md @@ -7,8 +7,7 @@ > than that of the exporter. > - For existing installations, the project import option has to be enabled in > application settings (`/admin/application_settings`) under 'Import sources'. -> Ask your administrator if you don't see the **GitLab export** button when -> creating a new project. +> You will have to be an administrator to enable and use the import functionality. > - You can find some useful raketasks if you are an administrator in the > [import_export](../../../administration/raketasks/project_import_export.md) > raketask. diff --git a/features/dashboard/new_project.feature b/features/dashboard/new_project.feature index 8ddafb6a7ac..046e2815d4e 100644 --- a/features/dashboard/new_project.feature +++ b/features/dashboard/new_project.feature @@ -9,7 +9,7 @@ Background: @javascript Scenario: I should see New Projects page Then I see "New Project" page - Then I see all possible import optios + Then I see all possible import options @javascript Scenario: I should see instructions on how to import from Git URL diff --git a/features/steps/dashboard/new_project.rb b/features/steps/dashboard/new_project.rb index 727a6a71373..f2b44734601 100644 --- a/features/steps/dashboard/new_project.rb +++ b/features/steps/dashboard/new_project.rb @@ -14,14 +14,13 @@ class Spinach::Features::NewProject < Spinach::FeatureSteps expect(page).to have_content('Project name') end - step 'I see all possible import optios' do + step 'I see all possible import options' do expect(page).to have_link('GitHub') expect(page).to have_link('Bitbucket') expect(page).to have_link('GitLab.com') expect(page).to have_link('Gitorious.org') expect(page).to have_link('Google Code') expect(page).to have_link('Repo by URL') - expect(page).to have_link('GitLab export') end step 'I click on "Import project from GitHub"' do diff --git a/spec/features/projects/import_export/import_file_spec.rb b/spec/features/projects/import_export/import_file_spec.rb index 7835e1678ad..f707ccf4e93 100644 --- a/spec/features/projects/import_export/import_file_spec.rb +++ b/spec/features/projects/import_export/import_file_spec.rb @@ -3,8 +3,9 @@ require 'spec_helper' feature 'project import', feature: true, js: true do include Select2Helper - let(:user) { create(:admin) } - let!(:namespace) { create(:namespace, name: "asd", owner: user) } + let(:admin) { create(:admin) } + let(:normal_user) { create(:user) } + let!(:namespace) { create(:namespace, name: "asd", owner: admin) } let(:file) { File.join(Rails.root, 'spec', 'features', 'projects', 'import_export', 'test_project_export.tar.gz') } let(:export_path) { "#{Dir::tmpdir}/import_file_spec" } let(:project) { Project.last } @@ -12,66 +13,87 @@ feature 'project import', feature: true, js: true do background do allow_any_instance_of(Gitlab::ImportExport).to receive(:storage_path).and_return(export_path) - login_as(user) end after(:each) do FileUtils.rm_rf(export_path, secure: true) end - scenario 'user imports an exported project successfully' do - expect(Project.all.count).to be_zero + context 'admin user' do + before do + login_as(admin) + end - visit new_project_path + scenario 'user imports an exported project successfully' do + expect(Project.all.count).to be_zero - select2('2', from: '#project_namespace_id') - fill_in :project_path, with: 'test-project-path', visible: true - click_link 'GitLab export' + visit new_project_path - expect(page).to have_content('GitLab project export') - expect(URI.parse(current_url).query).to eq('namespace_id=2&path=test-project-path') + select2('2', from: '#project_namespace_id') + fill_in :project_path, with: 'test-project-path', visible: true + click_link 'GitLab export' - attach_file('file', file) + expect(page).to have_content('GitLab project export') + expect(URI.parse(current_url).query).to eq('namespace_id=2&path=test-project-path') - click_on 'Import project' # import starts + attach_file('file', file) - expect(project).not_to be_nil - expect(project.issues).not_to be_empty - expect(project.merge_requests).not_to be_empty - expect(project_hook).to exist - expect(wiki_exists?).to be true - expect(project.import_status).to eq('finished') - end + click_on 'Import project' # import starts + + expect(project).not_to be_nil + expect(project.issues).not_to be_empty + expect(project.merge_requests).not_to be_empty + expect(project_hook).to exist + expect(wiki_exists?).to be true + expect(project.import_status).to eq('finished') + end - scenario 'invalid project' do - project = create(:project, namespace_id: 2) + scenario 'invalid project' do + project = create(:project, namespace_id: 2) - visit new_project_path + visit new_project_path - select2('2', from: '#project_namespace_id') - fill_in :project_path, with: project.name, visible: true - click_link 'GitLab export' + select2('2', from: '#project_namespace_id') + fill_in :project_path, with: project.name, visible: true + click_link 'GitLab export' - attach_file('file', file) - click_on 'Import project' + attach_file('file', file) + click_on 'Import project' - page.within('.flash-container') do - expect(page).to have_content('Project could not be imported') + page.within('.flash-container') do + expect(page).to have_content('Project could not be imported') + end + end + + scenario 'project with no name' do + create(:project, namespace_id: 2) + + visit new_project_path + + select2('2', from: '#project_namespace_id') + + # click on disabled element + find(:link, 'GitLab export').trigger('click') + + page.within('.flash-container') do + expect(page).to have_content('Please enter path and name') + end end end - scenario 'project with no name' do - create(:project, namespace_id: 2) + context 'normal user' do + before do + login_as(normal_user) + end - visit new_project_path + scenario 'non-admin user is not allowed to import a project' do + expect(Project.all.count).to be_zero - select2('2', from: '#project_namespace_id') + visit new_project_path - # click on disabled element - find(:link, 'GitLab export').trigger('click') + fill_in :project_path, with: 'test-project-path', visible: true - page.within('.flash-container') do - expect(page).to have_content('Please enter path and name') + expect(page).not_to have_content('GitLab export') end end -- cgit v1.2.1 From c42f5f8b56a919473d9adceaf84c9ef77179c5cb Mon Sep 17 00:00:00 2001 From: James Lopez Date: Thu, 11 Aug 2016 21:42:34 +0200 Subject: refactor parse_hash based on feedback --- lib/gitlab/import_export/json_hash_builder.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/import_export/json_hash_builder.rb b/lib/gitlab/import_export/json_hash_builder.rb index b7db0e7a06a..0cc10f40087 100644 --- a/lib/gitlab/import_export/json_hash_builder.rb +++ b/lib/gitlab/import_export/json_hash_builder.rb @@ -57,17 +57,17 @@ module Gitlab # +value+ existing model to be included in the hash # +json_config_hash+ the original hash containing the root model def create_model_value(current_key, value, json_config_hash) - json_config_hash[current_key] = parse_hash(value, { include: value }) + json_config_hash[current_key] = parse_hash(value) || { include: value } end # Calls attributes finder to parse the hash and add any attributes to it # # +value+ existing model to be included in the hash # +parsed_hash+ the original hash - def parse_hash(value, parsed_hash) + def parse_hash(value) @attributes_finder.parse(value) do |hash| { include: hash_or_merge(value, hash) } - end || parsed_hash + end end # Adds new model configuration to an existing hash with key +current_key+ -- cgit v1.2.1 From b44b09b302e6f4d19c7e6b2dac3be62fe83b31b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20D=C3=A1vila=20Santos?= Date: Tue, 16 Aug 2016 16:14:54 +0000 Subject: Revert "Merge branch '19957-write-tests-for-adding-comments-for-different-line-types-in-diff' into 'master'" This reverts merge request !5417 --- features/project/merge_requests.feature | 10 ++ spec/features/merge_requests/diff_notes_spec.rb | 159 ------------------------ 2 files changed, 10 insertions(+), 159 deletions(-) delete mode 100644 spec/features/merge_requests/diff_notes_spec.rb diff --git a/features/project/merge_requests.feature b/features/project/merge_requests.feature index 1b8e4262e40..6bac6011467 100644 --- a/features/project/merge_requests.feature +++ b/features/project/merge_requests.feature @@ -106,6 +106,16 @@ Feature: Project Merge Requests And I sort the list by "Least popular" Then The list should be sorted by "Least popular" + @javascript + Scenario: I comment on a merge request diff + Given project "Shop" have "Bug NS-05" open merge request with diffs inside + And I visit merge request page "Bug NS-05" + And I click on the Changes tab + And I leave a comment like "Line is wrong" on diff + And I switch to the merge request's comments tab + Then I should see a discussion has started on diff + And I should see a badge of "1" next to the discussion link + @javascript Scenario: I see a new comment on merge request diff from another user in the discussion tab Given project "Shop" have "Bug NS-05" open merge request with diffs inside diff --git a/spec/features/merge_requests/diff_notes_spec.rb b/spec/features/merge_requests/diff_notes_spec.rb deleted file mode 100644 index 12e89742b79..00000000000 --- a/spec/features/merge_requests/diff_notes_spec.rb +++ /dev/null @@ -1,159 +0,0 @@ -require 'spec_helper' - -feature 'Diff notes', js: true, feature: true do - include WaitForAjax - - before do - login_as :admin - @merge_request = create(:merge_request) - @project = @merge_request.source_project - end - - context 'merge request diffs' do - let(:comment_button_class) { '.add-diff-note' } - let(:notes_holder_input_class) { 'js-temp-notes-holder' } - let(:notes_holder_input_xpath) { './following-sibling::*[contains(concat(" ", @class, " "), " notes_holder ")]' } - let(:test_note_comment) { 'this is a test note!' } - - context 'when hovering over the parallel view diff file' do - before(:each) do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Side-by-side' - end - - context 'with an old line on the left and no line on the right' do - it 'should allow commenting on the left side' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'left') - end - - it 'should not allow commenting on the right side' do - should_not_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_23_22"]').find(:xpath, '..'), 'right') - end - end - - context 'with no line on the left and a new line on the right' do - it 'should not allow commenting on the left side' do - should_not_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'left') - end - - it 'should allow commenting on the right side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_15_15"]').find(:xpath, '..'), 'right') - end - end - - context 'with an old line on the left and a new line on the right' do - it 'should allow commenting on the left side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'left') - end - - it 'should allow commenting on the right side' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_9_9"]').find(:xpath, '..'), 'right') - end - end - - context 'with an unchanged line on the left and an unchanged line on the right' do - it 'should allow commenting on the left side' do - should_allow_commenting(first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'), 'left') - end - - it 'should allow commenting on the right side' do - should_allow_commenting(first('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]').find(:xpath, '..'), 'right') - end - end - - context 'with a match line' do - it 'should not allow commenting on the left side' do - should_not_allow_commenting(first('.match').find(:xpath, '..'), 'left') - end - - it 'should not allow commenting on the right side' do - should_not_allow_commenting(first('.match').find(:xpath, '..'), 'right') - end - end - end - - context 'when hovering over the inline view diff file' do - before do - visit diffs_namespace_project_merge_request_path(@project.namespace, @project, @merge_request) - click_link 'Inline' - end - - context 'with a new line' do - it 'should allow commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_10_9"]')) - end - end - - context 'with an old line' do - it 'should allow commenting' do - should_allow_commenting(find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9_22_22"]')) - end - end - - context 'with an unchanged line' do - it 'should allow commenting' do - should_allow_commenting(find('[id="2f6fcd96b88b36ce98c38da085c795a27d92a3dd_7_7"]')) - end - end - - context 'with a match line' do - it 'should not allow commenting' do - should_not_allow_commenting(first('.match')) - end - end - end - - def should_allow_commenting(line_holder, diff_side = nil) - line = get_line_components(line_holder, diff_side) - line[:content].hover - expect(line[:num]).to have_css comment_button_class - - comment_on_line(line_holder, line) - wait_for_ajax - - assert_comment_persistence(line_holder) - end - - def should_not_allow_commenting(line_holder, diff_side = nil) - line = get_line_components(line_holder, diff_side) - line[:content].hover - expect(line[:num]).not_to have_css comment_button_class - end - - def get_line_components(line_holder, diff_side = nil) - if diff_side.nil? - get_inline_line_components(line_holder) - else - get_parallel_line_components(line_holder, diff_side) - end - end - - def get_inline_line_components(line_holder) - { content: line_holder.first('.line_content'), num: line_holder.first('.diff-line-num') } - end - - def get_parallel_line_components(line_holder, diff_side = nil) - side_index = diff_side == 'left' ? 0 : 1 - { content: line_holder.all('.line_content')[side_index], num: line_holder.all('.diff-line-num')[side_index] } - end - - def comment_on_line(line_holder, line) - line[:num].find(comment_button_class).trigger 'click' - expect(line_holder).to have_xpath notes_holder_input_xpath - - notes_holder_input = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_input[:class]).to include(notes_holder_input_class) - - notes_holder_input.fill_in 'note[note]', with: test_note_comment - click_button 'Comment' - end - - def assert_comment_persistence(line_holder) - expect(line_holder).to have_xpath notes_holder_input_xpath - - notes_holder_saved = line_holder.find(:xpath, notes_holder_input_xpath) - expect(notes_holder_saved[:class]).not_to include(notes_holder_input_class) - expect(notes_holder_saved).to have_content test_note_comment - end - end -end -- cgit v1.2.1