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

AO3-5520 Admin roles restrict ability to add or modify wrangling guidelines #4398

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 16 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
26 changes: 17 additions & 9 deletions app/controllers/wrangling_guidelines_controller.rb
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
class WranglingGuidelinesController < ApplicationController
include WranglingHelper

Copy link
Contributor

Choose a reason for hiding this comment

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

Is anything from the helper used in this file?

before_action :admin_only, except: [:index, :show]

# GET /wrangling_guidelines
def index
@wrangling_guidelines = WranglingGuideline.order('position ASC')
@wrangling_guidelines = WranglingGuideline.order("position ASC")
end

# GET /wrangling_guidelines/1
Expand All @@ -13,57 +15,64 @@ def show

# GET /wrangling_guidelines/new
def new
authorize :wrangling if logged_in_as_admin?
Copy link
Contributor

Choose a reason for hiding this comment

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

All of these actions are admin only (see the before_action), so the logged_in_as_admin? check is redundant. Please remove it here and from the other authorize calls in this controller.

Suggested change
authorize :wrangling if logged_in_as_admin?
authorize :wrangling

@wrangling_guideline = WranglingGuideline.new
end

# GET /wrangling_guidelines/1/edit
def edit
authorize :wrangling if logged_in_as_admin?
@wrangling_guideline = WranglingGuideline.find(params[:id])
end

# GET /wrangling_guidelines/manage
def manage
@wrangling_guidelines = WranglingGuideline.order('position ASC')
authorize :wrangling if logged_in_as_admin?
@wrangling_guidelines = WranglingGuideline.order("position ASC")
end

# POST /wrangling_guidelines
def create
authorize :wrangling if logged_in_as_admin?
@wrangling_guideline = WranglingGuideline.new(wrangling_guideline_params)

if @wrangling_guideline.save
flash[:notice] = ts('Wrangling Guideline was successfully created.')
flash[:notice] = t("wrangling_guidelines.create")
redirect_to(@wrangling_guideline)
else
render action: 'new'
render action: "new"
end
end

# PUT /wrangling_guidelines/1
def update
authorize :wrangling if logged_in_as_admin?
@wrangling_guideline = WranglingGuideline.find(params[:id])

if @wrangling_guideline.update(wrangling_guideline_params)
flash[:notice] = ts('Wrangling Guideline was successfully updated.')
flash[:notice] = t("wrangling_guidelines.update")
redirect_to(@wrangling_guideline)
else
render action: 'edit'
render action: "edit"
end
end

# reorder FAQs
def update_positions
authorize :wrangling if logged_in_as_admin?
if params[:wrangling_guidelines]
@wrangling_guidelines = WranglingGuideline.reorder_list(params[:wrangling_guidelines])
flash[:notice] = ts('Wrangling Guidelines order was successfully updated.')
flash[:notice] = t("wrangling_guidelines.reorder")
end
redirect_to(wrangling_guidelines_path)
end

# DELETE /wrangling_guidelines/1
def destroy
authorize :wrangling if logged_in_as_admin?
@wrangling_guideline = WranglingGuideline.find(params[:id])
@wrangling_guideline.destroy
flash[:notice] = ts('Wrangling Guideline was successfully deleted.')
flash[:notice] = t("wrangling_guidelines.delete")
redirect_to(wrangling_guidelines_path)
end

Expand All @@ -72,5 +81,4 @@ def destroy
def wrangling_guideline_params
params.require(:wrangling_guideline).permit(:title, :content)
end

end
5 changes: 5 additions & 0 deletions app/policies/wrangling_policy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,4 +11,9 @@ def full_access?
alias destroy? full_access?
alias show? full_access?
alias report_csv? full_access?
alias new? full_access?
alias edit? full_access?
alias manage? full_access?
alias update? full_access?
alias update_positions? full_access?
end
11 changes: 6 additions & 5 deletions app/views/admin/_admin_nav.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
</li>
<% if params[:controller] == "admin_posts" && params[:action] == "edit" %>
<li>
<%= link_to t("admin.admin_nav.delete", default: "Delete Post"),
<%= link_to t("admin.admin_nav.delete"),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion to match #4891:

Suggested change
<%= link_to t("admin.admin_nav.delete"),
<%= link_to t(".news.delete_post"),

@admin_post,
data: { confirm: "Are you sure you want to delete this news post?" },
method: :delete %>
Expand All @@ -29,8 +29,9 @@
<li>
<%= span_if_current ts("Known Issues", key: "header"), known_issues_path %>
</li>
<li>
<%= span_if_current ts("Wrangling Guidelines", key: "header"),
wrangling_guidelines_path %>
</li>
<% if policy(:wrangling).new? %>
<li>
<%= span_if_current t("admin.admin_nav.wrangling_guidelines"), wrangling_guidelines_path %>
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be shortened by using a relative key:

Suggested change
<%= span_if_current t("admin.admin_nav.wrangling_guidelines"), wrangling_guidelines_path %>
<%= span_if_current t(".wrangling_guidelines"), wrangling_guidelines_path %>

</li>
<% end %>
</ul>
4 changes: 3 additions & 1 deletion app/views/admin/_header.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,9 @@
<% end %>
<li><%= link_to t(".nav.posts.faqs"), archive_faqs_path %></li>
<li><%= link_to t(".nav.posts.known_issues"), known_issues_path %></li>
<li><%= link_to t(".nav.posts.wrangling_guidelines"), wrangling_guidelines_path %></li>
<% if policy(:wrangling).new? %>
<li><%= link_to t(".nav.posts.wrangling_guidelines"), wrangling_guidelines_path %></li>
<% end %>
</ul>
</li>
<% if policy(AdminBlacklistedEmail).index? %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/wrangling_guidelines/index.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!--Descriptive page name, messages and instructions-->
<% if logged_in_as_admin? %>
<% if policy(:wrangling).new? %>
<%= render 'admin_index' %>
<% else %>
<h2 class="heading"><%= ts('Wrangling Guidelines') %></h2>
Expand Down
2 changes: 1 addition & 1 deletion app/views/wrangling_guidelines/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@

<!--main content-->
<div class="admin" role="article">
<% if logged_in_as_admin? %>
<% if policy(:wrangling).edit? %>
<div class="header">
<h3 class="heading">
Updated: <%=h @wrangling_guideline.updated_at %> | <%= link_to ts('Edit'), edit_wrangling_guideline_path(@wrangling_guideline) %>
Expand Down
2 changes: 0 additions & 2 deletions config/i18n-tasks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,6 @@ ignore_missing:
- successfully_sent # should be feedbacks.create.successfully_sent
# Files: app/controllers/languages_controller.rb and app/controllers/locales_controller.rb
- successfully_added # should be languages.create.successfully_added and locales.create.successfully_added
# Files: app/views/admin/_admin_nav.html.erb and app/views/admin_posts/show.html.erb
- admin.admin_nav.delete
# File: app/views/admin/admin_invitations/find.html.erb
- admin.admin_invitations.find.find_email
- admin.admin_invitations.find.find_token
Expand Down
5 changes: 5 additions & 0 deletions config/locales/controllers/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,3 +108,8 @@ en:
ban_notice_html: Your account has been banned. You are not permitted to add or edit archive content. Please %{contact_abuse_link} for more information.
contact_abuse: contact Abuse
suspension_notice_html: Your account has been suspended until %{suspended_until}. You may not add or edit content until your suspension has been resolved. Please %{contact_abuse_link} for more information.
wrangling_guidelines:
create: Wrangling Guideline was successfully created.
delete: Wrangling Guideline was successfully deleted.
reorder: Wrangling Guidelines order was successfully updated.
update: Wrangling Guideline was successfully updated.
3 changes: 3 additions & 0 deletions config/locales/views/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,9 @@ en:
queue: Manage Queue
requests: Manage Requests
page_heading: Invite New Users
admin_nav:
delete: Delete Post
wrangling_guidelines: Wrangling Guidelines
admin_options:
delete:
bookmark: Delete Bookmark
Expand Down
2 changes: 1 addition & 1 deletion features/step_definitions/tag_steps.rb
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@
end

Given /^I have posted a Wrangling Guideline?(?: titled "([^\"]*)")?$/ do |title|
step %{I am logged in as an admin}
step %{I am logged in as a "tag_wrangling" admin}
visit new_wrangling_guideline_path
if title
fill_in("Guideline text", with: "This is a page about how we wrangle things.")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ Scenario: relationship wrangling - syns, mergers, characters, autocompletes
And a canonical character "Zoe Washburne"
And a canonical character "Jack Harkness"
And a canonical character "Ianto Jones"
And I am logged in as an admin
And I am logged in as a "tag_wrangling" admin
And I follow "Tag Wrangling"

# create a new canonical relationship from tag wrangling interface
Expand Down
6 changes: 3 additions & 3 deletions features/tags_and_wrangling/wrangling_guidelines.feature
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ Feature: Wrangling Guidelines

Scenario: Post a Wrangling Guideline

Given I am logged in as an admin
Given I am logged in as a "tag_wrangling" admin
And I am on the wrangling guidelines page
And I follow "New Wrangling Guideline"
And I fill in "Guideline text" with "This series of documents (Wrangling Guidelines) are intended to help tag wranglers remain consistent as they go about the business of wrangling tags by providing a set of formatting guidelines."
Expand All @@ -29,7 +29,7 @@ Feature: Wrangling Guidelines

Scenario: Reorder Wrangling Guidelines

Given I am logged in as an admin
Given I am logged in as a "tag_wrangling" admin
And 3 Wrangling Guidelines exist
When I go to the Wrangling Guidelines reorder page
And I fill in "wrangling_guidelines_1" with "3"
Expand All @@ -44,7 +44,7 @@ Feature: Wrangling Guidelines

Scenario: Delete Wrangling Guideline

Given I am logged in as an admin
Given I am logged in as a "tag_wrangling" admin
And I have posted a Wrangling Guideline titled "Relationship Tags"
When I go to the Wrangling Guidelines page
And I follow "Delete"
Expand Down
Loading
Loading