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

bernardoamc · 2016-12-13T00:53:37Z

Right now the manager is already being added as a member, so we don't need to send the invite.

To solve the problem we have to make sure that the team is created properly and the manager is added as manager before inviting members from the usernames list.

bernardoamc · 2016-12-13T00:55:38Z

I touched a bunch of files, but the core change is in app/routes/teams.rb and lib/exercism/team.rb.

@kytrinyx @Insti @kotp could you validate those changes? <3

bernardoamc · 2016-12-13T01:14:05Z

lib/exercism/team.rb

@@ -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]


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

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

Insti · 2016-12-13T09:23:30Z

test/acceptance/profile_test.rb

@@ -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!


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.

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).

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.

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.

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".

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

Looking now.

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.

Insti

I'd like to see a more elegant solution.
(This may be an unreasonable request.)

bernardoamc · 2016-12-13T14:46:45Z

I'd like to see a more elegant solution.

I was tempted to delegated everything to the Team class and leave things as is, but I felt like it would be pushing too much into the class. All in all I don't feel like this is a bad solution, imo the responsibility to check if the team was persisted and only then invite people lies in the controller. Totally up to explore new implementations though! 👍

Insti · 2016-12-13T19:36:39Z

If you think your current solution is fine and you don't think the things I'm suggesting are fun or interesting, that's OK, I'll happily switch my review to 'approved.'

And I don't think your current solution is "bad" but I do think it leaves the code messier than it was to start with, and down that path lies all kinds of horrible.

I was tempted to delegated everything to the Team class and leave things as is, but I felt like it would be pushing too much into the class.

That seems like a step in the right direction. The trick is to not stop once you've put it in there but continue through the extraction of better defined objects.

The Team class is already doing too much, putting a little more in there will hopefully make this more obvious and lead to the extraction of some more useful objects. By spreading the responsibilities out all through the test files and the controller it effectively hides the responsibilities of the TeamAndInvitationAndMembershipCreationAndMaintainance class without putting them anywhere specific.

❤️

bernardoamc · 2016-12-14T01:30:33Z

@Insti Do you think it is not the job of the controller to check if the team is valid and then invite it's users after persisting it? Or do you think an abstraction that deal with this would be better?

And I don't think your current solution is "bad" but I do think it leaves the code messier than it was to start with, and down that path lies all kinds of horrible.

I agree and disagree at the same time. I would say the code is less practical, but more explicit. Also, right now we only try to send invites if the team is really persisted (which was not happening before).

The Team class is already doing too much, putting a little more in there will hopefully make this more obvious and lead to the extraction of some more useful objects.

I would say it is already obvious, right? I think that if we want to take the path to make things simpler it would be better to just extract this to a proper object (which I'm not against at all).

And thank you for the amazing feedback, it really helps to have these kind of conversations. ❤️

Insti · 2016-12-14T19:39:41Z

Do you think it is not the job of the controller to check if the team is valid and then invite it's users after persisting it? Or do you think an abstraction that deal with this would be better?

I don't think it's the job of the controller to do that, the controller's job is to take the http request, send the parameters to the right object and return some kind of http response.

I would prefer the team validation and invitations were dealt with by an abstraction.
Benefits:

You can unit test it without having to go through the controller.
It makes the controller code REALLY simple.

now we only try to send invites if the team is really persisted (which was not happening before).

This is a good change.

I think that if we want to take the path to make things simpler it would be better to just extract this to a proper object (which I'm not against at all).

If you want to do it that way that's up to you. I think we both agree there are some things that need extracting here, so extract away!

And thank you for the amazing feedback, it really helps to have these kind of conversations.

❤️

kotp · 2017-01-13T02:22:25Z

Wanted to bump this now that the holidays are over. Is there time for this?

bernardoamc · 2017-02-08T03:24:14Z

Sorry for taking so long to answer, I think I can say I'm officially back. Should I close this PR and try a different approach?

kotp · 2017-02-08T03:50:44Z

You could keep this PR and try a different approach... force push works well for this. :) Or keep this approach, name it alternative, and do another to compare with... up to you @bernardoamc so your choice.

bernardoamc · 2017-02-08T04:01:24Z

You could keep this PR and try a different approach... force push works well for this. :) Or keep this approach, name it alternative, and do another to compare with... up to you @bernardoamc so your choice.

That's true! I don't have a good alternative so far though, what do you think @kotp? Would love to hear your opinion on a proper solution to this problem. :)

kytrinyx · 2017-02-12T21:45:21Z

I think we should keep what we have for now. I think it is better than it was, even though it is a bit more verbose.

bernardoamc · 2017-02-18T21:06:50Z

I would love to come up with a better abstraction to clean this up, but as an upside I think the code more reliable than it was.

kotp · 2017-02-18T23:11:39Z

I think it is time for this to come in. We can think about the abstraction at some later date.

Thanks @bernardoamc !!

kytrinyx · 2017-02-19T16:07:16Z

The tests are failing after merging this. @bernardoamc would you have a chance to look into it today?

bernardoamc · 2017-02-19T16:26:48Z

The tests are failing after merging this. @bernardoamc would you have a chance to look into it today?

Weird, maybe we should have rebased before the merge? I will take a look @kytrinyx. 👍

kotp · 2017-02-19T18:11:55Z

I do have a branch to revert, in preparation...

bernardoamc · 2017-02-19T18:29:21Z

Don't worry @kotp, #3382 fix the tests.

Do not invite manager as member when creating a team.

47408a0

To solve the problem we have to make sure that the team is created properly and the manager is added as manager before inviting members from the usernames list.

bernardoamc commented Dec 13, 2016

View reviewed changes

bernardoamc added the ready label Dec 13, 2016

Insti reviewed Dec 13, 2016

View reviewed changes

Insti suggested changes Dec 13, 2016

View reviewed changes

Daniel-at-github mentioned this pull request Feb 2, 2017

inviting yourself when creating a team creates a faulty invitation #3371

Closed

bernardoamc mentioned this pull request Feb 8, 2017

Error page after Accept on team invitation to self #3372

Closed

kotp merged commit cf8bf31 into exercism:master Feb 18, 2017

kotp mentioned this pull request Feb 19, 2017

Explicitly invite members in tests. #3382

Merged

mhelmetag mentioned this pull request Feb 20, 2017

Inviting yourself to new team produces a weird notification #3383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

bernardoamc commented Dec 13, 2016

bernardoamc commented Dec 13, 2016 •

edited

bernardoamc Dec 13, 2016

kytrinyx Feb 12, 2017

Insti Dec 13, 2016 •

edited

bernardoamc Dec 13, 2016

Insti Dec 13, 2016 •

edited

bernardoamc Dec 14, 2016

bernardoamc Jan 2, 2017

kytrinyx Feb 12, 2017

kytrinyx Feb 12, 2017

Insti left a comment

bernardoamc commented Dec 13, 2016

Insti commented Dec 13, 2016

bernardoamc commented Dec 14, 2016

Insti commented Dec 14, 2016 •

edited

kotp commented Jan 13, 2017

bernardoamc commented Feb 8, 2017

kotp commented Feb 8, 2017

bernardoamc commented Feb 8, 2017

kytrinyx commented Feb 12, 2017

bernardoamc commented Feb 18, 2017

kotp commented Feb 18, 2017

kytrinyx commented Feb 19, 2017

bernardoamc commented Feb 19, 2017

kotp commented Feb 19, 2017

bernardoamc commented Feb 19, 2017

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

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

Conversation

bernardoamc commented Dec 13, 2016

bernardoamc commented Dec 13, 2016 • edited

bernardoamc Dec 13, 2016

Choose a reason for hiding this comment

kytrinyx Feb 12, 2017

Choose a reason for hiding this comment

Insti Dec 13, 2016 • edited

Choose a reason for hiding this comment

bernardoamc Dec 13, 2016

Choose a reason for hiding this comment

Insti Dec 13, 2016 • edited

Choose a reason for hiding this comment

bernardoamc Dec 14, 2016

Choose a reason for hiding this comment

bernardoamc Jan 2, 2017

Choose a reason for hiding this comment

kytrinyx Feb 12, 2017

Choose a reason for hiding this comment

kytrinyx Feb 12, 2017

Choose a reason for hiding this comment

Insti left a comment

Choose a reason for hiding this comment

bernardoamc commented Dec 13, 2016

Insti commented Dec 13, 2016

bernardoamc commented Dec 14, 2016

Insti commented Dec 14, 2016 • edited

kotp commented Jan 13, 2017

bernardoamc commented Feb 8, 2017

kotp commented Feb 8, 2017

bernardoamc commented Feb 8, 2017

kytrinyx commented Feb 12, 2017

bernardoamc commented Feb 18, 2017

kotp commented Feb 18, 2017

kytrinyx commented Feb 19, 2017

bernardoamc commented Feb 19, 2017

kotp commented Feb 19, 2017

bernardoamc commented Feb 19, 2017

bernardoamc commented Dec 13, 2016 •

edited

Insti Dec 13, 2016 •

edited

Insti Dec 13, 2016 •

edited

Insti commented Dec 14, 2016 •

edited