Team issues

[mkalininait]

• [ ] We restrict adding a coach, who is already a student in another team. At the same time I can add a student who is already a coach in another team. If I try to delete this coach from that team later, I get this error: "[Name] is already is a student on another team for 2017."
• [ ] When I try to add more than 2 students, I get this error: "Roles there cannot be more than 2 students on a team." <- weird phrase.
• [ ] When I try to add a user to a team, and this user is already a member of this team with the same role, I get this error: "Roles user has already been taken" <- weird wording.
• [ ] Additionally, on the "Add member" form, when I try to add a user to a team, and this user is already a member of this team with the same role, I get 2 error boxes. This is not user friendly. Can we show only one box? (or may be remove this 'Add member' form, since we can add members on the 'Edit team' form)

• [ ] When I edit a team, I can NOT edit roles of team members. Is it a bug or a feature? Here is a possible scenario: I added a member and set a wrong role for him/her (say coach instead of student). I press 'Edit', but I can't change the role. If I delete this member and add him/her again with another role, I get the error ''X can't have more than one role in this team!" (which happens because of Issue 420. The solution would be to delete, save and then add the member again, but it's not a good UX.
• [x] Can one person be a coach for more than one team? Now it's possible.
• [x] Little wish: make 'Help section' link open in a new tab. (done in https://github.com/rails-girls-summer-of-code/rgsoc-teams/pull/429/commits/40e5a353e7583b4ef8efb1d334012020560c425c)

[klappradla]

This is not user friendly. Can we show only one box?

I added the second box in order to make it more user-friendly 🙊 . With only one box, you get a error-message saying:

2 errors prohibited this role from being saved:
    User has already been taken
    Team is invalid

Team is invalid is not really a meaningful error-message for a user not knowing the internal datamodel of the application (anyone basically...), therefore I added what's the current approach.

I also thought about fully removing the Add member button, but then decided against, because it acts as a handy shortcut for admins to look and edit a team's members. But if it causes more trouble than benefit, we can also remove it - it does nothing the teams-form does not already do.

[mkalininait]

Tested this ticket on Staging: all unchecked problems are still relevant.

[bitboxer]

For 1: what should be done here? This is just saying what the current status is but not what should be done. For 2, 3: what should the wording be? For 4: is this still relevant after the comment by klappradia? For 5: does this need fixing or is this considered a bug?

[mkalininait]

@bitboxer Hey, sorry for not replying yesterday, i got stuck on the first question.

1. We should restrict adding user as a student if he/she is already added as a coach (confirmed or not confirmed, doesn't matter) in another team. A user can always delete him/herself as a coach from a team in order to become a student (coaches have this permission).

Btw, here is a nice example: Armin has been added to the Team Oberlaa as a coach. Then he has been added to the Team Sleepy Testers as a student. As a result of this, we can't edit the Team Oberlaa anymore because whatever you do you get this: "1 error prohibited this team from being saved: Armin Ronacher already is a student on another team for 2017."

[bitboxer]

Yes, I understand that this is the problem. But what do you want implemented as solution to this?

[mkalininait]

@bitboxer Exactly this -> We should restrict adding user as a student if he/she is already added as a coach (confirmed or not confirmed, doesn't matter) in another team. The error message could be: "1 error prohibited this team from being saved: [username] is already a coach on another team for 2017." Also: it should be possible to delete a coach who is already a student in another team (now it's impossible).

[mkalininait]

@bitboxer 2. "Roles: there cannot be more than 2 students on a team." 3. "Roles: user has already been taken" (If we use this error message only when trying to add one user twice within one team, the message could be "Roles: user has already been added". But I can't tell it just by looking at the UI, it should probably be clearer after reading the code).

I found two more error messages of this type:

• "Roles name is not included in the list"
• "Roles name can't be blank"

I'm not sure how many of them there are in the app. Maybe all we do is just add a colon after the word Roles for all error messages: "Roles" --> "Roles:"? WDYT?

[mkalininait]

@klappradla Thanks for the comment. In that particular screenshot, the error message in the second box repeats on of the error messages in the first box, that's why it looked redundant to me. Maybe there are other use cases though, when the second box provides more useful information. I just didn't see them... If you vouch for 2 boxes, then we won't change it :) //cc @bitboxer re item 4.

[mkalininait]

@bitboxer 5. I'm fine skipping it. WDYT @alicetragedy ?

[bitboxer]

For the error boxes: I will try to make it look like one big box with two messages inside.

[bitboxer]

For the message in the error text: this message is generated from Rails and uses the following convetion: "[Attribute] [Error Message]". I don't want to add a colon between the attribute and the message, this would mean we have to check every error message. I think the best approach is to form a sentence with "Role " at the beginning. For example: "Role need fixing. The user [USERNAME] has already be taken" . Would that be okay?

[bitboxer]

And for the "name can't be blank": This is sadly how rails creates the messages.

Ah, another way would be to inline the error messages into the form and show the text next to the field that has the error. But we would need to redo every form to stay consistant. That is also lots of work.

Maybe we can just fix the two text messages and just ignore it and hope it works for most students?

[bitboxer]

@mkalininait I tried to reproduce the two error message boxes, but I can't. It totally looks different on my machine:

I assume now it is already fixed. If not, please post details how to reproduce it with a new screenshot.

With all what we have discussed here, the things left to do:

• Prevent Coaches to be Students in other teams. This makes the "delete them" part obsolete, because you won't be able to create such teams.
• Change the error messages a bit to make them clearer.

[bitboxer]

After playing around with the coach / student rules I figured out that currently the system is setup in a way that everyone can get every role as often as they want. Inclduing that a person can always be additionally added as a student in other teams. But once this person is a student somewhere, that person can't be removed or edited in other teams because the check if the person is a student prevents this.

There are now two of ways to fix this:

• Prevent a Person to be on more than one team in whatever role.
• Prevent a Person to become a student in a team when that person is already added in other teams in whatever role

So this problem is not only limited to coaches, but to everyone who is added as a student and already has a role somewhere.

What should I do? The first or the second version? Currently I think I would vote for the 2nd one because it could be okay that a person coaches more than one team. Am I right?

[mkalininait]

Hi @bitboxer Regarding the "coach / student rules".

it could be okay that a person coaches more than one team. Am I right?

That's right, one person can coach more than one team.

The first or the second version?

The 2nd looks correct to me. Note: we should check teams within current season.

Prevent Coaches to be Students in other teams. This makes the "delete them" part obsolete, because you won't be able to create such teams.

That's true, you can't add new teams with this issue, but there may be old ones, which have been created before this bugfix. I don't think we will have such cases on Production in this season though.

[mkalininait]

@bitboxer Regarding two error boxex.

I still can reproduce this bug locally. It's important that you try to add the same user with the same role. E.g. there is a Team with student X and coach Y. One box shows up if you try to add X as a coach or Y as a student in this team. But if you try to add X as a student or Y as a coach again, you'll see two boxes.

[mkalininait]

@bitboxer Regarding the error messages.

Thanks for clarifying why they look this way at the moment. That's interesting.

We can definitely start the error messages with "Roles" (or "Role", whatever the name of this attribute is). If we want to change individual messages, we could also change them in a way that they start with a colon, couldn't we? :) Anyway, I don't really have a strong opinion on how the texts should look like ("Roles: [blah-blah]" or "Roles need fixing. [Blah-blah]" or "Roles error. [Blah-blah]") – as long as they are clear and grammatically correct. I'm fine with the option which you consider to be the best :raised_hands:

[carpodaster]

Hey @mkalininait do you find the time to triage your list in the issue description and see what's left? Ideally, we'll take the remainders into new, atomic issues as this one is difficult to manage due to its amount of comments