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

🧹 Addresses Hyrax 5 PR feedback: Rubocop Naming/PredicateName errors #2126

Merged
merged 1 commit into from
Jan 4, 2024
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
18 changes: 4 additions & 14 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,35 +29,25 @@ class ApplicationController < ActionController::Base

protected

# rubocop:disable Naming/PredicateName
def is_hidden
# rubocop:enable Naming/PredicateName
def hidden?
current_account.persisted? && !current_account.is_public?
end

# rubocop:disable Naming/PredicateName
def is_api_or_pdf
# rubocop:enable Naming/PredicateName
def api_or_pdf?
request.format.to_s.match('json') ||
params[:print] ||
request.path.include?('api') ||
request.path.include?('pdf')
end

# rubocop:disable Naming/PredicateName
def is_staging
# rubocop:enable Naming/PredicateName
def staging?
['staging'].include?(Rails.env)
end

##
# Extra authentication for palni-palci during development phase
def authenticate_if_needed
# Disable this extra authentication in test mode
return true if Rails.env.test?
# rubocop:disable Naming/PredicateName
return unless (is_hidden || is_staging) && !is_api_or_pdf
# rubocop:enable Naming/PredicateName
return unless (hidden? || staging?) && !api_or_pdf?
authenticate_or_request_with_http_basic do |username, password|
username == "samvera" && password == "hyku"
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/proprietor/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ def user_params

params.require(:user).permit(:email,
:password,
:is_superadmin,
:superadmin,
:facebook_handle,
:twitter_handle,
:googleplus_handle,
Expand Down
2 changes: 1 addition & 1 deletion app/models/hyrax/group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ def description_label
label
end

def has_site_role?(role_name) # rubocop:disable Naming/PredicateName
def site_role?(role_name)
site_roles = roles.select { |role| role.resource_type == 'Site' }

site_roles.map(&:name).include?(role_name.to_s)
Expand Down
10 changes: 3 additions & 7 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -46,22 +46,18 @@ def to_s
email
end

# rubocop:disable Naming/PredicateName
def is_admin
# rubocop:enable Naming/PredicateName
def admin?
has_role?(:admin) || has_role?(:admin, Site.instance)
end

# rubocop:disable Naming/PredicateName
def is_superadmin
# rubocop:enable Naming/PredicateName
def superadmin?
has_role? :superadmin
end

# This comes from a checkbox in the proprietor interface
# Rails checkboxes are often nil or "0" so we handle that
# case directly
def is_superadmin=(value)
def superadmin=(value)
value = ActiveModel::Type::Boolean.new.cast(value)
if value
add_role :superadmin
Expand Down
6 changes: 3 additions & 3 deletions app/services/group_aware_role_checker.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,20 @@ module GroupAwareRoleChecker
# their role checker methods are automatically defined
RolesService::DEFAULT_ROLES.each do |role_name|
define_method(:"#{role_name}?") do
has_group_aware_role?(role_name)
group_aware_role?(role_name)
end
end

private

# Check for the presence of the passed role_name in the User's Roles and
# the User's Hyrax::Group's Roles.
def has_group_aware_role?(role_name) # rubocop:disable Naming/PredicateName
def group_aware_role?(role_name)
return false if current_user.new_record?
return true if current_user.has_role?(role_name, Site.instance)

current_user.hyrax_groups.each do |group|
return true if group.has_site_role?(role_name)
return true if group.site_role?(role_name)
end

false
Expand Down
6 changes: 3 additions & 3 deletions app/services/hyrax/workflow/permission_grantor.rb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def grant_all_workflow_roles_to_creating_user_and_admins!
workflow_agents = [Hyrax::Group.find_by!(name: ::Ability.admin_group_name)]
# The default admin set does not have a creating user
workflow_agents << creating_user if creating_user
workflow_agents |= Hyrax::Group.select { |g| g.has_site_role?(:admin) }.tap do |agent_list|
workflow_agents |= Hyrax::Group.select { |g| g.site_role?(:admin) }.tap do |agent_list|
::User.find_each do |u|
agent_list << u if u.has_role?(:admin, Site.instance)
end
Expand All @@ -59,7 +59,7 @@ def grant_all_workflow_roles_to_creating_user_and_admins!

def grant_workflow_roles_to_editors!
editor_sipity_roles = [Hyrax::RoleRegistry::APPROVING, Hyrax::RoleRegistry::DEPOSITING]
workflow_agents = Hyrax::Group.select { |g| g.has_site_role?(:work_editor) }.tap do |agent_list|
workflow_agents = Hyrax::Group.select { |g| g.site_role?(:work_editor) }.tap do |agent_list|
::User.find_each do |u|
agent_list << u if u.has_role?(:work_editor, Site.instance)
end
Expand All @@ -70,7 +70,7 @@ def grant_workflow_roles_to_editors!

def grant_workflow_roles_to_depositors!
depositor_sipity_role = [Hyrax::RoleRegistry::DEPOSITING]
workflow_agents = Hyrax::Group.select { |g| g.has_site_role?(:work_depositor) }.tap do |agent_list|
workflow_agents = Hyrax::Group.select { |g| g.site_role?(:work_depositor) }.tap do |agent_list|
::User.find_each do |u|
agent_list << u if u.has_role?(:work_depositor, Site.instance)
end
Expand Down
2 changes: 1 addition & 1 deletion app/views/proprietor/users/_form.html.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<div class="form-inputs">
<%= f.input :display_name, label: 'Display Name' %>
<%= f.input :email, required: true, hint: '' %>
<%= f.input :is_superadmin, as: :boolean %>
<%= f.input :superadmin?, as: :boolean %>
<%= f.input :password, required: f.object.new_record? ? true : false %>
<%= f.input :facebook_handle %>
<%= f.input :twitter_handle %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/proprietor/users/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
<td><%= user.department %></td>
<td><%= user.title %></td>
<td><%= user.affiliation %></td>
<td><%= user.is_superadmin %></td>
<td><%= user.superadmin? %></td>
<td class="col-md-2">
<%= link_to t('.manage'), proprietor_user_path(user) %>&nbsp;|&nbsp;
<%= link_to t('helpers.action.edit'), edit_proprietor_user_path(user) %>&nbsp;|&nbsp;
Expand Down
1 change: 1 addition & 0 deletions config/locales/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,7 @@ en:
manage: Manage
title: Title
affiliation: Affiliation
superadmin: SuperAdmin
actions: Actions
header: Manage Users
confirm_delete: Are you sure you wish to delete this user?
Expand Down
2 changes: 1 addition & 1 deletion config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
mount Hyrax::IiifAv::Engine, at: '/'
mount Riiif::Engine => 'images', as: :riiif if Hyrax.config.iiif_image_server?

authenticate :user, ->(u) { u.is_superadmin || u.is_admin } do
authenticate :user, ->(u) { u.superadmin? || u.admin? } do
queue = ENV.fetch('HYRAX_ACTIVE_JOB_QUEUE', 'sidekiq')
case queue
when 'sidekiq'
Expand Down
10 changes: 5 additions & 5 deletions spec/models/hyrax/group_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ module Hyrax
end
end

describe '#has_site_role?' do
describe '#site_role?' do
subject(:group) { FactoryBot.build(:group) }

before do
Expand All @@ -265,27 +265,27 @@ module Hyrax
let(:role) { FactoryBot.build(:role, name: 'non-site role', resource_type: 'non-site type') }

it 'returns false' do
expect(group.has_site_role?('non-site role')).to eq(false)
expect(group.site_role?('non-site role')).to eq(false)
end
end

context 'when group has a site role that matches' do
let(:role) { FactoryBot.build(:role, name: 'my_role', resource_type: 'Site') }

it 'returns true' do
expect(group.has_site_role?('my_role')).to eq(true)
expect(group.site_role?('my_role')).to eq(true)
end

it 'handles being passed a symbol' do
expect(group.has_site_role?(:my_role)).to eq(true)
expect(group.site_role?(:my_role)).to eq(true)
end
end

context 'when group has a site role that does not matches' do
let(:role) { FactoryBot.build(:role, name: 'my_role', resource_type: 'Site') }

it 'returns false' do
expect(group.has_site_role?('your_role')).to eq(false)
expect(group.site_role?('your_role')).to eq(false)
end
end
end
Expand Down
Loading