Rearrange, not change the abilities in Ability #997

emcoding · 2018-05-15T13:40:23Z

Update:
Rearranged, not changed the abilities in Ability.

Following this principles for Defining Abilities:

The reason: it is easier to follow what a user can or cannot do. Less error prone and easier to reason about.
It is also 1000 times easier to write and read specs.

Follow up:

In the console, I assigned my team_less account a student_role. I added a conf. Is that even possible IRL? When hitting Conferences#create, it redirects to -path. => UrlGenerationError. Conference not created. Create issue or irrelevant?

Questions about current abilities/implementation:

A student can destroy their own team, which make sense. But what about an accepted team, mid-season?
A supervisor can read some extra details of the users; AFAIK it concerns email_addresses and team's blogpost_info. That makes sense for their own team, but not for users on other teams, right? It needs further investigation. -> issue Fix cancancan implementation of Supervisor ability #1001
Note: Some 'ability-ish' feature are managed by methods in application_helper.rb (:can_see_private_info, :can_only_review_private-info.).

…st practices This is the work doc. Reset to here before continuing

…d abilities to meet specs.

emcoding · 2018-05-15T19:38:29Z

@carpodaster Trying to unmess all things Ability from the previous PR. What do you think of this new setup? I have also made a start with the specs, but I don't want to throw too much stuf. LMK if you want to see them?

klappradla

Looks like a valid approach 👍

I guess we can in the next step probably remove half of the special abilities anyways, since they're probably not used 😉

As for the specs: you're right, they look a bit ☠️ ... I'd personally just rewrite them from scratch then.

Do you want to add more changes to this PR or just merge this as a "first step"?

klappradla · 2018-05-15T20:46:17Z

app/models/ability.rb

+    can :read_email, User, hide_email: false # view helper
+    can :read, Team
+    can :read, Project
+    can :read, :feed_entry


First off: forgive my questions, I'm not familiar with CanCan - I'm always using Pundit...

What does this syntax here mean? :feed_entry is a symbol, not a class / model... ❓

I <3 your questions.
It is an :kind of an Activity, and used in lib/feed/item. I left it in here for now, because I am not changing abilities until they are thoroughly tested 😇

emcoding · 2018-05-15T21:08:14Z

Thanks @klappradla ! Let's not merge yet, I'll go rewrite the specs first. Here is a taste of what they are going to look like. So excited about this! Look how neat it is going to be!

pgaspar · 2018-05-16T01:53:51Z

app/models/ability.rb

+    can :create, Project if user.confirmed?
+
+    # current_student
+    can :crud, Conference if user.current_student?


Would we need to add && user.confirmed? to all the checks below to make sure they are only available to confirmed users?

I like this organization - it seems clearer to me. Mixing "confirmed" and "unconfirmed" with user roles seems a bit odd to me, though.

Update: ok, things make a bit more sense after I read the "Give permissions, don't take them away" link you posted. Doing things like return unless user.confirmed? right before line 20 ensures that everything below is only for confirmed users. 🙌

Yes, @pgaspar , instead of all the confirmed? checks, we will have one return statement. 😍
I agree it would be even neater to have no authentication check in here at all, but we'll probably going to need that until we went through everything in #991. 🍀
Just a few steps away 🤣

emcoding · 2018-05-17T15:13:35Z

Wow! That was 💥
I TRIED to make this as easy for you reviewers as I could. 😇 :

Commit f6da595 is a clean file with some questions. The last commit before that has all kinds of annotations. They explain some of the cancancan quirks, so that should answer some of the questions you may have.
There is more work to do, but I'd thought this makes for a reasonable pit stop.
Note that the abilities are not complete; I didn't add things, only rearranged what we already had. And all of those are covered by tests now. 🎉
Questions about the current abilities, and if they should be changed, are listed in the PR description ^.
There is some noise in the specs. I'd like to stub things, so that we are only testing the ability. Help is much appreciated!

Let me know if I can do anything else to make reviewing less hard.
@klappradla @carpodaster @pgaspar @ramonh

…sation)

emcoding · 2018-05-17T15:46:09Z

spec/factories/users.rb

@@ -1,6 +1,6 @@
 FactoryBot.define do
  factory :user, aliases: [:member] do
-    github_handle { FFaker::InternetSE.user_name_variant_short }
+    github_handle { FFaker::InternetSE.unique.user_name_variant_short }


This fixes flickering specs because of failing uniqueness validation on gh handle.

klappradla

Solid work 💪

I left some comments and suggestion here and there.

I did not read through the diff of the old abilities specs because I though we're probably going to get rid of it on the long run anyways, right?

Huge ➕ on your work on the specs 👍
I'm not 100% sure if shared examples are the best approach here on the long run though 🤔
But that's only personal preference, no need to address this ✌️

klappradla · 2018-05-17T18:48:26Z

app/controllers/mailings_controller.rb

@@ -1,11 +1,10 @@
 # frozen_string_literal: true
 class MailingsController < ApplicationController

-  load_and_authorize_resource except: :index
+  load_and_authorize_resource


klappradla · 2018-05-17T18:53:05Z

app/models/ability.rb

+    end
+
+    # current_student
+    if user.current_student? # TODO is this a valid check?


From what I remember this checks whether the user is a student in an accepted team for the current season.

So this may already be scoped too narrow here... At least

user.teams.none?

and creating teams in general does not make much sense then 🤔

klappradla · 2018-05-17T18:55:05Z

spec/factories/users.rb

@@ -84,5 +84,9 @@
        create(:reviewer_role, user: user)
      end
    end
+
+    trait :unconfirmed do
+      confirmed_at { nil }


No need for a block here.

klappradla · 2018-05-17T18:58:39Z

spec/models/ability_spec.rb

+    it_behaves_like "can not comment"
+    it_behaves_like "has no access to other user's accounts"
+    it_behaves_like "can not read role restricted or owner restricted pages"
+    it "does not have an account" do


What is this doing?

It is a pro memory. If you have to ask, it is not working, I'll delete it.

klappradla · 2018-05-17T19:00:23Z

spec/models/ability_spec.rb

+    it_behaves_like "can see mailings list too"
+    it_behaves_like "can read mailings sent to them"
+    it_behaves_like "can comment now" # not implemented; false positives; needs work
+    it { expect(subject).to be_able_to([:join, :create], Team) }


Cosmetic, but it's not necessary to write subject when using the short hand syntax. You can simply write:

it { is_expected.to be_able_to %i(join create), Team }

Same for all further appearance 😉

klappradla · 2018-05-17T19:04:57Z

spec/models/ability_spec.rb

-            end
-          end
+        it 'cannot create a second team in current_season' do
+          pending 'fails; it is tested in team_spec.rb:33, and that passes'


Well, I fear you have to actually write the teams to the DB to have a test like this...

That ^ and we have this for confirmed users on line 27:

can [:join, :create], Team

By the way, instead of persisting the teams in the DB maybe you can get away with stubbing the user.teams call.

klappradla · 2018-05-17T19:07:56Z

spec/models/ability_spec.rb

-          it { expect(ability).not_to be_able_to(:resend_confirmation_instruction, other_user) }
+        before do
+          allow(user).to receive(:supervisor?).and_return(true)
+          allow(ability).to receive(:supervises?) { false } # Rspec complained: "stub default value first"


I sorta understand why RSpec does this 😉

I'd personally never use the receive(:thing).with when defining a mock (with allow). The matcher / syntax makes way more sense for expecting message calls in my opinion.
So within the test you'd have something like

expect(model).to receive(:thing?).with(other_thing).and_return(false)

☝️ testing the "contract" so to say.

I had to look up the difference, but I still don't understand what it is happening.
If I change the line to
expect(ability).to receive(:supervises?).with(other_user, user) { true }
it fails with received 0 times
Do you understand what I am missing here?

Oh sorry, I did not mean you should change it!

Expecting a method call only makes sense (and will only pass) within an example. So if your test want to check whether or not the specific message is passed to this object but you don't care about the inner logic and want to stub it, it may look like this:

it 'is able to do something' do expect(ability).to receive(:supervise?).with(other_user, user).and_return(true) expect(ability).to be_able_to(:read, :users_info, other_user) end

OOOOH... and you actually said that: "within an example". Sorry.
NOW I understand the fail message "provide a stub first" 💃

klappradla · 2018-05-17T19:21:44Z

spec/models/ability_spec.rb

+  let(:user){ nil }
+  let(:other_user) { create(:user) }
+
+  describe "Guest User" do


Personal preference and only a suggestion, bud I'd make all of these role-specific describes a context, such as

context 'when the user is a guest' do

klappradla · 2018-05-17T19:23:37Z

spec/support/shared_examples/abilities.rb

+# details for unrestricted
+shared_examples 'can view public pages' do
+  PUBLIC_INDEX_PAGES.each do |page|
+    it { expect(subject).to be_able_to(:read, page) }


Same here: you can ditch the subject when using this syntax:

it { is_expected.to be_able_to(:read, page) }

Same for the rest of this file of course.

I hate that syntax with a passion. 😅 There is no performance or functional difference, right?

Hm, I tbh. don't know, but probably not 🤔

I personally think the syntax reads way more natural than explictly saying "subject". Take for instance

describe thing - it is expected to be such a thing

Since I haven't yet been on a project where linters would allow you syntax 🚔 🚒 - I'm probably also just used to the implicit one 😂

klappradla · 2018-05-17T19:30:12Z

spec/support/shared_examples/abilities.rb

@@ -0,0 +1,95 @@
+# collections of shared examples


Using shared examples is definitely a valid approach ✔️
I'm personally not a huge fan of using them in this amount and spread across multiple files though.

Well, comes down to personal preference, but I'm just quickly dumping my brain here: From my experience, they are usually only maintainable if being used only a few times and defined in the same file - it's the only way how a human brain can still keep track of the implicit contracts between the files. Plus: updating the feature and respectively the specs behind it gets super complicated, especially when nesting them like this.
They don't even run faster 😉

Alternative approaches here could for instance be:

split the abilities specs in one file per role (prefer that tbh)

test based on the requested entity rather then the role (and maybe again split into muliple files)

I totally agree that is not an ideal solution. It crossed my mind to split the abilities themselves in different files, but your suggestion to split only the specs in different files sounds better. I'd rather save that for a next step, if that is okay.

Moreover, the shared examples are mostly dealing with 'authentication stages', not roles. If everything works out with the Big Overhaul, we don't have to have that in Ability anymore... 🍀

Sure!

As said, your approach is totally valid 👍

I'd personally have done it differently and just wanted to explain this in case you were actually looking for alternative approaches or did not think about certain gotchas that come with it.

So all good to stay with this.

pgaspar · 2018-05-18T01:22:40Z

app/models/ability.rb

+    can :resend_confirmation_instruction, User, id: user.id
+    can :read_email, User, hide_email: false # view helper # delete? only used once
+
+    return unless user.confirmed? && signed_in?(user)


I think the signed_in?(user) is redundant - you've already returned for signed out users above.

pgaspar · 2018-05-18T01:29:26Z

spec/support/shared_examples/abilities.rb

+# logged in, unconfirmed:
+shared_examples "same as guest user" do
+  it_behaves_like "can view public pages"
+  it_behaves_like "can not modify things on public pages"


Small stylistic suggestion: maybe use cannot instead of can not? I think it makes it easier to parse out negatives and matches CanCanCan's cannot.

(Same applies to the rest of the specs)

Hmmm They are not always used the same as cancancan's cannot's. That made it more confusing, imo, so I left them as is.

- remove redundant conditional

- remove redundant things

…o be able to create a new team -PM rspec green

…ot read a user's email address when it's marked as hidden -PM rspec green

… desired behaviour - Added new specs to described the intended behaviour whether a supervisor should or should not read hidden email addresses. - PM rspec green

…lappradla

…lappradla -PM rspec green

emcoding · 2018-05-18T22:47:40Z

@klappradla @pgaspar Thanks so much for your reviews! 🙇 Here is the next step.

The problems with the specs turned out to be covering bigger problems with the implementation of cancancan in other places. With the Main Goal in mind - the Registration Overhaul - I reverted some of the changes to the previous state; they are working, and not needed for fixing the registration process. I did add specs for the desired behaviour though.
With splitting the ability_spec file in separate spec files per role, it looks promising, but I don't know what to do now with the shared_examples. Should I just copy paste the examples into each file, and accept all the duplication? Or is there a better way?
Bonus: the first registrations' feature test, for the guest user.

Awaiting your reactions!

hola-soy-milk

@F3PiX Thank you very much for your great work here! I love the new clarity and specs.

As far as I understood, we need to continue with the cleanups before merging this. Is that correct? Or can we merge and carry on from there?

emcoding · 2018-05-20T19:00:59Z

Hi Ramon, we merge this one first, as soon as it is approved. And carry on from there.

emcoding · 2018-05-20T19:47:52Z

On hold! I found a way to remove one level of nesting in the shared_examples (inspired by Max' request tests) and I am adding issues for the TODO's.

…ut solving the issue :-) - Unchanged: a student cannot create a second team in one season, and can create a team in next season - Fixed: a student in a team in current season doesn't get the '+ new team' button - Fixed: a student in a team in current season has no access to the new team form - Driving by: use the new rails 5 redirect-er to redirect back to the referer or fall back to root. (So that the forbidden access message appears on the page where the user tried something forbidden.) - Update projects_spec for new redirect - Update teams_controller_spec that failed because a student can't create a second team in the same season

…n of testing role abilities.

emcoding · 2018-05-21T08:30:44Z

spec/controllers/teams_controller_spec.rb

-  before do
-    user.roles.create(name: 'student', team: team)
-  end
-
  describe "GET index" do


Now that the new ability rule prevents students from creating a second team, the specs can't assign two teams either. Moved the one global before into the contexts.

emcoding · 2018-05-21T08:32:07Z

spec/features/users/guest_user_spec.rb

+      after { Timecop.return }
+
+      it "can view the current season's accepted and selected projects" do
+        # project not visible on page. why?


Related to #1002?

Good catch! Could be...

AHA! I had to use a in_current_season scope 🌞

emcoding · 2018-05-21T08:38:45Z

app/controllers/application_controller.rb

@@ -33,7 +33,7 @@ def after_sign_in_path_for(user)
  end

  rescue_from CanCan::AccessDenied do |exception|
-    redirect_to root_url, alert: exception.message
+    redirect_back(fallback_location: root_path, alert: exception.message)
  end


🏎 The 'not authorised' method will now show up on the page where the user tried to do something forbidden. We couldn't redirect :back before, because it would mess up if there wasn't a referer present.

… in this PR

emcoding · 2018-05-21T09:04:23Z

Removed the nesting in the shared_examples
The common examples of confirmed users are not repeated in every confirmed role anymore.
Solved issue Don't show the Team new form if a user is not allowed to create one. #1003, because it was easier than to leave it alone.
Added the new Rails 5 redirect_back method, so that unauthorised users are getting the alert on the page where the forbidden action happened.

@klappradla @pgaspar

emcoding · 2018-05-23T09:51:18Z

Thanks all, for your reviews!! 💟

WIP Rearrange, not change the abilities in Ability, cf recommended be…

d00277c

…st practices This is the work doc. Reset to here before continuing

emcoding force-pushed the clean_cans branch from fa7a2ad to 87e56b6 Compare May 15, 2018 13:42

Clean file, without todo's and check comments

27ead4d

emcoding force-pushed the clean_cans branch from ff8b3f2 to 27ead4d Compare May 15, 2018 14:26

First examples of the New And Complete ability specs. Now with update…

e1b56f1

…d abilities to meet specs.

klappradla reviewed May 15, 2018

View reviewed changes

pgaspar reviewed May 16, 2018

View reviewed changes

emcoding added 3 commits May 16, 2018 23:30

Halfway

6b4664e

WIP All things processed and annotated

bfaeb99

Cleanup file ; see previous commit for annotations

f6da595

emcoding force-pushed the clean_cans branch from 9ac9df0 to f6da595 Compare May 17, 2018 14:46

hot fix for failing spec MailingsController (because of wrong authori…

b172a3f

…sation)

emcoding changed the title ~~WIP Rearrange, not change the abilities in Ability~~ Rearrange, not change the abilities in Ability May 17, 2018

emcoding commented May 17, 2018

View reviewed changes

klappradla suggested changes May 17, 2018

View reviewed changes

pgaspar reviewed May 18, 2018

View reviewed changes

emcoding added 3 commits May 18, 2018 14:14

Finetuning

ec3c037

- remove redundant conditional

Finetuning

f84460e

- remove redundant things

Write tests for desired behaviour of student ability whether or not t…

dad17d8

…o be able to create a new team -PM rspec green

emcoding mentioned this pull request May 18, 2018

Fix cancancan implementation of Supervisor ability #1001

Closed

7 tasks

emcoding added 4 commits May 18, 2018 19:45

Write tests for desired behaviour of whether a supervisor can or cann…

ff1e8ba

…ot read a user's email address when it's marked as hidden -PM rspec green

Restore old code for can read_email and can users_info. Add tests for…

975877f

… desired behaviour - Added new specs to described the intended behaviour whether a supervisor should or should not read hidden email addresses. - PM rspec green

WIP Split the ability specs in separate files per role - thanks to @k…

1e5db7d

…lappradla -PM rspec green

WIP First try feature test guest user access

cabcae9

hola-soy-milk approved these changes May 20, 2018

View reviewed changes

emcoding mentioned this pull request May 20, 2018

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

Closed

7 tasks

emcoding added 3 commits May 21, 2018 10:12

Remove one level of nesting in the ability specs, removed the repitio…

9a0f39b

…n of testing role abilities.

👮

e765a2b

emcoding commented May 21, 2018

View reviewed changes

Revert changes to authorize method in controllers that are not needed…

9d685da

… in this PR

emcoding force-pushed the clean_cans branch from 664d162 to 9d685da Compare May 21, 2018 08:49

Declutter

419bdf7

emcoding mentioned this pull request May 21, 2018

Temp Fix to restrict access for unconfirmed users #992

Closed

3 tasks

Add scope to solve failing specs.

cb126b9

klappradla approved these changes May 23, 2018

View reviewed changes

emcoding merged commit 7acbe86 into master May 23, 2018

emcoding deleted the clean_cans branch May 23, 2018 09:51

This was referenced May 23, 2018

User Registration Overhaul: break down #991

Open

Update comment from /mentors/applications/:id page #973

Closed

Updating comment from /mentors/applications/:id page results in 404 #960

Open

emcoding mentioned this pull request Jun 1, 2018

Update supervisor ability #1012

Open

5 tasks

Rearrange, not change the abilities in Ability #997

Rearrange, not change the abilities in Ability #997

Conversation

emcoding commented May 15, 2018 • edited Loading

emcoding commented May 15, 2018

klappradla left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 15, 2018

pgaspar May 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 17, 2018 • edited Loading

Choose a reason for hiding this comment

klappradla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgaspar May 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 18, 2018

hola-soy-milk left a comment

Choose a reason for hiding this comment

emcoding commented May 20, 2018

emcoding commented May 20, 2018

emcoding May 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emcoding commented May 21, 2018

emcoding commented May 23, 2018

emcoding commented May 15, 2018 •

edited

Loading

klappradla left a comment •

edited

Loading

pgaspar May 16, 2018 •

edited

Loading

emcoding commented May 17, 2018 •

edited

Loading

pgaspar May 18, 2018 •

edited

Loading

emcoding May 21, 2018 •

edited

Loading