Skip to content
This repository has been archived by the owner on Jun 27, 2023. It is now read-only.

Don't show the Team new form if a user is not allowed to create one. #1003

Closed
3 of 7 tasks
emcoding opened this issue May 20, 2018 · 5 comments
Closed
3 of 7 tasks

Don't show the Team new form if a user is not allowed to create one. #1003

emcoding opened this issue May 20, 2018 · 5 comments

Comments

@emcoding
Copy link
Member

emcoding commented May 20, 2018

Came up in #997 (Abilities)

OLD behaviour: when I logged in as a student in an accepted team in current season,
Visit '/teams/

  • The '+ New Team' button is present
  • Click; I fill in the form, set myself as a student, and on :submit the validation kicks in. See models/team_spec.rb:48
  • (BTW, when I myself as a coach, the new team is created in the same season. 🤔 But a student can't be a coach too, right?)

NEW behaviour:
When a user is not allowed to create a new team (for instance, a student is not allowed to create a new team in the same season):
a) the button 'Create a new team' is hidden
b) cancancan will not allow access to the new Team form.

DONE in #997: fix cancancan implementation for creating a second team as a student

  • Check cancancan view helper on Team index page (remove the params)
  • add :authorize to :new action
  • Update ability rule for creating a second team as a student. See the ability_specs for the intended behaviour
    TODO
  • Add a meaningful message for unauthorised access. This probably requires changes in the way we rescue Not Authorised errors.
@emcoding
Copy link
Member Author

This is already solved in 6e7d601 .
It does change some behaviour, as listed in the description ^.

Do we agree that we want the new behaviour?

@pgaspar
Copy link
Collaborator

pgaspar commented May 22, 2018

I like the new behavior 👍 especially hiding the buttons for people who shouldn't be able to use them.

@hola-soy-milk
Copy link
Member

Yes, thank you @F3PiX I think this is fantastic!

@klappradla
Copy link
Member

Great!

I'd even ditch the last TODO ("Add a meaningful message for unauthorised access"). With the button no longer being visible, users ending up as unauthorized either hit the URL manually, had an old tab open, etc. which I'd all consider as edge cases where the app does not necessarily need to provide more information 😉 So it's a nice to have, but not super relevant for getting the abilities back up to speed now.

For a student not being a coach in the same season: it looks like this is more of a "why would anyone want this", rather than an explicit restriction in the guides and the code. As far as I can see, this won't impact the duties the Teams App is doing for the program at all, so I'd again not care about this edge case.
The only negative implication I can think of are more shitty applications... 🤔

@emcoding
Copy link
Member Author

So happy with y'all's feedback! 🙇 I'll keep a note of the message and close this issue 💃

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants