Allow multiple abilities with associations #726

Open
wants to merge 1 commit into
from

Projects

None yet

6 participants

There are two issues with the current way cancan handles associations:

  1. Records are returned multiple times in some circumstances
  2. Several defined abilities prevent some records to show up under certain circumstances

This commit includes tests for both cases. It fixes both problems by changing joins to includes for the AR adapters. This could have performance implications, since includes will also select all columns in the associated records. We tried various ways of achieving the same thing using Arel directly, but were unable to make this work due to lack of support for outer joins in Rails 3.1.

This closes issues #724, #566 and #613

Jonas Nicklas and Nicklas Ramhöj Allow multiple abilities with associations
There are two issues with the current way cancan handles associations:

1) Records are returned multiple times in some circumstances
2) Several defined abilities prevent some records to show up under certain circumstances

This commit includes tests for both cases. It fixes both problems by changing `joins` to `includes` for the AR adapters. This could have performance implications, since `includes` will also select all columns in the associated records. We tried various ways of achieving the same thing using Arel directly, but were unable to make this work due to lack of support for outer joins in Rails 3.1.

This closes issues #724, #566 and #613
2ffdf46

This pull request passes (merged 2ffdf46 into b4285ae).

+1 for this PR. Currently I'm working around by chaining uniq every time I call accessible_by in one of these cases.

If you don't mind the extra dependency, I wrote this in the meantime, which adds outer joins: https://github.com/elabs/ar_outer_joins

This solves the problem even better because it doesn't have the ugly side effect of affecting the select clause.

In that case uniq needs to be added the query as well.

@ryanb why did you pull in 02841bbc195cdb17c6a4f511da44e0040e80f5f7, when this does the exact same change, except this also adds tests for the incorrect behaviour?

@jnicklas You are right. This should be the pull request to pull in. Or at least pull in the specs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment