Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Do not invite manager as member when creating a team. #3308

Merged
merged 1 commit into from Feb 18, 2017
Merged

Do not invite manager as member when creating a team. #3308

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion app/routes/teams.rb
View file
Open in desktop
Expand Up @@ -31,10 +31,14 @@ class Teams < Core
# Create a new team.
post '/teams/new' do
please_login

team = Team.by(current_user).defined_with(params[:team], current_user)
usernames_invited = params[:team].fetch('usernames', '')

if team.valid?
team.save
team.save!
TeamMembership.create(team: team, user: current_user, inviter: current_user, confirmed: true)
team.invite_with_usernames(usernames_invited, current_user)
redirect "/teams/#{team.slug}/directory"
else
erb :"teams/new", locals: { team: team }
Expand Down
4 changes: 0 additions & 4 deletions lib/exercism/team.rb
View file
Open in desktop
Expand Up @@ -56,10 +56,6 @@ def defined_with(options, inviter=nil)
tags = [options[:slug], options[:name], options[:tags]].join(',')
self.tags = Tag.create_from_text(tags)

users = User.find_or_create_in_usernames(potential_members(options[:usernames])) if options[:usernames]
users = options[:users] if options[:users]
invite(users, inviter)

self
end
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find a place where options[:users] were being used, so I dropped it. Thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's probably left-over from something.


Expand Down
6 changes: 4 additions & 2 deletions test/acceptance/profile_test.rb
View file
Open in desktop
Expand Up @@ -29,8 +29,10 @@ def test_profile_shows_progress

def test_display_team_invites_only_in_users_own_profile
new_user = create_user(username: 'new_user', github_id: 456)
attributes = { slug: 'some-team', name: 'Some Name', usernames: 'new_user' }
Team.by(@user).defined_with(attributes, @user).save!

team = Team.by(@user).defined_with({ slug: 'some-team', name: 'Some Name' }, @user)
Copy link

@Insti Insti Dec 13, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably a good reason, but why does this* need to change?

It seems like creating a team with existing members is a useful thing to be able to do in one shot.
This would probably require different changes to lib/exercism/team.rb but won't require changing everything else.

* In all these files, not specifically here in profile_test.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like creating a team with existing members is a useful thing to be able to do in one shot.

I would say yes, but we would need to wrap things in a transaction and start rescuing from everywhere. The alternative would be checking inside the Team class if the instance has been persisted and then invite its potential members. I don't really like this alternative because it should not be the responsibility of the team class to check for this stuff (I may be wrong in my assumption).

Copy link

@Insti Insti Dec 13, 2016 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you create the invitations using an after_save callback?

It's probably not the responsibility of a Team class, but that's not all it is any more.
Don't let what it's called distract you from what it is.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but I don't really like the idea that just because it is already doing too much we can just add more behaviour into the object.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome back @kytrinyx :)

Can I have fresh eyes on this? I like the current state of the code, but a new abstraction for creating a team and inviting members would also be valuable, I just can't come up with a good name for this "service".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, yes (sorry! it's taken forever to catch up with everything).

Looking now.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the idea of an after save, I do like the current state of the code, and agree that a new abstraction would be even better. If we could name it.

I honestly can't think of anything.

team.save!
team.invite_with_usernames(new_user.username, @user)

with_login(@user) do
visit "/#{new_user.username}"
Expand Down
6 changes: 4 additions & 2 deletions test/acceptance/team_test.rb
View file
Open in desktop
Expand Up @@ -5,8 +5,10 @@ def test_joining_a_team
creating_user = create_user(username: 'creating_user')
joining_user = create_user(username: 'joining_user', github_id: 123)

attributes = { slug: 'some-team', name: 'Some Name', usernames: 'joining_user' }
Team.by(creating_user).defined_with(attributes, creating_user).save!
attributes = { slug: 'some-team', name: 'Some Name' }
team = Team.by(creating_user).defined_with(attributes, creating_user)
team.save!
team.invite_with_usernames(joining_user.username, creating_user)

with_login(joining_user) do
click_on 'Account'
Expand Down
7 changes: 5 additions & 2 deletions test/app/invitations_test.rb
View file
Open in desktop
Expand Up @@ -62,8 +62,10 @@ def test_user_must_be_logged_in
end

def test_user_must_be_manager
team = Team.by(alice).defined_with(slug: 'abc', usernames: bob.username)
team = Team.by(alice).defined_with(slug: 'abc')
team.save

team.invite_with_usernames(bob.username, alice)
TeamMembershipInvite.pending.find_by(user: bob, team: team).accept!

verb, path, action = [:post, '/teams/abc/invitations', "invite members"]
Expand All @@ -90,9 +92,10 @@ def test_invitation
end

def test_only_managers_can_invite_members
team = Team.by(alice).defined_with(slug: 'members', usernames: bob.username)
team = Team.by(alice).defined_with(slug: 'members')
team.save

team.invite_with_usernames(bob.username, alice)
post "/teams/#{team.slug}/invitations", { usernames: charlie.username }, login(bob)

team.reload
Expand Down
50 changes: 38 additions & 12 deletions test/app/teams_test.rb
View file
Open in desktop
Expand Up @@ -71,9 +71,10 @@ def test_user_must_be_logged_in
end

def test_user_must_be_manager
team = Team.by(alice).defined_with(slug: 'abc', usernames: bob.username)
team.save
team = Team.by(alice).defined_with(slug: 'abc')
team.save!

team.invite_with_usernames(bob.username, alice)
TeamMembershipInvite.pending.find_by(user: bob, team: team).accept!

[
Expand All @@ -91,7 +92,7 @@ def test_user_must_be_manager
end

def test_user_must_be_on_team_to_view_team_page
team = Team.by(alice).defined_with(slug: 'abc', usernames: bob.username)
team = Team.by(alice).defined_with(slug: 'abc')
team.save

get '/teams/abc/directory', {}, login(alice)
Expand All @@ -101,7 +102,9 @@ def test_user_must_be_on_team_to_view_team_page
assert_equal 302, last_response.status
assert_equal "http://example.org/", last_response.location

team.invite_with_usernames(bob.username, alice)
TeamMembershipInvite.pending.find_by(user: bob, team: team).accept!

get '/teams/abc/directory', {}, login(bob)
assert_equal 200, last_response.status

Expand Down Expand Up @@ -168,7 +171,6 @@ def test_team_creation_with_no_slug

def test_team_creation_with_multiple_members
post '/teams/new', { team: { slug: 'members', usernames: "#{bob.username},#{charlie.username}" } }, login(alice)

team = Team.first

bob.reload
Expand All @@ -193,6 +195,21 @@ def test_team_creation_with_multiple_members
[alice, bob, charlie].each { |member| assert team.includes?(member) }
end

def test_manager_is_automatically_added_as_team_member
post '/teams/new', { team: { slug: 'awesome' } }, login(alice)
team = Team.first

assert TeamMembership.exists?(user: alice, team: team)
end

def test_manager_is_not_invited_when_adding_itself_in_member_list
post '/teams/new', { team: { slug: 'awesome', usernames: alice.username } }, login(alice)
team = Team.first

refute TeamMembershipInvite.exists?(user: alice, team: team)
assert TeamMembership.exists?(user: alice, team: team)
end

def test_member_removal
team = Team.by(alice).defined_with(slug: 'awesome', usernames: "#{bob.username},#{charlie.username}")
team.save
Expand All @@ -206,9 +223,11 @@ def test_member_removal
end

def test_leave_team
team = Team.by(alice).defined_with(slug: 'awesome', usernames: "#{bob.username},#{charlie.username}")
team = Team.by(alice).defined_with(slug: 'awesome')
team.save

team.invite_with_usernames("#{bob.username},#{charlie.username}", alice)

put "/teams/#{team.slug}/accept_invite", {}, login(bob)
put "/teams/#{team.slug}/leave", {}, login(bob)

Expand All @@ -218,9 +237,11 @@ def test_leave_team
end

def test_only_managers_can_dismiss_other_members
team = Team.by(alice).defined_with(slug: 'members', usernames: "#{bob.username},#{charlie.username}")
team = Team.by(alice).defined_with(slug: 'members')
team.save

team.invite_with_usernames("#{bob.username},#{charlie.username}", alice)

post "/teams/#{team.slug}/invitation/accept", {}, login(charlie)
delete "/teams/#{team.slug}/members/#{charlie.username}", {}, login(bob)

Expand All @@ -231,9 +252,11 @@ def test_only_managers_can_dismiss_other_members
end

def test_view_a_team_as_a_member
team = Team.by(alice).defined_with(slug: 'members', usernames: "#{bob.username},#{charlie.username}")
team = Team.by(alice).defined_with(slug: 'members')
team.save

team.invite_with_usernames("#{bob.username},#{charlie.username}", alice)

# unconfirmed member
get "/teams/#{team.slug}", {}, login(bob)

Expand All @@ -247,7 +270,7 @@ def test_view_a_team_as_a_member
end

def test_view_an_escaped_team_name
team = Team.by(alice).defined_with(slug: 'members', name: "<script>alert('esc_test');</script>", usernames: "#{bob.username},#{charlie.username}")
team = Team.by(alice).defined_with(slug: 'members', name: "<script>alert('esc_test');</script>")
team.save

get "/teams/#{team.slug}/directory", {}, login(alice)
Expand All @@ -257,7 +280,7 @@ def test_view_an_escaped_team_name
end

def test_view_team_as_a_non_member
team = Team.by(alice).defined_with(slug: 'members', usernames: bob.username.to_s)
team = Team.by(alice).defined_with(slug: 'members')
team.save

get "/teams/#{team.slug}/directory", {}, login(charlie)
Expand All @@ -266,17 +289,20 @@ def test_view_team_as_a_non_member
end

def test_delete_team_without_being_manager
team = Team.by(alice).defined_with(slug: 'delete', usernames: bob.username.to_s)
team = Team.by(alice).defined_with(slug: 'delete')
team.save

team.invite_with_usernames(bob.username, alice)
post "/teams/#{team.slug}/invitation/accept", {}, login(bob)

delete "/teams/#{team.slug}", {}, login(bob)

assert_response_status(302)
assert Team.exists?(slug: 'delete')
end

def test_delete_team_as_manager
team = Team.by(alice).defined_with(slug: 'delete', usernames: bob.username.to_s)
team = Team.by(alice).defined_with(slug: 'delete')
team.save

delete "/teams/#{team.slug}", {}, login(alice)
Expand All @@ -287,7 +313,7 @@ def test_delete_team_as_manager
end

def test_edit_teams_attributes
team = Team.by(alice).defined_with(slug: 'edit', usernames: bob.username.to_s, description: 'No name')
team = Team.by(alice).defined_with(slug: 'edit', description: 'No name')
team.save

refute team.public?
Expand Down
23 changes: 16 additions & 7 deletions test/exercism/team_test.rb
View file
Open in desktop
Expand Up @@ -78,8 +78,10 @@ def test_normalize_slug
end

def test_has_members
team = Team.by(bob).defined_with(slug: 'purple', usernames: ' alice , charlie-brown ')
team = Team.by(bob).defined_with(slug: 'purple')
team.save

team.invite_with_usernames(' alice , charlie-brown ', bob)
team.reload

assert_equal [alice, charlie], team.member_invites
Expand Down Expand Up @@ -109,8 +111,10 @@ def test_team_invite

def test_team_does_not_invite_more_than_once
inviter = User.create(username: 'inviter')
team = Team.by(alice).defined_with(slug: 'awesome', usernames: 'bob')
team = Team.by(alice).defined_with(slug: 'awesome')
team.save

team.invite_with_usernames('bob', alice)
team.reload

assert_equal 1, bob.team_membership_invites.where(team_id: team).count
Expand All @@ -119,10 +123,12 @@ def test_team_does_not_invite_more_than_once
end

def test_team_member_dismiss
team = Team.by(alice).defined_with(slug: 'awesome', usernames: 'bob')
team = Team.by(alice).defined_with(slug: 'awesome')
team.save

team.invite_with_usernames(bob.username, alice)
bob.team_membership_invites_for(team).accept!

team.reload
assert team.includes?(bob)

Expand All @@ -132,10 +138,12 @@ def test_team_member_dismiss
end

def test_team_member_dismiss_ignores_invalid_member
team = Team.by(alice).defined_with(slug: 'awesome', usernames: bob.username)
team = Team.by(alice).defined_with(slug: 'awesome')
team.save

team.invite_with_usernames(bob.username, alice)
bob.team_membership_invites_for(team).accept!

team.reload
assert_equal 1, team.members.size

Expand All @@ -144,9 +152,10 @@ def test_team_member_dismiss_ignores_invalid_member
end

def test_team_member_dismiss_removes_membership
team = Team.by(alice).defined_with(slug: 'awesome', usernames: 'bob')
team = Team.by(alice).defined_with(slug: 'awesome')
team.save

team.invite_with_usernames(bob.username, alice)
bob.team_membership_invites_for(team).accept!
assert TeamMembership.exists?(team_id: team, user_id: bob)

Expand All @@ -155,10 +164,10 @@ def test_team_member_dismiss_removes_membership
end

def test_destroy_doesnt_leave_orphan_team_memberships
attributes = { slug: 'delete', usernames: bob.username.to_s }
team = Team.by(alice).defined_with(attributes, alice)
team = Team.by(alice).defined_with({ slug: 'delete' }, alice)
team.save

team.invite_with_usernames(bob.username, alice)
bob.team_membership_invites_for(team).accept!

assert TeamMembership.exists?(team: team, user: bob, inviter: alice)
Expand Down
8 changes: 6 additions & 2 deletions test/exercism/user_test.rb
View file
Open in desktop
Expand Up @@ -89,11 +89,15 @@ def test_delete_team_memberships_with_user
alice = User.create(username: 'alice')
bob = User.create(username: 'bob')

team = Team.by(alice).defined_with({ slug: 'team a', usernames: bob.username }, alice)
other_team = Team.by(alice).defined_with({ slug: 'team b', usernames: bob.username }, alice)
team = Team.by(alice).defined_with({ slug: 'team a' }, alice)
other_team = Team.by(alice).defined_with({ slug: 'team b' }, alice)

team.save
other_team.save

team.invite_with_usernames(bob.username, alice)
other_team.invite_with_usernames(bob.username, alice)

TeamMembershipInvite.where(user: bob).first.accept!

assert TeamMembership.exists?(team: team, user: bob, inviter: alice), 'Confirmed TeamMembership for bob was created.'
Expand Down