rgsoc-teams icon indicating copy to clipboard operation
rgsoc-teams copied to clipboard

Issue 1012 - Supervisor unable to see team member email if hide_email: true

Open anubiskhan opened this issue 7 years ago • 4 comments

Related issue #1012

While this does result in a passing test, giving the supervisor the ability to see a team member's email regardless of hide_email flag being turned on, it also allows the supervisor to read all attributes for that team member. This is likely a result of me not fully understanding the codebase yet. I tried to emulate some of the other methodology/syntax present in the models/ability.rb file but was unsuccessful. The strangest part for me is that the 'other_user' (models/ability.rb:51-52) is only an email and not actually a User, hence the find_by_email. If this solution is unacceptable I would be happy to continue working on the issue until I have found the best option. Thanks!

anubiskhan avatar Jul 19 '18 05:07 anubiskhan

I ran the full test suite before and after making this change. All tests pass with the exception of one pending, and one failing test(spec/models/season_spec.rb:208) which was failing when I first cloned the repo.

anubiskhan avatar Jul 19 '18 05:07 anubiskhan

@klappradla @anubiskhan Great that you have this thing going! I think I can shed a light on some of the mysteries, but I won't be able to have a good look until somewhere this weekend.

emcoding avatar Jul 19 '18 18:07 emcoding

@anubiskhan I can see how this must be confusing... The issue was not written with a new contributor in mind. My bad, sorry! Let me give you a bit more context.

In previous PR's, we worked on rearranging the Abilities, and this supervisor's ability has not been updated yet. This is the behaviour we want to have:

  1. For users who did not set their email address hidden:
  • email address is available (in views) to all confirmed other users (not for guest users).
  1. For users who did set their email address hidden:
  • email addres is not available to any user, except:
    • admin users
    • the user herself (for now: you can make it only when that user herself is confirmed)
    • when user is a student in a current team: the team's supervisor

That is currently managed with the old code (marked FIXME): https://github.com/rails-girls-summer-of-code/rgsoc-teams/blob/6bfd1b83c2f7f760ab07b33426423e3c968089e1/app/models/ability.rb#L66-L74 And also present in the feature tests for guest and confirmed users.

The first part of this issue (= the first checkbox in the description of the issue) is about cleaning up this old code.

  • [ ] As you probably saw, we have abilities for confirmed users, and the can :read_mail is already implemented there for other users.
  • [ ] The rule to be able to see your own email should be added.
  • [ ] Check if the admin rules cover this can :read_email as well
  • [ ] Add abilities for supervisor's to the #team members part (where you already added your code 👍 ) with the can: read_email rule. And the can :read, :users_info You can use the existing method in https://github.com/rails-girls-summer-of-code/rgsoc-teams/blob/6bfd1b83c2f7f760ab07b33426423e3c968089e1/app/models/ability.rb#L123

With that in place, all the lines 66-74 (the code snippet above) can be deleted - not only the rule in line 70 - sorry, I can see how that was not clear in the description.

With the test update proposed by @klapraddla those test should reflect this behaviour.

The can :read_email is a bit different than 'normal' abilities, as @klappradla said, because it is kind of a view helper. The view helper is used in the User#show view (the user's profile page) and the Community page: https://github.com/rails-girls-summer-of-code/rgsoc-teams/search?l=Slim&q=can%3F+%3Aread_email

There are some more steps in the issue 1012 description , but let's see if the tests pass with this first part first. ;-) Unless you are totally discouraged, and want to take on a different issue. Either way, we are happy to help you out, and please let us know your thoughts!!

Happy coding!

emcoding avatar Jul 22 '18 20:07 emcoding

This looks great, I will take another swing at it. Thanks for all of the feedback!

anubiskhan avatar Jul 22 '18 21:07 anubiskhan