diff options
43 files changed, 628 insertions, 262 deletions
diff --git a/CHANGELOG b/CHANGELOG index c6c0c4559da..a52ac53bae7 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -1,9 +1,11 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.13.0 (unreleased) + - Add link from system note to compare with previous version - Use gitlab-shell v3.6.2 (GIT TRACE logging) - Fix centering of custom header logos (Ashley Dumaine) - AbstractReferenceFilter caches project_refs on RequestStore when active + - Replaced the check sign to arrow in the show build view. !6501 - Speed-up group milestones show page - Log LDAP lookup errors and don't swallow unrelated exceptions. !6103 (Markus Koller) - Add more tests for calendar contribution (ClemMakesApps) @@ -13,10 +15,12 @@ v 8.13.0 (unreleased) - Fix issue with page scrolling to top when closing or pinning sidebar (lukehowell) - Allow the Koding integration to be configured through the API - Added soft wrap button to repository file/blob editor + - Add word-wrap to issue title on issue and milestone boards (ClemMakesApps) - Fix robots.txt disallowing access to groups starting with "s" (Matt Harrison) - Close open merge request without source project (Katarzyna Kobierska Ula Budziszewska) - Use a ConnectionPool for Rails.cache on Sidekiq servers - Replace `alias_method_chain` with `Module#prepend` + - Preserve label filters when sorting !6136 (Joseph Frazier) - Only update issuable labels if they have been changed - Take filters in account in issuable counters. !6496 - Revoke button in Applications Settings underlines on hover. @@ -30,6 +34,7 @@ v 8.13.0 (unreleased) - Fix resolved discussion display in side-by-side diff view !6575 - Optimize GitHub importing for speed and memory - API: expose pipeline data in builds API (!6502, Guilherme Salazar) + - Notify the Merger about merge after successful build (Dimitris Karakasilis) - Fix broken repository 500 errors in project list - Close todos when accepting merge requests via the API !6486 (tonygambone) diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss index 9c84dceed05..ecc5b24e360 100644 --- a/app/assets/stylesheets/pages/boards.scss +++ b/app/assets/stylesheets/pages/boards.scss @@ -197,6 +197,7 @@ lex a { color: inherit; + word-wrap: break-word; } } diff --git a/app/assets/stylesheets/pages/milestone.scss b/app/assets/stylesheets/pages/milestone.scss index 6b865730487..8c2ba3ed58c 100644 --- a/app/assets/stylesheets/pages/milestone.scss +++ b/app/assets/stylesheets/pages/milestone.scss @@ -33,6 +33,7 @@ // Issue title span a { color: $gl-text-color; + word-wrap: break-word; } } } diff --git a/app/models/concerns/access_requestable.rb b/app/models/concerns/access_requestable.rb index eedd32a729f..62bc6b809f4 100644 --- a/app/models/concerns/access_requestable.rb +++ b/app/models/concerns/access_requestable.rb @@ -8,9 +8,6 @@ module AccessRequestable extend ActiveSupport::Concern def request_access(user) - members.create( - access_level: Gitlab::Access::DEVELOPER, - user: user, - requested_at: Time.now.utc) + Members::RequestAccessService.new(self, user).execute end end diff --git a/app/models/group.rb b/app/models/group.rb index aefb94b2ada..a2f88cca828 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -102,40 +102,44 @@ class Group < Namespace self[:lfs_enabled] end - def add_users(user_ids, access_level, current_user: nil, expires_at: nil) - user_ids.each do |user_id| - Member.add_user( - self.group_members, - user_id, - access_level, - current_user: current_user, - expires_at: expires_at - ) - end + def add_users(users, access_level, current_user: nil, expires_at: nil) + GroupMember.add_users_to_group( + self, + users, + access_level, + current_user: current_user, + expires_at: expires_at + ) end def add_user(user, access_level, current_user: nil, expires_at: nil) - add_users([user], access_level, current_user: current_user, expires_at: expires_at) + GroupMember.add_user( + self, + user, + access_level, + current_user: current_user, + expires_at: expires_at + ) end def add_guest(user, current_user = nil) - add_user(user, Gitlab::Access::GUEST, current_user: current_user) + add_user(user, :guest, current_user: current_user) end def add_reporter(user, current_user = nil) - add_user(user, Gitlab::Access::REPORTER, current_user: current_user) + add_user(user, :reporter, current_user: current_user) end def add_developer(user, current_user = nil) - add_user(user, Gitlab::Access::DEVELOPER, current_user: current_user) + add_user(user, :developer, current_user: current_user) end def add_master(user, current_user = nil) - add_user(user, Gitlab::Access::MASTER, current_user: current_user) + add_user(user, :master, current_user: current_user) end def add_owner(user, current_user = nil) - add_user(user, Gitlab::Access::OWNER, current_user: current_user) + add_user(user, :owner, current_user: current_user) end def has_owner?(user) diff --git a/app/models/member.rb b/app/models/member.rb index 69406379948..38a278ea559 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -80,49 +80,70 @@ class Member < ActiveRecord::Base find_by(invite_token: invite_token) end - # This method is used to find users that have been entered into the "Add members" field. - # These can be the User objects directly, their IDs, their emails, or new emails to be invited. - def user_for_id(user_id) - return user_id if user_id.is_a?(User) - - user = User.find_by(id: user_id) - user ||= User.find_by(email: user_id) - user ||= user_id - user - end - - def add_user(members, user_id, access_level, current_user: nil, expires_at: nil) - user = user_for_id(user_id) + def add_user(source, user, access_level, current_user: nil, expires_at: nil) + user = retrieve_user(user) + access_level = retrieve_access_level(access_level) # `user` can be either a User object or an email to be invited - if user.is_a?(User) - member = members.find_or_initialize_by(user_id: user.id) + member = + if user.is_a?(User) + source.members.find_by(user_id: user.id) || + source.requesters.find_by(user_id: user.id) || + source.members.build(user_id: user.id) + else + source.members.build(invite_email: user) + end + + return member unless can_update_member?(current_user, member) + + member.attributes = { + created_by: member.created_by || current_user, + access_level: access_level, + expires_at: expires_at + } + + if member.request? + ::Members::ApproveAccessRequestService.new(source, current_user, id: member.id).execute else - member = members.build - member.invite_email = user + member.save end - if can_update_member?(current_user, member) || project_creator?(member, access_level) - member.created_by ||= current_user - member.access_level = access_level - member.expires_at = expires_at + member + end - member.save - end + def access_levels + Gitlab::Access.sym_options end private + # This method is used to find users that have been entered into the "Add members" field. + # These can be the User objects directly, their IDs, their emails, or new emails to be invited. + def retrieve_user(user) + return user if user.is_a?(User) + + User.find_by(id: user) || User.find_by(email: user) || user + end + + def retrieve_access_level(access_level) + access_levels.fetch(access_level) { access_level.to_i } + end + def can_update_member?(current_user, member) # There is no current user for bulk actions, in which case anything is allowed - !current_user || - current_user.can?(:update_group_member, member) || - current_user.can?(:update_project_member, member) + !current_user || current_user.can?(:"update_#{member.type.underscore}", member) end - def project_creator?(member, access_level) - member.new_record? && member.owner? && - access_level.to_i == ProjectMember::MASTER + def add_users_to_source(source, users, access_level, current_user: nil, expires_at: nil) + users.each do |user| + add_user( + source, + user, + access_level, + current_user: current_user, + expires_at: expires_at + ) + end end end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 2f13d339c89..1b54a85d064 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -12,6 +12,22 @@ class GroupMember < Member Gitlab::Access.options_with_owner end + def self.access_levels + Gitlab::Access.sym_options_with_owner + end + + def self.add_users_to_group(group, users, access_level, current_user: nil, expires_at: nil) + self.transaction do + add_users_to_source( + group, + users, + access_level, + current_user: current_user, + expires_at: expires_at + ) + end + end + def group source end diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index ec2d40eb11c..125f26369d7 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -34,36 +34,20 @@ class ProjectMember < Member # :master # ) # - def add_users_to_projects(project_ids, user_ids, access, current_user: nil, expires_at: nil) - access_level = if roles_hash.has_key?(access) - roles_hash[access] - elsif roles_hash.values.include?(access.to_i) - access - else - raise "Non valid access" - end - - users = user_ids.map { |user_id| Member.user_for_id(user_id) } - - ProjectMember.transaction do + def add_users_to_projects(project_ids, users, access_level, current_user: nil, expires_at: nil) + self.transaction do project_ids.each do |project_id| project = Project.find(project_id) - users.each do |user| - Member.add_user( - project.project_members, - user, - access_level, - current_user: current_user, - expires_at: expires_at - ) - end + add_users_to_source( + project, + users, + access_level, + current_user: current_user, + expires_at: expires_at + ) end end - - true - rescue - false end def truncate_teams(project_ids) @@ -84,13 +68,15 @@ class ProjectMember < Member truncate_teams [project.id] end - def roles_hash - Gitlab::Access.sym_options - end - def access_level_roles Gitlab::Access.options end + + private + + def can_update_member?(current_user, member) + super || (member.owner? && member.new_record?) + end end def access_field diff --git a/app/models/project.rb b/app/models/project.rb index 7265cb55594..507228606df 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -146,6 +146,7 @@ class Project < ActiveRecord::Base delegate :name, to: :owner, allow_nil: true, prefix: true delegate :members, to: :team, prefix: true + delegate :add_user, to: :team # Validations validates :creator, presence: true, on: :create @@ -1016,10 +1017,6 @@ class Project < ActiveRecord::Base project_members.find_by(user_id: user) end - def add_user(user, access_level, current_user: nil, expires_at: nil) - team.add_user(user, access_level, current_user: current_user, expires_at: expires_at) - end - def default_branch @default_branch ||= repository.root_ref if repository.exists? end diff --git a/app/models/project_team.rb b/app/models/project_team.rb index d9ce5088903..79d041d2775 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -33,18 +33,24 @@ class ProjectTeam member end - def add_users(users, access, current_user: nil, expires_at: nil) + def add_users(users, access_level, current_user: nil, expires_at: nil) ProjectMember.add_users_to_projects( [project.id], users, - access, + access_level, current_user: current_user, expires_at: expires_at ) end - def add_user(user, access, current_user: nil, expires_at: nil) - add_users([user], access, current_user: current_user, expires_at: expires_at) + def add_user(user, access_level, current_user: nil, expires_at: nil) + ProjectMember.add_user( + project, + user, + access_level, + current_user: current_user, + expires_at: expires_at + ) end # Remove all users from project team diff --git a/app/services/members/request_access_service.rb b/app/services/members/request_access_service.rb new file mode 100644 index 00000000000..2614153d900 --- /dev/null +++ b/app/services/members/request_access_service.rb @@ -0,0 +1,25 @@ +module Members + class RequestAccessService < BaseService + attr_accessor :source + + def initialize(source, current_user) + @source = source + @current_user = current_user + end + + def execute + raise Gitlab::Access::AccessDeniedError unless can_request_access?(source) + + source.members.create( + access_level: Gitlab::Access::DEVELOPER, + user: current_user, + requested_at: Time.now.utc) + end + + private + + def can_request_access?(source) + source && can?(current_user, :request_access, source) + end + end +end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 6139ed56e25..de8049b8e2e 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -134,7 +134,8 @@ class NotificationService merge_request, merge_request.target_project, current_user, - :merged_merge_request_email + :merged_merge_request_email, + skip_current_user: !merge_request.merge_when_build_succeeds? ) end @@ -514,9 +515,16 @@ class NotificationService end end - def close_resource_email(target, project, current_user, method) + def close_resource_email(target, project, current_user, method, skip_current_user: true) action = method == :merged_merge_request_email ? "merge" : "close" - recipients = build_recipients(target, project, current_user, action: action) + + recipients = build_recipients( + target, + project, + current_user, + action: action, + skip_current_user: skip_current_user + ) recipients.each do |recipient| mailer.send(method, recipient.id, target.id, current_user.id).deliver_later @@ -557,7 +565,7 @@ class NotificationService end end - def build_recipients(target, project, current_user, action: nil, previous_assignee: nil) + def build_recipients(target, project, current_user, action: nil, previous_assignee: nil, skip_current_user: true) custom_action = build_custom_key(action, target) recipients = target.participants(current_user) @@ -586,7 +594,8 @@ class NotificationService recipients = reject_unsubscribed_users(recipients, target) recipients = reject_users_without_access(recipients, target) - recipients.delete(current_user) + recipients.delete(current_user) if skip_current_user + recipients.uniq end diff --git a/app/services/system_note_service.rb b/app/services/system_note_service.rb index 0c8446e7c3d..5ccaa5275b7 100644 --- a/app/services/system_note_service.rb +++ b/app/services/system_note_service.rb @@ -24,6 +24,7 @@ module SystemNoteService body = "Added #{commits_text}:\n\n" body << existing_commit_summary(noteable, existing_commits, oldrev) body << new_commit_summary(new_commits).join("\n") + body << "\n\n[Compare with previous version](#{diff_comparison_url(noteable, project, oldrev)})" create_note(noteable: noteable, project: project, author: author, note: body) end @@ -254,8 +255,7 @@ module SystemNoteService # # "Started branch `201-issue-branch-button`" def new_issue_branch(issue, project, author, branch) - h = Gitlab::Routing.url_helpers - link = h.namespace_project_compare_url(project.namespace, project, from: project.default_branch, to: branch) + link = url_helpers.namespace_project_compare_url(project.namespace, project, from: project.default_branch, to: branch) body = "Started branch [`#{branch}`](#{link})" create_note(noteable: issue, project: project, author: author, note: body) @@ -466,4 +466,20 @@ module SystemNoteService def escape_html(text) Rack::Utils.escape_html(text) end + + def url_helpers + @url_helpers ||= Gitlab::Routing.url_helpers + end + + def diff_comparison_url(merge_request, project, oldrev) + diff_id = merge_request.merge_request_diff.id + + url_helpers.diffs_namespace_project_merge_request_url( + project.namespace, + project, + merge_request.iid, + diff_id: diff_id, + start_sha: oldrev + ) + end end diff --git a/app/views/projects/builds/_sidebar.html.haml b/app/views/projects/builds/_sidebar.html.haml index 8846cf8577c..f5344091cae 100644 --- a/app/views/projects/builds/_sidebar.html.haml +++ b/app/views/projects/builds/_sidebar.html.haml @@ -128,7 +128,7 @@ - builds.select{|build| build.status == build_status}.each do |build| .build-job{class: ('active' if build == @build), data: {stage: build.stage}} = link_to namespace_project_build_path(@project.namespace, @project, build) do - = icon('check') + = icon('right-arrow') = ci_icon_for_status(build.status) %span - if build.name diff --git a/app/views/projects/empty.html.haml b/app/views/projects/empty.html.haml index 636beb73ec2..7a39064adc5 100644 --- a/app/views/projects/empty.html.haml +++ b/app/views/projects/empty.html.haml @@ -23,6 +23,8 @@ or a = link_to '.gitignore', add_special_file_path(@project, file_name: '.gitignore'), class: 'underlined-link' to this project. + %p + You will need to be owner or have the master permission level for the initial push, as the master branch is automatically protected. - if can?(current_user, :push_code, @project) %div{ class: container_class } diff --git a/app/views/shared/_sort_dropdown.html.haml b/app/views/shared/_sort_dropdown.html.haml index 249bce926ce..36bbac6fbf5 100644 --- a/app/views/shared/_sort_dropdown.html.haml +++ b/app/views/shared/_sort_dropdown.html.haml @@ -8,26 +8,26 @@ %b.caret %ul.dropdown-menu.dropdown-menu-align-right.dropdown-menu-sort %li - = link_to page_filter_path(sort: sort_value_priority) do + = link_to page_filter_path(sort: sort_value_priority, label: true) do = sort_title_priority - = link_to page_filter_path(sort: sort_value_recently_created) do + = link_to page_filter_path(sort: sort_value_recently_created, label: true) do = sort_title_recently_created - = link_to page_filter_path(sort: sort_value_oldest_created) do + = link_to page_filter_path(sort: sort_value_oldest_created, label: true) do = sort_title_oldest_created - = link_to page_filter_path(sort: sort_value_recently_updated) do + = link_to page_filter_path(sort: sort_value_recently_updated, label: true) do = sort_title_recently_updated - = link_to page_filter_path(sort: sort_value_oldest_updated) do + = link_to page_filter_path(sort: sort_value_oldest_updated, label: true) do = sort_title_oldest_updated - = link_to page_filter_path(sort: sort_value_milestone_soon) do + = link_to page_filter_path(sort: sort_value_milestone_soon, label: true) do = sort_title_milestone_soon - = link_to page_filter_path(sort: sort_value_milestone_later) do + = link_to page_filter_path(sort: sort_value_milestone_later, label: true) do = sort_title_milestone_later - if controller.controller_name == 'issues' || controller.action_name == 'issues' - = link_to page_filter_path(sort: sort_value_due_date_soon) do + = link_to page_filter_path(sort: sort_value_due_date_soon, label: true) do = sort_title_due_date_soon - = link_to page_filter_path(sort: sort_value_due_date_later) do + = link_to page_filter_path(sort: sort_value_due_date_later, label: true) do = sort_title_due_date_later - = link_to page_filter_path(sort: sort_value_upvotes) do + = link_to page_filter_path(sort: sort_value_upvotes, label: true) do = sort_title_upvotes - = link_to page_filter_path(sort: sort_value_downvotes) do + = link_to page_filter_path(sort: sort_value_downvotes, label: true) do = sort_title_downvotes diff --git a/db/fixtures/development/06_teams.rb b/db/fixtures/development/06_teams.rb index 3e8cdcd67b4..9739a5ac8d5 100644 --- a/db/fixtures/development/06_teams.rb +++ b/db/fixtures/development/06_teams.rb @@ -1,7 +1,7 @@ Gitlab::Seeder.quiet do Group.all.each do |group| User.all.sample(4).each do |user| - if group.add_users([user.id], Gitlab::Access.values.sample) + if group.add_user(user, Gitlab::Access.values.sample).persisted? print '.' else print 'F' diff --git a/doc/development/migration_style_guide.md b/doc/development/migration_style_guide.md index 295eae0a88e..61b0fbc89c9 100644 --- a/doc/development/migration_style_guide.md +++ b/doc/development/migration_style_guide.md @@ -9,10 +9,10 @@ a big burden for most organizations. For this reason it is important that your migrations are written carefully, can be applied online and adhere to the style guide below. Migrations should not require GitLab installations to be taken offline unless -_absolutely_ necessary. If a migration requires downtime this should be -clearly mentioned during the review process as well as being documented in the -monthly release post. For more information see the "Downtime Tagging" section -below. +_absolutely_ necessary - see the ["What Requires Downtime?"](what_requires_downtime.md) +page. If a migration requires downtime, this should be clearly mentioned during +the review process, as well as being documented in the monthly release post. For +more information, see the "Downtime Tagging" section below. When writing your migrations, also consider that databases might have stale data or inconsistencies and guard for that. Try to make as little assumptions as possible diff --git a/lib/api/members.rb b/lib/api/members.rb index 37f0a6512f4..a18ce769e29 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -59,13 +59,6 @@ module API authorize_admin_source!(source_type, source) required_attributes! [:user_id, :access_level] - access_requester = source.requesters.find_by(user_id: params[:user_id]) - if access_requester - # We pass current_user = access_requester so that the requester doesn't - # receive a "access denied" email - ::Members::DestroyService.new(access_requester, access_requester.user).execute - end - member = source.members.find_by(user_id: params[:user_id]) # This is to ensure back-compatibility but 409 behavior should be used @@ -73,18 +66,12 @@ module API conflict!('Member already exists') if source_type == 'group' && member unless member - source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) - member = source.members.find_by(user_id: params[:user_id]) + member = source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) end - if member + if member.persisted? && member.valid? present member.user, with: Entities::Member, member: member else - # Since `source.add_user` doesn't return a member object, we have to - # build a new one and populate its errors in order to render them. - member = source.members.build(attributes_for_keys([:user_id, :access_level, :expires_at])) - member.valid? # populate the errors - # This is to ensure back-compatibility but 400 behavior should be used # for all validation errors in 9.0! render_api_error!('Access level is not known', 422) if member.errors.key?(:access_level) diff --git a/lib/gitlab/access.rb b/lib/gitlab/access.rb index a533bac2692..9b484a2ecfd 100644 --- a/lib/gitlab/access.rb +++ b/lib/gitlab/access.rb @@ -53,6 +53,10 @@ module Gitlab } end + def sym_options_with_owner + sym_options.merge(owner: OWNER) + end + def protection_options { "Not protected: Both developers and masters can push new commits, force push, or delete the branch." => PROTECTION_NONE, diff --git a/spec/controllers/projects/templates_controller_spec.rb b/spec/controllers/projects/templates_controller_spec.rb index 7b3a26d7ca7..19a152bcb05 100644 --- a/spec/controllers/projects/templates_controller_spec.rb +++ b/spec/controllers/projects/templates_controller_spec.rb @@ -13,7 +13,7 @@ describe Projects::TemplatesController do end before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) end diff --git a/spec/factories/project_members.rb b/spec/factories/project_members.rb index cf3659ba275..1ddb305a8af 100644 --- a/spec/factories/project_members.rb +++ b/spec/factories/project_members.rb @@ -4,24 +4,9 @@ FactoryGirl.define do project master - trait :guest do - access_level ProjectMember::GUEST - end - - trait :reporter do - access_level ProjectMember::REPORTER - end - - trait :developer do - access_level ProjectMember::DEVELOPER - end - - trait :master do - access_level ProjectMember::MASTER - end - - trait :owner do - access_level ProjectMember::OWNER - end + trait(:guest) { access_level ProjectMember::GUEST } + trait(:reporter) { access_level ProjectMember::REPORTER } + trait(:developer) { access_level ProjectMember::DEVELOPER } + trait(:master) { access_level ProjectMember::MASTER } end end diff --git a/spec/features/merge_requests/merge_request_versions_spec.rb b/spec/features/merge_requests/merge_request_versions_spec.rb index 22d9e42119d..23cee891bac 100644 --- a/spec/features/merge_requests/merge_request_versions_spec.rb +++ b/spec/features/merge_requests/merge_request_versions_spec.rb @@ -1,12 +1,13 @@ require 'spec_helper' feature 'Merge Request versions', js: true, feature: true do + let(:merge_request) { create(:merge_request, importing: true) } + let(:project) { merge_request.source_project } + before do login_as :admin - merge_request = create(:merge_request, importing: true) merge_request.merge_request_diffs.create(head_commit_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9') merge_request.merge_request_diffs.create(head_commit_sha: '5937ac0a7beb003549fc5fd26fc247adbce4a52e') - project = merge_request.source_project visit diffs_namespace_project_merge_request_path(project.namespace, project, merge_request) end @@ -47,6 +48,16 @@ feature 'Merge Request versions', js: true, feature: true do end end + it 'has a path with comparison context' do + expect(page).to have_current_path diffs_namespace_project_merge_request_path( + project.namespace, + project, + merge_request.iid, + diff_id: 2, + start_sha: '6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9' + ) + end + it 'should have correct value in the compare dropdown' do page.within '.mr-version-compare-dropdown' do expect(page).to have_content 'version 1' @@ -61,10 +72,6 @@ feature 'Merge Request versions', js: true, feature: true do expect(page).to have_content '4 changed files with 15 additions and 6 deletions' end - it 'show diff between new and old version' do - expect(page).to have_content '4 changed files with 15 additions and 6 deletions' - end - it 'should return to latest version when "Show latest version" button is clicked' do click_link 'Show latest version' page.within '.mr-version-dropdown' do diff --git a/spec/features/projects/members/owner_cannot_leave_project_spec.rb b/spec/features/projects/members/owner_cannot_leave_project_spec.rb index 67811b1048e..6e948b7a616 100644 --- a/spec/features/projects/members/owner_cannot_leave_project_spec.rb +++ b/spec/features/projects/members/owner_cannot_leave_project_spec.rb @@ -1,12 +1,10 @@ require 'spec_helper' feature 'Projects > Members > Owner cannot leave project', feature: true do - let(:owner) { create(:user) } let(:project) { create(:project) } background do - project.team << [owner, :owner] - login_as(owner) + login_as(project.owner) visit namespace_project_path(project.namespace, project) end diff --git a/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb index 0e54c4fdf20..4ca9272b9c1 100644 --- a/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb +++ b/spec/features/projects/members/owner_cannot_request_access_to_his_project_spec.rb @@ -1,12 +1,10 @@ require 'spec_helper' feature 'Projects > Members > Owner cannot request access to his project', feature: true do - let(:owner) { create(:user) } let(:project) { create(:project) } background do - project.team << [owner, :owner] - login_as(owner) + login_as(project.owner) visit namespace_project_path(project.namespace, project) end diff --git a/spec/features/projects_spec.rb b/spec/features/projects_spec.rb index 2242cb6236a..c30d38b6508 100644 --- a/spec/features/projects_spec.rb +++ b/spec/features/projects_spec.rb @@ -82,7 +82,7 @@ feature 'Project', feature: true do before do login_with(user) - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) visit namespace_project_path(project.namespace, project) end @@ -101,8 +101,8 @@ feature 'Project', feature: true do context 'on issues page', js: true do before do login_with(user) - project.team.add_user(user, Gitlab::Access::MASTER) - project2.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) + project2.add_user(user, Gitlab::Access::MASTER) visit namespace_project_issue_path(project.namespace, project, issue) end diff --git a/spec/finders/joined_groups_finder_spec.rb b/spec/finders/joined_groups_finder_spec.rb index f90a8e007c8..29a47e005a6 100644 --- a/spec/finders/joined_groups_finder_spec.rb +++ b/spec/finders/joined_groups_finder_spec.rb @@ -43,7 +43,7 @@ describe JoinedGroupsFinder do context 'if profile visitor is in one of the private group projects' do before do project = create(:project, :private, group: private_group, name: 'B', path: 'B') - project.team.add_user(profile_visitor, Gitlab::Access::DEVELOPER) + project.add_user(profile_visitor, Gitlab::Access::DEVELOPER) end it 'shows group' do diff --git a/spec/finders/projects_finder_spec.rb b/spec/finders/projects_finder_spec.rb index 7a3a74335e8..13bda5f7c5a 100644 --- a/spec/finders/projects_finder_spec.rb +++ b/spec/finders/projects_finder_spec.rb @@ -38,7 +38,7 @@ describe ProjectsFinder do describe 'with private projects' do before do - private_project.team.add_user(user, Gitlab::Access::MASTER) + private_project.add_user(user, Gitlab::Access::MASTER) end it do diff --git a/spec/helpers/members_helper_spec.rb b/spec/helpers/members_helper_spec.rb index 7998209b7b0..6703d88e357 100644 --- a/spec/helpers/members_helper_spec.rb +++ b/spec/helpers/members_helper_spec.rb @@ -11,7 +11,7 @@ describe MembersHelper do describe '#remove_member_message' do let(:requester) { build(:user) } - let(:project) { create(:project) } + let(:project) { create(:empty_project, :public) } let(:project_member) { build(:project_member, project: project) } let(:project_member_invite) { build(:project_member, project: project).tap { |m| m.generate_invite_token! } } let(:project_member_request) { project.request_access(requester) } @@ -32,7 +32,7 @@ describe MembersHelper do describe '#remove_member_title' do let(:requester) { build(:user) } - let(:project) { create(:project) } + let(:project) { create(:empty_project, :public) } let(:project_member) { build(:project_member, project: project) } let(:project_member_request) { project.request_access(requester) } let(:group) { create(:group) } diff --git a/spec/lib/gitlab/template/issue_template_spec.rb b/spec/lib/gitlab/template/issue_template_spec.rb index f770857e958..d2d334e6413 100644 --- a/spec/lib/gitlab/template/issue_template_spec.rb +++ b/spec/lib/gitlab/template/issue_template_spec.rb @@ -10,7 +10,7 @@ describe Gitlab::Template::IssueTemplate do let(:file_path_3) { '.gitlab/issue_templates/feature_proposal.md' } before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) @@ -53,7 +53,7 @@ describe Gitlab::Template::IssueTemplate do context 'when repo is bare or empty' do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "returns empty array" do templates = subject.by_category('', empty_project) @@ -78,7 +78,7 @@ describe Gitlab::Template::IssueTemplate do context "when repo is empty" do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "raises file not found" do issue_template = subject.new('.gitlab/issue_templates/not_existent.md', empty_project) diff --git a/spec/lib/gitlab/template/merge_request_template_spec.rb b/spec/lib/gitlab/template/merge_request_template_spec.rb index bb0f68043fa..ddf68c4cf78 100644 --- a/spec/lib/gitlab/template/merge_request_template_spec.rb +++ b/spec/lib/gitlab/template/merge_request_template_spec.rb @@ -10,7 +10,7 @@ describe Gitlab::Template::MergeRequestTemplate do let(:file_path_3) { '.gitlab/merge_request_templates/feature_proposal.md' } before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) project.repository.commit_file(user, file_path_1, "something valid", "test 3", "master", false) project.repository.commit_file(user, file_path_2, "template_test", "test 1", "master", false) project.repository.commit_file(user, file_path_3, "feature_proposal", "test 2", "master", false) @@ -53,7 +53,7 @@ describe Gitlab::Template::MergeRequestTemplate do context 'when repo is bare or empty' do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "returns empty array" do templates = subject.by_category('', empty_project) @@ -78,7 +78,7 @@ describe Gitlab::Template::MergeRequestTemplate do context "when repo is empty" do let(:empty_project) { create(:empty_project) } - before { empty_project.team.add_user(user, Gitlab::Access::MASTER) } + before { empty_project.add_user(user, Gitlab::Access::MASTER) } it "raises file not found" do issue_template = subject.new('.gitlab/merge_request_templates/not_existent.md', empty_project) diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index 0363bc74939..cd8578b6f49 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -402,7 +402,7 @@ describe Notify do describe 'project access requested' do context 'for a project in a user namespace' do - let(:project) { create(:project).tap { |p| p.team << [p.owner, :master, p.owner] } } + let(:project) { create(:project, :public).tap { |p| p.team << [p.owner, :master, p.owner] } } let(:user) { create(:user) } let(:project_member) do project.request_access(user) @@ -429,7 +429,7 @@ describe Notify do context 'for a project in a group' do let(:group_owner) { create(:user) } let(:group) { create(:group).tap { |g| g.add_owner(group_owner) } } - let(:project) { create(:project, namespace: group) } + let(:project) { create(:project, :public, namespace: group) } let(:user) { create(:user) } let(:project_member) do project.request_access(user) @@ -492,21 +492,22 @@ describe Notify do end end - def invite_to_project(project:, email:, inviter:) - Member.add_user( - project.project_members, - 'toto@example.com', - Gitlab::Access::DEVELOPER, - current_user: inviter + def invite_to_project(project, inviter:) + create( + :project_member, + :developer, + project: project, + invite_token: '1234', + invite_email: 'toto@example.com', + user: nil, + created_by: inviter ) - - project.project_members.invite.last end describe 'project invitation' do let(:project) { create(:project) } let(:master) { create(:user).tap { |u| project.team << [u, :master] } } - let(:project_member) { invite_to_project(project: project, email: 'toto@example.com', inviter: master) } + let(:project_member) { invite_to_project(project, inviter: master) } subject { Notify.member_invited_email('project', project_member.id, project_member.invite_token) } @@ -525,10 +526,10 @@ describe Notify do describe 'project invitation accepted' do let(:project) { create(:project) } - let(:invited_user) { create(:user) } + let(:invited_user) { create(:user, name: 'invited user') } let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:project_member) do - invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master) + invitee = invite_to_project(project, inviter: master) invitee.accept_invite!(invited_user) invitee end @@ -552,7 +553,7 @@ describe Notify do let(:project) { create(:project) } let(:master) { create(:user).tap { |u| project.team << [u, :master] } } let(:project_member) do - invitee = invite_to_project(project: project, email: 'toto@example.com', inviter: master) + invitee = invite_to_project(project, inviter: master) invitee.decline_invite! invitee end @@ -744,21 +745,22 @@ describe Notify do end end - def invite_to_group(group:, email:, inviter:) - Member.add_user( - group.group_members, - 'toto@example.com', - Gitlab::Access::DEVELOPER, - current_user: inviter + def invite_to_group(group, inviter:) + create( + :group_member, + :developer, + group: group, + invite_token: '1234', + invite_email: 'toto@example.com', + user: nil, + created_by: inviter ) - - group.group_members.invite.last end describe 'group invitation' do let(:group) { create(:group) } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } - let(:group_member) { invite_to_group(group: group, email: 'toto@example.com', inviter: owner) } + let(:group_member) { invite_to_group(group, inviter: owner) } subject { Notify.member_invited_email('group', group_member.id, group_member.invite_token) } @@ -777,10 +779,10 @@ describe Notify do describe 'group invitation accepted' do let(:group) { create(:group) } - let(:invited_user) { create(:user) } + let(:invited_user) { create(:user, name: 'invited user') } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } let(:group_member) do - invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner) + invitee = invite_to_group(group, inviter: owner) invitee.accept_invite!(invited_user) invitee end @@ -804,7 +806,7 @@ describe Notify do let(:group) { create(:group) } let(:owner) { create(:user).tap { |u| group.add_user(u, Gitlab::Access::OWNER) } } let(:group_member) do - invitee = invite_to_group(group: group, email: 'toto@example.com', inviter: owner) + invitee = invite_to_group(group, inviter: owner) invitee.decline_invite! invitee end diff --git a/spec/models/issue_spec.rb b/spec/models/issue_spec.rb index 3259f795296..3b8b743af2d 100644 --- a/spec/models/issue_spec.rb +++ b/spec/models/issue_spec.rb @@ -494,7 +494,7 @@ describe Issue, models: true do context 'with an admin user' do let(:project) { create(:empty_project) } - let(:user) { create(:user, admin: true) } + let(:user) { create(:admin) } it 'returns true for a regular issue' do issue = build(:issue, project: project) diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 0b1634f654a..bda23eaed43 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -57,7 +57,7 @@ describe Member, models: true do describe 'Scopes & finders' do before do - project = create(:empty_project) + project = create(:empty_project, :public) group = create(:group) @owner_user = create(:user).tap { |u| group.add_owner(u) } @owner = group.members.find_by(user_id: @owner_user.id) @@ -74,22 +74,17 @@ describe Member, models: true do @blocked_master = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::MASTER) @blocked_developer = project.members.find_by(user_id: @blocked_user.id, access_level: Gitlab::Access::DEVELOPER) - Member.add_user( - project.members, - 'toto1@example.com', - Gitlab::Access::DEVELOPER, - current_user: @master_user - ) - @invited_member = project.members.invite.find_by_invite_email('toto1@example.com') + @invited_member = create(:project_member, :developer, + project: project, + invite_token: '1234', + invite_email: 'toto1@example.com') accepted_invite_user = build(:user, state: :active) - Member.add_user( - project.members, - 'toto2@example.com', - Gitlab::Access::DEVELOPER, - current_user: @master_user - ) - @accepted_invite_member = project.members.invite.find_by_invite_email('toto2@example.com').tap { |u| u.accept_invite!(accepted_invite_user) } + @accepted_invite_member = create(:project_member, :developer, + project: project, + invite_token: '1234', + invite_email: 'toto2@example.com'). + tap { |u| u.accept_invite!(accepted_invite_user) } requested_user = create(:user).tap { |u| project.request_access(u) } @requested_member = project.requesters.find_by(user_id: requested_user.id) @@ -176,39 +171,209 @@ describe Member, models: true do it { is_expected.to respond_to(:user_email) } end - describe ".add_user" do - let!(:user) { create(:user) } - let(:project) { create(:project) } + describe '.add_user' do + %w[project group].each do |source_type| + context "when source is a #{source_type}" do + let!(:source) { create(source_type) } + let!(:user) { create(:user) } + let!(:admin) { create(:admin) } - context "when called with a user id" do - it "adds the user as a member" do - Member.add_user(project.project_members, user.id, ProjectMember::MASTER) + it 'returns a <Source>Member object' do + member = described_class.add_user(source, user, :master) - expect(project.users).to include(user) - end - end + expect(member).to be_a "#{source_type.classify}Member".constantize + expect(member).to be_persisted + end - context "when called with a user object" do - it "adds the user as a member" do - Member.add_user(project.project_members, user, ProjectMember::MASTER) + it 'sets members.created_by to the given current_user' do + member = described_class.add_user(source, user, :master, current_user: admin) - expect(project.users).to include(user) - end - end + expect(member.created_by).to eq(admin) + end - context "when called with a known user email" do - it "adds the user as a member" do - Member.add_user(project.project_members, user.email, ProjectMember::MASTER) + it 'sets members.expires_at to the given expires_at' do + member = described_class.add_user(source, user, :master, expires_at: Date.new(2016, 9, 22)) - expect(project.users).to include(user) - end - end + expect(member.expires_at).to eq(Date.new(2016, 9, 22)) + end + + described_class.access_levels.each do |sym_key, int_access_level| + it "accepts the :#{sym_key} symbol as access level" do + expect(source.users).not_to include(user) + + member = described_class.add_user(source, user.id, sym_key) + + expect(member.access_level).to eq(int_access_level) + expect(source.users.reload).to include(user) + end + + it "accepts the #{int_access_level} integer as access level" do + expect(source.users).not_to include(user) + + member = described_class.add_user(source, user.id, int_access_level) + + expect(member.access_level).to eq(int_access_level) + expect(source.users.reload).to include(user) + end + end + + context 'with no current_user' do + context 'when called with a known user id' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user.id, :master) + + expect(source.users.reload).to include(user) + end + end + + context 'when called with an unknown user id' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, 42, :master) + + expect(source.users.reload).not_to include(user) + end + end + + context 'when called with a user object' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user, :master) + + expect(source.users.reload).to include(user) + end + end + + context 'when called with a requester user object' do + before do + source.request_access(user) + end + + it 'adds the requester as a member' do + expect(source.users).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + + expect { described_class.add_user(source, user, :master) }. + to raise_error(Gitlab::Access::AccessDeniedError) + + expect(source.users.reload).not_to include(user) + expect(source.requesters.reload.exists?(user_id: user)).to be_truthy + end + end + + context 'when called with a known user email' do + it 'adds the user as a member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user.email, :master) + + expect(source.users.reload).to include(user) + end + end + + context 'when called with an unknown user email' do + it 'creates an invited member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, 'user@example.com', :master) + + expect(source.members.invite.pluck(:invite_email)).to include('user@example.com') + end + end + end + + context 'when current_user can update member' do + it 'creates the member' do + expect(source.users).not_to include(user) + + described_class.add_user(source, user, :master, current_user: admin) + + expect(source.users.reload).to include(user) + end + + context 'when called with a requester user object' do + before do + source.request_access(user) + end + + it 'adds the requester as a member' do + expect(source.users).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + + described_class.add_user(source, user, :master, current_user: admin) + + expect(source.users.reload).to include(user) + expect(source.requesters.reload.exists?(user_id: user)).to be_falsy + end + end + end + + context 'when current_user cannot update member' do + it 'does not create the member' do + expect(source.users).not_to include(user) + + member = described_class.add_user(source, user, :master, current_user: user) + + expect(source.users.reload).not_to include(user) + expect(member).not_to be_persisted + end + + context 'when called with a requester user object' do + before do + source.request_access(user) + end + + it 'does not destroy the requester' do + expect(source.users).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + + described_class.add_user(source, user, :master, current_user: user) + + expect(source.users.reload).not_to include(user) + expect(source.requesters.exists?(user_id: user)).to be_truthy + end + end + end + + context 'when member already exists' do + before do + source.add_user(user, :developer) + end + + context 'with no current_user' do + it 'updates the member' do + expect(source.users).to include(user) + + described_class.add_user(source, user, :master) + + expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER) + end + end + + context 'when current_user can update member' do + it 'updates the member' do + expect(source.users).to include(user) + + described_class.add_user(source, user, :master, current_user: admin) + + expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::MASTER) + end + end + + context 'when current_user cannot update member' do + it 'does not update the member' do + expect(source.users).to include(user) - context "when called with an unknown user email" do - it "adds a member invite" do - Member.add_user(project.project_members, "user@example.com", ProjectMember::MASTER) + described_class.add_user(source, user, :master, current_user: user) - expect(project.project_members.invite.pluck(:invite_email)).to include("user@example.com") + expect(source.members.find_by(user_id: user).access_level).to eq(Gitlab::Access::DEVELOPER) + end + end + end end end end diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index 56fa7fa6134..370aeb9e0a9 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -1,6 +1,33 @@ require 'spec_helper' describe GroupMember, models: true do + describe '.access_level_roles' do + it 'returns Gitlab::Access.options_with_owner' do + expect(described_class.access_level_roles).to eq(Gitlab::Access.options_with_owner) + end + end + + describe '.access_levels' do + it 'returns Gitlab::Access.options_with_owner' do + expect(described_class.access_levels).to eq(Gitlab::Access.sym_options_with_owner) + end + end + + describe '.add_users_to_group' do + it 'adds the given users to the given group' do + group = create(:group) + users = create_list(:user, 2) + + described_class.add_users_to_group( + group, + [users.first.id, users.second], + described_class::MASTER + ) + + expect(group.users).to include(users.first, users.second) + end + end + describe 'notifications' do describe "#after_create" do it "sends email to user" do diff --git a/spec/models/members/project_member_spec.rb b/spec/models/members/project_member_spec.rb index 805c15a4e5e..d85a1c1e3b2 100644 --- a/spec/models/members/project_member_spec.rb +++ b/spec/models/members/project_member_spec.rb @@ -15,6 +15,26 @@ describe ProjectMember, models: true do it { is_expected.to include_module(Gitlab::ShellAdapter) } end + describe '.access_level_roles' do + it 'returns Gitlab::Access.options' do + expect(described_class.access_level_roles).to eq(Gitlab::Access.options) + end + end + + describe '.add_user' do + context 'when called with the project owner' do + it 'adds the user as a member' do + project = create(:empty_project) + + expect(project.users).not_to include(project.owner) + + described_class.add_user(project, project.owner, :master, current_user: project.owner) + + expect(project.users.reload).to include(project.owner) + end + end + end + describe '#real_source_type' do subject { create(:project_member).real_source_type } @@ -50,7 +70,7 @@ describe ProjectMember, models: true do end end - describe :import_team do + describe '.import_team' do before do @project_1 = create :project @project_2 = create :project @@ -81,25 +101,21 @@ describe ProjectMember, models: true do end describe '.add_users_to_projects' do - before do - @project_1 = create :project - @project_2 = create :project + it 'adds the given users to the given projects' do + projects = create_list(:empty_project, 2) + users = create_list(:user, 2) - @user_1 = create :user - @user_2 = create :user - - ProjectMember.add_users_to_projects( - [@project_1.id, @project_2.id], - [@user_1.id, @user_2.id], - ProjectMember::MASTER - ) - end + described_class.add_users_to_projects( + [projects.first.id, projects.second], + [users.first.id, users.second], + described_class::MASTER) - it { expect(@project_1.users).to include(@user_1) } - it { expect(@project_1.users).to include(@user_2) } + expect(projects.first.users).to include(users.first) + expect(projects.first.users).to include(users.second) - it { expect(@project_2.users).to include(@user_1) } - it { expect(@project_2.users).to include(@user_2) } + expect(projects.second.users).to include(users.first) + expect(projects.second.users).to include(users.second) + end end describe '.truncate_teams' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 83f61f0af0a..ef854a25321 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -68,7 +68,7 @@ describe Project, models: true do it { is_expected.to have_many(:forks).through(:forked_project_links) } describe '#members & #requesters' do - let(:project) { create(:project) } + let(:project) { create(:project, :public) } let(:requester) { create(:user) } let(:developer) { create(:user) } before do @@ -836,7 +836,7 @@ describe Project, models: true do describe 'when a user has access to a project' do before do - project.team.add_user(user, Gitlab::Access::MASTER) + project.add_user(user, Gitlab::Access::MASTER) end it { is_expected.to eq([project]) } diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index f979d66c88c..e0f2dadf189 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -137,7 +137,7 @@ describe ProjectTeam, models: true do describe '#find_member' do context 'personal project' do - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } let(:requester) { create(:user) } before do @@ -200,7 +200,7 @@ describe ProjectTeam, models: true do let(:requester) { create(:user) } context 'personal project' do - let(:project) { create(:empty_project) } + let(:project) { create(:empty_project, :public) } context 'when project is not shared with group' do before do diff --git a/spec/requests/api/access_requests_spec.rb b/spec/requests/api/access_requests_spec.rb index d78494b76fa..905a7311372 100644 --- a/spec/requests/api/access_requests_spec.rb +++ b/spec/requests/api/access_requests_spec.rb @@ -64,12 +64,12 @@ describe API::AccessRequests, api: true do context 'when authenticated as a member' do %i[developer master].each do |type| context "as a #{type}" do - it 'returns 400' do + it 'returns 403' do expect do user = public_send(type) post api("/#{source_type.pluralize}/#{source.id}/access_requests", user) - expect(response).to have_http_status(400) + expect(response).to have_http_status(403) end.not_to change { source.requesters.count } end end @@ -87,6 +87,20 @@ describe API::AccessRequests, api: true do end context 'when authenticated as a stranger' do + context "when access request is disabled for the #{source_type}" do + before do + source.update(request_access_enabled: false) + end + + it 'returns 403' do + expect do + post api("/#{source_type.pluralize}/#{source.id}/access_requests", stranger) + + expect(response).to have_http_status(403) + end.not_to change { source.requesters.count } + end + end + it 'returns 201' do expect do post api("/#{source_type.pluralize}/#{source.id}/access_requests", stranger) diff --git a/spec/requests/api/merge_requests_spec.rb b/spec/requests/api/merge_requests_spec.rb index a7930c59df9..b813ee967f8 100644 --- a/spec/requests/api/merge_requests_spec.rb +++ b/spec/requests/api/merge_requests_spec.rb @@ -15,7 +15,7 @@ describe API::API, api: true do let(:milestone) { create(:milestone, title: '1.0.0', project: project) } before do - project.team << [user, :reporters] + project.team << [user, :reporter] end describe "GET /projects/:id/merge_requests" do @@ -299,7 +299,7 @@ describe API::API, api: true do let!(:unrelated_project) { create(:project, namespace: create(:user).namespace, creator_id: user2.id) } before :each do |each| - fork_project.team << [user2, :reporters] + fork_project.team << [user2, :reporter] end it "returns merge_request" do diff --git a/spec/services/members/request_access_service_spec.rb b/spec/services/members/request_access_service_spec.rb new file mode 100644 index 00000000000..dff5b4917ae --- /dev/null +++ b/spec/services/members/request_access_service_spec.rb @@ -0,0 +1,57 @@ +require 'spec_helper' + +describe Members::RequestAccessService, services: true do + let(:user) { create(:user) } + let(:project) { create(:project, :private) } + let(:group) { create(:group, :private) } + + shared_examples 'a service raising Gitlab::Access::AccessDeniedError' do + it 'raises Gitlab::Access::AccessDeniedError' do + expect { described_class.new(source, user).execute }.to raise_error(Gitlab::Access::AccessDeniedError) + end + end + + shared_examples 'a service creating a access request' do + it 'succeeds' do + expect { described_class.new(source, user).execute }.to change { source.requesters.count }.by(1) + end + + it 'returns a <Source>Member' do + member = described_class.new(source, user).execute + + expect(member).to be_a "#{source.class.to_s}Member".constantize + expect(member.requested_at).to be_present + end + end + + context 'when source is nil' do + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let(:source) { nil } + end + end + + context 'when current user cannot request access to the project' do + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let(:source) { project } + end + + it_behaves_like 'a service raising Gitlab::Access::AccessDeniedError' do + let(:source) { group } + end + end + + context 'when current user can request access to the project' do + before do + project.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + group.update(visibility_level: Gitlab::VisibilityLevel::PUBLIC) + end + + it_behaves_like 'a service creating a access request' do + let(:source) { project } + end + + it_behaves_like 'a service creating a access request' do + let(:source) { group } + end + end +end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 0d152534c38..d820646ebdf 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -962,6 +962,20 @@ describe NotificationService, services: true do should_not_email(@u_lazy_participant) end + it "notifies the merger when merge_when_build_succeeds is true" do + merge_request.merge_when_build_succeeds = true + notification.merge_mr(merge_request, @u_watcher) + + should_email(@u_watcher) + end + + it "does not notify the merger when merge_when_build_succeeds is false" do + merge_request.merge_when_build_succeeds = false + notification.merge_mr(merge_request, @u_watcher) + + should_not_email(@u_watcher) + end + context 'participating' do context 'by assignee' do before do diff --git a/spec/services/system_note_service_spec.rb b/spec/services/system_note_service_spec.rb index 3d854a959f3..b16840a1238 100644 --- a/spec/services/system_note_service_spec.rb +++ b/spec/services/system_note_service_spec.rb @@ -40,6 +40,12 @@ describe SystemNoteService, services: true do describe 'note body' do let(:note_lines) { subject.note.split("\n").reject(&:blank?) } + describe 'comparison diff link line' do + it 'adds the comparison text' do + expect(note_lines[2]).to match "[Compare with previous version]" + end + end + context 'without existing commits' do it 'adds a message header' do expect(note_lines[0]).to eq "Added #{new_commits.size} commits:" |