From 073e5444ac169ea1578d72176c1649c6c1c499ba Mon Sep 17 00:00:00 2001 From: Andy Zhao Date: Wed, 11 Apr 2018 18:16:49 -0400 Subject: [PATCH] Add tag moderation feature (#201) * Fix edge case for tag admin update * Change single quotes to double quotes * Add MVP of tag edit page * Update rubocop version * Use actual hex colors since vars broke * Add enabling tag moderators via admin panel * Remove unused boolean and variables * Add a description for scholars * Remove unintended method * Update auth to work correctly * Update boolean properly --- .rubocop.yml | 2 +- app/assets/stylesheets/minimal.scss | 1 + app/assets/stylesheets/tag-edit.scss | 39 +++++++++ app/controllers/admin/tags_controller.rb | 53 ++++++++++--- app/controllers/admin/users_controller.rb | 7 +- app/controllers/tags_controller.rb | 42 ++++++++++ app/dashboards/tag_dashboard.rb | 3 + app/fields/tag_moderators_field.rb | 7 ++ app/models/comment.rb | 3 +- app/models/role.rb | 2 +- app/models/tag.rb | 16 ++-- app/services/user_role_service.rb | 33 +++++--- .../tag_moderators_field/_form.html.erb | 7 ++ .../tag_moderators_field/_index.html.erb | 1 + .../tag_moderators_field/_show.html.erb | 1 + .../fields/user_scholar_field/_form.html.erb | 1 + app/views/tags/edit.html.erb | 79 +++++++++++++++++++ config/routes.rb | 74 ++++++++--------- 18 files changed, 297 insertions(+), 74 deletions(-) create mode 100644 app/assets/stylesheets/tag-edit.scss create mode 100644 app/fields/tag_moderators_field.rb create mode 100644 app/views/fields/tag_moderators_field/_form.html.erb create mode 100644 app/views/fields/tag_moderators_field/_index.html.erb create mode 100644 app/views/fields/tag_moderators_field/_show.html.erb create mode 100644 app/views/tags/edit.html.erb diff --git a/.rubocop.yml b/.rubocop.yml index 8a110a22b..17c65164a 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -15,7 +15,7 @@ RSpec/DescribeClass: AllCops: Exclude: - db/schema.rb - TargetRubyVersion: 2.3.4 + TargetRubyVersion: 2.5.0 Naming/AccessorMethodName: Description: Check the naming of accessor methods for get_/set_. diff --git a/app/assets/stylesheets/minimal.scss b/app/assets/stylesheets/minimal.scss index a26fd21ce..fa1804397 100644 --- a/app/assets/stylesheets/minimal.scss +++ b/app/assets/stylesheets/minimal.scss @@ -24,6 +24,7 @@ @import 'live'; @import 'delete-confirm'; @import 'preact/onboarding-modal'; +@import 'tag-edit'; @import 'ltags/LiquidTags'; diff --git a/app/assets/stylesheets/tag-edit.scss b/app/assets/stylesheets/tag-edit.scss new file mode 100644 index 000000000..7b00877cf --- /dev/null +++ b/app/assets/stylesheets/tag-edit.scss @@ -0,0 +1,39 @@ + .tag-edit-form { + margin-bottom: 5em; + display: flex; + align-items: center; + justify-content: center; + } + .tag-edit-container { + margin-top: 5em; + max-width: 50%; + } + .tag-form-text-field { + height: 2em; + width: 20.5em; + text-align: left; + overflow-wrap: break-word; + font-size: 1em; + } + .tag-area-text { + height: 4em; + } + .tag-edit-submit { + margin-top: 5px; + background-color: #557de8; + color: white; + padding: 15px 20px; + text-align: center; + font-size: 16px; + } + .tag-edit-flash-success { + color: white; + background: #388019; + padding: 15px; + } + .tag-edit-flash-error { + color: white; + background: #ff0000; + padding: 15px 15px 15px 40px; + } + diff --git a/app/controllers/admin/tags_controller.rb b/app/controllers/admin/tags_controller.rb index f9306845d..9fe795169 100644 --- a/app/controllers/admin/tags_controller.rb +++ b/app/controllers/admin/tags_controller.rb @@ -1,19 +1,46 @@ module Admin class TagsController < Admin::ApplicationController - # To customize the behavior of this controller, - # simply overwrite any of the RESTful actions. For example: - # - # def index - # super - # @resources = Tag.all.paginate(10, params[:page]) - # end + def update + @tag = Tag.find(params[:id]) + if @tag.update(tag_params) && @tag.errors.messages.blank? && handle_moderators + flash[:notice] = "Tag successfully updated" + redirect_to "/admin/tags/#{@tag.id}" + else + render :new, locals: { page: Administrate::Page::Form.new(dashboard, @tag) } + end + end - # Define a custom finder by overriding the `find_resource` method: - # def find_resource(param) - # Tag.find_by!(slug: param) - # end + private - # See https://administrate-docs.herokuapp.com/customizing_controller_actions - # for more information + def convert_empty_string_to_nil + # Andy: nil plays nicely with our hex colors, whereas empty string doesn't + params[:tag][:text_color_hex] = nil if params[:tag][:text_color_hex] == "" + params[:tag][:bg_color_hex] = nil if params[:tag][:bg_color_hex] == "" + end + + def tag_params + accessible = %i[ + name + supported + alias_for + wiki_body_markdown + rules_markdown + short_summary + requires_approval + submission_template + submission_rules_headsup + pretty_name + bg_color_hex + text_color_hex + keywords_for_search + ] + convert_empty_string_to_nil + params.require(:tag).permit(accessible) + end + + def handle_moderators + user_ids = params[:tag][:tag_moderator_ids].split(",") + UserRoleService.new(nil).update_tag_moderators(user_ids.sort, @tag) + end end end diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 4ca157c2c..c57a88f43 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -7,17 +7,12 @@ module Admin flash[:notice] = "User successfully updated" redirect_to "/admin/users/#{params[:id]}" else - render_with_errors(user) + render :new, locals: { page: Administrate::Page::Form.new(dashboard, user) } end end private - def render_with_errors(user) - flash.now[:notice] = user.errors.full_messages - render :new, locals: { page: Administrate::Page::Form.new(dashboard, user) } - end - def user_params accessible = %i[name email diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index b57714500..c25360b38 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -5,4 +5,46 @@ class TagsController < ApplicationController @tags_index = true @tags = Tag.all.order("hotness_score DESC").first(100) end + + def edit + @tag = Tag.find_by!(name: params[:tag]) + check_authorization + end + + def update + @tag = Tag.find_by!(id: params[:id]) + check_authorization + if @tag.errors.messages.blank? && @tag.update(tag_params) + flash[:success] = "Tag successfully updated! 👍 " + redirect_to "/t/#{@tag.name}/edit" + else + flash[:error] = @tag.errors.full_messages + render :edit + end + end + + private + + def check_authorization + raise unless current_user.has_role?(:super_admin) || current_user.has_role?(:tag_moderator, @tag) + end + + def convert_empty_string_to_nil + # Andy: nil plays nicely with our hex colors, whereas empty string doesn't + params[:tag][:text_color_hex] = nil if params[:tag][:text_color_hex] == "" + params[:tag][:bg_color_hex] = nil if params[:tag][:bg_color_hex] == "" + end + + def tag_params + accessible = %i[ + wiki_body_markdown + rules_markdown + short_summary + pretty_name + bg_color_hex + text_color_hex + ] + convert_empty_string_to_nil + params.require(:tag).permit(accessible) + end end diff --git a/app/dashboards/tag_dashboard.rb b/app/dashboards/tag_dashboard.rb index c4fd97bc4..3ee998001 100644 --- a/app/dashboards/tag_dashboard.rb +++ b/app/dashboards/tag_dashboard.rb @@ -11,6 +11,7 @@ class TagDashboard < Administrate::BaseDashboard id: Field::Number, name: Field::String, supported: Field::Boolean, + tag_moderator_ids: TagModeratorsField, wiki_body_markdown: Field::Text, wiki_body_html: Field::Text, rules_markdown: Field::Text, @@ -48,6 +49,7 @@ class TagDashboard < Administrate::BaseDashboard :id, :name, :supported, + :tag_moderator_ids, :alias_for, :wiki_body_markdown, :wiki_body_html, @@ -71,6 +73,7 @@ class TagDashboard < Administrate::BaseDashboard FORM_ATTRIBUTES = [ :name, :supported, + :tag_moderator_ids, :alias_for, :wiki_body_markdown, :rules_markdown, diff --git a/app/fields/tag_moderators_field.rb b/app/fields/tag_moderators_field.rb new file mode 100644 index 000000000..9ef208eb6 --- /dev/null +++ b/app/fields/tag_moderators_field.rb @@ -0,0 +1,7 @@ +require "administrate/field/base" + +class TagModeratorsField < Administrate::Field::Base + def to_s + data.join(",") + end +end diff --git a/app/models/comment.rb b/app/models/comment.rb index 99ade3413..2d0c57434 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -244,8 +244,7 @@ class Comment < ApplicationRecord end def wrap_timestamps_if_video_present! - return if commentable_type == 'PodcastEpisode' - return unless commentable.video.present? + return unless commentable_type != "PodcastEpisode" && commentable.video.present? self.processed_html = processed_html.gsub(/(([0-9]:)?)(([0-5][0-9]|[0-9])?):[0-5][0-9]/) {|s| "#{s}"} end diff --git a/app/models/role.rb b/app/models/role.rb index 8df406c67..ab7e4e1ec 100644 --- a/app/models/role.rb +++ b/app/models/role.rb @@ -13,7 +13,7 @@ class Role < ApplicationRecord in: %w( super_admin admin - moderator + tag_moderator trusted banned warned diff --git a/app/models/tag.rb b/app/models/tag.rb index 03a31d389..af8c1b253 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -1,12 +1,14 @@ class Tag < ActsAsTaggableOn::Tag acts_as_followable + resourcify mount_uploader :profile_image, ProfileImageUploader mount_uploader :social_image, ProfileImageUploader - validates :text_color_hex, format: /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/, allow_blank: true - validates :bg_color_hex, format: /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/, allow_blank: true - + validates :text_color_hex, + format: /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/, allow_nil: true + validates :bg_color_hex, + format: /\A#([A-Fa-f0-9]{6}|[A-Fa-f0-9]{3})\z/, allow_nil: true validate :validate_alias before_validation :evaluate_markdown @@ -18,6 +20,10 @@ class Tag < ActsAsTaggableOn::Tag submission_template.gsub("PARAM_0", param_0) end + def tag_moderator_ids + User.with_role(:tag_moderator, self).map(&:id).sort + end + private def evaluate_markdown @@ -49,7 +55,7 @@ class Tag < ActsAsTaggableOn::Tag end def pound_it - text_color_hex&.prepend("#") unless text_color_hex&.starts_with?("#") - bg_color_hex&.prepend("#") unless bg_color_hex&.starts_with?("#") + text_color_hex&.prepend("#") unless text_color_hex&.starts_with?("#") || text_color_hex.blank? + bg_color_hex&.prepend("#") unless bg_color_hex&.starts_with?("#") || bg_color_hex.blank? end end diff --git a/app/services/user_role_service.rb b/app/services/user_role_service.rb index 459fea92a..8c0fd2627 100644 --- a/app/services/user_role_service.rb +++ b/app/services/user_role_service.rb @@ -1,8 +1,6 @@ class UserRoleService def initialize(user) @user = user - @bannable = nil - @warnable = nil end def check_for_roles(params) @@ -15,6 +13,22 @@ class UserRoleService new_roles?(params) end + def update_tag_moderators(user_ids, tag) + users = user_ids.map { |id| User.find(id) } + # Andy: Don't have to worry about comparing old and new values. + tag.tag_moderator_ids.each do |id| + User.find(id).remove_role(:tag_moderator, tag) + puts "removed #{id}" + end + users.each do |user| + user.add_role(:tag_moderator, tag) + puts "added #{user.id}" + end + rescue ActiveRecord::RecordNotFound + tag.errors[:moderator_ids] << ": user #{id} was not found" + return false + end + private def new_roles?(params) @@ -32,29 +46,22 @@ class UserRoleService def bannable?(params) if params[:banned] == "0" && !params[:reason_for_ban].blank? @user.errors[:banned] << "was not checked but had the reason filled out" - @bannable = false elsif params[:banned] == "1" && params[:reason_for_ban].blank? @user.errors[:reason_for_ban] << "can't be blank if banned is checked" - @bannable = false elsif params[:banned] == "1" ban(params[:reason_for_ban]) - @bannable = true else unban - @bannable = true end end def warnable?(params) if params[:warned] == "0" && !params[:reason_for_warning].blank? @user.errors[:warned] << "was not checked but had the reason filled out" - @warnable = false elsif params[:warned] == "1" && params[:reason_for_warning].blank? @user.errors[:reason_for_warning] << "can't be blank if warned is checked" - @warnable = false elsif params[:warned] == "1" give_warning(params[:reason_for_warning]) - @warnable = true end end @@ -80,10 +87,16 @@ class UserRoleService @user.remove_role :banned end + # Andy: Only give warning method b/c no need to remove warnings def give_warning(content) @user.add_role :warned create_or_update_note("warned", content) end - # Andy: no need to remove a warning from a user + + def validate_user_ids(user_ids) + user_ids.each do |id| + User.find(id) + end + end end diff --git a/app/views/fields/tag_moderators_field/_form.html.erb b/app/views/fields/tag_moderators_field/_form.html.erb new file mode 100644 index 000000000..4c5afbb01 --- /dev/null +++ b/app/views/fields/tag_moderators_field/_form.html.erb @@ -0,0 +1,7 @@ +
+ <%= f.label field.attribute %> +
+
+ <%= f.text_field field.attribute, value: field.to_s, placeholder: "comma separated, space is optional. Example: 1,2,3" %> + comma separated, space is optional. Example: 1,2,3 +
diff --git a/app/views/fields/tag_moderators_field/_index.html.erb b/app/views/fields/tag_moderators_field/_index.html.erb new file mode 100644 index 000000000..6d9dbc907 --- /dev/null +++ b/app/views/fields/tag_moderators_field/_index.html.erb @@ -0,0 +1 @@ +<%= field.to_s %> diff --git a/app/views/fields/tag_moderators_field/_show.html.erb b/app/views/fields/tag_moderators_field/_show.html.erb new file mode 100644 index 000000000..6d9dbc907 --- /dev/null +++ b/app/views/fields/tag_moderators_field/_show.html.erb @@ -0,0 +1 @@ +<%= field.to_s %> diff --git a/app/views/fields/user_scholar_field/_form.html.erb b/app/views/fields/user_scholar_field/_form.html.erb index c896a4a3e..b3bc6f6b1 100644 --- a/app/views/fields/user_scholar_field/_form.html.erb +++ b/app/views/fields/user_scholar_field/_form.html.erb @@ -1,5 +1,6 @@
<%= f.label field.attribute %> + Defaults to 1 year. Clear the date for no expiration.
<%= f.check_box field.attribute %> diff --git a/app/views/tags/edit.html.erb b/app/views/tags/edit.html.erb new file mode 100644 index 000000000..d8796f341 --- /dev/null +++ b/app/views/tags/edit.html.erb @@ -0,0 +1,79 @@ + + +
+
+
+ + + + <%= form_tag("/tag/#{@tag.id}", method: :patch, class: "tag-edit-form") do %> +
+ <% if flash[:success] %> +

+ <%= flash[:success] %> +

+ <% elsif flash[:error] %> +
    + <% flash[:error].each do |message| %> +
  • <%= message %>
    • + <% end %> +
+ <% end %> +

+ + Click here to see an example of attributes. + +

+
+ <%= label_tag :pretty_name %> +
+ <%= text_field_tag "tag[pretty_name]", @tag.pretty_name, placeholder: "This is the name at the top of the show page.", class: "tag-form-text-field" %> +
+
+ <%= label_tag :bg_color_hex %> +
+ <%= text_field_tag "tag[bg_color_hex]", @tag.bg_color_hex, placeholder: "Click for color picker", class: "tag-form-text-field jscolor {hash:true, required:false}" %> +
+
+ <%= label_tag :text_color_hex %> +
+ <%= text_field_tag "tag[text_color_hex]", @tag.text_color_hex, placeholder: "Click for color picker", class: "tag-form-text-field jscolor {hash:true, required:false}" %> +
+
+ <%= label_tag :short_summary %> +
+ <%= text_area_tag "tag[short_summary]", @tag.short_summary, placeholder: "", class: "tag-form-text-field tag-area-text" %> +
+
+ <%= label_tag :rules_markdown %> +
+ <%= text_area_tag "tag[rules_markdown]", @tag.rules_markdown, placeholder: "", class: "tag-form-text-field tag-area-text" %> +
+
+ <%= label_tag :wiki_body_markdown %> +
+ <%= text_area_tag "tag[wiki_body_markdown]", @tag.wiki_body_markdown, placeholder: "", class: "tag-form-text-field tag-area-text" %> +
+
+ <%= submit_tag "Save Changes", class: "tag-edit-submit" %> +
+
+<% end %> + diff --git a/config/routes.rb b/config/routes.rb index a0180f661..800c9d5c3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -1,11 +1,11 @@ Rails.application.routes.draw do devise_for :users, controllers: { - omniauth_callbacks: 'omniauth_callbacks', + omniauth_callbacks: "omniauth_callbacks", session: "sessions", registrations: "registrations" } devise_scope :user do delete "/sign_out" => "devise/sessions#destroy" - get '/enter' => 'registrations#new', :as => :new_user_registration_path + get "/enter" => "registrations#new", :as => :new_user_registration_path end namespace :admin do @@ -36,7 +36,7 @@ Rails.application.routes.draw do end - namespace :api, defaults: {format: 'json'} do + namespace :api, defaults: {format: "json"} do scope module: :v0, constraints: ApiConstraints.new(version: 0, default: true) do resources :articles, only: [:index, :show] do collection do @@ -97,7 +97,7 @@ Rails.application.routes.draw do ### Subscription vanity url post "membership-action" => "stripe_subscriptions#create" - get '/async_info/base_data', controller: 'async_info#base_data', defaults: { format: :json } + get "/async_info/base_data", controller: "async_info#base_data", defaults: { format: :json } get "/hello-goodbye-to-the-go-go-go", to: redirect("ben/hello-goodbye-to-the-go-go-go") get "/dhh-on-the-future-of-rails", to: redirect("ben/dhh-on-the-future-of-rails") @@ -174,7 +174,7 @@ Rails.application.routes.draw do match "/delayed_job_admin" => DelayedJobWeb, :anchor => false, via: [:get, :post] - match '/users/:id/finish_signup' => 'users#finish_signup', via: [:get, :patch], :as => :finish_signup + match "/users/:id/finish_signup" => "users#finish_signup", via: [:get, :patch], :as => :finish_signup get "/settings/(:tab)" => "users#edit" get "/signout_confirm" => "users#signout_confirm" @@ -187,51 +187,53 @@ Rails.application.routes.draw do # for testing rails mailers if !Rails.env.production? - get '/rails/mailers' => "rails/mailers#index" - get '/rails/mailers/*path' => "rails/mailers#preview" + get "/rails/mailers" => "rails/mailers#index" + get "/rails/mailers/*path" => "rails/mailers#preview" end - get "/new" => 'articles#new' - get "/new/:template" => 'articles#new' + get "/new" => "articles#new" + get "/new/:template" => "articles#new" get "/pod" => "podcast_episodes#index" get "/readinglist" => "reading_list_items#index" - get "/feed" => "articles#feed", :as => "feed", :defaults => { :format => 'rss' } - get "/feed/:username" => "articles#feed", :as => "user_feed", :defaults => { :format => 'rss' } - get "/rss" => "articles#feed", :defaults => { :format => 'rss' } + get "/feed" => "articles#feed", :as => "feed", :defaults => { :format => "rss" } + get "/feed/:username" => "articles#feed", :as => "user_feed", :defaults => { :format => "rss" } + get "/rss" => "articles#feed", :defaults => { :format => "rss" } - get "/amp/:slug" => 'articles#amp' - get "/amp/:username/:slug" => 'articles#amp', :constraints => { username: /@[\S]*/} + get "/amp/:slug" => "articles#amp" + get "/amp/:username/:slug" => "articles#amp", :constraints => { username: /@[\S]*/} - get "/tag/:tag" => 'stories#index' - get "/t/:tag" => 'stories#index' - get "/t/:tag/top/:timeframe" => 'stories#index' - get "/t/:tag/:timeframe" => 'stories#index', constraints: { timeframe: /latest/} + get "/tag/:tag" => "stories#index" + get "/t/:tag" => "stories#index" + get "/t/:tag/edit", to: "tags#edit" + patch "/tag/:id", to: "tags#update" + get "/t/:tag/top/:timeframe" => "stories#index" + get "/t/:tag/:timeframe" => "stories#index", constraints: { timeframe: /latest/} get "/getting-started" => "onboarding#index" - get "/top/:timeframe" => 'stories#index' + get "/top/:timeframe" => "stories#index" - get "/:timeframe" => 'stories#index', constraints: { timeframe: /latest/} + get "/:timeframe" => "stories#index", constraints: { timeframe: /latest/} - get "/:username/:slug/comments/new/:parent_id_code" => 'comments#new' - get "/:username/:slug/comments/new" => 'comments#new' - get "/:username/:slug/comments" => 'comments#index' - get "/:username/:slug/comments/:id_code" => 'comments#index' - get "/:username/:slug/comments/:id_code/edit" => 'comments#edit' - get "/:username/:slug/comments/:id_code/delete_confirm" => 'comments#delete_confirm' + get "/:username/:slug/comments/new/:parent_id_code" => "comments#new" + get "/:username/:slug/comments/new" => "comments#new" + get "/:username/:slug/comments" => "comments#index" + get "/:username/:slug/comments/:id_code" => "comments#index" + get "/:username/:slug/comments/:id_code/edit" => "comments#edit" + get "/:username/:slug/comments/:id_code/delete_confirm" => "comments#delete_confirm" - get "/:username/comment/:id_code" => 'comments#index' - get "/:username/comment/:id_code/edit" => 'comments#edit' - get "/:username/comment/:id_code/delete_confirm" => 'comments#delete_confirm' + get "/:username/comment/:id_code" => "comments#index" + get "/:username/comment/:id_code/edit" => "comments#edit" + get "/:username/comment/:id_code/delete_confirm" => "comments#delete_confirm" - get "/:username/:slug/:view" => 'stories#show', constraints: { view: /moderate/} - get "/:username/:slug/edit" => 'articles#edit' - get "/:username/:slug/delete_confirm" => 'articles#delete_confirm' - get "/:username/:view" => 'stories#index', constraints: { view: /comments|moderate|admin/} - get "/:username/:slug" => 'stories#show' - get "/:username" => 'stories#index' + get "/:username/:slug/:view" => "stories#show", constraints: { view: /moderate/} + get "/:username/:slug/edit" => "articles#edit" + get "/:username/:slug/delete_confirm" => "articles#delete_confirm" + get "/:username/:view" => "stories#index", constraints: { view: /comments|moderate|admin/} + get "/:username/:slug" => "stories#show" + get "/:username" => "stories#index" - root 'stories#index' + root "stories#index" end