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

Class API #50

Merged
merged 12 commits into from Jun 17, 2018

Conversation

Projects
None yet
5 participants
@phyrog
Collaborator

phyrog commented Jun 9, 2018

Closes #30, but still work in progress.

The implementation is finished, but the scope tests are incomplete. Style-wise there is also still work to do.

Edit: Should be ready now.

The implementation does not build on top of the old implementation, as it is already planned to remove the define API in graphql version 1.10, so this should make it easier for us to just delete the old stuff.

@phyrog phyrog added the in progress label Jun 9, 2018

@@ -0,0 +1,83 @@
module GraphQL::Pundit
module Authorization
module ClassMethods

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Use nested module/class definitions instead of compact style.

module GraphQL::Pundit
module Authorization
module ClassMethods
def current_user(current_user = nil)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Missing top-level module documentation comment.

module Authorization
module ClassMethods
def current_user(current_user = nil)
return self.class_variable_get(:@@current_user) unless current_user

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Missing top-level module documentation comment.

def self.prepended(base)
@@current_user = :current_user
base.extend(ClassMethods)
end

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Replace class var @@current_user with a class instance var.

def do_authorize(root, arguments, context)
return true unless @authorize
return @authorize.call(root, arguments, context) if callable?(@authorize)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Assignment Branch Condition size for do_authorize is too high. [19.95/15]
C: Cyclomatic complexity for do_authorize is too high. [8/6]
C: Method has too many lines. [14/10]
C: Perceived complexity for do_authorize is too high. [10/7]

return root unless scope
case
when scope.respond_to?(:call)
return scope.call(root, arguments, context)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Use a guard clause instead of wrapping the code inside a conditional expression.

let(:field) do
Field.new(name: :to_a,
type: String,
before_scope: ->(scope, _args, _ctx) { scope.where { |e| e > 20 } },

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Line is too long. [86/80]

let(:field) do
Field.new(name: :to_a,
type: String,
before_scope: ->(scope, _args, _ctx) { scope.where { |e| e > 20 } },

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Line is too long. [86/80]

end
class TestPolicy
def initialize(_, value)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Method parameter must be at least 3 characters long.

end
class AlternativeTestPolicy
def initialize(_, value)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Method parameter must be at least 3 characters long.

@@ -0,0 +1,85 @@
# frozen_string_literal: true
module GraphQL::Pundit

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Use nested module/class definitions instead of compact style.

# frozen_string_literal: true
module GraphQL::Pundit
module Authorization

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Missing top-level module documentation comment.

module GraphQL::Pundit
module Authorization
module ClassMethods

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Missing top-level module documentation comment.

end
def self.prepended(base)
@@current_user = :current_user

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Replace class var @@current_user with a class instance var.

private
def do_authorize(root, arguments, context)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Assignment Branch Condition size for do_authorize is too high. [19.95/15]
C: Cyclomatic complexity for do_authorize is too high. [8/6]
C: Method has too many lines. [14/10]
C: Perceived complexity for do_authorize is too high. [10/7]

@@ -0,0 +1,42 @@
# frozen_string_literal: true
module GraphQL::Pundit

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Use nested module/class definitions instead of compact style.

# frozen_string_literal: true
module GraphQL::Pundit
module Scope

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Missing top-level module documentation comment.

private
def apply_scope(scope, root, arguments, context)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Method has too many lines. [12/10]

def apply_scope(scope, root, arguments, context)
return root unless scope
if scope.respond_to?(:call)

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Use a guard clause instead of wrapping the code inside a conditional expression.

let(:field) do
Field.new(name: :to_a,
type: String,
before_scope: ->(scope, _args, _ctx) { scope.where { |e| e > 20 } },

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Line is too long. [86/80]

@codecov-io

This comment has been minimized.

codecov-io commented Jun 9, 2018

Codecov Report

Merging #50 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master    #50    +/-   ##
======================================
  Coverage     100%   100%            
======================================
  Files           9     16     +7     
  Lines         517    784   +267     
======================================
+ Hits          517    784   +267
Impacted Files Coverage Δ
lib/graphql-pundit/scope.rb 100% <100%> (ø)
spec/graphql-pundit/scope_spec.rb 100% <100%> (ø)
lib/graphql-pundit/common.rb 100% <100%> (ø)
lib/graphql-pundit/field.rb 100% <100%> (ø)
spec/graphql-pundit/authorization_spec.rb 100% <100%> (ø)
spec/graphql-pundit/common_spec.rb 100% <100%> (ø)
lib/graphql-pundit/authorization.rb 100% <100%> (ø)
.../graphql-pundit/instrumenters/before_scope_spec.rb 100% <100%> (ø) ⬆️
lib/graphql-pundit.rb 100% <100%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f5a132...a8474ab. Read the comment docs.

module GraphQL::Pundit
module Authorization
module ClassMethods

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Missing top-level module documentation comment.

let(:field) do
Field.new(name: :to_a,
type: String,
before_scope: ->(scope, _args, _ctx) { scope.where { |e| e > 20 } },

This comment has been minimized.

@stickler-ci

stickler-ci Jun 9, 2018

Contributor

C: Line is too long. [86/80]

phyrog added some commits Jun 12, 2018

@phyrog phyrog changed the title from [WIP] Class api to Class API Jun 12, 2018

@phyrog phyrog requested a review from eugenk Jun 12, 2018

@casiodk

This comment has been minimized.

casiodk commented Jun 12, 2018

It looks promising, thanks for putting in the effort

@casiodk

This comment has been minimized.

casiodk commented Jun 12, 2018

I could be nice to have a way to call the pundit policy_scope in a before resolver

@phyrog

This comment has been minimized.

Collaborator

phyrog commented Jun 13, 2018

Ah, you can. I forgot to add documentation to the readme on that. You can use before_scope to apply the scope before the resolver and after_scope to apply it after the resolver.

end
context 'authorized' do
let(:auth_result) { true }

This comment has been minimized.

@eugenk

eugenk Jun 17, 2018

Member

I would name this expected_auth_result, but it's nothing that has to be changed for the approval

{name: 'BAIC', country: 'China'},
{name: 'Dongfeng Motor', country: 'China'},
{name: 'Geely', country: 'China'},
{name: 'Great Wall', country: 'China'}].

This comment has been minimized.

@eugenk

eugenk Jun 17, 2018

Member

Wow, they actually do exist...

@eugenk

eugenk approved these changes Jun 17, 2018

@phyrog phyrog merged commit 0e2fb3e into master Jun 17, 2018

6 checks passed

codeclimate All good!
Details
codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to 2f5a132
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci No lint errors found.

@phyrog phyrog deleted the class_api branch Jun 17, 2018

@casiodk

This comment has been minimized.

casiodk commented Jun 18, 2018

If you add the policy class as a before scope, how do you access the scope in the resolver?

authorize: true,
record: ->(obj, args, ctx) { ctx[:current_user] }
field :password_hash, ... do
authorize policy: ->(obj, args, ctx) { ctx[:current_user] }

This comment has been minimized.

@casiodk

casiodk Jun 18, 2018

Is this correct @phyrog

Seems like policy: should be record:

This comment has been minimized.

@phyrog

phyrog Jun 19, 2018

Collaborator

True. Thanks! Fixed it in #52.

@phyrog

This comment has been minimized.

Collaborator

phyrog commented Jun 19, 2018

The result of the before_scope is passed as what normally is the parent object to the resolver (so the first argument if you're using a resolve lambda).

Edit: #52 addresses this now, too; thanks for the hints 😄

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