-
Notifications
You must be signed in to change notification settings - Fork 900
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ActsAsArModel supports more report and rbac use cases #10268
Conversation
@@ -451,6 +448,11 @@ def method_with_scope(ar_scope, options) | |||
end | |||
|
|||
def apply_scope(klass, scope) | |||
klass = to_class(klass) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This ends up calling to_class twice (once on line 177) and then again here. Is that expected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to_class
takes a string and calls constantize on it. If a class is passed in, it is a no-op.
I want just this call, but wasn't able to delete line 177 yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this call to make more clear
@Fryguy changed the naming a bit. added some comments and added a test class here too to show what is happening. I tried to update the changes your suggested. If you note, the blocked PRs are at the beginning of the commit list, but this PR is only 1 commit. and should be pretty close to the original PR that you reviewed. |
eb60297
to
dfe96fc
Compare
rebased with fixes to other PR. (you can see green dots in commit lists |
2090b1c
to
802e501
Compare
ensure all is called so the aaar query object is used
802e501
to
7d8baed
Compare
Checked commit kbrock@7d8baed with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1 |
@Fryguy unblocked. not sure if you have any further comments |
BLOCKED ON: #10273Screens that display
ActsAsArModel
are broken. There are a few steps to merge:apply_legacy_finder_options
apply_legacy_finder_options
(calling on query blew things up)ActsAsArModel
works for basic casesFixes #10087
Details:
includes(:table => {})
(an similar references / order). It is not smart enought to properly the second level of hashes. but:includes
is not really supported by ourActsAsArModel
implementations.references(:table => {})
. Though we do not use:references
in any of our implementations, it was blowing up before. it was free with theincludes()
implementationorder(Hash)
. It was free. We may need to support other syntaxes for this as well.length
attribute, which is essentially the same assize
where(nil)
which is getting called more often now that we are sql munging less./cc @lucasponce