Team: ensure that student-restrictions cannot be tricked by editing a role directly

[carpodaster]

There are (or will be) validations for a team's composition on Team, but they won't be fired when editing a role:

• Student must not be part of another team (Ensure a student can only be student in one team (per season) at a time #138)
• Team must not have more than two students (Team: ensure that only two students can be part of a team, not more #204)

[klappradla]

I'd love to look into this (and related) if noone is on it right now.
Would also suggest to, while on it, hide the + New Team button in the team's index view for students already being part of a team. This button tricked me at first.

[alicetragedy]

as far as I know @klappradla no one is on it. ( @ramonh @carpodaster correct me if I'm wrong ) so please go ahead :) +1 on hiding the New Team button, I think it's sensible. In fact, I believe there are quite a few UX things we could be fixing on the teams app ;) thank you!!

[hola-soy-milk]

All 👍 from me!

[klappradla]

I added the requested validation process in this PR: #389
It includes some rather subjective thoughts on the UX I would be totally fine to withdraw again. As the issue addresses more model-level stuff, I didn't really want to make any view- and UX-related decisions and therefore did not hide the New Team button yet.

If someone could give me a quick feedback wether UX-wise I'm getting this right or wrong, I'd take this as a basis to then go ahead with button +/-.

[klappradla]

I'll try to submit the UX changes (hide New Team and remove Add Member button) tonight if I'm not to sore from work (will be Saturday otherwise).

@alicetragedy since you're changing quite a lot around the forms, is it still reasonable for me to start from master for this?

[alicetragedy]

@klappradla absolutely. I'm not dealing with anything in the teams views, just application_drafts, so we won't run into any problems if you start from master. go ahead and — thank you! ✨

[carpodaster]

I've stuffed a few "security" holes in a85416d but I noticed another: When you from a new team, your own user is preset as the first role but you can choose freely between being a coach or a student. It makes no sense for other roles than a student to form a team, hence this shouldn't be possible. Any takers?

[klappradla]

I think I could also prevent this through CanCan abilities. Then disable the radios like in a85416d?

It it's not super urgent, I'll look at it, but may not find the time today.

[alicetragedy]

is this still a thing? or can we close?

[carpodaster]

I fear this is still somewhat of a thing. I would also count the "a student can create a team with them as coaches" as being part of this.

[alicetragedy]

@carpodaster then #425 would partly take care of this, at least. :)

[klappradla]

Oh, just realized this is assigned to me - I actually don't really remember the context right now. But it's still relevant and should be fixed @alicetragedy ?

[alicetragedy]

I don't remember much of the context either.
Ramon's PR seems to have fixed the bug with students adding themselves as coaches, so I don't have an overview anymore on what the exact problem is here. I'll try to test this tomorrow to see if it's still an issue!

[emcoding]

I am pretty sure this one is very relevant. IIRC suggestion was somewhere to add a cannot change role ability for students. Or maybe that was my implementation idea. 🤣

[alicetragedy]

So, I've taken a look at this ticket and tested how it currently works in the app, to see how much has been resolved. The issues were:

1. keep students creating a team from setting / changing their own role (fixed in Block user from being multiple members of a team #425)
2. keep team members from changing their roles after they've been set (fixed in a85416d)
3. remove “new team” button from UI for students that already belong to a team for that season (fixed in https://github.com/klappradla/rgsoc-teams/commit/153114aab14b7d8ef88c26d5169bd1cf847f9afe with CanCan)

I am pretty sure that this ticket has hence been tackled ;)
An improvement to 2) would be to add a CanCan rule (see a85416d#commitcomment-16658878) but this is not urgent, definitely not a bug anymore, and could be closed, or at least removed from the Application milestone.

@carpodaster could I quickly ask for your confirmation on the above? I think I got everything but might have missed something important.

[alicetragedy]

Sorry, ignore what I wrote above. Seems like 1. is partly still there after all. As a user creating a team for the first time, it's still possible to change the preselected “Student” to “Coach” and add yourself to a team as a coach instead of a student. 😞

[carpodaster]

I fear we have to completly rething and rebuild the team creation / editing process.

[alicetragedy]

yep, looks that way unfortunately. :(

[emcoding]

Cool!

[emcoding]

Can we make this a bit more actionable? Sounds like a really good thing to do before Summer of 18, si?
@klappradla You still in?

[alicetragedy]

+1 from me