Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Change way of finding policy for scopes #29

Merged
merged 4 commits into from
Nov 27, 2017

Conversation

phyrog
Copy link
Collaborator

@phyrog phyrog commented Nov 22, 2017

Should fix #28. It's not as straight forward as it could be, but unfortunately Pundit only recognizes model_name, which is not defined by Sequel.

@phyrog phyrog requested a review from eugenk November 22, 2017 10:13
@phyrog phyrog force-pushed the 28-stop_calling_define_singleton_method_on_root branch from c148320 to 897b253 Compare November 22, 2017 10:14
@codecov-io
Copy link

codecov-io commented Nov 22, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #29   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         315    344   +29     
=====================================
+ Hits          315    344   +29
Impacted Files Coverage Δ
lib/graphql-pundit/instrumenters/scope.rb 100% <100%> (ø) ⬆️
spec/graphql-pundit/instrumenters/scope_spec.rb 100% <100%> (ø) ⬆️

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 4df7182...f0b358c. Read the comment docs.

@eugenk
Copy link
Member

eugenk commented Nov 22, 2017

But model_name is defined by ActiveModel and there is an :active_model plugin for sequel if I remember correctly.

@phyrog
Copy link
Collaborator Author

phyrog commented Nov 22, 2017

Hm, but I wouldn't want to force someone to use the active_model plugin just for this, so I think handling model in this code is fine.

Copy link
Member

@eugenk eugenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I include this in the backend, I get:

  1) Rest::RepositoriesController successful action: index
     Failure/Error: old_resolve.call(resource, arguments, context)

     NameError:
       uninitialized constant RepositoryPolicy::Scope::Scope
     # graphql-pundit/lib/graphql-pundit/instrumenters/scope.rb:43:in `find_scope'

if !inferred?(scope)
scope::Scope
else
infer_from = if root.respond_to?(:model)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a comment why we do this.

@phyrog phyrog force-pushed the 28-stop_calling_define_singleton_method_on_root branch from e4a144e to 7bef11a Compare November 27, 2017 15:41
@phyrog
Copy link
Collaborator Author

phyrog commented Nov 27, 2017

e4f9187 should have fixed the backend error. I think it was some strange interplay with some lazy code (the lambda).

Copy link
Member

@eugenk eugenk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@phyrog phyrog merged commit e43207c into master Nov 27, 2017
@phyrog phyrog deleted the 28-stop_calling_define_singleton_method_on_root branch November 27, 2017 16:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stop calling define_singleton_method on root
3 participants