From ebf73a19ce08afda8357c0997401f2ca3e4c4dca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 13 Dec 2018 16:45:44 +0100 Subject: Use 2 replicas for the Deployment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- scripts/review_apps/review-apps.sh | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/scripts/review_apps/review-apps.sh b/scripts/review_apps/review-apps.sh index 9e52366f800..1ee6f502b8e 100755 --- a/scripts/review_apps/review-apps.sh +++ b/scripts/review_apps/review-apps.sh @@ -31,7 +31,9 @@ function ensure_namespace() { function install_tiller() { echo "Checking Tiller..." - helm init --upgrade + helm init \ + --upgrade \ + --replicas 2 kubectl rollout status -n "$TILLER_NAMESPACE" -w "deployment/tiller-deploy" if ! helm version --debug; then echo "Failed to init Tiller." -- cgit v1.2.1 From 12e01daec9f24fda742103f9e6c795c358693de5 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Thu, 15 Nov 2018 14:56:51 +0800 Subject: Add group milestones in upcoming filter --- app/finders/issuable_finder.rb | 20 +++++++++++++++++--- app/models/milestone.rb | 28 ++++++++++++++++++++-------- spec/finders/issues_finder_spec.rb | 19 ++++++++++++------- spec/models/milestone_spec.rb | 33 +++++++++++++++++++++++++++------ 4 files changed, 76 insertions(+), 24 deletions(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index b73a3fa6e01..a144db9fcd1 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -149,6 +149,18 @@ class IssuableFinder end end + def related_groups + if project? && project && project.group && Ability.allowed?(current_user, :read_group, project.group) + project.group.self_and_ancestors + elsif group + [group] + elsif current_user + Gitlab::GroupHierarchy.new(current_user.authorized_groups, current_user.groups).all_groups + else + [] + end + end + def project? params[:project_id].present? end @@ -163,8 +175,10 @@ class IssuableFinder end # rubocop: disable CodeReuse/ActiveRecord - def projects(items = nil) - return @projects = project if project? + def projects + return @projects if defined?(@projects) + + return @projects = [project] if project? projects = if current_user && params[:authorized_only].presence && !current_user_related? @@ -459,7 +473,7 @@ class IssuableFinder elsif filter_by_any_milestone? items = items.any_milestone elsif filter_by_upcoming_milestone? - upcoming_ids = Milestone.upcoming_ids_by_projects(projects(items)) + upcoming_ids = Milestone.upcoming_ids(projects, related_groups) items = items.left_joins_milestones.where(milestone_id: upcoming_ids) elsif filter_by_started_milestone? items = items.left_joins_milestones.where('milestones.start_date <= NOW()') diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 6dc0fca68e6..30b5884d405 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -40,6 +40,7 @@ class Milestone < ActiveRecord::Base scope :for_projects_and_groups, -> (project_ids, group_ids) do conditions = [] + conditions << arel_table[:project_id].in(project_ids) if project_ids&.compact&.any? conditions << arel_table[:group_id].in(group_ids) if group_ids&.compact&.any? @@ -129,18 +130,29 @@ class Milestone < ActiveRecord::Base @link_reference_pattern ||= super("milestones", /(?\d+)/) end - def self.upcoming_ids_by_projects(projects) - rel = unscoped.of_projects(projects).active.where('due_date > ?', Time.now) + def self.upcoming_ids(projects, groups) + rel = unscoped + .for_projects_and_groups(projects&.map(&:id), groups&.map(&:id)) + .active.where('milestones.due_date > NOW()') if Gitlab::Database.postgresql? - rel.order(:project_id, :due_date).select('DISTINCT ON (project_id) id') + rel.order(:project_id, :group_id, :due_date).select('DISTINCT ON (project_id, group_id) id') else + # We need to use MySQL's NULL-safe comparison operator `<=>` here + # because one of `project_id` or `group_id` is always NULL + join_clause = <<~HEREDOC + LEFT OUTER JOIN milestones earlier_milestones + ON milestones.project_id <=> earlier_milestones.project_id + AND milestones.group_id <=> earlier_milestones.group_id + AND milestones.due_date > earlier_milestones.due_date + AND earlier_milestones.due_date > NOW() + AND earlier_milestones.state = 'active' + HEREDOC + rel - .group(:project_id, :due_date, :id) - .having('due_date = MIN(due_date)') - .pluck(:id, :project_id, :due_date) - .uniq(&:second) - .map(&:first) + .joins(join_clause) + .where('earlier_milestones.id IS NULL') + .select(:id) end end diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index 80f7232f282..682fae06434 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -174,9 +174,13 @@ describe IssuesFinder do context 'filtering by upcoming milestone' do let(:params) { { milestone_title: Milestone::Upcoming.name } } + let!(:group) { create(:group, :public) } + let!(:group_member) { create(:group_member, group: group, user: user) } + let(:project_no_upcoming_milestones) { create(:project, :public) } let(:project_next_1_1) { create(:project, :public) } let(:project_next_8_8) { create(:project, :public) } + let(:project_in_group) { create(:project, :public, namespace: group) } let(:yesterday) { Date.today - 1.day } let(:tomorrow) { Date.today + 1.day } @@ -187,21 +191,22 @@ describe IssuesFinder do [ create(:milestone, :closed, project: project_no_upcoming_milestones), create(:milestone, project: project_next_1_1, title: '1.1', due_date: two_days_from_now), - create(:milestone, project: project_next_1_1, title: '8.8', due_date: ten_days_from_now), - create(:milestone, project: project_next_8_8, title: '1.1', due_date: yesterday), - create(:milestone, project: project_next_8_8, title: '8.8', due_date: tomorrow) + create(:milestone, project: project_next_1_1, title: '8.9', due_date: ten_days_from_now), + create(:milestone, project: project_next_8_8, title: '1.2', due_date: yesterday), + create(:milestone, project: project_next_8_8, title: '8.8', due_date: tomorrow), + create(:milestone, group: group, title: '9.9', due_date: tomorrow) ] end before do milestones.each do |milestone| - create(:issue, project: milestone.project, milestone: milestone, author: user, assignees: [user]) + create(:issue, project: milestone.project || project_in_group, milestone: milestone, author: user, assignees: [user]) end end - it 'returns issues in the upcoming milestone for each project' do - expect(issues.map { |issue| issue.milestone.title }).to contain_exactly('1.1', '8.8') - expect(issues.map { |issue| issue.milestone.due_date }).to contain_exactly(tomorrow, two_days_from_now) + it 'returns issues in the upcoming milestone for each project or group' do + expect(issues.map { |issue| issue.milestone.title }).to contain_exactly('1.1', '8.8', '9.9') + expect(issues.map { |issue| issue.milestone.due_date }).to contain_exactly(tomorrow, two_days_from_now, tomorrow) end end diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index b3d31e65c85..23f9f054673 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -240,7 +240,22 @@ describe Milestone do end end - describe '.upcoming_ids_by_projects' do + describe '.upcoming_ids' do + let(:group_1) { create(:group) } + let(:group_2) { create(:group) } + let(:group_3) { create(:group) } + let(:groups) { [group_1, group_2, group_3] } + + let!(:past_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now - 1.day) } + let!(:current_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now + 1.day) } + let!(:future_milestone_group_1) { create(:milestone, group: group_1, due_date: Time.now + 2.days) } + + let!(:past_milestone_group_2) { create(:milestone, group: group_2, due_date: Time.now - 1.day) } + let!(:closed_milestone_group_2) { create(:milestone, :closed, group: group_2, due_date: Time.now + 1.day) } + let!(:current_milestone_group_2) { create(:milestone, group: group_2, due_date: Time.now + 2.days) } + + let!(:past_milestone_group_3) { create(:milestone, group: group_3, due_date: Time.now - 1.day) } + let(:project_1) { create(:project) } let(:project_2) { create(:project) } let(:project_3) { create(:project) } @@ -258,14 +273,20 @@ describe Milestone do # The call to `#try` is because this returns a relation with a Postgres DB, # and an array of IDs with a MySQL DB. - let(:milestone_ids) { described_class.upcoming_ids_by_projects(projects).map { |id| id.try(:id) || id } } - - it 'returns the next upcoming open milestone ID for each project' do - expect(milestone_ids).to contain_exactly(current_milestone_project_1.id, current_milestone_project_2.id) + let(:milestone_ids) { described_class.upcoming_ids(projects, groups).map { |id| id.try(:id) || id } } + + it 'returns the next upcoming open milestone ID for each project and group' do + expect(milestone_ids).to contain_exactly( + current_milestone_project_1.id, + current_milestone_project_2.id, + current_milestone_group_1.id, + current_milestone_group_2.id + ) end - context 'when the projects have no open upcoming milestones' do + context 'when the projects and groups have no open upcoming milestones' do let(:projects) { [project_3] } + let(:groups) { [group_3] } it 'returns no results' do expect(milestone_ids).to be_empty -- cgit v1.2.1 From 030c7068fc8127e1ff8afa7c8daffee034c7b929 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Thu, 15 Nov 2018 15:03:01 +0800 Subject: Add changelog entry --- .../unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml diff --git a/changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml b/changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml new file mode 100644 index 00000000000..1e9c7f3913c --- /dev/null +++ b/changelogs/unreleased/53431-fix-upcoming-milestone-filter-for-groups.yml @@ -0,0 +1,5 @@ +--- +title: Fix upcoming milestones filter not including group milestones +merge_request: 23098 +author: Heinrich Lee Yu +type: fixed -- cgit v1.2.1 From 2490cfeeb2f8426b1a8f4e24bd0297e41a870ca2 Mon Sep 17 00:00:00 2001 From: Heinrich Lee Yu Date: Mon, 17 Dec 2018 20:59:23 +0800 Subject: Refactor Milestone.for_projects_and_groups Refactored to use Rails 5 ActiveRecord `or` so that it can handle ids, objects, array of objects, or relations properly. --- app/finders/issuable_finder.rb | 2 +- app/models/milestone.rb | 13 ++++---- spec/models/milestone_spec.rb | 70 ++++++++++++++++++++++++++++++++++++++++-- 3 files changed, 75 insertions(+), 10 deletions(-) diff --git a/app/finders/issuable_finder.rb b/app/finders/issuable_finder.rb index a144db9fcd1..1a69ec85d18 100644 --- a/app/finders/issuable_finder.rb +++ b/app/finders/issuable_finder.rb @@ -155,7 +155,7 @@ class IssuableFinder elsif group [group] elsif current_user - Gitlab::GroupHierarchy.new(current_user.authorized_groups, current_user.groups).all_groups + Gitlab::ObjectHierarchy.new(current_user.authorized_groups, current_user.groups).all_objects else [] end diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 30b5884d405..7c397f3499f 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -38,13 +38,14 @@ class Milestone < ActiveRecord::Base scope :closed, -> { with_state(:closed) } scope :for_projects, -> { where(group: nil).includes(:project) } - scope :for_projects_and_groups, -> (project_ids, group_ids) do - conditions = [] + scope :for_projects_and_groups, -> (projects, groups) do + projects = projects.compact if projects.is_a? Array + projects = [] if projects.nil? - conditions << arel_table[:project_id].in(project_ids) if project_ids&.compact&.any? - conditions << arel_table[:group_id].in(group_ids) if group_ids&.compact&.any? + groups = groups.compact if groups.is_a? Array + groups = [] if groups.nil? - where(conditions.reduce(:or)) + where(project: projects).or(where(group: groups)) end scope :order_by_name_asc, -> { order(Arel::Nodes::Ascending.new(arel_table[:title].lower)) } @@ -132,7 +133,7 @@ class Milestone < ActiveRecord::Base def self.upcoming_ids(projects, groups) rel = unscoped - .for_projects_and_groups(projects&.map(&:id), groups&.map(&:id)) + .for_projects_and_groups(projects, groups) .active.where('milestones.due_date > NOW()') if Gitlab::Database.postgresql? diff --git a/spec/models/milestone_spec.rb b/spec/models/milestone_spec.rb index 23f9f054673..015db4d4e96 100644 --- a/spec/models/milestone_spec.rb +++ b/spec/models/milestone_spec.rb @@ -240,6 +240,72 @@ describe Milestone do end end + describe '#for_projects_and_groups' do + let(:project) { create(:project) } + let(:project_other) { create(:project) } + let(:group) { create(:group) } + let(:group_other) { create(:group) } + + before do + create(:milestone, project: project) + create(:milestone, project: project_other) + create(:milestone, group: group) + create(:milestone, group: group_other) + end + + subject { described_class.for_projects_and_groups(projects, groups) } + + shared_examples 'filters by projects and groups' do + it 'returns milestones filtered by project' do + milestones = described_class.for_projects_and_groups(projects, []) + + expect(milestones.count).to eq(1) + expect(milestones.first.project_id).to eq(project.id) + end + + it 'returns milestones filtered by group' do + milestones = described_class.for_projects_and_groups([], groups) + + expect(milestones.count).to eq(1) + expect(milestones.first.group_id).to eq(group.id) + end + + it 'returns milestones filtered by both project and group' do + milestones = described_class.for_projects_and_groups(projects, groups) + + expect(milestones.count).to eq(2) + expect(milestones).to contain_exactly(project.milestones.first, group.milestones.first) + end + end + + context 'ids as params' do + let(:projects) { [project.id] } + let(:groups) { [group.id] } + + it_behaves_like 'filters by projects and groups' + end + + context 'relations as params' do + let(:projects) { Project.where(id: project.id) } + let(:groups) { Group.where(id: group.id) } + + it_behaves_like 'filters by projects and groups' + end + + context 'objects as params' do + let(:projects) { [project] } + let(:groups) { [group] } + + it_behaves_like 'filters by projects and groups' + end + + it 'returns no records if projects and groups are nil' do + milestones = described_class.for_projects_and_groups(nil, nil) + + expect(milestones).to be_empty + end + end + describe '.upcoming_ids' do let(:group_1) { create(:group) } let(:group_2) { create(:group) } @@ -271,9 +337,7 @@ describe Milestone do let!(:past_milestone_project_3) { create(:milestone, project: project_3, due_date: Time.now - 1.day) } - # The call to `#try` is because this returns a relation with a Postgres DB, - # and an array of IDs with a MySQL DB. - let(:milestone_ids) { described_class.upcoming_ids(projects, groups).map { |id| id.try(:id) || id } } + let(:milestone_ids) { described_class.upcoming_ids(projects, groups).map(&:id) } it 'returns the next upcoming open milestone ID for each project and group' do expect(milestone_ids).to contain_exactly( -- cgit v1.2.1 From 118edd2d537bdc3b5cd80a4112d4865a17956568 Mon Sep 17 00:00:00 2001 From: Dhiraj Bodicherla Date: Sat, 5 Jan 2019 22:00:31 -0800 Subject: Emoji and cancel button are taller than input in set user status modal --- app/assets/stylesheets/framework/header.scss | 4 ++++ ...cel-buttons-height-in-user-status-modal-when-emoji-is-changed.yml | 5 +++++ 2 files changed, 9 insertions(+) create mode 100644 changelogs/unreleased/55884-adjust-emoji-and-cancel-buttons-height-in-user-status-modal-when-emoji-is-changed.yml diff --git a/app/assets/stylesheets/framework/header.scss b/app/assets/stylesheets/framework/header.scss index 5574873fa22..36dd1cee4de 100644 --- a/app/assets/stylesheets/framework/header.scss +++ b/app/assets/stylesheets/framework/header.scss @@ -596,6 +596,10 @@ .emoji-menu-toggle-button { @include emoji-menu-toggle-button; } + + .input-group { + height: 34px; + } } .nav-links > li > a { diff --git a/changelogs/unreleased/55884-adjust-emoji-and-cancel-buttons-height-in-user-status-modal-when-emoji-is-changed.yml b/changelogs/unreleased/55884-adjust-emoji-and-cancel-buttons-height-in-user-status-modal-when-emoji-is-changed.yml new file mode 100644 index 00000000000..2fbf334f5e9 --- /dev/null +++ b/changelogs/unreleased/55884-adjust-emoji-and-cancel-buttons-height-in-user-status-modal-when-emoji-is-changed.yml @@ -0,0 +1,5 @@ +--- +title: Emoji and cancel button are taller than input in set user status modal +merge_request: 24173 +author: Dhiraj Bodicherla +type: fixed -- cgit v1.2.1 From 7df39921f06023bf3481fe84c9ae90968fdf61e0 Mon Sep 17 00:00:00 2001 From: robertfoss Date: Tue, 8 Jan 2019 18:32:37 +0000 Subject: web-terminal: Add support for URL to Link parsing This is desirable since it enables people who are viewing job results to quickly go from the logs to more detailed information about an issue. --- app/assets/javascripts/terminal/terminal.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/assets/javascripts/terminal/terminal.js b/app/assets/javascripts/terminal/terminal.js index 560f50ebf8f..e5dd7a465ea 100644 --- a/app/assets/javascripts/terminal/terminal.js +++ b/app/assets/javascripts/terminal/terminal.js @@ -2,11 +2,13 @@ import _ from 'underscore'; import $ from 'jquery'; import { Terminal } from 'xterm'; import * as fit from 'xterm/lib/addons/fit/fit'; +import * as webLinks from 'xterm/lib/addons/webLinks/webLinks'; import { canScrollUp, canScrollDown } from '~/lib/utils/dom_utils'; const SCROLL_MARGIN = 5; Terminal.applyAddon(fit); +Terminal.applyAddon(webLinks); export default class GLTerminal { constructor(element, options = {}) { @@ -48,6 +50,7 @@ export default class GLTerminal { this.terminal.open(this.container); this.terminal.fit(); + this.terminal.webLinksInit(); this.terminal.focus(); this.socket.onopen = () => { -- cgit v1.2.1 From 9e2cea31940734c79d37dda780312944fb34ee10 Mon Sep 17 00:00:00 2001 From: Mark Lapierre Date: Tue, 8 Jan 2019 16:06:19 -0500 Subject: Split reply_to_discussion method The method has 2 parts, entering text and submitting the comment. They have to be separable because the EE batch comments test performs different actions after entering text - it doesn't always immediately submit the comment. --- qa/qa/page/component/note.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/qa/qa/page/component/note.rb b/qa/qa/page/component/note.rb index 67d7f114786..f5add6bc9b5 100644 --- a/qa/qa/page/component/note.rb +++ b/qa/qa/page/component/note.rb @@ -32,9 +32,13 @@ module QA click_element :comment_button end - def reply_to_discussion(reply_text) + def type_reply_to_discussion(reply_text) all_elements(:discussion_reply).last.click fill_element :reply_input, reply_text + end + + def reply_to_discussion(reply_text) + type_reply_to_discussion(reply_text) click_element :reply_comment_button end -- cgit v1.2.1 From ab6b9a1c4350bbaf8b907efb28fbe456e4fe09d5 Mon Sep 17 00:00:00 2001 From: Steve Azzopardi Date: Wed, 9 Jan 2019 16:33:22 +0100 Subject: Remove get_build method for find_by_id The original intention of `get_build` was as a workaround not to violate `CodeReuse/ActiveRecord`. `find_by_id` does the exact same thing but does not violate the rubocop rule. --- app/controllers/projects/artifacts_controller.rb | 2 +- .../projects/build_artifacts_controller.rb | 2 +- app/models/project.rb | 4 ---- spec/models/project_spec.rb | 23 ---------------------- 4 files changed, 2 insertions(+), 29 deletions(-) diff --git a/app/controllers/projects/artifacts_controller.rb b/app/controllers/projects/artifacts_controller.rb index 9a0997e92ee..2ef18d900f2 100644 --- a/app/controllers/projects/artifacts_controller.rb +++ b/app/controllers/projects/artifacts_controller.rb @@ -86,7 +86,7 @@ class Projects::ArtifactsController < Projects::ApplicationController end def build_from_id - project.get_build(params[:job_id]) if params[:job_id] + project.builds.find_by_id(params[:job_id]) if params[:job_id] end def build_from_ref diff --git a/app/controllers/projects/build_artifacts_controller.rb b/app/controllers/projects/build_artifacts_controller.rb index d3d5ba5c75d..4274c356227 100644 --- a/app/controllers/projects/build_artifacts_controller.rb +++ b/app/controllers/projects/build_artifacts_controller.rb @@ -45,7 +45,7 @@ class Projects::BuildArtifactsController < Projects::ApplicationController end def job_from_id - project.get_build(params[:build_id]) if params[:build_id] + project.builds.find_by_id(params[:build_id]) if params[:build_id] end def job_from_ref diff --git a/app/models/project.rb b/app/models/project.rb index a66ed6736ca..9d28ff81cda 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -658,10 +658,6 @@ class Project < ActiveRecord::Base latest_successful_build_for(job_name, ref) || raise(ActiveRecord::RecordNotFound.new("Couldn't find job #{job_name}")) end - def get_build(id) - builds.find_by(id: id) - end - def merge_base_commit(first_commit_id, second_commit_id) sha = repository.merge_base(first_commit_id, second_commit_id) commit_by(oid: sha) if sha diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 3044150bca8..eee7fef17de 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -2026,29 +2026,6 @@ describe Project do end end - describe '#get_build' do - let(:project) { create(:project, :repository) } - let(:ci_pipeline) { create(:ci_pipeline, project: project) } - - context 'when build exists' do - context 'build is associated with project' do - let(:build) { create(:ci_build, :success, pipeline: ci_pipeline) } - - it { expect(project.get_build(build.id)).to eq(build) } - end - - context 'build is not associated with project' do - let(:build) { create(:ci_build, :success) } - - it { expect(project.get_build(build.id)).to be_nil } - end - end - - context 'build does not exists' do - it { expect(project.get_build(rand 100)).to be_nil } - end - end - describe '#import_status' do context 'with import_state' do it 'returns the right status' do -- cgit v1.2.1 From 1daa2c4ad5fce4c0913486779b4b06a100d1fc2c Mon Sep 17 00:00:00 2001 From: Winnie Hellmann Date: Tue, 8 Jan 2019 12:27:20 +0100 Subject: Adjust height of "Add list" dropdown in issue boards --- app/assets/stylesheets/framework/dropdowns.scss | 4 ++++ app/assets/stylesheets/pages/boards.scss | 4 ---- app/views/shared/issuable/_board_create_list_dropdown.html.haml | 2 +- changelogs/unreleased/winh-add-list-dropdown-height.yml | 5 +++++ 4 files changed, 10 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/winh-add-list-dropdown-height.yml diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index afcb230797a..cb01a41cb7e 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -32,6 +32,10 @@ max-height: $dropdown-max-height; overflow-y: auto; + &.dropdown-extended-height { + max-height: 400px; + } + @include media-breakpoint-down(xs) { width: 100%; } diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss index 37984a8666f..c5a0eaaf704 100644 --- a/app/assets/stylesheets/pages/boards.scss +++ b/app/assets/stylesheets/pages/boards.scss @@ -29,10 +29,6 @@ .dropdown-menu-issues-board-new { width: 320px; - .open & { - max-height: 400px; - } - .dropdown-content { max-height: 162px; } diff --git a/app/views/shared/issuable/_board_create_list_dropdown.html.haml b/app/views/shared/issuable/_board_create_list_dropdown.html.haml index 4597d9439fa..fd413bd68c8 100644 --- a/app/views/shared/issuable/_board_create_list_dropdown.html.haml +++ b/app/views/shared/issuable/_board_create_list_dropdown.html.haml @@ -1,7 +1,7 @@ .dropdown.prepend-left-10#js-add-list %button.btn.btn-success.btn-inverted.js-new-board-list{ type: "button", data: board_list_data } Add list - .dropdown-menu.dropdown-menu-paging.dropdown-menu-right.dropdown-menu-issues-board-new.dropdown-menu-selectable.js-tab-container-labels + .dropdown-menu.dropdown-extended-height.dropdown-menu-paging.dropdown-menu-right.dropdown-menu-issues-board-new.dropdown-menu-selectable.js-tab-container-labels = render partial: "shared/issuable/label_page_default", locals: { show_footer: true, show_create: true, show_boards_content: true, title: "Add list" } - if can?(current_user, :admin_label, board.parent) = render partial: "shared/issuable/label_page_create" diff --git a/changelogs/unreleased/winh-add-list-dropdown-height.yml b/changelogs/unreleased/winh-add-list-dropdown-height.yml new file mode 100644 index 00000000000..6bcedc15cc9 --- /dev/null +++ b/changelogs/unreleased/winh-add-list-dropdown-height.yml @@ -0,0 +1,5 @@ +--- +title: Adjust height of "Add list" dropdown in issue boards +merge_request: 24227 +author: +type: fixed -- cgit v1.2.1 From 53e0a012ff7696a7ab2e3d4ff4bf3cb1db6be727 Mon Sep 17 00:00:00 2001 From: Mark Lapierre Date: Wed, 9 Jan 2019 16:04:32 -0500 Subject: Quarantine failing e2e test --- .../browser_ui/3_create/wiki/create_edit_clone_push_wiki_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/qa/qa/specs/features/browser_ui/3_create/wiki/create_edit_clone_push_wiki_spec.rb b/qa/qa/specs/features/browser_ui/3_create/wiki/create_edit_clone_push_wiki_spec.rb index 210271705d9..a7d0998d42c 100644 --- a/qa/qa/specs/features/browser_ui/3_create/wiki/create_edit_clone_push_wiki_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/wiki/create_edit_clone_push_wiki_spec.rb @@ -17,7 +17,8 @@ module QA login end - it 'user creates, edits, clones, and pushes to the wiki' do + # Failure reported: https://gitlab.com/gitlab-org/quality/nightly/issues/24 + it 'user creates, edits, clones, and pushes to the wiki', :quarantine do wiki = Resource::Wiki.fabricate! do |resource| resource.title = 'Home' resource.content = '# My First Wiki Content' -- cgit v1.2.1 From 1c248cd47ca1e02a32396c59f080c0170d81412a Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 9 Jan 2019 14:18:46 +0900 Subject: Fix unexpected exception by failed to find an actual head pipeline Add changelog --- app/models/merge_request.rb | 7 ++++--- .../unreleased/fix-udpate-head-pipeline-method.yml | 5 +++++ spec/models/merge_request_spec.rb | 17 +++++++++++++++++ 3 files changed, 26 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/fix-udpate-head-pipeline-method.yml diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5310f2ee765..a9d1ece0d7e 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -1108,9 +1108,10 @@ class MergeRequest < ActiveRecord::Base end def update_head_pipeline - self.head_pipeline = find_actual_head_pipeline - - update_column(:head_pipeline_id, head_pipeline.id) if head_pipeline_id_changed? + find_actual_head_pipeline.try do |pipeline| + self.head_pipeline = pipeline + update_column(:head_pipeline_id, head_pipeline.id) if head_pipeline_id_changed? + end end def merge_request_pipeline_exists? diff --git a/changelogs/unreleased/fix-udpate-head-pipeline-method.yml b/changelogs/unreleased/fix-udpate-head-pipeline-method.yml new file mode 100644 index 00000000000..8dbb9f8e42b --- /dev/null +++ b/changelogs/unreleased/fix-udpate-head-pipeline-method.yml @@ -0,0 +1,5 @@ +--- +title: Fix unexpected exception by failure of finding an actual head pipeline +merge_request: 24257 +author: +type: fixed diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index e18b29df321..bfc9035cb56 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -1418,6 +1418,23 @@ describe MergeRequest do .to change { merge_request.reload.head_pipeline } .from(nil).to(pipeline) end + + context 'when merge request has already had head pipeline' do + before do + merge_request.update!(head_pipeline: pipeline) + end + + context 'when failed to find an actual head pipeline' do + before do + allow(merge_request).to receive(:find_actual_head_pipeline) { } + end + + it 'does not update the current head pipeline' do + expect { subject } + .not_to change { merge_request.reload.head_pipeline } + end + end + end end context 'when there are no pipelines with the diff head sha' do -- cgit v1.2.1 From 6540a9468a8bce3f496423179db1862cfb9f5c8c Mon Sep 17 00:00:00 2001 From: Scott Escue Date: Tue, 22 May 2018 15:04:19 -0500 Subject: Preserve URL fragment across sign-in and sign-up redirects If window.location contains a URL fragment, append the fragment to all sign-in forms, the sign-up form, and all button based providers. --- app/controllers/omniauth_callbacks_controller.rb | 16 ++++++++++ app/views/devise/sessions/new.html.haml | 30 ++++++++++++++++++- .../omniauth_callbacks_controller_spec.rb | 34 ++++++++++++++++++++++ 3 files changed, 79 insertions(+), 1 deletion(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 30be50d4595..12f11976439 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -75,6 +75,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController private def omniauth_flow(auth_module, identity_linker: nil) + omniauth_params = request.env['omniauth.params'] + + if omniauth_params.present? && omniauth_params['redirect_fragment'].present? + store_redirect_fragment(omniauth_params['redirect_fragment']) + end + if current_user log_audit_event(current_user, with: oauth['provider']) @@ -189,4 +195,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController request_params = request.env['omniauth.params'] (request_params['remember_me'] == '1') if request_params.present? end + + def store_redirect_fragment(redirect_fragment) + key = stored_location_key_for(:user) + location = session[key] + unless location.to_s.strip.empty? + uri = URI.parse(location) + uri.fragment = redirect_fragment + store_location_for(:user, uri.to_s) + end + end end diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index 34d4293bd45..3840cbb0b31 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/app/views/devise/sessions/new.html.haml @@ -1,6 +1,34 @@ - page_title "Sign in" -%div +- content_for :page_specific_javascripts do + -# haml-lint:disable InlineJavaScript + :javascript + document.addEventListener('DOMContentLoaded', function() { + // Determine if the current URL location contains a fragment identifier (aka hash or anchor ref). + // This should be present if the user was redirected to sign in after attempting to access a + // protected URL that included a fragment identifier. + var fragment = window.location.hash; + if (fragment && fragment !== '') { + // Append the fragment to all signin/signup form actions so it is preserved when + // the user is eventually redirected back to the originally requested URL. + $('div#signin-container form').attr('action', function (index, action) { + return action + fragment; + }); + + // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment + // query param will be passed to the omniauth callback upon successful authentication + $('div#signin-container a.oauth-login').attr('href', function (index, href) { + const tokens = href.split('?'); + let url = tokens[0] + '?redirect_fragment=' + encodeURIComponent(fragment.substr(1)); + if (tokens.length > 1) { + url += '&' + tokens[1]; + } + return url; + }); + } + }); + +#signin-container - if form_based_providers.any? = render 'devise/shared/tabs_ldap' - else diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb index d377d69457f..21936491ffc 100644 --- a/spec/controllers/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/omniauth_callbacks_controller_spec.rb @@ -45,6 +45,40 @@ describe OmniauthCallbacksController, type: :controller do end end + context 'when a redirect fragment is provided' do + let(:provider) { :jwt } + let(:extern_uid) { 'my-uid' } + + before do + request.env['omniauth.params'] = { 'redirect_fragment' => 'L101' } + end + + context 'when a redirect url is stored' do + it 'redirects with fragment' do + post provider, nil, { user_return_to: '/fake/url' } + + expect(response).to redirect_to('/fake/url#L101') + end + end + + context 'when a redirect url with a fragment is stored' do + it 'redirects with the new fragment' do + post provider, nil, { user_return_to: '/fake/url#replaceme' } + + expect(response).to redirect_to('/fake/url#L101') + end + end + + context 'when no redirect url is stored' do + it 'does not redirect with the fragment' do + post provider + + expect(response.redirect?).to be true + expect(response.location).not_to include('#L101') + end + end + end + context 'strategies' do context 'github' do let(:extern_uid) { 'my-uid' } -- cgit v1.2.1 From 4dcaa4df3622ae267363fcff184d0929b2102035 Mon Sep 17 00:00:00 2001 From: Scott Escue Date: Mon, 4 Jun 2018 16:28:18 -0500 Subject: Addressing peer review feedback. Replacing inline JS with ES 2015 functions included in pages/sessions/new. Also applying suggested server-side syntax improvements to OmniAuthCallbacksController. --- app/assets/javascripts/lib/utils/common_utils.js | 81 ++++++++++++++++++++++ app/assets/javascripts/pages/sessions/new/index.js | 5 ++ .../pages/sessions/new/preserve_url_fragment.js | 29 ++++++++ app/controllers/omniauth_callbacks_controller.rb | 9 +-- app/views/devise/sessions/new.html.haml | 28 -------- .../fixtures/signin_forms_and_buttons.html.haml | 21 ++++++ spec/javascripts/lib/utils/common_utils_spec.js | 61 ++++++++++++++++ .../sessions/new/preserve_url_fragment_spec.js | 26 +++++++ 8 files changed, 226 insertions(+), 34 deletions(-) create mode 100644 app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js create mode 100644 spec/javascripts/fixtures/signin_forms_and_buttons.html.haml create mode 100644 spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index fc34d243dd7..ccf1d924ef2 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -180,6 +180,87 @@ export const urlParamsToObject = (path = '') => return data; }, {}); +/** + * Apply the query param and value to the given url by returning a new url string that includes + * the param/value pair. If the given url already includes the query param, the query param value + * will be updated in the new url string. Otherwise, the query param and value will by added in + * the new url string. + * + * @param url {string} - url to which the query param will be applied + * @param param {string} - name of the query param to set + * @param value {string|number} - value to give the query param + * @returns {string} A copy of the original url with the new or updated query param + */ +export const setUrlParam = (url, param, value) => { + const [rootAndQuery, fragment] = url.split('#'); + const [root, query] = rootAndQuery.split('?'); + const encodedParam = encodeURIComponent(param); + const encodedPair = `${encodedParam}=${encodeURIComponent(value)}`; + + let paramExists = false; + const paramArray = + (query ? query.split('&') : []) + .map(paramPair => { + const [foundParam] = paramPair.split('='); + if (foundParam === encodedParam) { + paramExists = true; + return encodedPair; + } + return paramPair; + }); + + if (paramExists === false) { + paramArray.push(encodedPair); + } + + const writableFragment = fragment ? `#${fragment}` : ''; + return `${root}?${paramArray.join('&')}${writableFragment}`; +}; + +/** + * Remove the query param from the given url by returning a new url string that no longer includes + * the param/value pair. + * + * @param url {string} - url from which the query param will be removed + * @param param {string} - the name of the query param to remove + * @returns {string} A copy of the original url but without the query param + */ +export const removeUrlParam = (url, param) => { + const [rootAndQuery, fragment] = url.split('#'); + const [root, query] = rootAndQuery.split('?'); + + if (query === undefined) { + return url; + } + + const encodedParam = encodeURIComponent(param); + const updatedQuery = query + .split('&') + .filter(paramPair => { + const [foundParam] = paramPair.split('='); + return foundParam !== encodedParam; + }) + .join('&'); + + const writableQuery = updatedQuery.length > 0 ? `?${updatedQuery}` : ''; + const writableFragment = fragment ? `#${fragment}` : ''; + return `${root}${writableQuery}${writableFragment}`; +}; + +/** + * Apply the fragment to the given url by returning a new url string that includes + * the fragment. If the given url already contains a fragment, the original fragment + * will be removed. + * + * @param url {string} - url to which the fragment will be applied + * @param fragment {string} - fragment to append + */ +export const setUrlFragment = (url, fragment) => { + const [rootUrl] = url.split('#'); + const encodedFragment = encodeURIComponent(fragment.replace(/^#/, '')); + return `${rootUrl}#${encodedFragment}`; +}; + export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; // Identify following special clicks diff --git a/app/assets/javascripts/pages/sessions/new/index.js b/app/assets/javascripts/pages/sessions/new/index.js index 07f32210d93..d54bff88f70 100644 --- a/app/assets/javascripts/pages/sessions/new/index.js +++ b/app/assets/javascripts/pages/sessions/new/index.js @@ -2,6 +2,7 @@ import $ from 'jquery'; import UsernameValidator from './username_validator'; import SigninTabsMemoizer from './signin_tabs_memoizer'; import OAuthRememberMe from './oauth_remember_me'; +import preserveUrlFragment from './preserve_url_fragment'; document.addEventListener('DOMContentLoaded', () => { new UsernameValidator(); // eslint-disable-line no-new @@ -10,4 +11,8 @@ document.addEventListener('DOMContentLoaded', () => { new OAuthRememberMe({ container: $('.omniauth-container'), }).bindEvents(); + + // Save the URL fragment from the current window location. This will be present if the user was + // redirected to sign-in after attempting to access a protected URL that included a fragment. + preserveUrlFragment(window.location.hash); }); diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js new file mode 100644 index 00000000000..82ac59224df --- /dev/null +++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js @@ -0,0 +1,29 @@ +import { setUrlFragment, setUrlParam } from '../../../lib/utils/common_utils'; + +/** + * Ensure the given URL fragment is preserved by appending it to sign-in/sign-up form actions and + * OAuth/SAML login links. + * + * @param fragment {string} - url fragment to be preserved + */ +export default function preserveUrlFragment(fragment) { + if (fragment && fragment !== '') { + const normalFragment = fragment.replace(/^#/, ''); + + // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is + // eventually redirected back to the originally requested URL. + const forms = document.querySelectorAll('#signin-container form'); + Array.prototype.forEach.call(forms, (form) => { + const actionWithFragment = setUrlFragment(form.getAttribute('action'), `#${normalFragment}`); + form.setAttribute('action', actionWithFragment); + }); + + // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment + // query param will be available in the omniauth callback upon successful authentication + const anchors = document.querySelectorAll('#signin-container a.oauth-login'); + Array.prototype.forEach.call(anchors, (anchor) => { + const newHref = setUrlParam(anchor.getAttribute('href'), 'redirect_fragment', normalFragment); + anchor.setAttribute('href', newHref); + }); + } +} diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index 12f11976439..f8e482937d5 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -75,10 +75,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController private def omniauth_flow(auth_module, identity_linker: nil) - omniauth_params = request.env['omniauth.params'] - - if omniauth_params.present? && omniauth_params['redirect_fragment'].present? - store_redirect_fragment(omniauth_params['redirect_fragment']) + if fragment = request.env.dig('omniauth.params', 'redirect_fragment').presence + store_redirect_fragment(fragment) end if current_user @@ -199,8 +197,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def store_redirect_fragment(redirect_fragment) key = stored_location_key_for(:user) location = session[key] - unless location.to_s.strip.empty? - uri = URI.parse(location) + if uri = parse_uri(location) uri.fragment = redirect_fragment store_location_for(:user, uri.to_s) end diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml index 3840cbb0b31..30ed7ed6b29 100644 --- a/app/views/devise/sessions/new.html.haml +++ b/app/views/devise/sessions/new.html.haml @@ -1,33 +1,5 @@ - page_title "Sign in" -- content_for :page_specific_javascripts do - -# haml-lint:disable InlineJavaScript - :javascript - document.addEventListener('DOMContentLoaded', function() { - // Determine if the current URL location contains a fragment identifier (aka hash or anchor ref). - // This should be present if the user was redirected to sign in after attempting to access a - // protected URL that included a fragment identifier. - var fragment = window.location.hash; - if (fragment && fragment !== '') { - // Append the fragment to all signin/signup form actions so it is preserved when - // the user is eventually redirected back to the originally requested URL. - $('div#signin-container form').attr('action', function (index, action) { - return action + fragment; - }); - - // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment - // query param will be passed to the omniauth callback upon successful authentication - $('div#signin-container a.oauth-login').attr('href', function (index, href) { - const tokens = href.split('?'); - let url = tokens[0] + '?redirect_fragment=' + encodeURIComponent(fragment.substr(1)); - if (tokens.length > 1) { - url += '&' + tokens[1]; - } - return url; - }); - } - }); - #signin-container - if form_based_providers.any? = render 'devise/shared/tabs_ldap' diff --git a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml new file mode 100644 index 00000000000..32a9becb636 --- /dev/null +++ b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml @@ -0,0 +1,21 @@ +#signin-container + .tab-content + .active.login-box.tab-pane + .login-body + %form#new_ldap_user{ action: '/users/auth/ldapmain/callback', method: 'post' } + + .login-box.tab-pane + .login-body + %form#new_user.new_user{ action: '/users/sign_in', method: 'post' } + + #register-pane.login-box.tab-pane + .login-body + %form#new_new_user.new_new_user{ action: '/users', method: 'post' } + + .omniauth-container + %span.light + %a#oauth-login-auth0.oauth-login.btn{ href: '/users/auth/auth0' } Auth0 + %span.light + %a#oauth-login-facebook.oauth-login.btn{ href:'/users/auth/facebook?remember_me=1' } Facebook + %span.light + %a#oauth-login-saml.oauth-login.btn{ href:'/users/auth/saml' } SAML diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index f320f232687..3a25be766cb 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -65,6 +65,67 @@ describe('common_utils', () => { }); }); + describe('setUrlParam', () => { + it('should append param when url has no other params', () => { + const url = commonUtils.setUrlParam('/feature/home', 'newParam', 'yes'); + expect(url).toBe('/feature/home?newParam=yes'); + }); + + it('should append param when url has other params', () => { + const url = commonUtils.setUrlParam('/feature/home?showAll=true', 'newParam', 'yes'); + expect(url).toBe('/feature/home?showAll=true&newParam=yes'); + }); + + it('should replace param when url contains the param', () => { + const url = commonUtils.setUrlParam('/feature/home?showAll=true&limit=5', 'limit', '100'); + expect(url).toBe('/feature/home?showAll=true&limit=100'); + }); + + it('should update param and preserve fragment', () => { + const url = commonUtils.setUrlParam('/home?q=no&limit=5&showAll=true#H1', 'limit', '100'); + expect(url).toBe('/home?q=no&limit=100&showAll=true#H1'); + }); + }); + + describe('removeUrlParam', () => { + it('should remove param when url has no other params', () => { + const url = commonUtils.removeUrlParam('/feature/home?size=5', 'size'); + expect(url).toBe('/feature/home'); + }); + + it('should remove param when url has other params', () => { + const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'size'); + expect(url).toBe('/feature/home?q=1&f=html'); + }); + + it('should remove param and preserve fragment', () => { + const url = commonUtils.removeUrlParam('/feature/home?size=5#H2', 'size'); + expect(url).toBe('/feature/home#H2'); + }); + + it('should not modify url if param does not exist', () => { + const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'locale'); + expect(url).toBe('/feature/home?q=1&size=5&f=html'); + }); + }); + + describe('setUrlFragment', () => { + it('should set fragment when url has no fragment', () => { + const url = commonUtils.setUrlFragment('/home/feature', 'usage'); + expect(url).toBe('/home/feature#usage'); + }); + + it('should set fragment when url has existing fragment', () => { + const url = commonUtils.setUrlFragment('/home/feature#overview', 'usage'); + expect(url).toBe('/home/feature#usage'); + }); + + it('should set fragment when given fragment includes #', () => { + const url = commonUtils.setUrlFragment('/home/feature#overview', '#install'); + expect(url).toBe('/home/feature#install'); + }); + }); + describe('handleLocationHash', () => { beforeEach(() => { spyOn(window.document, 'getElementById').and.callThrough(); diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js new file mode 100644 index 00000000000..c3be06ce6f9 --- /dev/null +++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js @@ -0,0 +1,26 @@ +import $ from 'jquery'; +import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment'; + +describe('preserve_url_fragment', () => { + preloadFixtures('static/signin_forms_and_buttons.html.raw'); + + beforeEach(() => { + loadFixtures('static/signin_forms_and_buttons.html.raw'); + }); + + it('adds the url fragment to all login and sign up form actions', () => { + preserveUrlFragment('#L65'); + + expect($('#new_ldap_user').attr('action')).toBe('/users/auth/ldapmain/callback#L65'); + expect($('#new_user').attr('action')).toBe('/users/sign_in#L65'); + expect($('#new_new_user').attr('action')).toBe('/users#L65'); + }); + + it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => { + preserveUrlFragment('#L65'); + + expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe('/users/auth/auth0?redirect_fragment=L65'); + expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe('/users/auth/facebook?remember_me=1&redirect_fragment=L65'); + expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe('/users/auth/saml?redirect_fragment=L65'); + }); +}); -- cgit v1.2.1 From 6b067fe470857a478939a6037280beb07cf9680d Mon Sep 17 00:00:00 2001 From: Scott Escue Date: Mon, 4 Jun 2018 16:30:59 -0500 Subject: Updating OAuthRememberMe to use new common utility functions when manipulating query parameters on OAuth buttons. This ensures the 'remember_me' parameter is safely added and removed when other query parameters are present. --- app/assets/javascripts/pages/sessions/new/oauth_remember_me.js | 5 +++-- spec/javascripts/fixtures/oauth_remember_me.html.haml | 1 + spec/javascripts/oauth_remember_me_spec.js | 5 +++++ 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js index 761618109a4..0c6ccd6e495 100644 --- a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js +++ b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js @@ -1,4 +1,5 @@ import $ from 'jquery'; +import { setUrlParam, removeUrlParam } from '~/lib/utils/common_utils'; /** * OAuth-based login buttons have a separate "remember me" checkbox. @@ -24,9 +25,9 @@ export default class OAuthRememberMe { const href = $(element).attr('href'); if (rememberMe) { - $(element).attr('href', `${href}?remember_me=1`); + $(element).attr('href', setUrlParam(href, 'remember_me', 1)); } else { - $(element).attr('href', href.replace('?remember_me=1', '')); + $(element).attr('href', removeUrlParam(href, 'remember_me')); } }); } diff --git a/spec/javascripts/fixtures/oauth_remember_me.html.haml b/spec/javascripts/fixtures/oauth_remember_me.html.haml index 7886e995e57..a5d7c4e816a 100644 --- a/spec/javascripts/fixtures/oauth_remember_me.html.haml +++ b/spec/javascripts/fixtures/oauth_remember_me.html.haml @@ -3,3 +3,4 @@ %a.oauth-login.twitter{ href: "http://example.com/" } %a.oauth-login.github{ href: "http://example.com/" } + %a.oauth-login.facebook{ href: "http://example.com/?redirect_fragment=L1" } diff --git a/spec/javascripts/oauth_remember_me_spec.js b/spec/javascripts/oauth_remember_me_spec.js index 2caa266b85f..f363aa7a407 100644 --- a/spec/javascripts/oauth_remember_me_spec.js +++ b/spec/javascripts/oauth_remember_me_spec.js @@ -20,6 +20,10 @@ describe('OAuthRememberMe', () => { expect($('#oauth-container .oauth-login.github').attr('href')).toBe( 'http://example.com/?remember_me=1', ); + + expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe( + 'http://example.com/?redirect_fragment=L1&remember_me=1' + ); }); it('removes the "remember_me" query parameter from all OAuth login buttons', () => { @@ -28,5 +32,6 @@ describe('OAuthRememberMe', () => { expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/'); expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/'); + expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe('http://example.com/?redirect_fragment=L1'); }); }); -- cgit v1.2.1 From a3541a8d8dd1f4db690b7293d2a7674287020e84 Mon Sep 17 00:00:00 2001 From: Scott Escue Date: Thu, 7 Jun 2018 23:06:44 -0500 Subject: Removing the URL manipulation functions added to 'common_utils.js' in favor of the functions that already existed in 'url_utility.js'. Refactoring 'removeParams' function in 'url_utility.js' to allow url to be passed and to preserve the original host and/or path provided in the url. --- app/assets/javascripts/lib/utils/common_utils.js | 89 ++-------------------- app/assets/javascripts/lib/utils/url_utility.js | 57 ++++++++++---- .../pages/sessions/new/oauth_remember_me.js | 6 +- .../pages/sessions/new/preserve_url_fragment.js | 4 +- spec/javascripts/lib/utils/common_utils_spec.js | 61 --------------- spec/javascripts/lib/utils/url_utility_spec.js | 82 +++++++++++++++++--- 6 files changed, 127 insertions(+), 172 deletions(-) diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index ccf1d924ef2..afdca012127 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -4,6 +4,14 @@ import { getLocationHash } from './url_utility'; import { convertToCamelCase } from './text_utility'; import { isObject } from './type_utility'; +/** + * Simply returns `window.location`. This function exists to provide a means to spy + * `window.location` in unit tests. + * + * @returns {Location | string | any} The browser's `window.location` + */ +export const windowLocation = () => window.location; + export const getPagePath = (index = 0) => { const page = $('body').attr('data-page') || ''; @@ -180,87 +188,6 @@ export const urlParamsToObject = (path = '') => return data; }, {}); -/** - * Apply the query param and value to the given url by returning a new url string that includes - * the param/value pair. If the given url already includes the query param, the query param value - * will be updated in the new url string. Otherwise, the query param and value will by added in - * the new url string. - * - * @param url {string} - url to which the query param will be applied - * @param param {string} - name of the query param to set - * @param value {string|number} - value to give the query param - * @returns {string} A copy of the original url with the new or updated query param - */ -export const setUrlParam = (url, param, value) => { - const [rootAndQuery, fragment] = url.split('#'); - const [root, query] = rootAndQuery.split('?'); - const encodedParam = encodeURIComponent(param); - const encodedPair = `${encodedParam}=${encodeURIComponent(value)}`; - - let paramExists = false; - const paramArray = - (query ? query.split('&') : []) - .map(paramPair => { - const [foundParam] = paramPair.split('='); - if (foundParam === encodedParam) { - paramExists = true; - return encodedPair; - } - return paramPair; - }); - - if (paramExists === false) { - paramArray.push(encodedPair); - } - - const writableFragment = fragment ? `#${fragment}` : ''; - return `${root}?${paramArray.join('&')}${writableFragment}`; -}; - -/** - * Remove the query param from the given url by returning a new url string that no longer includes - * the param/value pair. - * - * @param url {string} - url from which the query param will be removed - * @param param {string} - the name of the query param to remove - * @returns {string} A copy of the original url but without the query param - */ -export const removeUrlParam = (url, param) => { - const [rootAndQuery, fragment] = url.split('#'); - const [root, query] = rootAndQuery.split('?'); - - if (query === undefined) { - return url; - } - - const encodedParam = encodeURIComponent(param); - const updatedQuery = query - .split('&') - .filter(paramPair => { - const [foundParam] = paramPair.split('='); - return foundParam !== encodedParam; - }) - .join('&'); - - const writableQuery = updatedQuery.length > 0 ? `?${updatedQuery}` : ''; - const writableFragment = fragment ? `#${fragment}` : ''; - return `${root}${writableQuery}${writableFragment}`; -}; - -/** - * Apply the fragment to the given url by returning a new url string that includes - * the fragment. If the given url already contains a fragment, the original fragment - * will be removed. - * - * @param url {string} - url to which the fragment will be applied - * @param fragment {string} - fragment to append - */ -export const setUrlFragment = (url, fragment) => { - const [rootUrl] = url.split('#'); - const encodedFragment = encodeURIComponent(fragment.replace(/^#/, '')); - return `${rootUrl}#${encodedFragment}`; -}; - export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey; // Identify following special clicks diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js index 9850f7ce782..61f53a632b8 100644 --- a/app/assets/javascripts/lib/utils/url_utility.js +++ b/app/assets/javascripts/lib/utils/url_utility.js @@ -1,3 +1,5 @@ +import { windowLocation } from './common_utils'; + // Returns an array containing the value(s) of the // of the key passed as an argument export function getParameterValues(sParam) { @@ -42,22 +44,35 @@ export function mergeUrlParams(params, url) { return `${urlparts[1]}?${query}${urlparts[3]}`; } -export function removeParamQueryString(url, param) { - const decodedUrl = decodeURIComponent(url); - const urlVariables = decodedUrl.split('&'); - - return urlVariables.filter(variable => variable.indexOf(param) === -1).join('&'); -} - -export function removeParams(params, source = window.location.href) { - const url = document.createElement('a'); - url.href = source; +/** + * Removes specified query params from the url by returning a new url string that no longer + * includes the param/value pair. If no url is provided, `window.location.href` is used as + * the default value. + * + * @param {string[]} params - the query param names to remove + * @param {string} [url=windowLocation().href] - url from which the query param will be removed + * @returns {string} A copy of the original url but without the query param + */ +export function removeParams(params, url = windowLocation().href) { + const [rootAndQuery, fragment] = url.split('#'); + const [root, query] = rootAndQuery.split('?'); + + if (query === undefined) { + return url; + } - params.forEach(param => { - url.search = removeParamQueryString(url.search, param); - }); + const encodedParams = params.map(param => encodeURIComponent(param)); + const updatedQuery = query + .split('&') + .filter(paramPair => { + const [foundParam] = paramPair.split('='); + return encodedParams.indexOf(foundParam) < 0; + }) + .join('&'); - return url.href; + const writableQuery = updatedQuery.length > 0 ? `?${updatedQuery}` : ''; + const writableFragment = fragment ? `#${fragment}` : ''; + return `${root}${writableQuery}${writableFragment}`; } export function getLocationHash(url = window.location.href) { @@ -66,6 +81,20 @@ export function getLocationHash(url = window.location.href) { return hashIndex === -1 ? null : url.substring(hashIndex + 1); } +/** + * Apply the fragment to the given url by returning a new url string that includes + * the fragment. If the given url already contains a fragment, the original fragment + * will be removed. + * + * @param {string} url - url to which the fragment will be applied + * @param {string} fragment - fragment to append + */ +export const setUrlFragment = (url, fragment) => { + const [rootUrl] = url.split('#'); + const encodedFragment = encodeURIComponent(fragment.replace(/^#/, '')); + return `${rootUrl}#${encodedFragment}`; +}; + export function visitUrl(url, external = false) { if (external) { // Simulate `target="blank" rel="noopener noreferrer"` diff --git a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js index 0c6ccd6e495..191221a48cd 100644 --- a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js +++ b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js @@ -1,5 +1,5 @@ import $ from 'jquery'; -import { setUrlParam, removeUrlParam } from '~/lib/utils/common_utils'; +import { mergeUrlParams, removeParams } from '~/lib/utils/url_utility'; /** * OAuth-based login buttons have a separate "remember me" checkbox. @@ -25,9 +25,9 @@ export default class OAuthRememberMe { const href = $(element).attr('href'); if (rememberMe) { - $(element).attr('href', setUrlParam(href, 'remember_me', 1)); + $(element).attr('href', mergeUrlParams({ remember_me: 1 }, href)); } else { - $(element).attr('href', removeUrlParam(href, 'remember_me')); + $(element).attr('href', removeParams(['remember_me'], href)); } }); } diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js index 82ac59224df..71b7ca8ec31 100644 --- a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js +++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js @@ -1,4 +1,4 @@ -import { setUrlFragment, setUrlParam } from '../../../lib/utils/common_utils'; +import { mergeUrlParams, setUrlFragment } from '~/lib/utils/url_utility'; /** * Ensure the given URL fragment is preserved by appending it to sign-in/sign-up form actions and @@ -22,7 +22,7 @@ export default function preserveUrlFragment(fragment) { // query param will be available in the omniauth callback upon successful authentication const anchors = document.querySelectorAll('#signin-container a.oauth-login'); Array.prototype.forEach.call(anchors, (anchor) => { - const newHref = setUrlParam(anchor.getAttribute('href'), 'redirect_fragment', normalFragment); + const newHref = mergeUrlParams({ redirect_fragment: normalFragment }, anchor.getAttribute('href')); anchor.setAttribute('href', newHref); }); } diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 3a25be766cb..f320f232687 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -65,67 +65,6 @@ describe('common_utils', () => { }); }); - describe('setUrlParam', () => { - it('should append param when url has no other params', () => { - const url = commonUtils.setUrlParam('/feature/home', 'newParam', 'yes'); - expect(url).toBe('/feature/home?newParam=yes'); - }); - - it('should append param when url has other params', () => { - const url = commonUtils.setUrlParam('/feature/home?showAll=true', 'newParam', 'yes'); - expect(url).toBe('/feature/home?showAll=true&newParam=yes'); - }); - - it('should replace param when url contains the param', () => { - const url = commonUtils.setUrlParam('/feature/home?showAll=true&limit=5', 'limit', '100'); - expect(url).toBe('/feature/home?showAll=true&limit=100'); - }); - - it('should update param and preserve fragment', () => { - const url = commonUtils.setUrlParam('/home?q=no&limit=5&showAll=true#H1', 'limit', '100'); - expect(url).toBe('/home?q=no&limit=100&showAll=true#H1'); - }); - }); - - describe('removeUrlParam', () => { - it('should remove param when url has no other params', () => { - const url = commonUtils.removeUrlParam('/feature/home?size=5', 'size'); - expect(url).toBe('/feature/home'); - }); - - it('should remove param when url has other params', () => { - const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'size'); - expect(url).toBe('/feature/home?q=1&f=html'); - }); - - it('should remove param and preserve fragment', () => { - const url = commonUtils.removeUrlParam('/feature/home?size=5#H2', 'size'); - expect(url).toBe('/feature/home#H2'); - }); - - it('should not modify url if param does not exist', () => { - const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'locale'); - expect(url).toBe('/feature/home?q=1&size=5&f=html'); - }); - }); - - describe('setUrlFragment', () => { - it('should set fragment when url has no fragment', () => { - const url = commonUtils.setUrlFragment('/home/feature', 'usage'); - expect(url).toBe('/home/feature#usage'); - }); - - it('should set fragment when url has existing fragment', () => { - const url = commonUtils.setUrlFragment('/home/feature#overview', 'usage'); - expect(url).toBe('/home/feature#usage'); - }); - - it('should set fragment when given fragment includes #', () => { - const url = commonUtils.setUrlFragment('/home/feature#overview', '#install'); - expect(url).toBe('/home/feature#install'); - }); - }); - describe('handleLocationHash', () => { beforeEach(() => { spyOn(window.document, 'getElementById').and.callThrough(); diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js index e4df8441793..fe787baf08e 100644 --- a/spec/javascripts/lib/utils/url_utility_spec.js +++ b/spec/javascripts/lib/utils/url_utility_spec.js @@ -1,4 +1,4 @@ -import { webIDEUrl, mergeUrlParams } from '~/lib/utils/url_utility'; +import UrlUtility, * as urlUtils from '~/lib/utils/url_utility'; describe('URL utility', () => { describe('webIDEUrl', () => { @@ -8,7 +8,7 @@ describe('URL utility', () => { describe('without relative_url_root', () => { it('returns IDE path with route', () => { - expect(webIDEUrl('/gitlab-org/gitlab-ce/merge_requests/1')).toBe( + expect(urlUtils.webIDEUrl('/gitlab-org/gitlab-ce/merge_requests/1')).toBe( '/-/ide/project/gitlab-org/gitlab-ce/merge_requests/1', ); }); @@ -20,7 +20,7 @@ describe('URL utility', () => { }); it('returns IDE path with route', () => { - expect(webIDEUrl('/gitlab/gitlab-org/gitlab-ce/merge_requests/1')).toBe( + expect(urlUtils.webIDEUrl('/gitlab/gitlab-org/gitlab-ce/merge_requests/1')).toBe( '/gitlab/-/ide/project/gitlab-org/gitlab-ce/merge_requests/1', ); }); @@ -29,23 +29,83 @@ describe('URL utility', () => { describe('mergeUrlParams', () => { it('adds w', () => { - expect(mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag'); - expect(mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag'); - expect(mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1'); - expect(mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag'); - expect(mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag'); + expect(urlUtils.mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag'); + expect(urlUtils.mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag'); + expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1'); + expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag'); + expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag'); }); it('updates w', () => { - expect(mergeUrlParams({ w: 1 }, '?k1=v1&w=0#frag')).toBe('?k1=v1&w=1#frag'); + expect(urlUtils.mergeUrlParams({ w: 1 }, '?k1=v1&w=0#frag')).toBe('?k1=v1&w=1#frag'); }); it('adds multiple params', () => { - expect(mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag'); + expect(urlUtils.mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag'); }); it('adds and updates encoded params', () => { - expect(mergeUrlParams({ a: '&', q: '?' }, '?a=%23#frag')).toBe('?a=%26&q=%3F#frag'); + expect(urlUtils.mergeUrlParams({ a: '&', q: '?' }, '?a=%23#frag')).toBe('?a=%26&q=%3F#frag'); + }); + }); + + describe('removeParams', () => { + describe('when url is passed', () => { + it('removes query param with encoded ampersand', () => { + const url = urlUtils.removeParams(['filter'], '/mail?filter=n%3Djoe%26l%3Dhome'); + + expect(url).toBe('/mail'); + }); + + it('should remove param when url has no other params', () => { + const url = urlUtils.removeParams(['size'], '/feature/home?size=5'); + expect(url).toBe('/feature/home'); + }); + + it('should remove param when url has other params', () => { + const url = urlUtils.removeParams(['size'], '/feature/home?q=1&size=5&f=html'); + expect(url).toBe('/feature/home?q=1&f=html'); + }); + + it('should remove param and preserve fragment', () => { + const url = urlUtils.removeParams(['size'], '/feature/home?size=5#H2'); + expect(url).toBe('/feature/home#H2'); + }); + + it('should remove multiple params', () => { + const url = urlUtils.removeParams(['z', 'a'], '/home?z=11111&l=en_US&a=true#H2'); + expect(url).toBe('/home?l=en_US#H2'); + }); + }); + + describe('when no url is passed', () => { + it('should remove params from window.location.href', () => { + spyOnDependency(UrlUtility, 'windowLocation').and.callFake(() => { + const anchor = document.createElement('a'); + anchor.href = 'https://mysite.com/?zip=11111&locale=en_US&ads=false#privacy'; + return anchor; + }); + + const url = urlUtils.removeParams(['locale']); + expect(url).toBe('https://mysite.com/?zip=11111&ads=false#privacy'); + }); + }); + }); + + describe('setUrlFragment', () => { + it('should set fragment when url has no fragment', () => { + const url = urlUtils.setUrlFragment('/home/feature', 'usage'); + expect(url).toBe('/home/feature#usage'); + }); + + it('should set fragment when url has existing fragment', () => { + const url = urlUtils.setUrlFragment('/home/feature#overview', 'usage'); + expect(url).toBe('/home/feature#usage'); + }); + + it('should set fragment when given fragment includes #', () => { + const url = urlUtils.setUrlFragment('/home/feature#overview', '#install'); + expect(url).toBe('/home/feature#install'); }); }); }); -- cgit v1.2.1 From 2cbc475e5327a860032414561916c7bd725685ac Mon Sep 17 00:00:00 2001 From: Scott Escue Date: Tue, 27 Nov 2018 07:16:24 -0600 Subject: Fixing static analysis issues --- .../pages/sessions/new/preserve_url_fragment.js | 9 ++++++--- spec/javascripts/lib/utils/url_utility_spec.js | 17 +++++++++++++++-- spec/javascripts/oauth_remember_me_spec.js | 6 ++++-- .../pages/sessions/new/preserve_url_fragment_spec.js | 14 +++++++++++--- 4 files changed, 36 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js index 71b7ca8ec31..56ea39f1259 100644 --- a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js +++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js @@ -13,7 +13,7 @@ export default function preserveUrlFragment(fragment) { // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is // eventually redirected back to the originally requested URL. const forms = document.querySelectorAll('#signin-container form'); - Array.prototype.forEach.call(forms, (form) => { + Array.prototype.forEach.call(forms, form => { const actionWithFragment = setUrlFragment(form.getAttribute('action'), `#${normalFragment}`); form.setAttribute('action', actionWithFragment); }); @@ -21,8 +21,11 @@ export default function preserveUrlFragment(fragment) { // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment // query param will be available in the omniauth callback upon successful authentication const anchors = document.querySelectorAll('#signin-container a.oauth-login'); - Array.prototype.forEach.call(anchors, (anchor) => { - const newHref = mergeUrlParams({ redirect_fragment: normalFragment }, anchor.getAttribute('href')); + Array.prototype.forEach.call(anchors, anchor => { + const newHref = mergeUrlParams( + { redirect_fragment: normalFragment }, + anchor.getAttribute('href'), + ); anchor.setAttribute('href', newHref); }); } diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js index fe787baf08e..1ac1c414df7 100644 --- a/spec/javascripts/lib/utils/url_utility_spec.js +++ b/spec/javascripts/lib/utils/url_utility_spec.js @@ -32,8 +32,13 @@ describe('URL utility', () => { expect(urlUtils.mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag'); expect(urlUtils.mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag'); expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1'); - expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag'); - expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag'); + expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe( + 'https://host/path?w=1#frag', + ); + + expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe( + 'https://h/p?k1=v1&w=1#frag', + ); }); it('updates w', () => { @@ -59,21 +64,25 @@ describe('URL utility', () => { it('should remove param when url has no other params', () => { const url = urlUtils.removeParams(['size'], '/feature/home?size=5'); + expect(url).toBe('/feature/home'); }); it('should remove param when url has other params', () => { const url = urlUtils.removeParams(['size'], '/feature/home?q=1&size=5&f=html'); + expect(url).toBe('/feature/home?q=1&f=html'); }); it('should remove param and preserve fragment', () => { const url = urlUtils.removeParams(['size'], '/feature/home?size=5#H2'); + expect(url).toBe('/feature/home#H2'); }); it('should remove multiple params', () => { const url = urlUtils.removeParams(['z', 'a'], '/home?z=11111&l=en_US&a=true#H2'); + expect(url).toBe('/home?l=en_US#H2'); }); }); @@ -87,6 +96,7 @@ describe('URL utility', () => { }); const url = urlUtils.removeParams(['locale']); + expect(url).toBe('https://mysite.com/?zip=11111&ads=false#privacy'); }); }); @@ -95,16 +105,19 @@ describe('URL utility', () => { describe('setUrlFragment', () => { it('should set fragment when url has no fragment', () => { const url = urlUtils.setUrlFragment('/home/feature', 'usage'); + expect(url).toBe('/home/feature#usage'); }); it('should set fragment when url has existing fragment', () => { const url = urlUtils.setUrlFragment('/home/feature#overview', 'usage'); + expect(url).toBe('/home/feature#usage'); }); it('should set fragment when given fragment includes #', () => { const url = urlUtils.setUrlFragment('/home/feature#overview', '#install'); + expect(url).toBe('/home/feature#install'); }); }); diff --git a/spec/javascripts/oauth_remember_me_spec.js b/spec/javascripts/oauth_remember_me_spec.js index f363aa7a407..4125706a407 100644 --- a/spec/javascripts/oauth_remember_me_spec.js +++ b/spec/javascripts/oauth_remember_me_spec.js @@ -22,7 +22,7 @@ describe('OAuthRememberMe', () => { ); expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe( - 'http://example.com/?redirect_fragment=L1&remember_me=1' + 'http://example.com/?redirect_fragment=L1&remember_me=1', ); }); @@ -32,6 +32,8 @@ describe('OAuthRememberMe', () => { expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/'); expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/'); - expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe('http://example.com/?redirect_fragment=L1'); + expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe( + 'http://example.com/?redirect_fragment=L1', + ); }); }); diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js index c3be06ce6f9..de308127a0f 100644 --- a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js +++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js @@ -19,8 +19,16 @@ describe('preserve_url_fragment', () => { it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => { preserveUrlFragment('#L65'); - expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe('/users/auth/auth0?redirect_fragment=L65'); - expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe('/users/auth/facebook?remember_me=1&redirect_fragment=L65'); - expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe('/users/auth/saml?redirect_fragment=L65'); + expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe( + '/users/auth/auth0?redirect_fragment=L65', + ); + + expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe( + '/users/auth/facebook?remember_me=1&redirect_fragment=L65', + ); + + expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe( + '/users/auth/saml?redirect_fragment=L65', + ); }); }); -- cgit v1.2.1 From 87c571f8a3e9af9de0d979dc26f9838bb0fc924d Mon Sep 17 00:00:00 2001 From: Scott Escue Date: Sun, 2 Dec 2018 00:46:11 -0600 Subject: Addressing feedback from most recent reviews. --- app/assets/javascripts/lib/utils/common_utils.js | 8 ---- app/assets/javascripts/lib/utils/url_utility.js | 6 +-- .../pages/sessions/new/preserve_url_fragment.js | 4 +- spec/javascripts/fixtures/sessions.rb | 26 ++++++++++ .../fixtures/signin_forms_and_buttons.html.haml | 21 --------- spec/javascripts/lib/utils/url_utility_spec.js | 16 +------ .../sessions/new/preserve_url_fragment_spec.js | 55 ++++++++++++++++------ 7 files changed, 72 insertions(+), 64 deletions(-) create mode 100644 spec/javascripts/fixtures/sessions.rb delete mode 100644 spec/javascripts/fixtures/signin_forms_and_buttons.html.haml diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index afdca012127..fc34d243dd7 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -4,14 +4,6 @@ import { getLocationHash } from './url_utility'; import { convertToCamelCase } from './text_utility'; import { isObject } from './type_utility'; -/** - * Simply returns `window.location`. This function exists to provide a means to spy - * `window.location` in unit tests. - * - * @returns {Location | string | any} The browser's `window.location` - */ -export const windowLocation = () => window.location; - export const getPagePath = (index = 0) => { const page = $('body').attr('data-page') || ''; diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js index 61f53a632b8..4ba84589705 100644 --- a/app/assets/javascripts/lib/utils/url_utility.js +++ b/app/assets/javascripts/lib/utils/url_utility.js @@ -1,5 +1,3 @@ -import { windowLocation } from './common_utils'; - // Returns an array containing the value(s) of the // of the key passed as an argument export function getParameterValues(sParam) { @@ -53,11 +51,11 @@ export function mergeUrlParams(params, url) { * @param {string} [url=windowLocation().href] - url from which the query param will be removed * @returns {string} A copy of the original url but without the query param */ -export function removeParams(params, url = windowLocation().href) { +export function removeParams(params, url = window.location.href) { const [rootAndQuery, fragment] = url.split('#'); const [root, query] = rootAndQuery.split('?'); - if (query === undefined) { + if (!query) { return url; } diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js index 56ea39f1259..e617fecaa0f 100644 --- a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js +++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js @@ -6,8 +6,8 @@ import { mergeUrlParams, setUrlFragment } from '~/lib/utils/url_utility'; * * @param fragment {string} - url fragment to be preserved */ -export default function preserveUrlFragment(fragment) { - if (fragment && fragment !== '') { +export default function preserveUrlFragment(fragment = '') { + if (fragment) { const normalFragment = fragment.replace(/^#/, ''); // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is diff --git a/spec/javascripts/fixtures/sessions.rb b/spec/javascripts/fixtures/sessions.rb new file mode 100644 index 00000000000..e90a58e8c54 --- /dev/null +++ b/spec/javascripts/fixtures/sessions.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe 'Sessions (JavaScript fixtures)' do + include JavaScriptFixturesHelpers + + before(:all) do + clean_frontend_fixtures('sessions/') + end + + describe SessionsController, '(JavaScript fixtures)', type: :controller do + include DeviseHelpers + + render_views + + before do + set_devise_mapping(context: @request) + end + + it 'sessions/new.html.raw' do |example| + get :new + + expect(response).to be_success + store_frontend_fixture(response, example.description) + end + end +end diff --git a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml deleted file mode 100644 index 32a9becb636..00000000000 --- a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml +++ /dev/null @@ -1,21 +0,0 @@ -#signin-container - .tab-content - .active.login-box.tab-pane - .login-body - %form#new_ldap_user{ action: '/users/auth/ldapmain/callback', method: 'post' } - - .login-box.tab-pane - .login-body - %form#new_user.new_user{ action: '/users/sign_in', method: 'post' } - - #register-pane.login-box.tab-pane - .login-body - %form#new_new_user.new_new_user{ action: '/users', method: 'post' } - - .omniauth-container - %span.light - %a#oauth-login-auth0.oauth-login.btn{ href: '/users/auth/auth0' } Auth0 - %span.light - %a#oauth-login-facebook.oauth-login.btn{ href:'/users/auth/facebook?remember_me=1' } Facebook - %span.light - %a#oauth-login-saml.oauth-login.btn{ href:'/users/auth/saml' } SAML diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js index 1ac1c414df7..381c7b2d0a6 100644 --- a/spec/javascripts/lib/utils/url_utility_spec.js +++ b/spec/javascripts/lib/utils/url_utility_spec.js @@ -1,4 +1,4 @@ -import UrlUtility, * as urlUtils from '~/lib/utils/url_utility'; +import * as urlUtils from '~/lib/utils/url_utility'; describe('URL utility', () => { describe('webIDEUrl', () => { @@ -86,20 +86,6 @@ describe('URL utility', () => { expect(url).toBe('/home?l=en_US#H2'); }); }); - - describe('when no url is passed', () => { - it('should remove params from window.location.href', () => { - spyOnDependency(UrlUtility, 'windowLocation').and.callFake(() => { - const anchor = document.createElement('a'); - anchor.href = 'https://mysite.com/?zip=11111&locale=en_US&ads=false#privacy'; - return anchor; - }); - - const url = urlUtils.removeParams(['locale']); - - expect(url).toBe('https://mysite.com/?zip=11111&ads=false#privacy'); - }); - }); }); describe('setUrlFragment', () => { diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js index de308127a0f..7a8227479d4 100644 --- a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js +++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js @@ -2,33 +2,60 @@ import $ from 'jquery'; import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment'; describe('preserve_url_fragment', () => { - preloadFixtures('static/signin_forms_and_buttons.html.raw'); + preloadFixtures('sessions/new.html.raw'); beforeEach(() => { - loadFixtures('static/signin_forms_and_buttons.html.raw'); + loadFixtures('sessions/new.html.raw'); }); it('adds the url fragment to all login and sign up form actions', () => { preserveUrlFragment('#L65'); - expect($('#new_ldap_user').attr('action')).toBe('/users/auth/ldapmain/callback#L65'); - expect($('#new_user').attr('action')).toBe('/users/sign_in#L65'); - expect($('#new_new_user').attr('action')).toBe('/users#L65'); + expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in#L65'); + expect($('#new_new_user').attr('action')).toBe('http://test.host/users#L65'); }); - it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => { - preserveUrlFragment('#L65'); + it('does not add an empty url fragment to login and sign up form actions', () => { + preserveUrlFragment(); + + expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in'); + expect($('#new_new_user').attr('action')).toBe('http://test.host/users'); + }); + + it('does not add an empty query parameter to OmniAuth login buttons', () => { + preserveUrlFragment(); + + expect($('#oauth-login-cas3').attr('href')).toBe('http://test.host/users/auth/cas3'); expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe( - '/users/auth/auth0?redirect_fragment=L65', + 'http://test.host/users/auth/auth0', ); + }); - expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe( - '/users/auth/facebook?remember_me=1&redirect_fragment=L65', - ); + describe('adds "redirect_fragment" query parameter to OmniAuth login buttons', () => { + it('when "remember_me" is not present', () => { + preserveUrlFragment('#L65'); - expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe( - '/users/auth/saml?redirect_fragment=L65', - ); + expect($('#oauth-login-cas3').attr('href')).toBe( + 'http://test.host/users/auth/cas3?redirect_fragment=L65', + ); + + expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe( + 'http://test.host/users/auth/auth0?redirect_fragment=L65', + ); + }); + + it('when "remember-me" is present', () => { + $('a.omniauth-btn').attr('href', (i, href) => `${href}?remember_me=1`); + preserveUrlFragment('#L65'); + + expect($('#oauth-login-cas3').attr('href')).toBe( + 'http://test.host/users/auth/cas3?remember_me=1&redirect_fragment=L65', + ); + + expect($('#oauth-login-auth0').attr('href')).toBe( + 'http://test.host/users/auth/auth0?remember_me=1&redirect_fragment=L65', + ); + }); }); }); -- cgit v1.2.1 From 9c6f183da0157d08b7990d420a7b71ab69dbb5d7 Mon Sep 17 00:00:00 2001 From: Scott Escue Date: Sun, 2 Dec 2018 13:32:03 -0600 Subject: Adding changelog entry. --- .../iss-32584-preserve-line-number-fragment-after-redirect.yml | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml diff --git a/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml b/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml new file mode 100644 index 00000000000..8025cd472bd --- /dev/null +++ b/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml @@ -0,0 +1,6 @@ +--- +title: Fix lost line number when navigating to a specific line in a protected file + before authenticating. +merge_request: 19165 +author: Scott Escue +type: fixed -- cgit v1.2.1 From f9b54ae1131e3a83008c19d16d90ce05c50c528f Mon Sep 17 00:00:00 2001 From: Rajat Jain Date: Wed, 9 Jan 2019 14:32:44 +0530 Subject: Make issuable empty state actionable As per https://gitlab.com/gitlab-org/gitlab-ce/issues/25043, we want to make the empty state for issuables a little friendly. This applies during searching in opened state and in the closed state. --- app/views/shared/empty_states/_issues.html.haml | 18 +++++++++ .../shared/empty_states/_merge_requests.html.haml | 19 ++++++++++ changelogs/unreleased/25043-empty-states.yml | 5 +++ locale/gitlab.pot | 18 +++++++++ spec/features/groups/empty_states_spec.rb | 44 ++++++++++++++++++++-- 5 files changed, 101 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/25043-empty-states.yml diff --git a/app/views/shared/empty_states/_issues.html.haml b/app/views/shared/empty_states/_issues.html.haml index 0434860dec4..2691ec4cd46 100644 --- a/app/views/shared/empty_states/_issues.html.haml +++ b/app/views/shared/empty_states/_issues.html.haml @@ -2,6 +2,10 @@ - project_select_button = local_assigns.fetch(:project_select_button, false) - show_import_button = local_assigns.fetch(:show_import_button, false) && Feature.enabled?(:issues_import_csv) && can?(current_user, :import_issues, @project) - has_button = button_path || project_select_button +- closed_issues_count = issuables_count_for_state(:issues, :closed) +- opened_issues_count = issuables_count_for_state(:issues, :opened) +- is_opened_state = params[:state] == 'opened' +- is_closed_state = params[:state] == 'closed' .row.empty-state .col-12 @@ -14,6 +18,20 @@ = _("Sorry, your filter produced no results") %p.text-center = _("To widen your search, change or remove filters above") + - if show_new_issue_link?(@project) + .text-center + = link_to _("New issue"), new_project_issue_path(@project), class: "btn btn-success", title: _("New issue"), id: "new_issue_body_link" + - elsif is_opened_state && opened_issues_count == 0 && closed_issues_count > 0 + %h4.text-center + = _("There are no open issues") + %p.text-center + = _("To keep this project going, create a new issue") + - if show_new_issue_link?(@project) + .text-center + = link_to _("New issue"), new_project_issue_path(@project), class: "btn btn-success", title: _("New issue"), id: "new_issue_body_link" + - elsif is_closed_state && opened_issues_count > 0 && closed_issues_count == 0 + %h4.text-center + = _("There are no closed issues") - elsif current_user %h4 = _("The Issue Tracker is the place to add things that need to be improved or solved in a project") diff --git a/app/views/shared/empty_states/_merge_requests.html.haml b/app/views/shared/empty_states/_merge_requests.html.haml index 06ceb9738bc..be5b1c6b6ce 100644 --- a/app/views/shared/empty_states/_merge_requests.html.haml +++ b/app/views/shared/empty_states/_merge_requests.html.haml @@ -1,6 +1,11 @@ - button_path = local_assigns.fetch(:button_path, false) - project_select_button = local_assigns.fetch(:project_select_button, false) - has_button = button_path || project_select_button +- closed_merged_count = issuables_count_for_state(:merged, :closed) +- opened_merged_count = issuables_count_for_state(:merged, :opened) +- is_opened_state = params[:state] == 'opened' +- is_closed_state = params[:state] == 'closed' +- can_create_merge_request = merge_request_source_project_for_project(@project) .row.empty-state.merge-requests .col-12 @@ -13,6 +18,20 @@ = _("Sorry, your filter produced no results") %p.text-center = _("To widen your search, change or remove filters above") + .text-center + - if can_create_merge_request + = link_to _("New merge request"), project_new_merge_request_path(@project), class: "btn btn-success", title: _("New merge request"), id: "new_merge_request_body_link" + - elsif is_opened_state && opened_merged_count == 0 && closed_merged_count > 0 + %h4.text-center + = _("There are no open merge requests") + %p.text-center + = _("To keep this project going, create a new merge request") + .text-center + - if can_create_merge_request + = link_to _("New merge request"), project_new_merge_request_path(@project), class: "btn btn-success", title: _("New merge request"), id: "new_merge_request_body_link" + - elsif is_closed_state && opened_merged_count > 0 && closed_merged_count == 0 + %h4.text-center + = _("There are no closed merge requests") - else %h4 = _("Merge requests are a place to propose changes you've made to a project and discuss those changes with others") diff --git a/changelogs/unreleased/25043-empty-states.yml b/changelogs/unreleased/25043-empty-states.yml new file mode 100644 index 00000000000..529a8b3206f --- /dev/null +++ b/changelogs/unreleased/25043-empty-states.yml @@ -0,0 +1,5 @@ +--- +title: Make issuable empty states actionable +merge_request: 24077 +author: +type: changed diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 59e9dd67774..9fd16c7bbc1 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -6764,12 +6764,24 @@ msgstr "" msgid "There are no archived projects yet" msgstr "" +msgid "There are no closed issues" +msgstr "" + +msgid "There are no closed merge requests" +msgstr "" + msgid "There are no issues to show" msgstr "" msgid "There are no labels yet" msgstr "" +msgid "There are no open issues" +msgstr "" + +msgid "There are no open merge requests" +msgstr "" + msgid "There are no projects shared with this group yet" msgstr "" @@ -7194,6 +7206,12 @@ msgstr "" msgid "To import an SVN repository, check out %{svn_link}." msgstr "" +msgid "To keep this project going, create a new issue" +msgstr "" + +msgid "To keep this project going, create a new merge request" +msgstr "" + msgid "To link Sentry to GitLab, enter your Sentry URL and Auth Token." msgstr "" diff --git a/spec/features/groups/empty_states_spec.rb b/spec/features/groups/empty_states_spec.rb index 8f5ca781b2c..e4eb0d355d1 100644 --- a/spec/features/groups/empty_states_spec.rb +++ b/spec/features/groups/empty_states_spec.rb @@ -23,14 +23,52 @@ describe 'Group empty states' do end context "the project has #{issuable_name}s" do - before do + it 'does not display an empty state' do create(issuable, project_relation => project) visit path + expect(page).not_to have_selector('.empty-state') end - it 'does not display an empty state' do - expect(page).not_to have_selector('.empty-state') + it "displays link to create new #{issuable} when no open #{issuable} is found" do + create("closed_#{issuable}", project_relation => project) + issuable_link_fn = "project_#{issuable}s_path" + + visit public_send(issuable_link_fn, project) + + page.within(find('.empty-state')) do + expect(page).to have_content(/There are no open #{issuable.to_s.humanize.downcase}/) + expect(page).to have_selector("#new_#{issuable}_body_link") + end + end + + it 'displays link to create new issue when the current search gave no results' do + create(issuable, project_relation => project) + + issuable_link_fn = "project_#{issuable}s_path" + + visit public_send(issuable_link_fn, project, author_username: 'foo', scope: 'all', state: 'opened') + + page.within(find('.empty-state')) do + expect(page).to have_content(/Sorry, your filter produced no results/) + new_issuable_path = issuable == :issue ? 'new_project_issue_path' : 'project_new_merge_request_path' + + path = public_send(new_issuable_path, project) + + expect(page).to have_selector("#new_#{issuable}_body_link[href='#{path}']") + end + end + + it "displays conditional text when no closed #{issuable} is found" do + create(issuable, project_relation => project) + + issuable_link_fn = "project_#{issuable}s_path" + + visit public_send(issuable_link_fn, project, state: 'closed') + + page.within(find('.empty-state')) do + expect(page).to have_content(/There are no closed #{issuable.to_s.humanize.downcase}/) + end end end -- cgit v1.2.1 From 4ac4ba2654c9ffa6065e6d4789c279d676c5971b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 9 Jan 2019 22:50:51 -0800 Subject: Fix requests profiler in admin page not rendering HTML properly By default in Rails 5, content passed to `render` will be escaped. This doesn't work for the HTML profile output, which should be considered safe HTML already. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/56152 --- .../admin/requests_profiles_controller.rb | 2 +- .../unreleased/sh-fix-request-profiles-html.yml | 5 +++ .../admin/requests_profiles_controller_spec.rb | 47 ++++++++++++++++++++++ 3 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/sh-fix-request-profiles-html.yml create mode 100644 spec/controllers/admin/requests_profiles_controller_spec.rb diff --git a/app/controllers/admin/requests_profiles_controller.rb b/app/controllers/admin/requests_profiles_controller.rb index 57f7d3e3951..89d4c4f18d9 100644 --- a/app/controllers/admin/requests_profiles_controller.rb +++ b/app/controllers/admin/requests_profiles_controller.rb @@ -11,7 +11,7 @@ class Admin::RequestsProfilesController < Admin::ApplicationController profile = Gitlab::RequestProfiler::Profile.find(clean_name) if profile - render html: profile.content + render html: profile.content.html_safe else redirect_to admin_requests_profiles_path, alert: 'Profile not found' end diff --git a/changelogs/unreleased/sh-fix-request-profiles-html.yml b/changelogs/unreleased/sh-fix-request-profiles-html.yml new file mode 100644 index 00000000000..74e4115db8e --- /dev/null +++ b/changelogs/unreleased/sh-fix-request-profiles-html.yml @@ -0,0 +1,5 @@ +--- +title: Fix requests profiler in admin page not rendering HTML properly +merge_request: 24291 +author: +type: fixed diff --git a/spec/controllers/admin/requests_profiles_controller_spec.rb b/spec/controllers/admin/requests_profiles_controller_spec.rb new file mode 100644 index 00000000000..10850cb4603 --- /dev/null +++ b/spec/controllers/admin/requests_profiles_controller_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Admin::RequestsProfilesController do + set(:admin) { create(:admin) } + + before do + sign_in(admin) + end + + describe '#show' do + let(:basename) { "profile_#{Time.now.to_i}.html" } + let(:tmpdir) { Dir.mktmpdir('profiler-test') } + let(:test_file) { File.join(tmpdir, basename) } + let(:profile) { Gitlab::RequestProfiler::Profile.new(basename) } + let(:sample_data) do + <<~HTML + + + +

My First Heading

+

My first paragraph.

+ + + HTML + end + + before do + stub_const('Gitlab::RequestProfiler::PROFILES_DIR', tmpdir) + output = File.open(test_file, 'w') + output.write(sample_data) + output.close + end + + after do + File.unlink(test_file) + end + + it 'loads an HTML profile' do + get :show, params: { name: basename } + + expect(response).to have_gitlab_http_status(200) + expect(response.body).to eq(sample_data) + end + end +end -- cgit v1.2.1 From 05f30ac6bbb5c6f5dcc7fb045bb8761434290367 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Wed, 9 Jan 2019 22:50:51 -0800 Subject: Disable audit event logging for pipeline destruction AuditEventService isn't equipped to handle logging of the destruction of entities such as CI pipelines. It's a project-level event that operates on a pipeline. The current log doesn't even indicate that the pipeline is being destroyed. This is a CE backport of https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/9105. We're removing the auditing call because it breaks the EE implementation. --- app/services/ci/destroy_pipeline_service.rb | 2 -- spec/requests/api/pipelines_spec.rb | 4 ++-- spec/services/ci/destroy_pipeline_service_spec.rb | 4 ++-- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/services/ci/destroy_pipeline_service.rb b/app/services/ci/destroy_pipeline_service.rb index 13f892aabb8..5c4a34043c1 100644 --- a/app/services/ci/destroy_pipeline_service.rb +++ b/app/services/ci/destroy_pipeline_service.rb @@ -5,8 +5,6 @@ module Ci def execute(pipeline) raise Gitlab::Access::AccessDeniedError unless can?(current_user, :destroy_pipeline, pipeline) - AuditEventService.new(current_user, pipeline).security_event - pipeline.destroy! end end diff --git a/spec/requests/api/pipelines_spec.rb b/spec/requests/api/pipelines_spec.rb index eb002de62a2..52599db9a9e 100644 --- a/spec/requests/api/pipelines_spec.rb +++ b/spec/requests/api/pipelines_spec.rb @@ -456,8 +456,8 @@ describe API::Pipelines do expect(json_response['message']).to eq '404 Not found' end - it 'logs an audit event' do - expect { delete api("/projects/#{project.id}/pipelines/#{pipeline.id}", owner) }.to change { SecurityEvent.count }.by(1) + it 'does not log an audit event' do + expect { delete api("/projects/#{project.id}/pipelines/#{pipeline.id}", owner) }.not_to change { SecurityEvent.count } end context 'when the pipeline has jobs' do diff --git a/spec/services/ci/destroy_pipeline_service_spec.rb b/spec/services/ci/destroy_pipeline_service_spec.rb index 097daf67feb..d896f990470 100644 --- a/spec/services/ci/destroy_pipeline_service_spec.rb +++ b/spec/services/ci/destroy_pipeline_service_spec.rb @@ -17,8 +17,8 @@ describe ::Ci::DestroyPipelineService do expect { pipeline.reload }.to raise_error(ActiveRecord::RecordNotFound) end - it 'logs an audit event' do - expect { subject }.to change { SecurityEvent.count }.by(1) + it 'does not log an audit event' do + expect { subject }.not_to change { SecurityEvent.count } end context 'when the pipeline has jobs' do -- cgit v1.2.1 From 40887a94bda88b184491a3182f08eb86cf1aeb4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Thu, 10 Jan 2019 12:30:19 +0000 Subject: Fix files/blob api endpoint content disposition --- .../fj-55882-fix-files-api-content-disposition.yml | 5 ++++ lib/api/helpers.rb | 6 +++- spec/lib/api/helpers_spec.rb | 32 ++++++++++++---------- spec/requests/api/files_spec.rb | 5 ++-- spec/requests/api/repositories_spec.rb | 5 ++-- 5 files changed, 34 insertions(+), 19 deletions(-) create mode 100644 changelogs/unreleased/fj-55882-fix-files-api-content-disposition.yml diff --git a/changelogs/unreleased/fj-55882-fix-files-api-content-disposition.yml b/changelogs/unreleased/fj-55882-fix-files-api-content-disposition.yml new file mode 100644 index 00000000000..f64b29644b0 --- /dev/null +++ b/changelogs/unreleased/fj-55882-fix-files-api-content-disposition.yml @@ -0,0 +1,5 @@ +--- +title: Fix files/blob api endpoints content disposition +merge_request: 24267 +author: +type: fixed diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index aae54fb34bc..74927b4db81 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -496,7 +496,11 @@ module API def send_git_blob(repository, blob) env['api.format'] = :txt content_type 'text/plain' - header['Content-Disposition'] = content_disposition('attachment', blob.name) + header['Content-Disposition'] = content_disposition('inline', blob.name) + + # Let Workhorse examine the content and determine the better content disposition + header[Gitlab::Workhorse::DETECT_HEADER] = "true" + header(*Gitlab::Workhorse.send_git_blob(repository, blob)) end diff --git a/spec/lib/api/helpers_spec.rb b/spec/lib/api/helpers_spec.rb index 1c73a936e17..e1aea82653d 100644 --- a/spec/lib/api/helpers_spec.rb +++ b/spec/lib/api/helpers_spec.rb @@ -150,32 +150,36 @@ describe API::Helpers do end describe '#send_git_blob' do - context 'content disposition' do - let(:repository) { double } - let(:blob) { double(name: 'foobar') } + let(:repository) { double } + let(:blob) { double(name: 'foobar') } - let(:send_git_blob) do - subject.send(:send_git_blob, repository, blob) - end + let(:send_git_blob) do + subject.send(:send_git_blob, repository, blob) + end - before do - allow(subject).to receive(:env).and_return({}) - allow(subject).to receive(:content_type) - allow(subject).to receive(:header).and_return({}) - allow(Gitlab::Workhorse).to receive(:send_git_blob) - end + before do + allow(subject).to receive(:env).and_return({}) + allow(subject).to receive(:content_type) + allow(subject).to receive(:header).and_return({}) + allow(Gitlab::Workhorse).to receive(:send_git_blob) + end + + it 'sets Gitlab::Workhorse::DETECT_HEADER header' do + expect(send_git_blob[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" + end + context 'content disposition' do context 'when blob name is null' do let(:blob) { double(name: nil) } it 'returns only the disposition' do - expect(send_git_blob['Content-Disposition']).to eq 'attachment' + expect(send_git_blob['Content-Disposition']).to eq 'inline' end end context 'when blob name is not null' do it 'returns disposition with the blob name' do - expect(send_git_blob['Content-Disposition']).to eq 'attachment; filename="foobar"' + expect(send_git_blob['Content-Disposition']).to eq 'inline; filename="foobar"' end end end diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index a0aee937185..9b32dc78274 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -183,14 +183,15 @@ describe API::Files do get api(url, current_user), params: params expect(response).to have_gitlab_http_status(200) + expect(headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" end - it 'forces attachment content disposition' do + it 'sets inline content disposition by default' do url = route(file_path) + "/raw" get api(url, current_user), params: params - expect(headers['Content-Disposition']).to eq('attachment; filename="popen.rb"') + expect(headers['Content-Disposition']).to eq('inline; filename="popen.rb"') end context 'when mandatory params are not given' do diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb index b6b57803a6a..0adc95cfbeb 100644 --- a/spec/requests/api/repositories_spec.rb +++ b/spec/requests/api/repositories_spec.rb @@ -166,12 +166,13 @@ describe API::Repositories do get api(route, current_user) expect(response).to have_gitlab_http_status(200) + expect(headers[Gitlab::Workhorse::DETECT_HEADER]).to eq "true" end - it 'forces attachment content disposition' do + it 'sets inline content disposition by default' do get api(route, current_user) - expect(headers['Content-Disposition']).to eq 'attachment' + expect(headers['Content-Disposition']).to eq 'inline' end context 'when sha does not exist' do -- cgit v1.2.1 From 81a52c27c3d4e8db91bd60ce4619b6200dcda3ac Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 10 Jan 2019 12:37:42 +0000 Subject: Correctly show rebase state in MR widget Correctly shows the rebase state even if a merge requests pipeline has failed. Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/45942 --- .../stores/get_state_key.js | 4 ++-- changelogs/unreleased/mr-rebase-failing-tests.yml | 5 +++++ .../vue_mr_widget/stores/get_state_key_spec.js | 24 ++++++++++++++++++++++ 3 files changed, 31 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/mr-rebase-failing-tests.yml diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js index 066a3b833d7..0cc4fd59f5e 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js @@ -13,6 +13,8 @@ export default function deviseState(data) { return stateKey.conflicts; } else if (data.work_in_progress) { return stateKey.workInProgress; + } else if (this.shouldBeRebased) { + return stateKey.rebase; } else if (this.onlyAllowMergeIfPipelineSucceeds && this.isPipelineFailed) { return stateKey.pipelineFailed; } else if (this.hasMergeableDiscussionsState) { @@ -25,8 +27,6 @@ export default function deviseState(data) { return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds; } else if (!this.canMerge) { return stateKey.notAllowedToMerge; - } else if (this.shouldBeRebased) { - return stateKey.rebase; } else if (this.canBeMerged) { return stateKey.readyToMerge; } diff --git a/changelogs/unreleased/mr-rebase-failing-tests.yml b/changelogs/unreleased/mr-rebase-failing-tests.yml new file mode 100644 index 00000000000..07ae05766b1 --- /dev/null +++ b/changelogs/unreleased/mr-rebase-failing-tests.yml @@ -0,0 +1,5 @@ +--- +title: Fixed rebase button not showing in merge request widget +merge_request: +author: +type: fixed diff --git a/spec/javascripts/vue_mr_widget/stores/get_state_key_spec.js b/spec/javascripts/vue_mr_widget/stores/get_state_key_spec.js index 61ef26cd080..b356ea85cad 100644 --- a/spec/javascripts/vue_mr_widget/stores/get_state_key_spec.js +++ b/spec/javascripts/vue_mr_widget/stores/get_state_key_spec.js @@ -76,4 +76,28 @@ describe('getStateKey', () => { expect(bound()).toEqual('archived'); }); + + it('returns rebased state key', () => { + const context = { + mergeStatus: 'checked', + mergeWhenPipelineSucceeds: false, + canMerge: true, + onlyAllowMergeIfPipelineSucceeds: true, + isPipelineFailed: true, + hasMergeableDiscussionsState: false, + isPipelineBlocked: false, + canBeMerged: false, + shouldBeRebased: true, + }; + const data = { + project_archived: false, + branch_missing: false, + commits_count: 2, + has_conflicts: false, + work_in_progress: false, + }; + const bound = getStateKey.bind(context, data); + + expect(bound()).toEqual('rebase'); + }); }); -- cgit v1.2.1 From 432148b606770611c0ac3058c0f9e878f403d7a1 Mon Sep 17 00:00:00 2001 From: Abubakar Siddiq Ango Date: Thu, 10 Jan 2019 13:19:51 +0000 Subject: Setting Unicorn Worker memory limit in gitlab.rb --- doc/administration/operations/unicorn.md | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/doc/administration/operations/unicorn.md b/doc/administration/operations/unicorn.md index bad61151bda..0e2079cb093 100644 --- a/doc/administration/operations/unicorn.md +++ b/doc/administration/operations/unicorn.md @@ -60,7 +60,17 @@ Unicorn master then automatically replaces the worker process. This is a robust way to handle memory leaks: Unicorn is designed to handle workers that 'crash' so no user requests will be dropped. The unicorn-worker-killer gem is designed to only terminate a worker process _in -between requests_, so no user requests are affected. +between requests_, so no user requests are affected. You can set the minimum and +maximum memory threshold (in bytes) for the Unicorn worker killer by +setting the following values `/etc/gitlab/gitlab.rb`: + +```ruby +unicorn['worker_memory_limit_min'] = "400 * 1 << 20" +unicorn['worker_memory_limit_max'] = "650 * 1 << 20" +``` + +Otherwise, you can set the `GITLAB_UNICORN_MEMORY_MIN` and `GITLAB_UNICORN_MEMORY_MIN` +[environment variables](../environment_variables.md). This is what a Unicorn worker memory restart looks like in unicorn_stderr.log. You see that worker 4 (PID 125918) is inspecting itself and decides to exit. -- cgit v1.2.1 From 10fd13185336364726b6bcc45a2a164b416b0ac8 Mon Sep 17 00:00:00 2001 From: Tristan Read Date: Thu, 10 Jan 2019 13:24:47 +0000 Subject: Add documentation for Error Tracking --- doc/user/project/operations/error_tracking.md | 30 +++++++++++++++++++++ .../project/operations/img/error_tracking_list.png | Bin 0 -> 230740 bytes doc/user/project/settings/index.md | 6 +++++ 3 files changed, 36 insertions(+) create mode 100644 doc/user/project/operations/error_tracking.md create mode 100644 doc/user/project/operations/img/error_tracking_list.png diff --git a/doc/user/project/operations/error_tracking.md b/doc/user/project/operations/error_tracking.md new file mode 100644 index 00000000000..2b5abc7233f --- /dev/null +++ b/doc/user/project/operations/error_tracking.md @@ -0,0 +1,30 @@ +# Error Tracking + +> [Introduced](https://gitlab.com/groups/gitlab-org/-/epics/169) in GitLab 11.7. + +Error tracking allows developers to easily discover and view the errors that their application may be generating. By surfacing error information where the code is being developed, efficiency and awareness can be increased. + +## Sentry error tracking + +[Sentry](https://sentry.io/) is an open source error tracking system. GitLab allows administrators to connect Sentry to GitLab, to allow users to view a list of Sentry errors in GitLab itself. + +### Deploying Sentry + +You may sign up to the cloud hosted https://sentry.io or deploy your own [on-premise instance](https://docs.sentry.io/server/installation/). + +### Enabling Sentry + +GitLab provides an easy way to connect Sentry to your project: + +1. Sign up to Sentry.io or [deploy your own](#deploying-sentry) Sentry instance. +1. [Find or generate](https://docs.sentry.io/api/auth/) a Sentry auth token for your Sentry project. +1. Navigate to your project’s **Settings > Operations** and provide the Sentry API URL and auth token. +1. Ensure that the 'Active' checkbox is set. +1. Click **Save changes** for the changes to take effect. +1. You can now visit **Operations > Error Tracking** in your project's sidebar to [view a list](#error-tracking-list) of Sentry errors. + +## Error Tracking List + +The Error Tracking list may be found at **Operations > Error Tracking** in your project's sidebar. + +![Error Tracking list](img/error_tracking_list.png) diff --git a/doc/user/project/operations/img/error_tracking_list.png b/doc/user/project/operations/img/error_tracking_list.png new file mode 100644 index 00000000000..aa0f9867fdb Binary files /dev/null and b/doc/user/project/operations/img/error_tracking_list.png differ diff --git a/doc/user/project/settings/index.md b/doc/user/project/settings/index.md index d6754372816..221fa4b3d9f 100644 --- a/doc/user/project/settings/index.md +++ b/doc/user/project/settings/index.md @@ -122,3 +122,9 @@ GitLab administrators can use the admin interface to move any project to any namespace if needed. [permissions]: ../../permissions.md##project-members-permissions + +## Operations settings + +### Error Tracking + +Configure Error Tracking to discover and view [Sentry errors within GitLab](../operations/error_tracking.md). -- cgit v1.2.1 From f48f9460fd97ee9e818a16ef0ddf41c135173cfb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 10 Jan 2019 13:08:44 +0100 Subject: [QA] Retrieve the current user name and email MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- qa/qa/resource/base.rb | 4 ---- qa/qa/resource/user.rb | 9 ++++++--- .../repository/user_views_raw_diff_patch_requests_spec.rb | 6 +++++- qa/spec/resource/base_spec.rb | 4 ---- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/qa/qa/resource/base.rb b/qa/qa/resource/base.rb index dcea144ab74..f325162d1c0 100644 --- a/qa/qa/resource/base.rb +++ b/qa/qa/resource/base.rb @@ -126,10 +126,6 @@ module QA mod end - def self.attributes_names - dynamic_attributes.instance_methods(false).sort.grep_v(/=$/) - end - class DSL def initialize(base) @base = base diff --git a/qa/qa/resource/user.rb b/qa/qa/resource/user.rb index c26f0c84a1f..b9580d81171 100644 --- a/qa/qa/resource/user.rb +++ b/qa/qa/resource/user.rb @@ -6,9 +6,12 @@ module QA module Resource class User < Base attr_reader :unique_id - attr_writer :username, :password, :name, :email + attr_writer :username, :password attr_accessor :provider, :extern_uid + attribute :name + attribute :email + def initialize @unique_id = SecureRandom.hex(8) end @@ -22,11 +25,11 @@ module QA end def name - @name ||= username + @name ||= api_resource&.dig(:name) || username end def email - @email ||= "#{username}@example.com" + @email ||= api_resource&.dig(:email) || "#{username}@example.com" end def credentials_given? diff --git a/qa/qa/specs/features/browser_ui/3_create/repository/user_views_raw_diff_patch_requests_spec.rb b/qa/qa/specs/features/browser_ui/3_create/repository/user_views_raw_diff_patch_requests_spec.rb index 203338ddf77..3a5d89e6b83 100644 --- a/qa/qa/specs/features/browser_ui/3_create/repository/user_views_raw_diff_patch_requests_spec.rb +++ b/qa/qa/specs/features/browser_ui/3_create/repository/user_views_raw_diff_patch_requests_spec.rb @@ -39,11 +39,15 @@ module QA end it 'user views raw email patch' do + user = Resource::User.fabricate_via_api! do |user| + user.username = Runtime::User.username + end + view_commit Page::Project::Commit::Show.perform(&:select_email_patches) - expect(page).to have_content('From: Administrator ') + expect(page).to have_content("From: #{user.name} <#{user.email}>") expect(page).to have_content('Subject: [PATCH] Add second file') expect(page).to have_content('diff --git a/second b/second') end diff --git a/qa/spec/resource/base_spec.rb b/qa/spec/resource/base_spec.rb index dc9e16792d3..b8c406ae72a 100644 --- a/qa/spec/resource/base_spec.rb +++ b/qa/spec/resource/base_spec.rb @@ -138,10 +138,6 @@ describe QA::Resource::Base do describe '.attribute' do include_context 'simple resource' - it 'appends new attribute' do - expect(subject.attributes_names).to eq([:no_block, :test, :web_url]) - end - context 'when the attribute is populated via a block' do it 'returns value from the block' do result = subject.fabricate!(resource: resource) -- cgit v1.2.1 From 211c946a672c1500cb512d13438d60658b07b9cd Mon Sep 17 00:00:00 2001 From: Constance Okoghenun Date: Thu, 10 Jan 2019 14:00:36 +0000 Subject: Discussion filters - ensured note links resolves to the right note --- .../notes/components/discussion_filter.vue | 23 ++++++++++- ...tem-notes-can-break-with-discussion-filters.yml | 5 +++ .../notes/components/discussion_filter_spec.js | 46 +++++++++++++++++++--- 3 files changed, 67 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/54484-anchor-links-to-comments-or-system-notes-can-break-with-discussion-filters.yml diff --git a/app/assets/javascripts/notes/components/discussion_filter.vue b/app/assets/javascripts/notes/components/discussion_filter.vue index f5c410211b6..2d7c04ea614 100644 --- a/app/assets/javascripts/notes/components/discussion_filter.vue +++ b/app/assets/javascripts/notes/components/discussion_filter.vue @@ -1,6 +1,7 @@ diff --git a/changelogs/unreleased/54484-anchor-links-to-comments-or-system-notes-can-break-with-discussion-filters.yml b/changelogs/unreleased/54484-anchor-links-to-comments-or-system-notes-can-break-with-discussion-filters.yml new file mode 100644 index 00000000000..4d543db567d --- /dev/null +++ b/changelogs/unreleased/54484-anchor-links-to-comments-or-system-notes-can-break-with-discussion-filters.yml @@ -0,0 +1,5 @@ +--- +title: Ensured links to a comment or system note anchor resolves to the right note if a user has a discussion filter. +merge_request: 24228 +author: +type: changed diff --git a/spec/javascripts/notes/components/discussion_filter_spec.js b/spec/javascripts/notes/components/discussion_filter_spec.js index 5efcab436e4..91dab58ba7f 100644 --- a/spec/javascripts/notes/components/discussion_filter_spec.js +++ b/spec/javascripts/notes/components/discussion_filter_spec.js @@ -1,6 +1,7 @@ import Vue from 'vue'; import createStore from '~/notes/stores'; import DiscussionFilter from '~/notes/components/discussion_filter.vue'; +import { DISCUSSION_FILTERS_DEFAULT_VALUE } from '~/notes/constants'; import { mountComponentWithStore } from '../../helpers/vue_mount_component_helper'; import { discussionFiltersMock, discussionMock } from '../mock_data'; @@ -20,16 +21,14 @@ describe('DiscussionFilter component', () => { }, ]; const Component = Vue.extend(DiscussionFilter); - const selectedValue = discussionFiltersMock[0].value; + const selectedValue = DISCUSSION_FILTERS_DEFAULT_VALUE; + const props = { filters: discussionFiltersMock, selectedValue }; store.state.discussions = discussions; return mountComponentWithStore(Component, { el: null, store, - props: { - filters: discussionFiltersMock, - selectedValue, - }, + props, }); }; @@ -115,4 +114,41 @@ describe('DiscussionFilter component', () => { }); }); }); + + describe('URL with Links to notes', () => { + afterEach(() => { + window.location.hash = ''; + }); + + it('updates the filter when the URL links to a note', done => { + window.location.hash = `note_${discussionMock.notes[0].id}`; + vm.currentValue = discussionFiltersMock[2].value; + vm.handleLocationHash(); + + vm.$nextTick(() => { + expect(vm.currentValue).toEqual(DISCUSSION_FILTERS_DEFAULT_VALUE); + done(); + }); + }); + + it('does not update the filter when the current filter is "Show all activity"', done => { + window.location.hash = `note_${discussionMock.notes[0].id}`; + vm.handleLocationHash(); + + vm.$nextTick(() => { + expect(vm.currentValue).toEqual(DISCUSSION_FILTERS_DEFAULT_VALUE); + done(); + }); + }); + + it('only updates filter when the URL links to a note', done => { + window.location.hash = `testing123`; + vm.handleLocationHash(); + + vm.$nextTick(() => { + expect(vm.currentValue).toEqual(DISCUSSION_FILTERS_DEFAULT_VALUE); + done(); + }); + }); + }); }); -- cgit v1.2.1 From 42bfffc495f7347c5a60a61a88e525a596a49ffd Mon Sep 17 00:00:00 2001 From: Fernando Arias Date: Sun, 30 Dec 2018 20:05:24 -0600 Subject: Move job cancel/new button * Move cancel button to where the Retry is normally placed * Move new issue button to be in sidebar view for desktop versions * Update specs Add changelog entry for moving cancel btn Remove mobile version job action buttons * Remove buttons showing up on seperate row on job sidebar * Show Retry button in mobile version next to build name Improve flexbox usage for job action buttons * Allow for cleaner scaling of job name and alignment of buttons Adjust the debug button icon size * Adjust debug icon size on the build sidebar so the debug button is the same height as the other buttons Tweak job name vertical margins * Changes are only made to job side vue * Remove old class that only changed margin on top * Adjust veritcal margin uniformy set to 0 via bootstrap margin class Update Karma tests for job side view * Update karms tests for mobile version of side view * Removed tests for new issue button since we are fixing in https://gitlab.com/gitlab-org/gitlab-ce/issues/55944 Fix rspec and static-analysis failures * Run Prettier * Update rspec css selectors Move job action buttons to new line * Move "New Issue" and "Debug" buttons to seperate line in job sidebar * Update specs Run Prettier to format JS Remove New Issue button from job header Make sure button spacing follows styleguide --- app/assets/javascripts/jobs/components/sidebar.vue | 70 ++++++++++------------ app/assets/javascripts/jobs/store/getters.js | 11 ---- app/assets/stylesheets/pages/builds.scss | 18 +++--- changelogs/unreleased/move-job-cancel-btn.yml | 5 ++ spec/javascripts/jobs/components/sidebar_spec.js | 4 +- spec/javascripts/jobs/store/getters_spec.js | 12 +--- 6 files changed, 48 insertions(+), 72 deletions(-) create mode 100644 changelogs/unreleased/move-job-cancel-btn.yml diff --git a/app/assets/javascripts/jobs/components/sidebar.vue b/app/assets/javascripts/jobs/components/sidebar.vue index ad3e7dabc79..a2141dc3760 100644 --- a/app/assets/javascripts/jobs/components/sidebar.vue +++ b/app/assets/javascripts/jobs/components/sidebar.vue @@ -48,8 +48,7 @@ export default { return `${this.job.runner.description} (#${this.job.runner.id})`; }, retryButtonClass() { - let className = - 'js-retry-button float-right btn btn-retry d-none d-md-block d-lg-block d-xl-block'; + let className = 'js-retry-button btn btn-retry'; className += this.job.status && this.job.recoverable ? ' btn-primary' : ' btn-inverted-secondary'; return className; @@ -110,24 +109,27 @@ export default {