-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Support conditional setting of serialization_scope in controllers #1711
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,12 @@ def json_key | |
end | ||
class PostTestController < ActionController::Base | ||
attr_accessor :current_user | ||
|
||
def render_post | ||
self.current_user = User.new(id: 3, name: 'Pete') | ||
render json: new_post, serializer: serializer, adapter: :json | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this condition is identical to the one below it :) |
||
|
||
def render_post_by_non_admin | ||
self.current_user = User.new(id: 3, name: 'Pete', admin: false) | ||
render json: new_post, serializer: serializer, adapter: :json | ||
|
@@ -71,10 +77,12 @@ class DefaultScopeTest < ActionController::TestCase | |
tests PostTestController | ||
|
||
def test_default_serialization_scope | ||
get :render_post | ||
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is supposed to be missing in this test :) |
||
assert_equal :current_user, @controller._serialization_scope | ||
end | ||
|
||
def test_default_serialization_scope_object | ||
get :render_post | ||
assert_equal @controller.current_user, @controller.serialization_scope | ||
end | ||
|
||
|
@@ -121,10 +129,12 @@ def serializer | |
tests PostViewContextTestController | ||
|
||
def test_defined_serialization_scope | ||
get :render_post | ||
assert_equal :view_context, @controller._serialization_scope | ||
end | ||
|
||
def test_defined_serialization_scope_object | ||
get :render_post | ||
assert_equal @controller.view_context.class, @controller.serialization_scope.class | ||
end | ||
|
||
|
@@ -158,6 +168,42 @@ def test_serialization_scope_admin | |
assert_equal expected_json, @response.body | ||
end | ||
end | ||
class ConditionalSerializationScopeTest < ActionController::TestCase | ||
class PostConditionalViewContextTestController < PostTestController | ||
serialization_scope :view_context, only: :render_post_by_admin | ||
|
||
private | ||
|
||
def serializer | ||
PostViewContextSerializer | ||
end | ||
end | ||
|
||
tests PostConditionalViewContextTestController | ||
|
||
def test_conditional_serialization_scope_admin | ||
get :render_post_by_admin | ||
expected_json = { | ||
post: { | ||
id: 4, | ||
title: 'Title', | ||
body: "The 'scope' is the 'view_context': true", | ||
comments: [ | ||
{ id: 1, body: 'Admin' } | ||
] | ||
} | ||
}.to_json | ||
assert_equal expected_json, @response.body | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unrelated change, no? |
||
|
||
def test_conditional_serialization_scope_non_admin | ||
exception_matcher = /view_context/ | ||
exception = assert_raises(NameError) do | ||
get :render_post_by_non_admin | ||
end | ||
assert_match exception_matcher, exception.message | ||
end | ||
end | ||
class NilSerializationScopeTest < ActionController::TestCase | ||
class PostViewContextTestController < ActionController::Base | ||
serialization_scope nil | ||
|
@@ -187,14 +233,6 @@ def new_post | |
end | ||
tests PostViewContextTestController | ||
|
||
def test_nil_serialization_scope | ||
assert_nil @controller._serialization_scope | ||
end | ||
|
||
def test_nil_serialization_scope_object | ||
assert_nil @controller.serialization_scope | ||
end | ||
|
||
def test_nil_scope | ||
exception_matcher = /current_user/ | ||
exception = assert_raises(NameError) do | ||
|
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.
Since the method is only called at
render
, if not passed in as an option, I don't think I like having it eagerly evaluated in code that is pretty easy to handle in userland. Would you be okay with removing the code changes in favor documenting using abefore_action
as an alternative to passing an option torender
?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 also would except there to be bugs with
current_user
being called in abefore_action
with all the side-effects authentication-related libs can haveThere 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 agree to your suggestion @bf4 about moving this to userland, created #1824 with added documentation.