Skip to content
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

Added with_context qualifier to have_authorized_scope matcher #260

Merged

Conversation

killondark
Copy link
Contributor

Hi @palkan. I added with_context qualifier to have_authorized_scope matcher
Discussed in #258

PR checklist:

  • Tests included
  • Documentation updated

Copy link

coveralls-official bot commented Apr 5, 2024

Coverage Status

coverage: 93.639% (+0.05%) from 93.587%
when pulling 86d553e on killondark:add-with_context-to-have_authorized_scope
into 8e69239 on palkan:master.

Copy link
Owner

@palkan palkan left a comment

Choose a reason for hiding this comment

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

Thanks!

Left some comments, please, take a look.

policy_class == policy.class &&
type == actual_type &&
name == actual_name &&
actual_scope_options === scope_options
actual_scope_options === scope_options &&
actual_context.all? { |key, value| context[key] == value }
Copy link
Owner

Choose a reason for hiding this comment

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

We should use the same logic here as for authorization calls:

def context_matches?(context, actual)
return true unless context
context === actual || actual >= context
end

It covers subsets and RSpec composable matchers.


def initialize(type)
@type = type
@name = :default
@scope_options = nil
@context = {}
Copy link
Owner

Choose a reason for hiding this comment

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

The default MUST be nil (meaning that we don't want to verify context)


def initialize(policy, target, type, name, scope_options)
@policy = policy
@target = target
@type = type
@name = name
@scope_options = scope_options
@context = policy.authorization_context
Copy link
Owner

Choose a reason for hiding this comment

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

We already have policy in the state, no need to extract context into its own instance variable.

docs/testing.md Outdated
Comment on lines 327 to 329
def favorite
authorized_scope(User.all, context: {favorite: true})
end
Copy link
Owner

Choose a reason for hiding this comment

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

A context is usually a record (it's an acting subject); it's unlikely a bool. Let's change this example:

Suggested change
def favorite
authorized_scope(User.all, context: {favorite: true})
end
def for_user
user = User.find(params[:id])
authorized_scope(User.all, context: {user:})
end

docs/testing.md Outdated
Comment on lines 418 to 420
expect { get :favorite }.to have_authorized_scope(:scope)
.with_scope_options(matching(with_deleted: a_falsey_value))
.with_context(favorite: true)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
expect { get :favorite }.to have_authorized_scope(:scope)
.with_scope_options(matching(with_deleted: a_falsey_value))
.with_context(favorite: true)
expect { get :for_user, params: {id: user.id} }.to have_authorized_scope(:scope)
.with_scope_options(matching(with_deleted: a_falsey_value))
.with_context(a_hash_including(user:))

@killondark
Copy link
Contributor Author

Thanks for your comments. Corrected. Please check it.

@killondark
Copy link
Contributor Author

Hi @palkan. Have you seen my latest fixes?

Copy link
Owner

@palkan palkan left a comment

Choose a reason for hiding this comment

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

Thanks!

@@ -105,7 +104,7 @@ def scope_options_message
end

def context_message
context.empty? ? "without context" : "with context: #{context}"
context.blank? ? "without context" : "with context: #{context}"
Copy link
Owner

Choose a reason for hiding this comment

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

We cannot use #blank? here, we have no dependency on Active Support core extensions (I'll fix it).

@palkan palkan merged commit 64edca8 into palkan:master Apr 19, 2024
11 of 13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants