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

Extend query interface #815

Merged
merged 4 commits into from
Aug 21, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions app/models/pageflow/account_member_query.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
module Pageflow
# Query accounts for members, e.g. based on role
class AccountMemberQuery < ApplicationQuery
class Scope < ApplicationQuery::Scope
# Account whose members we scope
# @return [Pageflow::Account]
attr_reader :account

# Base scope which is further scoped according to account role
# @return [ActiveRecord::Relation<User>]
attr_reader :scope

# Create scope that can limit base scope to account members at
# or above a given role
#
# @param [Pageflow::Account] account
# Required. Membership account to check.
# @param [ActiveRecord::Relation<User>] scope
# Optional. Membership entity to check.
def initialize(account, scope = User.all)
@account = account
@scope = scope
end

# Scope to those members from scope on account who have at least
# role
#
# @param [String] role
# Required. Minimum role that we compare against.
# @return [ActiveRecord::Relation<User>]
def with_role_at_least(role)
scope.joins(memberships_for_account_with_at_least_role(role))
.where(membership_is_present)
end

private

def memberships_for_account_with_at_least_role(role)
options = {roles: Roles.at_least(role), account_id: account.id}

sanitize_sql(<<-SQL, options)
LEFT OUTER JOIN pageflow_memberships as
pageflow_account_memberships ON
pageflow_account_memberships.user_id = users.id AND
pageflow_account_memberships.role IN (:roles) AND
pageflow_account_memberships.entity_id = :account_id AND
pageflow_account_memberships.entity_type = 'Pageflow::Account'
SQL
end

def membership_is_present
'pageflow_account_memberships.entity_id IS NOT NULL'
end
end
end
end
31 changes: 31 additions & 0 deletions app/models/pageflow/account_role_query.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
module Pageflow
# Query users for their role on accounts based on role
class AccountRoleQuery
# Create query that can be used for role comparisons
#
# @param [User] user
# Required. Membership user to check.
# @param [Pageflow::Account] account
# Required. Membership entity to check.
def initialize(user, account)
@user = user
@account = account
end

# Return true if there is a membership with at least role for
# user/account
#
# @param [String] role
# Required. Minimum role that we compare against.
# @return [Boolean]
def has_at_least_role?(role)
@user
.memberships
.where(role: Roles.at_least(role))
.where('(entity_id = :account_id AND '\
"entity_type = 'Pageflow::Account')",
account_id: @account.id)
.any?
end
end
end
13 changes: 13 additions & 0 deletions app/models/pageflow/application_query.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module Pageflow
# Abstraction layer for Pageflow's query interface
class ApplicationQuery
class Scope
protected

# @api private
def sanitize_sql(sql, interpolations)
ActiveRecord::Base.send(:sanitize_sql_array, [sql, interpolations])
end
end
end
end
8 changes: 2 additions & 6 deletions app/models/pageflow/entry_role_query.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
module Pageflow
class EntryRoleQuery
class Scope
class EntryRoleQuery < ApplicationQuery
class Scope < Scope
attr_reader :user, :scope

def initialize(user, scope)
Expand Down Expand Up @@ -50,10 +50,6 @@ def either_membership_is_present
'pageflow_entry_memberships.entity_id IS NOT NULL OR ' \
'pageflow_entry_account_memberships.entity_id IS NOT NULL'
end

def sanitize_sql(sql, interpolations)
ActiveRecord::Base.send(:sanitize_sql_array, [sql, interpolations])
end
end

def initialize(user, entry)
Expand Down
19 changes: 10 additions & 9 deletions app/policies/pageflow/account_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -79,14 +79,16 @@ def membership_is_present
end
end

attr_reader :user, :query

def initialize(user, account)
@user = user
@account = account
@query = AccountRoleQuery.new(user, account)
end

def publish?
@user.admin? ||
allows?(%w(publisher manager))
user.admin? || query.has_at_least_role?(:publisher)
end

def configure_folder_on?
Expand All @@ -98,8 +100,7 @@ def update_theming_on_entry_of?
end

def manage?
@user.admin? ||
allows?(%w(manager))
user.admin? || query.has_at_least_role?(:manager)
end

def read?
Expand All @@ -123,13 +124,13 @@ def destroy_membership_on?
end

def admin?
@user.admin?
user.admin?
end

def see_badge_belonging_to?
(@account.entries & @user.entries).any? ||
@user.memberships.on_accounts.as_previewer_or_above.where(entity: @account).any? ||
Copy link
Member

Choose a reason for hiding this comment

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

I think, FolderPolicy could also benefit from using the query. Please check if some these _or_above methods on Membership are unused then and can be removed.

Copy link
Contributor Author

@aviav aviav Aug 2, 2017

Choose a reason for hiding this comment

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

EDIT: Disregard the following two comments. I think adding a Scope is the way to go here:

Would this be okay in principle? Would we want to catch the error/throw something else?

    def has_at_least_role?(role, on_as_many_accounts_as: 1)
      case on_as_many_accounts_as
      when 0
        does_not_have_role?(role)
      when 1
        has_role_at_least_once?(role)
      else
        if on_as_many_accounts_as > 1
          has_role_at_least?(role, on_as_many_accounts_as)
        else
          raise ArgumentError('Integer 0 or greater expected for '\
                              'on_as_many_accounts_as')
        end
      end
    end

Copy link
Contributor Author

@aviav aviav Aug 2, 2017

Choose a reason for hiding this comment

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

(We would need on_as_many_accounts_as: 2 for now, but doing it for 1 and 2 only seems excessively lazy)

Also, the other methods are supposed to be private, so the ambiguous naming is okay because of context and because long method names impede reading

Specific usage:

    def show_account_selection_on?
      (@user.admin? && Account.all.size > 1) ||
        query.has_at_least_role?(:publisher, on_as_many_accounts_as: 2)
    end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding the Scope requires specs and maybe thought, so I will leave it for the next apparent opportunity. I agree, however, that we can probably use the extended query interface in other policies as well and thus improve readability, design and such

@user.admin?
(@account.entries & user.entries).any? ||
query.has_at_least_role?(:previewer) ||
user.admin?
end

def index?
Expand All @@ -139,7 +140,7 @@ def index?
private

def allows?(roles)
@user.memberships.where(role: roles, entity: @account).any?
user.memberships.where(role: roles, entity: @account).any?
end
end
end
62 changes: 62 additions & 0 deletions spec/models/pageflow/account_member_query_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
require 'spec_helper'

module Pageflow
describe AccountMemberQuery do

Choose a reason for hiding this comment

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

Block has too many lines. [42/25]

describe AccountMemberQuery::Scope do

Choose a reason for hiding this comment

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

Block has too many lines. [40/25]

describe '.with_role_at_least' do

Choose a reason for hiding this comment

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

Block has too many lines. [38/25]

it 'includes members with account membership with required role' do
user = create(:user)
account = create(:account, with_previewer: user)

result = AccountMemberQuery::Scope.new(account)
.with_role_at_least(:previewer)

expect(result).to include(user)
end

it 'includes members with account membership with stronger role' do
user = create(:user)
account = create(:account, with_editor: user)

result = AccountMemberQuery::Scope.new(account)
.with_role_at_least(:previewer)

expect(result).to include(user)
end

it 'does not include members with account membership with '\
'insufficient role' do
user = create(:user)
account = create(:account, with_member: user)

result = AccountMemberQuery::Scope.new(account)
.with_role_at_least(:previewer)

expect(result).not_to include(user)
end

it 'does not include members with required role on other account' do
user = create(:user)
account = create(:account, with_member: user)
create(:account, with_previewer: user)

result = AccountMemberQuery::Scope.new(account)
.with_role_at_least(:previewer)

expect(result).not_to include(user)
end

it 'does not include members with required role on entry of account' do
user = create(:user)
account = create(:account, with_member: user)
create(:entry, with_previewer: user)

result = AccountMemberQuery::Scope.new(account)
.with_role_at_least(:previewer)

expect(result).not_to include(user)
end
end
end
end
end
61 changes: 61 additions & 0 deletions spec/models/pageflow/account_role_query_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
require 'spec_helper'

module Pageflow
describe AccountRoleQuery do

Choose a reason for hiding this comment

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

Block has too many lines. [41/25]

describe '.has_at_least_role?' do

Choose a reason for hiding this comment

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

Block has too many lines. [39/25]

it 'returns true if user has account membership with given role' do
user = create(:user)
account = create(:account, with_publisher: user)

result = AccountRoleQuery.new(user, account)
.has_at_least_role?(:publisher)

expect(result).to be true
end

it 'returns true if user has account membership with stronger role' do
user = create(:user)
account = create(:account, with_manager: user)

result = AccountRoleQuery.new(user, account)
.has_at_least_role?(:publisher)

expect(result).to be true
end

it 'returns false if user has account membership with weaker role' do
user = create(:user)
account = create(:account, with_editor: user)

result = AccountRoleQuery.new(user, account)
.has_at_least_role?(:publisher)

expect(result).to be false
end

it 'returns false if user has entry membership with given role on '\
'account' do
user = create(:user)
account = create(:account, with_editor: user)
create(:entry, with_publisher: user)

result = AccountRoleQuery.new(user, account)
.has_at_least_role?(:publisher)

expect(result).to be false
end

it 'returns false if user has account membership with given role on '\
'other account' do
user = create(:user)
account = create(:account, with_editor: user)
create(:account, with_publisher: user)

result = AccountRoleQuery.new(user, account)
.has_at_least_role?(:publisher)

expect(result).to be false
end
end
end
end