Open carpodaster opened 9 years ago
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.
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!!
All :+1: from me!
I added the requested validation process in this PR: https://github.com/rails-girls-summer-of-code/rgsoc-teams/pull/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 +/-.
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?
@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! :sparkles:
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?
I think I could also prevent this through CanCan abilities. Then disable the radios like in https://github.com/rails-girls-summer-of-code/rgsoc-teams/commit/a85416d72d8c45a82f0111a8e15a11c123cd2940?
It it's not super urgent, I'll look at it, but may not find the time today.
is this still a thing? or can we close?
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.
@carpodaster then https://github.com/rails-girls-summer-of-code/rgsoc-teams/pull/425 would partly take care of this, at least. :)
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 ?
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!
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. 🤣
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 https://github.com/rails-girls-summer-of-code/rgsoc-teams/pull/425) 2) keep team members from changing their roles after they've been set (fixed in https://github.com/rails-girls-summer-of-code/rgsoc-teams/commit/a85416d72d8c45a82f0111a8e15a11c123cd2940) 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 https://github.com/rails-girls-summer-of-code/rgsoc-teams/commit/a85416d72d8c45a82f0111a8e15a11c123cd2940#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.
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. 😞
I fear we have to completly rething and rebuild the team creation / editing process.
yep, looks that way unfortunately. :(
Cool!
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?
+1 from me
There are (or will be) validations for a team's composition on
Team
, but they won't be fired when editing a role: