diff --git a/app/controllers/abilities_controller.rb b/app/controllers/abilities_controller.rb index 015cf6d96..c2d23eb79 100644 --- a/app/controllers/abilities_controller.rb +++ b/app/controllers/abilities_controller.rb @@ -1,18 +1,42 @@ class AbilitiesController < ApplicationController + include DraftManagement + + before_action :authenticate_user!, except: [:index, :show] + before_action :set_ability, only: [:show, :edit, :update] before_action :set_user - before_action :verify_moderator, only: [:recalc] + before_action :verify_moderator, only: [:edit, :recalc, :update] def index @abilities = Ability.all end def show - @ability = Ability.where(internal_id: params[:id]).first - return not_found! if @ability.nil? - @your_ability = @user&.community_user&.privilege @ability.internal_id end + def edit; end + + def update + if push_to_network?(@ability) + abilities = Ability.unscoped.where(internal_id: @ability.internal_id, + description: @ability.description) + + if do_update_network(@ability, abilities) + do_delete_draft(current_user, URI(request.referer || '').path) + flash[:success] = "#{helpers.pluralize(abilities.to_a.size, 'ability')} updated." + redirect_to ability_path(id: @ability.internal_id) + else + render :edit, status: :bad_request + end + elsif @ability.update(ability_update_params) + do_delete_draft(current_user, URI(request.referer || '').path) + flash[:success] = I18n.t('abilities.success.update_generic') + redirect_to ability_path(id: @ability.internal_id) + else + render :edit, status: :bad_request + end + end + def recalc @user.community_user.recalc_privileges! redirect_to user_privileges_url(@user.id) @@ -20,6 +44,43 @@ def recalc private + def ability_update_params + params.require(:ability).permit(:description, :name) + end + + # Actually update a given set of abilities network-wide + # @param ability [Ability] ability from which the push is initiated + # @param abilities [ActiveRecord::Relation] network abilities to update + # @return [Boolean] status of the operation + def do_update_network(ability, abilities) + Ability.transaction do + abilities.each do |network_ability| + unless network_ability.update(ability_update_params) + ability.errors.merge!(network_ability.errors) + raise ActiveRecord::Rollback + end + end + true + rescue + false + end + end + + # Should push to network? + # @param ability [Ability] ability to check + # @return [Boolean] check result + def push_to_network?(ability) + return false unless params[:network_push] == 'true' + return false unless current_user.present? + + current_user.can_push_to_network?(ability) + end + + def set_ability + @ability = Ability.where(internal_id: params[:id]).first + not_found! unless @ability.present? + end + def set_user @user = if params[:for].present? User.where(id: params[:for]).first || @user diff --git a/app/models/ability.rb b/app/models/ability.rb index b377084c8..228b89230 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -2,6 +2,7 @@ class Ability < ApplicationRecord include CommunityRelated include AbilitiesHelper + validates :name, presence: true validates :internal_id, uniqueness: { scope: [:community_id], case_sensitive: false } def manual? diff --git a/app/models/user.rb b/app/models/user.rb index 5e1607028..ea240fcfa 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -155,6 +155,12 @@ def can_delete?(target) privilege?('flag_curate') && !target.deleted? end + # Can the user edit abilities? + # @return [Boolean] check result + def can_edit_abilities? + at_least_moderator? + end + # Can the user edit tags? # @return [Boolean] check result def can_edit_tags? @@ -218,11 +224,17 @@ def can_see_deleted_posts? privilege?('flag_curate') || false end - # Can the user push a given post type to network? - # @param post_type [PostType] type of the post to be pushed + # Can the user push a given target network-wide? + # @param target [Ability, PostType] target to be pushed # @return [Boolean] check result - def can_push_to_network?(post_type) - post_type.system? && (is_global_moderator || is_global_admin) + def can_push_to_network?(target) + return false unless global_moderator? || global_admin? + + if target.is_a?(PostType) + target.system? + else + true + end end # Can the user directly update a given post? diff --git a/app/views/abilities/_form.html.erb b/app/views/abilities/_form.html.erb new file mode 100644 index 000000000..435f1f89c --- /dev/null +++ b/app/views/abilities/_form.html.erb @@ -0,0 +1,49 @@ +<%= render 'posts/markdown_script' %> +<%= render 'posts/image_upload' %> + +<% if @ability.errors.any? %> +
+ There were some errors while saving this ability: + + +
+<% end %> + +<%= form_for @ability, url: submit_path do |f| %> + <%= render 'shared/body_field', + f: f, + field_name: :description, + field_label: t('abilities.body_label'), + post: @ability, + cur_length: @ability.description&.length, + min_length: 15, + max_length: 30_000 %> +
+ + + <% if current_user&.can_push_to_network?(@ability) %> +
+
+
+ <%= label_tag :network_push, 'Push to network', class: 'form-element' %> + Copy to all communities (where the ability has not been edited)? +
+
+ <%= check_box_tag :network_push, true, false, class: 'form-checkbox-element' %> +
+
+
+ <% end %> + + <%= f.submit 'Save', class: 'button is-filled' %> + <%= link_to t('g.cancel').titlecase, + ability_path(id: @ability.internal_id), + class: 'button is-muted is-outlined is-danger js-cancel-edit', + data: { + question_body: t('abilities.unsaved_changes_confirmation') + } %> +<% end %> diff --git a/app/views/abilities/edit.html.erb b/app/views/abilities/edit.html.erb new file mode 100644 index 000000000..3bea65bc1 --- /dev/null +++ b/app/views/abilities/edit.html.erb @@ -0,0 +1,14 @@ +<%# + "Instance variables: + @ability : Ability to edit +"%> + +<% + title = "Edit ability \"#{@ability.name}\"" +%> + +<% content_for :title, title %> + +

<%= title %>

+ +<%= render 'form', submit_path: update_ability_path(id: @ability.internal_id) %> diff --git a/app/views/abilities/show.html.erb b/app/views/abilities/show.html.erb index a2b0f84dd..52ecbff80 100644 --- a/app/views/abilities/show.html.erb +++ b/app/views/abilities/show.html.erb @@ -4,6 +4,9 @@ <% else %> Return to list <% end %> + <% if current_user&.can_edit_abilities? %> + <%= link_to 'edit', edit_ability_path(id: @ability.internal_id), class: "button is-outlined is-muted" %> + <% end %>

<%= @ability.name %> Ability

diff --git a/config/locales/strings/en.abilities.yml b/config/locales/strings/en.abilities.yml new file mode 100644 index 000000000..da7abb350 --- /dev/null +++ b/config/locales/strings/en.abilities.yml @@ -0,0 +1,10 @@ +en: + abilities: + #_form.html.erb + body_label: > + Description + unsaved_changes_confirmation: > + Any unsaved changes will be lost. Are you sure? + success: + update_generic: > + Ability successfully updated. diff --git a/config/routes.rb b/config/routes.rb index 05961caad..f42d48d1d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -344,9 +344,11 @@ end scope 'abilities' do - root to: 'abilities#index', as: :abilities - get 'recalc', to: 'abilities#recalc', as: :abilities_recalc - get ':id', to: 'abilities#show', as: :ability + root to: 'abilities#index', as: :abilities + get 'recalc', to: 'abilities#recalc', as: :abilities_recalc + get ':id', to: 'abilities#show', as: :ability + get ':id/edit', to: 'abilities#edit', as: :edit_ability + patch ':id/edit', to: 'abilities#update', as: :update_ability end scope 'donate' do diff --git a/db/schema.rb b/db/schema.rb index ac8ed1016..277027158 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -115,6 +115,7 @@ t.index ["community_id"], name: "index_categories_on_community_id" t.index ["default_filter_id"], name: "index_categories_on_default_filter_id" t.index ["license_id"], name: "index_categories_on_license_id" + t.index ["min_view_trust_level"], name: "index_categories_on_min_view_trust_level" t.index ["sequence"], name: "index_categories_on_sequence" t.index ["tag_set_id"], name: "index_categories_on_tag_set_id" end diff --git a/test/controllers/abilities_controller_test.rb b/test/controllers/abilities_controller_test.rb index a9ea0fe94..da60b80b1 100644 --- a/test/controllers/abilities_controller_test.rb +++ b/test/controllers/abilities_controller_test.rb @@ -65,4 +65,80 @@ class AbilitiesControllerTest < ActionController::TestCase assert_not_nil assigns(:user) assert_not_nil assigns(:your_ability) end + + test ':update should require authentication' do + ability = abilities(:everyone) + try_update_ability(ability, description: 'anonymous') + assert_redirected_to_sign_in + end + + test ':update should correctly update abilities' do + ability = abilities(:everyone) + + users.each do |user| + description = "#{user.name}'s edit" + + sign_in user + try_update_ability(ability, description: description) + + if user.deleted? || user.community_user.deleted? + assert_redirected_to_sign_in + elsif user.can_edit_abilities? + assert_redirected_to ability_url(id: ability.internal_id) + assert_equal assigns(:ability).description, description + else + assert_response(:not_found) + end + end + end + + test ':update should prevent invalid updates' do + ability = abilities(:everyone) + old_name = ability.name + user = users(:global_admin) + + sign_in user + + [false, true].each do |network_push| + try_update_ability(ability, description: 'valid', + name: '', + network_push: network_push) + assert_response(:bad_request) + + ability.reload + assert_equal ability.name, old_name + end + end + + test ':update should allow global mods & admins to network push' do + ability = abilities(:everyone) + + users.select { |u| u.can_push_to_network?(ability) }.each do |user| + description = "#{user.name}'s edit" + + sign_in user + try_update_ability(ability, description: description, network_push: true) + assert_redirected_to ability_url(id: ability.internal_id) + + network_abilities = Ability.unscoped.where(internal_id: ability.internal_id) + assert network_abilities.any? + + network_abilities.each do |network_ability| + assert_equal network_ability.description, description + end + end + end + + private + + # @param ability [Ability] ability to update + def try_update_ability(ability, **opts) + network_push = opts.delete(:network_push) || false + + patch :update, params: { + ability: {}.merge(opts), + id: ability.internal_id, + network_push: network_push + } + end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index c92638738..1acebcd7c 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -85,11 +85,27 @@ class UserTest < ActiveSupport::TestCase assert_equal true, basic_user.can_update?(post, post_types(:free_edit)) end - test 'can_push_to_network? should determine if the user can push updates to network' do - post_type = post_types(:help_doc) - assert_equal false, users(:standard_user).can_push_to_network?(post_type) - assert_equal true, users(:global_moderator).can_push_to_network?(post_type) - assert_equal true, users(:global_admin).can_push_to_network?(post_type) + test 'can_push_to_network? should correctly check network push permissions' do + global_mod = users(:global_moderator) + global_adm = users(:global_admin) + std_user = users(:standard_user) + network_users = [global_mod, global_adm] + + abilities.each do |ability| + assert_equal false, std_user.can_push_to_network?(ability) + + network_users.each do |user| + assert user.can_push_to_network?(ability) + end + end + + post_types.each do |type| + assert_equal false, std_user.can_push_to_network?(type) + + network_users.each do |user| + assert_equal type.system?, user.can_push_to_network?(type) + end + end end test 'community_user is based on context' do