FEATURE: Watched words improvements (#7899)

This commit contains 3 features:

- FEATURE: Allow downloading watched words
This introduces a button that allows admins to download watched words per action in a `.txt` file.

- FEATURE: Allow clearing watched words in bulk
This adds a "Clear All" button that clears all deleted words per action (e.g. block, flag etc.)

- FEATURE: List all blocked words contained in the post when it's blocked
When a post is rejected because it contains one or more blocked words, the error message now lists all the blocked words contained in the post.

-------

This also changes the format of the file for importing watched words from `.csv` to `.txt` so it becomes inconsistent with the extension of the file when watched words are exported.

app/assets/javascripts/admin/components/watched-word-uploader.js.es6

@@ -2,13 +2,13 @@ import computed from "ember-addons/ember-computed-decorators";
 import UploadMixin from "discourse/mixins/upload";
 
 export default Ember.Component.extend(UploadMixin, {
-  type: "csv",
+  type: "txt",
   classNames: "watched-words-uploader",
   uploadUrl: "/admin/logs/watched_words/upload",
   addDisabled: Ember.computed.alias("uploading"),
 
   validateUploadedFilesOptions() {
-    return { csvOnly: true };
+    return { skipValidation: true };
   },
 
   @computed("actionKey")

app/assets/javascripts/admin/controllers/admin-watched-words-action.js.es6

@@ -1,5 +1,7 @@
 import computed from "ember-addons/ember-computed-decorators";
 import WatchedWord from "admin/models/watched-word";
+import { ajax } from "discourse/lib/ajax";
+import { fmt } from "discourse/lib/computed";
 
 export default Ember.Controller.extend({
   actionNameKey: null,
@@ -8,6 +10,10 @@ export default Ember.Controller.extend({
     "adminWatchedWords.filtered",
     "adminWatchedWords.showWords"
   ),
+  downloadLink: fmt(
+    "actionNameKey",
+    "/admin/logs/watched_words/action/%@/download"
+  ),
 
   findAction(actionName) {
     return (this.get("adminWatchedWords.model") || []).findBy(
@@ -17,13 +23,13 @@ export default Ember.Controller.extend({
   },
 
   @computed("actionNameKey", "adminWatchedWords.model")
-  filteredContent(actionNameKey) {
+  currentAction(actionName) {
-    if (!actionNameKey) {
+    return this.findAction(actionName);
-      return [];
+  },
-    }
 
-    const a = this.findAction(actionNameKey);
+  @computed("currentAction.words.[]", "adminWatchedWords.model")
-    return a ? a.words : [];
+  filteredContent(words) {
+    return words || [];
   },
 
   @computed("actionNameKey")
@@ -31,10 +37,9 @@ export default Ember.Controller.extend({
     return I18n.t("admin.watched_words.action_descriptions." + actionNameKey);
   },
 
-  @computed("actionNameKey", "adminWatchedWords.model")
+  @computed("currentAction.count")
-  wordCount(actionNameKey) {
+  wordCount(count) {
-    const a = this.findAction(actionNameKey);
+    return count || 0;
-    return a ? a.words.length : 0;
   },
 
   actions: {
@@ -62,10 +67,9 @@ export default Ember.Controller.extend({
     },
 
     recordRemoved(arg) {
-      const a = this.findAction(this.actionNameKey);
+      if (this.currentAction) {
-      if (a) {
+        this.currentAction.words.removeObject(arg);
-        a.words.removeObject(arg);
+        this.currentAction.decrementProperty("count");
-        a.decrementProperty("count");
       }
     },
 
@@ -73,6 +77,30 @@ export default Ember.Controller.extend({
       WatchedWord.findAll().then(data => {
         this.set("adminWatchedWords.model", data);
       });
+    },
+
+    clearAll() {
+      const actionKey = this.actionNameKey;
+      bootbox.confirm(
+        I18n.t(`admin.watched_words.clear_all_confirm_${actionKey}`),
+        I18n.t("no_value"),
+        I18n.t("yes_value"),
+        result => {
+          if (result) {
+            ajax(`/admin/logs/watched_words/action/${actionKey}.json`, {
+              method: "DELETE"
+            }).then(() => {
+              const action = this.findAction(actionKey);
+              if (action) {
+                action.setProperties({
+                  words: [],
+                  count: 0
+                });
+              }
+            });
+          }
+        }
+      );
     }
   }
 });

app/assets/javascripts/admin/templates/components/watched-word-uploader.hbs

@@ -1,7 +1,6 @@
 <label class="btn btn-default {{if addDisabled 'disabled'}}">
   {{d-icon "upload"}}
   {{i18n 'admin.watched_words.form.upload'}}
-  <input class="hidden-upload-field" disabled={{addDisabled}} type="file" accept="text/plain,text/csv" />
+  <input class="hidden-upload-field" disabled={{addDisabled}} type="file" accept="text/plain" />
 </label>
-<br/>
 <span class="instructions">{{i18n 'admin.watched_words.one_word_per_line'}}</span>

app/assets/javascripts/admin/templates/watched-words-action.hbs

@@ -3,14 +3,24 @@
 <p class="about">{{actionDescription}}</p>
 
 <div class="watched-word-controls">
-{{watched-word-form
+  {{watched-word-form
     actionKey=actionNameKey
     action=(action "recordAdded")
     filteredContent=filteredContent
     regularExpressions=adminWatchedWords.regularExpressions}}
 
-{{watched-word-uploader uploading=uploading actionKey=actionNameKey done=(action "uploadComplete")}}
+  <div class="download-upload-controls">
+    <div class="download">
+      {{d-button
+        class="btn-default download-link"
+        href=downloadLink
+        icon="download"
+        label="admin.watched_words.download"}}
+    </div>
+    {{watched-word-uploader uploading=uploading actionKey=actionNameKey done=(action "uploadComplete")}}
+  </div>
 </div>
+
 <div>
   <label class="show-words-checkbox">
     {{input type="checkbox" checked=adminWatchedWords.showWords disabled=adminWatchedWords.disableShowWords}}
@@ -26,3 +36,11 @@
     {{i18n 'admin.watched_words.word_count' count=wordCount}}
   {{/if}}
 </div>
+
+<div class="clear-all-row">
+  {{d-button
+    class="btn-danger clear-all"
+    label="admin.watched_words.clear_all"
+    icon="trash-alt"
+    action=(action "clearAll")}}
+</div>

app/assets/javascripts/discourse/lib/url.js.es6

@@ -23,7 +23,8 @@ const SERVER_SIDE_ONLY = [
   /\.rss$/,
   /\.json$/,
   /^\/admin\/upgrade$/,
-  /^\/logs($|\/)/
+  /^\/logs($|\/)/,
+  /^\/admin\/logs\/watched_words\/action\/[^\/]+\/download$/
 ];
 
 export function rewritePath(path) {

app/assets/stylesheets/common/admin/staff_logs.scss

@@ -362,17 +362,33 @@ table.screened-ip-addresses {
   display: inline-block;
   width: 250px;
   margin-bottom: 1em;
-  float: left;
+  vertical-align: top;
+}
+
+.admin-watched-words {
+  .clear-all-row {
+    display: flex;
+    margin-top: 10px;
+    justify-content: flex-end;
+  }
 }
 
 .watched-word-controls {
   display: flex;
   flex-wrap: wrap;
   margin-bottom: 1em;
+  justify-content: space-between;
+  .download-upload-controls {
+    display: flex;
+  }
+  .download {
+    justify-content: flex-end;
+  }
 }
 
 .watched-words-list {
   margin-top: 20px;
+  display: inline-block;
 }
 
 .watched-word {
@@ -395,13 +411,17 @@ table.screened-ip-addresses {
 }
 
 .watched-words-uploader {
-  margin-left: auto;
+  margin-left: 5px;
+  display: flex;
+  flex-direction: column;
+  align-items: flex-end;
   @media screen and (max-width: 500px) {
     flex: 1 1 100%;
     margin-top: 0.5em;
   }
   .instructions {
     font-size: $font-down-1;
+    margin-top: 5px;
   }
 }
 

app/controllers/admin/watched_words_controller.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 class Admin::WatchedWordsController < Admin::AdminController
+  skip_before_action :check_xhr, only: [:download]
 
   def index
     render_json_dump WatchedWordListSerializer.new(WatchedWord.by_action, scope: guardian, root: false)
@@ -35,12 +36,36 @@ class Admin::WatchedWordsController < Admin::AdminController
       rescue => e
         data = failed_json.merge(errors: [e.message])
       end
-      MessageBus.publish("/uploads/csv", data.as_json, client_ids: [params[:client_id]])
+      MessageBus.publish("/uploads/txt", data.as_json, client_ids: [params[:client_id]])
     end
 
     render json: success_json
   end
 
+  def download
+    params.require(:id)
+    name = watched_words_params[:id].to_sym
+    action = WatchedWord.actions[name]
+    raise Discourse::NotFound if !action
+
+    content = WatchedWord.where(action: action).pluck(:word).join("\n")
+    headers['Content-Length'] = content.bytesize.to_s
+    send_data content,
+      filename: "#{Discourse.current_hostname}-watched-words-#{name}.txt",
+      content_type: "text/plain"
+  end
+
+  def clear_all
+    params.require(:id)
+    name = watched_words_params[:id].to_sym
+    action = WatchedWord.actions[name]
+    raise Discourse::NotFound if !action
+
+    WatchedWord.where(action: action).delete_all
+    WordWatcher.clear_cache!
+    render json: success_json
+  end
+
   private
 
   def watched_words_params

app/controllers/export_csv_controller.rb

@@ -12,6 +12,7 @@ class ExportCsvController < ApplicationController
   end
 
   private
+
   def export_params
     @_export_params ||= begin
       params.require(:entity)

app/services/word_watcher.rb

@@ -14,17 +14,27 @@ class WordWatcher
     WatchedWord.where(action: WatchedWord.actions[action.to_sym]).exists?
   end
 
+  def self.get_cached_words(action)
+    Discourse.cache.fetch(word_matcher_regexp_key(action), expires_in: 1.day) do
+      words_for_action(action).presence
+    end
+  end
+
   def self.word_matcher_regexp(action)
-    s = Discourse.cache.fetch(word_matcher_regexp_key(action), expires_in: 1.day) do
+    words = get_cached_words(action)
-      words = words_for_action(action)
+    if words
-      if words.empty?
+      words = words.map do |w|
-        nil
+        word = word_to_regexp(w)
-      else
+        word = "(#{word})" if SiteSetting.watched_words_regular_expressions?
-        regexp = '(' + words.map { |w| word_to_regexp(w) }.join('|'.freeze) + ')'
+        word
-        SiteSetting.watched_words_regular_expressions? ? regexp : "(?<!\\w)(#{regexp})(?!\\w)"
       end
+      regexp = words.join('|')
+      if !SiteSetting.watched_words_regular_expressions?
+        regexp = "(#{regexp})"
+        regexp = "(?<!\\w)(#{regexp})(?!\\w)"
+      end
+      Regexp.new(regexp, Regexp::IGNORECASE)
     end
-    s.present? ? Regexp.new(s, Regexp::IGNORECASE) : nil
   end
 
   def self.word_to_regexp(word)
@@ -37,7 +47,7 @@ class WordWatcher
   end
 
   def self.word_matcher_regexp_key(action)
-    "watched-words-regexp:#{action}"
+    "watched-words-list:#{action}"
   end
 
   def self.clear_cache!
@@ -55,12 +65,35 @@ class WordWatcher
   end
 
   def should_block?
-    word_matches_for_action?(:block)
+    word_matches_for_action?(:block, all_matches: true)
   end
 
-  def word_matches_for_action?(action)
+  def word_matches_for_action?(action, all_matches: false)
-    r = self.class.word_matcher_regexp(action)
+    regexp = self.class.word_matcher_regexp(action)
-    r ? r.match(@raw) : false
+    if regexp
-  end
+      match = regexp.match(@raw)
+      return match if !all_matches || !match
 
+      if SiteSetting.watched_words_regular_expressions?
+        set = Set.new
+        @raw.scan(regexp).each do |m|
+          if Array === m
+            set.add(m.find(&:present?))
+          elsif String === m
+            set.add(m)
+          end
+        end
+        matches = set.to_a
+      else
+        matches = @raw.scan(regexp)
+        matches.flatten!
+        matches.uniq!
+      end
+      matches.compact!
+      matches.sort!
+      matches
+    else
+      false
+    end
+  end
 end

config/locales/client.en.yml

@@ -3874,6 +3874,12 @@ en:
         clear_filter: "Clear"
         show_words: "show words"
         one_word_per_line: "One word per line"
+        download: Download
+        clear_all: Clear All
+        clear_all_confirm_block: "Are you sure you want to clear all watched words for the Block action?"
+        clear_all_confirm_censor: "Are you sure you want to clear all watched words for the Censor action?"
+        clear_all_confirm_flag: "Are you sure you want to clear all watched words for the Flag action?"
+        clear_all_confirm_require_approval: "Are you sure you want to clear all watched words for the Require Approval action?"
         word_count:
           one: "%{count} word"
           other: "%{count} words"
@@ -3894,7 +3900,7 @@ en:
           add: "Add"
           success: "Success"
           exists: "Already exists"
-          upload: "Upload"
+          upload: "Add from file"
           upload_successful: "Upload successful. Words have been added."
 
       impersonate:

config/locales/server.en.yml

@@ -304,7 +304,8 @@ en:
   too_many_links:
     one: "Sorry, new users can only put one link in a post."
     other: "Sorry, new users can only put %{count} links in a post."
-  contains_blocked_words: "Your post contains a word that's not allowed: %{word}"
+  contains_blocked_word: "Your post contains a word that's not allowed: %{word}"
+  contains_blocked_words: "Your post contains multiple words that aren't allowed: %{words}"
 
   spamming_host: "Sorry you cannot post a link to that host."
   user_is_suspended: "Suspended users are not allowed to post."

config/routes.rb

@@ -177,6 +177,8 @@ Discourse::Application.routes.draw do
       resources :watched_words, only: [:index, :create, :update, :destroy] do
         collection do
           get "action/:id" => "watched_words#index"
+          get "action/:id/download" => "watched_words#download"
+          delete "action/:id" => "watched_words#clear_all"
         end
       end
       post "watched_words/upload" => "watched_words#upload"

lib/new_post_manager.rb

@@ -172,9 +172,16 @@ class NewPostManager
   end
 
   def perform
-    if !self.class.exempt_user?(@user) && matches = WordWatcher.new("#{@args[:title]} #{@args[:raw]}").should_block?
+    if !self.class.exempt_user?(@user) && matches = WordWatcher.new("#{@args[:title]} #{@args[:raw]}").should_block?.presence
       result = NewPostResult.new(:created_post, false)
-      result.errors.add(:base, I18n.t('contains_blocked_words', word: matches[0]))
+      if matches.size == 1
+        key = 'contains_blocked_word'
+        translation_args = { word: matches[0] }
+      else
+        key = 'contains_blocked_words'
+        translation_args = { words: matches.join(', ') }
+      end
+      result.errors.add(:base, I18n.t(key, translation_args))
       return result
     end
 

lib/validators/post_validator.rb

@@ -60,8 +60,15 @@ class Validators::PostValidator < ActiveModel::Validator
   end
 
   def watched_words(post)
-    if !post.acting_user&.staged && matches = WordWatcher.new(post.raw).should_block?
+    if !post.acting_user&.staged && matches = WordWatcher.new(post.raw).should_block?.presence
-      post.errors.add(:base, I18n.t('contains_blocked_words', word: matches[0]))
+      if matches.size == 1
+        key = 'contains_blocked_word'
+        translation_args = { word: matches[0] }
+      else
+        key = 'contains_blocked_words'
+        translation_args = { words: matches.join(', ') }
+      end
+      post.errors.add(:base, I18n.t(key, translation_args))
     end
   end
 

spec/integration/watched_words_spec.rb

@@ -13,6 +13,7 @@ describe WatchedWord do
   let(:require_approval_word) { Fabricate(:watched_word, action: WatchedWord.actions[:require_approval]) }
   let(:flag_word) { Fabricate(:watched_word, action: WatchedWord.actions[:flag]) }
   let(:block_word) { Fabricate(:watched_word, action: WatchedWord.actions[:block]) }
+  let(:another_block_word) { Fabricate(:watched_word, action: WatchedWord.actions[:block]) }
 
   before_all do
     WordWatcher.clear_cache!
@@ -27,7 +28,7 @@ describe WatchedWord do
       expect {
         result = manager.perform
         expect(result).to_not be_success
-        expect(result.errors[:base]&.first).to eq(I18n.t('contains_blocked_words', word: block_word.word))
+        expect(result.errors[:base]&.first).to eq(I18n.t('contains_blocked_word', word: block_word.word))
       }.to_not change { Post.count }
     end
 
@@ -51,6 +52,15 @@ describe WatchedWord do
       should_block_post(manager)
     end
 
+    it "should block the post if it contains multiple blocked words" do
+      manager = NewPostManager.new(moderator, raw: "Want some #{block_word.word} #{another_block_word.word} for cheap?", topic_id: topic.id)
+      expect {
+        result = manager.perform
+        expect(result).to_not be_success
+        expect(result.errors[:base]&.first).to eq(I18n.t('contains_blocked_words', words: [block_word.word, another_block_word.word].join(', ')))
+      }.to_not change { Post.count }
+    end
+
     it "should block in a private message too" do
       manager = NewPostManager.new(
         tl2_user,

spec/requests/admin/watched_words_controller_spec.rb

@@ -49,4 +49,70 @@ RSpec.describe Admin::WatchedWordsController do
       end
     end
   end
+
+  describe '#download' do
+    context 'not logged in as admin' do
+      it "doesn't allow performing #download" do
+        get "/admin/logs/watched_words/action/block/download"
+        expect(response.status).to eq(404)
+      end
+    end
+
+    context 'logged in as admin' do
+      before do
+        sign_in(admin)
+      end
+
+      it "words of different actions are downloaded separately" do
+        block_word_1 = Fabricate(:watched_word, action: WatchedWord.actions[:block])
+        block_word_2 = Fabricate(:watched_word, action: WatchedWord.actions[:block])
+        censor_word_1 = Fabricate(:watched_word, action: WatchedWord.actions[:censor])
+
+        get "/admin/logs/watched_words/action/block/download"
+        expect(response.status).to eq(200)
+        block_words = response.body.split("\n")
+        expect(block_words).to contain_exactly(block_word_1.word, block_word_2.word)
+
+        get "/admin/logs/watched_words/action/censor/download"
+        expect(response.status).to eq(200)
+        censor_words = response.body.split("\n")
+        expect(censor_words).to eq([censor_word_1.word])
+      end
+    end
+  end
+
+  context '#clear_all' do
+    context 'non admins' do
+      it "doesn't allow them to perform #clear_all" do
+        word = Fabricate(:watched_word, action: WatchedWord.actions[:block])
+        delete "/admin/logs/watched_words/action/block"
+        expect(response.status).to eq(404)
+        expect(WatchedWord.pluck(:word)).to include(word.word)
+      end
+    end
+
+    context 'admins' do
+      before do
+        sign_in(admin)
+      end
+
+      it "allows them to perform #clear_all" do
+        word = Fabricate(:watched_word, action: WatchedWord.actions[:block])
+        delete "/admin/logs/watched_words/action/block.json"
+        expect(response.status).to eq(200)
+        expect(WatchedWord.pluck(:word)).not_to include(word.word)
+      end
+
+      it "doesn't delete words of multiple actions in one call" do
+        block_word = Fabricate(:watched_word, action: WatchedWord.actions[:block])
+        flag_word = Fabricate(:watched_word, action: WatchedWord.actions[:flag])
+
+        delete "/admin/logs/watched_words/action/flag.json"
+        expect(response.status).to eq(200)
+        all_words = WatchedWord.pluck(:word)
+        expect(all_words).to include(block_word.word)
+        expect(all_words).not_to include(flag_word.word)
+      end
+    end
+  end
 end

spec/services/word_watcher_spec.rb

@@ -10,6 +10,25 @@ describe WordWatcher do
     $redis.flushall
   end
 
+  describe '.word_matcher_regexp' do
+    let!(:word1) { Fabricate(:watched_word, action: WatchedWord.actions[:block]).word }
+    let!(:word2) { Fabricate(:watched_word, action: WatchedWord.actions[:block]).word }
+
+    context 'format of the result regexp' do
+      it "is correct when watched_words_regular_expressions = true" do
+        SiteSetting.watched_words_regular_expressions = true
+        regexp = WordWatcher.word_matcher_regexp(:block)
+        expect(regexp.inspect).to eq("/(#{word1})|(#{word2})/i")
+      end
+
+      it "is correct when watched_words_regular_expressions = false" do
+        SiteSetting.watched_words_regular_expressions = false
+        regexp = WordWatcher.word_matcher_regexp(:block)
+        expect(regexp.inspect).to eq("/(?<!\\w)((#{word1}|#{word2}))(?!\\w)/i")
+      end
+    end
+  end
+
   describe "word_matches_for_action?" do
     it "is falsey when there are no watched words" do
       expect(WordWatcher.new(raw).word_matches_for_action?(:require_approval)).to be_falsey
@@ -62,6 +81,37 @@ describe WordWatcher do
         end
       end
 
+      context 'multiple matches' do
+        context 'non regexp words' do
+          it 'lists all matching words' do
+            %w{bananas hate hates}.each do |word|
+              Fabricate(:watched_word, word: word, action: WatchedWord.actions[:block])
+            end
+            matches = WordWatcher.new("I hate bananas").word_matches_for_action?(:block, all_matches: true)
+            expect(matches).to contain_exactly('hate', 'bananas')
+            matches = WordWatcher.new("She hates bananas too").word_matches_for_action?(:block, all_matches: true)
+            expect(matches).to contain_exactly('hates', 'bananas')
+          end
+        end
+
+        context 'regexp words' do
+          before do
+            SiteSetting.watched_words_regular_expressions = true
+          end
+
+          it 'lists all matching patterns' do
+            Fabricate(:watched_word, word: "(pine)?apples", action: WatchedWord.actions[:block])
+            Fabricate(:watched_word, word: "((move|store)(d)?)|((watch|listen)(ed|ing)?)", action: WatchedWord.actions[:block])
+
+            matches = WordWatcher.new("pine pineapples apples").word_matches_for_action?(:block, all_matches: true)
+            expect(matches).to contain_exactly('pineapples', 'apples')
+
+            matches = WordWatcher.new("go watched watch ed ing move d moveed moved moving").word_matches_for_action?(:block, all_matches: true)
+            expect(matches).to contain_exactly(*%w{watched watch move moved})
+          end
+        end
+      end
+
       context "emojis" do
         it "handles emoji" do
           Fabricate(:watched_word, word: ":joy:", action: WatchedWord.actions[:require_approval])
@@ -94,7 +144,7 @@ describe WordWatcher do
             action: WatchedWord.actions[:block]
           )
           m = WordWatcher.new("this is not a test.").word_matches_for_action?(:block)
-          expect(m[1]).to eq("test")
+          expect(m[0]).to eq("test")
         end
 
         it "supports regular expressions as a site setting" do
@@ -104,11 +154,11 @@ describe WordWatcher do
             action: WatchedWord.actions[:require_approval]
           )
           m = WordWatcher.new("Evil Trout is cool").word_matches_for_action?(:require_approval)
-          expect(m[1]).to eq("Trout")
+          expect(m[0]).to eq("Trout")
           m = WordWatcher.new("Evil Troot is cool").word_matches_for_action?(:require_approval)
-          expect(m[1]).to eq("Troot")
+          expect(m[0]).to eq("Troot")
           m = WordWatcher.new("trooooooooot").word_matches_for_action?(:require_approval)
-          expect(m[1]).to eq("trooooooooot")
+          expect(m[0]).to eq("trooooooooot")
         end
 
         it "support uppercase" do
@@ -121,9 +171,9 @@ describe WordWatcher do
           m = WordWatcher.new('Amazing place').word_matches_for_action?(:require_approval)
           expect(m).to be_nil
           m = WordWatcher.new('Amazing applesauce').word_matches_for_action?(:require_approval)
-          expect(m[1]).to eq('applesauce')
+          expect(m[0]).to eq('applesauce')
           m = WordWatcher.new('Amazing AppleSauce').word_matches_for_action?(:require_approval)
-          expect(m[1]).to eq('AppleSauce')
+          expect(m[0]).to eq('AppleSauce')
         end
       end