From 303a02b90100a70ac5bc5b0d580ec65c1eb3465c Mon Sep 17 00:00:00 2001 From: Ryan Mulligan Date: Thu, 24 Mar 2016 11:40:25 -0700 Subject: [PATCH 01/69] FEATURE: make discourse remap optionally do regex_replace This adds a --regex option to discourse remap to use the regexp_replace feature in PostgreSQL. Example usage: discourse remap --regex "\[\/?color(=[^\]]*)*]" "" removes all the "color" bbcodes. Also, this commit fixes the --global option, which did not work because of how Thor processes the options. --- script/discourse | 37 ++++++++++++++++++++++++------------- 1 file changed, 24 insertions(+), 13 deletions(-) diff --git a/script/discourse b/script/discourse index af03de89b3..8c892ac460 100755 --- a/script/discourse +++ b/script/discourse @@ -6,27 +6,31 @@ class DiscourseCLI < Thor class_option :verbose, default: false, aliases: :v desc "remap", "Remap a string sequence accross all tables" - def remap(from, to, global=nil) + option :global, :type => :boolean + option :regex, :type => :boolean + def remap(from, to) load_rails - global = global == "--global" - - puts "Rewriting all occurences of #{from} to #{to}" + if options[:regex] + puts "Rewriting all occurences of #{from} to #{to} using regexp_replace" + else + puts "Rewriting all occurences of #{from} to #{to}" + end puts "THIS TASK WILL REWRITE DATA, ARE YOU SURE (type YES)" - puts "WILL RUN ON ALL #{RailsMultisite::ConnectionManagement.all_dbs.length} DBS" if global + puts "WILL RUN ON ALL #{RailsMultisite::ConnectionManagement.all_dbs.length} DBS" if options[:global] text = STDIN.gets if text.strip != "YES" puts "aborting." exit end - if global + if options[:global] RailsMultisite::ConnectionManagement.each_connection do |db| puts "","Remapping tables on #{db}...","" - do_remap(from, to) + do_remap(from, to, options[:regex]) end else - do_remap(from, to) + do_remap(from, to, options[:regex]) end end @@ -185,7 +189,7 @@ class DiscourseCLI < Thor require File.expand_path(File.dirname(__FILE__) + "/../lib/import_export/import_export") end - def do_remap(from, to) + def do_remap(from, to, regex=false) sql = "SELECT table_name, column_name FROM information_schema.columns WHERE table_schema='public' and (data_type like 'char%' or data_type like 'text%') and is_updatable = 'YES'" @@ -199,10 +203,17 @@ WHERE table_schema='public' and (data_type like 'char%' or data_type like 'text% column_name = result["column_name"] puts "Remapping #{table_name} #{column_name}" begin - result = cnn.async_exec("UPDATE #{table_name} - SET #{column_name} = replace(#{column_name}, $1, $2) - WHERE NOT #{column_name} IS NULL - AND #{column_name} <> replace(#{column_name}, $1, $2)", [from, to]) + result = if regex + cnn.async_exec("UPDATE #{table_name} + SET #{column_name} = regexp_replace(#{column_name}, $1, $2, 'g') + WHERE NOT #{column_name} IS NULL + AND #{column_name} <> regexp_replace(#{column_name}, $1, $2, 'g')", [from, to]) + else + cnn.async_exec("UPDATE #{table_name} + SET #{column_name} = replace(#{column_name}, $1, $2) + WHERE NOT #{column_name} IS NULL + AND #{column_name} <> replace(#{column_name}, $1, $2)", [from, to]) + end puts "#{result.cmd_tuples} rows affected!" rescue => ex puts "Error: #{ex}" From e9db03e465998dba5520b926e75042ecae8cb9bc Mon Sep 17 00:00:00 2001 From: Acshi Haggenmiller Date: Fri, 22 Jul 2016 15:33:21 -0400 Subject: [PATCH 02/69] allow localhost as an embeddedable host --- app/models/embeddable_host.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/embeddable_host.rb b/app/models/embeddable_host.rb index ffa1005c6f..d42879ad94 100644 --- a/app/models/embeddable_host.rb +++ b/app/models/embeddable_host.rb @@ -25,7 +25,8 @@ class EmbeddableHost < ActiveRecord::Base def host_must_be_valid if host !~ /\A[a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.[a-z]{2,7}(:[0-9]{1,5})?(\/.*)?\Z/i && - host !~ /\A(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})\Z/ + host !~ /\A(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})\Z/ && + host !~ /\A([a-z0-9]+([\-\.]{1}[a-z0-9]+)*\.)?localhost(\:[0-9]{1,5})?(\/.*)?\Z/i errors.add(:host, I18n.t('errors.messages.invalid')) end end From afa88f68ce4e5764a947c4222892a0498bc78a08 Mon Sep 17 00:00:00 2001 From: Acshi Haggenmiller Date: Fri, 22 Jul 2016 17:12:57 -0400 Subject: [PATCH 03/69] added spec for localhost embeddable host validation --- spec/models/embeddable_host_spec.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/spec/models/embeddable_host_spec.rb b/spec/models/embeddable_host_spec.rb index 4ee44c5608..c6596370a5 100644 --- a/spec/models/embeddable_host_spec.rb +++ b/spec/models/embeddable_host_spec.rb @@ -26,6 +26,29 @@ describe EmbeddableHost do expect(eh.host).to eq('192.168.0.1') end + it "supports localhost" do + eh = EmbeddableHost.new(host: 'localhost') + expect(eh).to be_valid + expect(eh.host).to eq('localhost') + end + + it "supports ports of localhost" do + eh = EmbeddableHost.new(host: 'localhost:8080') + expect(eh).to be_valid + expect(eh.host).to eq('localhost:8080') + end + + it "supports subdomains of localhost" do + eh = EmbeddableHost.new(host: 'discourse.localhost') + expect(eh).to be_valid + expect(eh.host).to eq('discourse.localhost') + end + + it "rejects misspellings of localhost" do + eh = EmbeddableHost.new(host: 'alocalhost') + expect(eh).not_to be_valid + end + describe "allows_embeddable_host" do let!(:host) { Fabricate(:embeddable_host) } From c2819e99f4c6bd7de90f57476de454d116b4f596 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Wed, 3 Aug 2016 12:29:38 -0400 Subject: [PATCH 04/69] Don't halt notification emails for those on daily mailing list mode --- app/jobs/regular/user_email.rb | 1 + spec/jobs/user_email_spec.rb | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/jobs/regular/user_email.rb b/app/jobs/regular/user_email.rb index bb6b8cc1d0..50109e2a68 100644 --- a/app/jobs/regular/user_email.rb +++ b/app/jobs/regular/user_email.rb @@ -103,6 +103,7 @@ module Jobs end if user.user_option.mailing_list_mode? && + user.user_option.mailing_list_mode_frequency == 1 && # don't catch notifications for users on daily mailing list mode (!post.try(:topic).try(:private_message?)) && NOTIFICATIONS_SENT_BY_MAILING_LIST.include?(email_args[:notification_type]) # no need to log a reason when the mail was already sent via the mailing list job diff --git a/spec/jobs/user_email_spec.rb b/spec/jobs/user_email_spec.rb index 4f4d07d22b..f99a403b4e 100644 --- a/spec/jobs/user_email_spec.rb +++ b/spec/jobs/user_email_spec.rb @@ -201,6 +201,13 @@ describe Jobs::UserEmail do Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id) end + it "does send the email if the user is using daily mailing list mode" do + Email::Sender.any_instance.expects(:send) + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0) + + Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id) + end + it "does not send notification if limit is reached" do SiteSetting.max_emails_per_day_per_user = 2 @@ -218,9 +225,9 @@ describe Jobs::UserEmail do expect(EmailLog.where(user_id: user.id, skipped: true).count).to eq(1) end - it "doesn't send the mail if the user is using mailing list mode" do + it "doesn't send the mail if the user is using individual mailing list mode" do Email::Sender.any_instance.expects(:send).never - user.user_option.update_column(:mailing_list_mode, true) + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 1) # sometimes, we pass the notification_id Jobs::UserEmail.new.execute(type: :user_mentioned, user_id: user.id, notification_id: notification.id, post_id: post.id) # other times, we only pass the type of notification From d8165d7cee84c1568678c7acde6f91af63908169 Mon Sep 17 00:00:00 2001 From: cpradio Date: Sat, 6 Aug 2016 11:18:10 -0400 Subject: [PATCH 05/69] FEATURE: Allow keyboard shortcuts for topic list to start from last viewed topic --- .../javascripts/discourse/components/topic-list-item.js.es6 | 5 +++-- .../javascripts/discourse/lib/keyboard-shortcuts.js.es6 | 4 +++- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 index 28c00c4345..765154cb31 100644 --- a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 +++ b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 @@ -113,11 +113,12 @@ export default Ember.Component.extend(StringBuffer, { } }, - highlight() { + highlight(isLastViewedTopic = false) { const $topic = this.$(); const originalCol = $topic.css('backgroundColor'); $topic .addClass('highlighted') + .attr('data-islastviewedtopic', isLastViewedTopic) .stop() .animate({ backgroundColor: originalCol }, 2500, 'swing', function() { $topic.removeClass('highlighted'); @@ -128,7 +129,7 @@ export default Ember.Component.extend(StringBuffer, { // highlight the last topic viewed if (this.session.get('lastTopicIdViewed') === this.get('topic.id')) { this.session.set('lastTopicIdViewed', null); - this.highlight(); + this.highlight(true); } else if (this.get('topic.highlight')) { // highlight new topics that have been loaded from the server or the one we just created this.set('topic.highlight', false); diff --git a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 index 70af9c9526..5701e4b735 100644 --- a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 +++ b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 @@ -277,7 +277,9 @@ export default { return; } - const $selected = $articles.filter('.selected'); + const $selected = ($articles.filter('.selected').length !== 0) + ? $articles.filter('.selected') + : $articles.filter('[data-islastviewedtopic=true]'); let index = $articles.index($selected); if ($selected.length !== 0) { //boundries check From fce902ab1ede77c66cc057590d22251148b98e54 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 8 Aug 2016 07:24:09 +0800 Subject: [PATCH 06/69] UX: Centering Badge notification styles on mobile. --- app/assets/stylesheets/mobile/topic-list.scss | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/mobile/topic-list.scss b/app/assets/stylesheets/mobile/topic-list.scss index 660446f6f5..c2a658e32a 100644 --- a/app/assets/stylesheets/mobile/topic-list.scss +++ b/app/assets/stylesheets/mobile/topic-list.scss @@ -86,7 +86,12 @@ position: relative; top: -1px; font-size: 1.071em; - padding: 4px 6px 3px 6px; + line-height: 20px; + min-width: 14px; + height: 20px; + border-radius: 10px; + padding: 0px 3px; + i {color: $secondary;} &.new-topic::before { @@ -141,6 +146,10 @@ box-sizing: inherit; } + .topic-post-badges { + box-sizing: border-box; + } + .posts { width: 10%; vertical-align: top; @@ -149,6 +158,12 @@ .age { margin-left: 5px; } + + td.posts { + .badge-notification { + padding: 0px 6px; + } + } } tr.category-topic-link { From 8539f02b5e4345ccef6860e9bedf2ca845c459ca Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 8 Aug 2016 07:49:37 +0800 Subject: [PATCH 07/69] FIX: Backuper should return the full path. --- lib/backup_restore/backuper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/backup_restore/backuper.rb b/lib/backup_restore/backuper.rb index daf328c632..5c547b7545 100644 --- a/lib/backup_restore/backuper.rb +++ b/lib/backup_restore/backuper.rb @@ -52,7 +52,7 @@ module BackupRestore @success = false else @success = true - @backup_filename + File.join(@archive_directory, @backup_filename) ensure begin notify_user From 72b321b7b40b748eadb9f0b6809164e24752474f Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 8 Aug 2016 09:02:46 +0800 Subject: [PATCH 08/69] Revert "UX: Centering Badge notification styles on mobile." This reverts commit fce902ab1ede77c66cc057590d22251148b98e54. --- .../common/components/badges.css.scss | 9 +++------ app/assets/stylesheets/mobile/topic-list.scss | 17 +---------------- 2 files changed, 4 insertions(+), 22 deletions(-) diff --git a/app/assets/stylesheets/common/components/badges.css.scss b/app/assets/stylesheets/common/components/badges.css.scss index c81be42813..3dd2d80eb4 100644 --- a/app/assets/stylesheets/common/components/badges.css.scss +++ b/app/assets/stylesheets/common/components/badges.css.scss @@ -9,7 +9,7 @@ display: inline-block; font-weight: normal; white-space: nowrap; - @include border-radius-all(9px); + @include border-radius-all(10px); } // Category badges @@ -234,16 +234,13 @@ .badge-notification { @extend %badge; + padding: 4px 5px 2px 5px; vertical-align: middle; color: $secondary; font-size: 11px; - line-height: 18px; - min-width: 12px; - height: 18px; - padding: 0px 3px; + line-height: 1; text-align: center; background-color: dark-light-choose(scale-color($primary, $lightness: 70%), scale-color($secondary, $lightness: 70%)); - &[href] { color: $secondary; } diff --git a/app/assets/stylesheets/mobile/topic-list.scss b/app/assets/stylesheets/mobile/topic-list.scss index c2a658e32a..660446f6f5 100644 --- a/app/assets/stylesheets/mobile/topic-list.scss +++ b/app/assets/stylesheets/mobile/topic-list.scss @@ -86,12 +86,7 @@ position: relative; top: -1px; font-size: 1.071em; - line-height: 20px; - min-width: 14px; - height: 20px; - border-radius: 10px; - padding: 0px 3px; - + padding: 4px 6px 3px 6px; i {color: $secondary;} &.new-topic::before { @@ -146,10 +141,6 @@ box-sizing: inherit; } - .topic-post-badges { - box-sizing: border-box; - } - .posts { width: 10%; vertical-align: top; @@ -158,12 +149,6 @@ .age { margin-left: 5px; } - - td.posts { - .badge-notification { - padding: 0px 6px; - } - } } tr.category-topic-link { From 02d63d5bc6ffb521508d1c1a20c15a902f249d25 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 8 Aug 2016 09:33:50 +0800 Subject: [PATCH 09/69] FIX: Backup script with custom filename needs to include version number. --- script/discourse | 3 +++ 1 file changed, 3 insertions(+) diff --git a/script/discourse b/script/discourse index b6c14276bf..d5d800a331 100755 --- a/script/discourse +++ b/script/discourse @@ -41,6 +41,9 @@ class DiscourseCLI < Thor backup = backuper.run if filename.present? puts "Moving '#{backup}' to '#{filename}'" + puts "Including version number into '#{filename}'" + version_string = File.basename(backup)[/-#{BackupRestore::VERSION_PREFIX}\d{14}/] + filename = filename.dup.insert(filename.index('.'), version_string) FileUtils.mv(backup, filename) backup = filename end From aa561006602cb65c9b26024daf1063c3db7ddb14 Mon Sep 17 00:00:00 2001 From: Matt Palmer Date: Mon, 8 Aug 2016 16:02:23 +1000 Subject: [PATCH 10/69] Failover messages really aren't warnings "I'm going to do something entirely sane and reasonable" doesn't warrant a warning-level log message. It's perfectly fine and reasonable to just log that sort of thing at info level. --- lib/discourse_redis.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/discourse_redis.rb b/lib/discourse_redis.rb index 8fd9a39599..f3f4993c81 100644 --- a/lib/discourse_redis.rb +++ b/lib/discourse_redis.rb @@ -28,10 +28,10 @@ class DiscourseRedis def initiate_fallback_to_master begin slave_client = ::Redis::Client.new(@slave_config) - logger.warn "#{log_prefix}: Checking connection to master server..." + logger.info "#{log_prefix}: Checking connection to master server..." if slave_client.call([:info]).split("\r\n").include?(MASTER_LINK_STATUS) - logger.warn "#{log_prefix}: Master server is active, killing all connections to slave..." + logger.info "#{log_prefix}: Master server is active, killing all connections to slave..." CONNECTION_TYPES.each do |connection_type| slave_client.call([:client, [:kill, 'type', connection_type]]) From 90d4ea70999195750570198fe3a26b670fa4a042 Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Mon, 8 Aug 2016 20:40:50 +0530 Subject: [PATCH 11/69] bump onebox version --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 85da258e69..33a317fb9a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -216,7 +216,7 @@ GEM omniauth-twitter (1.2.1) json (~> 1.3) omniauth-oauth (~> 1.1) - onebox (1.5.44) + onebox (1.5.45) htmlentities (~> 4.3.4) moneta (~> 0.8) multi_json (~> 1.11) From 3db020f95e57b9bb519a8740dce26f0c9f78fb92 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 8 Aug 2016 12:24:30 -0400 Subject: [PATCH 12/69] FIX: Display anonymous counts nicely on badges --- .../javascripts/discourse/widgets/hamburger-categories.js.es6 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/discourse/widgets/hamburger-categories.js.es6 b/app/assets/javascripts/discourse/widgets/hamburger-categories.js.es6 index 4000467e94..37bdae69fb 100644 --- a/app/assets/javascripts/discourse/widgets/hamburger-categories.js.es6 +++ b/app/assets/javascripts/discourse/widgets/hamburger-categories.js.es6 @@ -16,7 +16,7 @@ createWidget('hamburger-category', { } if (!this.currentUser) { - results.push(h('b.topics-count', c.get('topic_count').toString())); + results.push(h('b.topics-count', number(c.get('topic_count')))); } return results; From a9ae99bf82b003bc89a57bdc9e70e8fc0dff55be Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Tue, 9 Aug 2016 00:32:43 +0800 Subject: [PATCH 13/69] FIX: Admin dashboard problems not displaying when there is one error. --- app/assets/javascripts/admin/controllers/admin-dashboard.js.es6 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/admin/controllers/admin-dashboard.js.es6 b/app/assets/javascripts/admin/controllers/admin-dashboard.js.es6 index 82cedfee59..2e39c15880 100644 --- a/app/assets/javascripts/admin/controllers/admin-dashboard.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-dashboard.js.es6 @@ -22,7 +22,7 @@ export default Ember.Controller.extend({ @computed('problems.length') foundProblems(problemsLength) { - return this.currentUser.get('admin') && (problemsLength || 0) > 1; + return this.currentUser.get('admin') && (problemsLength || 0) > 0; }, @computed('foundProblems') From 01ced67ab39a3055cced66e58c9fcf857f4db920 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 8 Aug 2016 13:57:05 -0400 Subject: [PATCH 14/69] FIX: Focus on usernames if it's blank when composing a message --- app/assets/javascripts/discourse/controllers/composer.js.es6 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/discourse/controllers/composer.js.es6 b/app/assets/javascripts/discourse/controllers/composer.js.es6 index b89d61a98b..c04063ed1b 100644 --- a/app/assets/javascripts/discourse/controllers/composer.js.es6 +++ b/app/assets/javascripts/discourse/controllers/composer.js.es6 @@ -69,7 +69,9 @@ export default Ember.Controller.extend({ focusTarget(replyingToTopic, creatingPM, usernames) { if (this.capabilities.isIOS) { return "none"; } - if (creatingPM && usernames === this.currentUser.get('username')) { + // Focus on usernames if it's blank or if it's just you + usernames = usernames || ""; + if (creatingPM && usernames.length === 0 || usernames === this.currentUser.get('username')) { return 'usernames'; } From 17b51bb465bb284a8496eb89d7b29fa3835f8a94 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Mon, 8 Aug 2016 15:14:18 -0400 Subject: [PATCH 15/69] FIX: topics tagged with muted tags should not be included in digest emails --- app/models/topic.rb | 7 +++++++ spec/models/topic_spec.rb | 18 ++++++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/app/models/topic.rb b/app/models/topic.rb index ce5b9c6ae3..20745168bb 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -361,6 +361,13 @@ class Topic < ActiveRecord::Base topics = topics.where("topics.category_id NOT IN (?)", muted_category_ids) end + # Remove muted categories + muted_tag_ids = TagUser.lookup(user, :muted).pluck(:tag_id) + unless muted_tag_ids.empty? + topics = topics.joins("LEFT OUTER JOIN topic_tags ON topic_tags.topic_id = topics.id") + .where("topic_tags.tag_id NOT IN (?)", muted_tag_ids) + end + topics end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 36b6376b47..9bac6b9344 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1364,6 +1364,24 @@ describe Topic do expect(Topic.for_digest(u, 1.year.ago, top_order: true)).to eq([topic]) end + it "doesn't return topics with only muted tags" do + user = Fabricate(:user) + tag = Fabricate(:tag) + TagUser.change(user.id, tag.id, TagUser.notification_levels[:muted]) + topic = Fabricate(:topic, tags: [tag]) + + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to be_blank + end + + it "returns topics with both muted and not muted tags" do + user = Fabricate(:user) + muted_tag, other_tag = Fabricate(:tag), Fabricate(:tag) + TagUser.change(user.id, muted_tag.id, TagUser.notification_levels[:muted]) + topic = Fabricate(:topic, tags: [muted_tag, other_tag]) + + expect(Topic.for_digest(user, 1.year.ago, top_order: true)).to eq([topic]) + end + end describe 'secured' do From 754e3b2287b806a97123037ce2e52bc82c3c88b7 Mon Sep 17 00:00:00 2001 From: cpradio Date: Mon, 8 Aug 2016 15:15:27 -0400 Subject: [PATCH 16/69] Convert boolean to opts object --- .../javascripts/discourse/components/topic-list-item.js.es6 | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 index 765154cb31..4d2facadf1 100644 --- a/app/assets/javascripts/discourse/components/topic-list-item.js.es6 +++ b/app/assets/javascripts/discourse/components/topic-list-item.js.es6 @@ -113,12 +113,12 @@ export default Ember.Component.extend(StringBuffer, { } }, - highlight(isLastViewedTopic = false) { + highlight(opts = { isLastViewedTopic: false }) { const $topic = this.$(); const originalCol = $topic.css('backgroundColor'); $topic .addClass('highlighted') - .attr('data-islastviewedtopic', isLastViewedTopic) + .attr('data-islastviewedtopic', opts.isLastViewedTopic) .stop() .animate({ backgroundColor: originalCol }, 2500, 'swing', function() { $topic.removeClass('highlighted'); @@ -129,7 +129,7 @@ export default Ember.Component.extend(StringBuffer, { // highlight the last topic viewed if (this.session.get('lastTopicIdViewed') === this.get('topic.id')) { this.session.set('lastTopicIdViewed', null); - this.highlight(true); + this.highlight({ isLastViewedTopic: true }); } else if (this.get('topic.highlight')) { // highlight new topics that have been loaded from the server or the one we just created this.set('topic.highlight', false); From 5c06076b5cd4bf2957574d6d6d621b09d46dbdeb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 8 Aug 2016 12:30:37 +0200 Subject: [PATCH 17/69] FIX: strip blacklisted attachments before checking for empty email body --- lib/email/receiver.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/lib/email/receiver.rb b/lib/email/receiver.rb index a5ff182dfb..597bc0726e 100644 --- a/lib/email/receiver.rb +++ b/lib/email/receiver.rb @@ -77,7 +77,7 @@ module Email body, @elided = select_body body ||= "" - raise NoBodyDetectedError if body.blank? && !@mail.has_attachments? + raise NoBodyDetectedError if body.blank? && attachments.empty? if is_auto_generated? @incoming_email.update_columns(is_auto_generated: true) @@ -436,15 +436,17 @@ module Email raise InvalidPostAction.new(e) end - + def attachments + # strip blacklisted attachments (mostly signatures) + @attachments ||= @mail.attachments.select do |attachment| + attachment.content_type !~ SiteSetting.attachment_content_type_blacklist_regex && + attachment.filename !~ SiteSetting.attachment_filename_blacklist_regex + end + end def create_post_with_attachments(options={}) # deal with attachments - @mail.attachments.each do |attachment| - # strip blacklisted attachments (mostly signatures) - next if attachment.content_type =~ SiteSetting.attachment_content_type_blacklist_regex - next if attachment.filename =~ SiteSetting.attachment_filename_blacklist_regex - + attachments.each do |attachment| tmp = Tempfile.new("discourse-email-attachment") begin # read attachment From 51322a46b3fabab11cfa8f29ac4f386d8227aedf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 8 Aug 2016 22:28:27 +0200 Subject: [PATCH 18/69] FEATURE: retry processing incoming emails on rate limit --- app/jobs/regular/process_email.rb | 16 +++++++++++++++ config/locales/server.en.yml | 7 ------- lib/email/processor.rb | 14 ++++++------- spec/components/email/processor_spec.rb | 27 +++++++++++++++++++++++++ spec/jobs/process_email_spec.rb | 12 +++++++++++ 5 files changed, 61 insertions(+), 15 deletions(-) create mode 100644 app/jobs/regular/process_email.rb create mode 100644 spec/components/email/processor_spec.rb create mode 100644 spec/jobs/process_email_spec.rb diff --git a/app/jobs/regular/process_email.rb b/app/jobs/regular/process_email.rb new file mode 100644 index 0000000000..a01e3bc07d --- /dev/null +++ b/app/jobs/regular/process_email.rb @@ -0,0 +1,16 @@ +module Jobs + + class ProcessEmail < Jobs::Base + sidekiq_options retry: 3 + + def execute(args) + Email::Processor.process!(args[:mail], false) + end + + sidekiq_retries_exhausted do |msg| + Rails.logger.warn("Incoming email could not be processed after 3 retries.\n\n#{msg["args"][:mail]}") + end + + end + +end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 9fdcd7c71c..50188377cf 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2054,13 +2054,6 @@ en: If you can correct the problem, please try again. - email_reject_rate_limit_specified: - subject_template: "[%{site_name}] Email issue -- Rate limited" - text_body_template: | - We're sorry, but your email message to %{destination} (titled %{former_title}) didn't work. - - Reason: %{rate_limit_description} - email_reject_invalid_post_action: subject_template: "[%{site_name}] Email issue -- Invalid Post Action" text_body_template: | diff --git a/lib/email/processor.rb b/lib/email/processor.rb index 3005b00d75..71c9b846f7 100644 --- a/lib/email/processor.rb +++ b/lib/email/processor.rb @@ -2,18 +2,21 @@ module Email class Processor - def initialize(mail) + def initialize(mail, retry_on_rate_limit=true) @mail = mail + @retry_on_rate_limit = retry_on_rate_limit end - def self.process!(mail) - Email::Processor.new(mail).process! + def self.process!(mail, retry_on_rate_limit=true) + Email::Processor.new(mail, retry_on_rate_limit).process! end def process! begin receiver = Email::Receiver.new(@mail) receiver.process! + rescue RateLimiter::LimitExceeded + @retry_on_rate_limit ? Jobs.enqueue(:process_email, mail: @mail) : raise rescue Email::Receiver::BouncedEmailError => e # never reply to bounced emails log_email_process_failure(@mail, e) @@ -49,7 +52,6 @@ module Email when ActiveRecord::Rollback then :email_reject_invalid_post when Email::Receiver::InvalidPostAction then :email_reject_invalid_post_action when Discourse::InvalidAccess then :email_reject_invalid_access - when RateLimiter::LimitExceeded then :email_reject_rate_limit_specified end template_args = {} @@ -61,10 +63,6 @@ module Email template_args[:post_error] = e.message end - if message_template == :email_reject_rate_limit_specified - template_args[:rate_limit_description] = e.description - end - if message_template # inform the user about the rejection message = Mail::Message.new(mail_string) diff --git a/spec/components/email/processor_spec.rb b/spec/components/email/processor_spec.rb new file mode 100644 index 0000000000..c30b09ad4e --- /dev/null +++ b/spec/components/email/processor_spec.rb @@ -0,0 +1,27 @@ +require "rails_helper" +require "email/processor" + +describe Email::Processor do + + describe "rate limits" do + + let(:mail) { "From: foo@bar.com\nTo: bar@foo.com\nSubject: FOO BAR\n\nFoo foo bar bar?" } + let(:limit_exceeded) { RateLimiter::LimitExceeded.new(10) } + + before do + Email::Receiver.any_instance.expects(:process!).raises(limit_exceeded) + end + + it "enqueues a background job by default" do + Jobs.expects(:enqueue).with(:process_email, mail: mail) + Email::Processor.process!(mail) + end + + it "doesn't enqueue a background job when retry is disabled" do + Jobs.expects(:enqueue).with(:process_email, mail: mail).never + expect { Email::Processor.process!(mail, false) }.to raise_error(limit_exceeded) + end + + end + +end diff --git a/spec/jobs/process_email_spec.rb b/spec/jobs/process_email_spec.rb new file mode 100644 index 0000000000..f740a8f2b1 --- /dev/null +++ b/spec/jobs/process_email_spec.rb @@ -0,0 +1,12 @@ +require "rails_helper" + +describe Jobs::ProcessEmail do + + let(:mail) { "From: foo@bar.com\nTo: bar@foo.com\nSubject: FOO BAR\n\nFoo foo bar bar?" } + + it "process an email without retry" do + Email::Processor.expects(:process!).with(mail, false) + Jobs::ProcessEmail.new.execute(mail: mail) + end + +end From 277e7383f342fc841591fece93f2ed8b22862c39 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 8 Aug 2016 17:05:22 -0400 Subject: [PATCH 19/69] Revert "FEATURE: make discourse remap optionally do regex_replace (#4367)" This reverts commit f8dda198bd63e6944e71dc952363b01e62fc353e, reversing changes made to 01ced67ab39a3055cced66e58c9fcf857f4db920. --- script/discourse | 37 +++++++++++++------------------------ 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/script/discourse b/script/discourse index a9ed089a40..d5d800a331 100755 --- a/script/discourse +++ b/script/discourse @@ -6,31 +6,27 @@ class DiscourseCLI < Thor class_option :verbose, default: false, aliases: :v desc "remap", "Remap a string sequence accross all tables" - option :global, :type => :boolean - option :regex, :type => :boolean - def remap(from, to) + def remap(from, to, global=nil) load_rails - if options[:regex] - puts "Rewriting all occurences of #{from} to #{to} using regexp_replace" - else - puts "Rewriting all occurences of #{from} to #{to}" - end + global = global == "--global" + + puts "Rewriting all occurences of #{from} to #{to}" puts "THIS TASK WILL REWRITE DATA, ARE YOU SURE (type YES)" - puts "WILL RUN ON ALL #{RailsMultisite::ConnectionManagement.all_dbs.length} DBS" if options[:global] + puts "WILL RUN ON ALL #{RailsMultisite::ConnectionManagement.all_dbs.length} DBS" if global text = STDIN.gets if text.strip != "YES" puts "aborting." exit end - if options[:global] + if global RailsMultisite::ConnectionManagement.each_connection do |db| puts "","Remapping tables on #{db}...","" - do_remap(from, to, options[:regex]) + do_remap(from, to) end else - do_remap(from, to, options[:regex]) + do_remap(from, to) end end @@ -203,7 +199,7 @@ class DiscourseCLI < Thor require File.expand_path(File.dirname(__FILE__) + "/../lib/import_export/import_export") end - def do_remap(from, to, regex=false) + def do_remap(from, to) sql = "SELECT table_name, column_name FROM information_schema.columns WHERE table_schema='public' and (data_type like 'char%' or data_type like 'text%') and is_updatable = 'YES'" @@ -217,17 +213,10 @@ WHERE table_schema='public' and (data_type like 'char%' or data_type like 'text% column_name = result["column_name"] puts "Remapping #{table_name} #{column_name}" begin - result = if regex - cnn.async_exec("UPDATE #{table_name} - SET #{column_name} = regexp_replace(#{column_name}, $1, $2, 'g') - WHERE NOT #{column_name} IS NULL - AND #{column_name} <> regexp_replace(#{column_name}, $1, $2, 'g')", [from, to]) - else - cnn.async_exec("UPDATE #{table_name} - SET #{column_name} = replace(#{column_name}, $1, $2) - WHERE NOT #{column_name} IS NULL - AND #{column_name} <> replace(#{column_name}, $1, $2)", [from, to]) - end + result = cnn.async_exec("UPDATE #{table_name} + SET #{column_name} = replace(#{column_name}, $1, $2) + WHERE NOT #{column_name} IS NULL + AND #{column_name} <> replace(#{column_name}, $1, $2)", [from, to]) puts "#{result.cmd_tuples} rows affected!" rescue => ex puts "Error: #{ex}" From 274a11f24443c4984aef9e32ad3e692060a68817 Mon Sep 17 00:00:00 2001 From: Guillaume Klein Date: Tue, 9 Aug 2016 08:12:37 +0900 Subject: [PATCH 20/69] Add French translations to the details plugin --- plugins/discourse-details/config/locales/client.fr.yml | 7 +++++++ plugins/discourse-details/config/locales/server.fr.yml | 3 +++ 2 files changed, 10 insertions(+) create mode 100644 plugins/discourse-details/config/locales/client.fr.yml create mode 100644 plugins/discourse-details/config/locales/server.fr.yml diff --git a/plugins/discourse-details/config/locales/client.fr.yml b/plugins/discourse-details/config/locales/client.fr.yml new file mode 100644 index 0000000000..84f9494dd8 --- /dev/null +++ b/plugins/discourse-details/config/locales/client.fr.yml @@ -0,0 +1,7 @@ +fr: + js: + details: + title: Cacher le texte + composer: + details_title: Résumé + details_text: "Ce texte sera caché" diff --git a/plugins/discourse-details/config/locales/server.fr.yml b/plugins/discourse-details/config/locales/server.fr.yml new file mode 100644 index 0000000000..4066a0937d --- /dev/null +++ b/plugins/discourse-details/config/locales/server.fr.yml @@ -0,0 +1,3 @@ +fr: + site_settings: + details_enabled: "Activer le plugin details. Si vous modifiez ceci, vous devez exécuter la commande \"rake posts:rebake\"." From 5cc8bb535bd25adc8c92f197943a2081694f4b51 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 9 Aug 2016 10:02:18 +1000 Subject: [PATCH 21/69] SECURITY: do cookie auth rate limiting earlier --- lib/auth/default_current_user_provider.rb | 8 ++++++-- .../auth/default_current_user_provider_spec.rb | 10 ++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/lib/auth/default_current_user_provider.rb b/lib/auth/default_current_user_provider.rb index 066f55d2c1..d726be453a 100644 --- a/lib/auth/default_current_user_provider.rb +++ b/lib/auth/default_current_user_provider.rb @@ -38,14 +38,18 @@ class Auth::DefaultCurrentUserProvider current_user = nil if auth_token && auth_token.length == 32 - current_user = User.where(auth_token: auth_token) + limiter = RateLimiter.new(nil, "cookie_auth_#{request.ip}", COOKIE_ATTEMPTS_PER_MIN ,60) + + if limiter.can_perform? + current_user = User.where(auth_token: auth_token) .where('auth_token_updated_at IS NULL OR auth_token_updated_at > ?', SiteSetting.maximum_session_age.hours.ago) .first + end unless current_user begin - RateLimiter.new(nil, "cookie_auth_#{request.ip}", COOKIE_ATTEMPTS_PER_MIN ,60).performed! + limiter.performed! rescue RateLimiter::LimitExceeded raise Discourse::InvalidAccess end diff --git a/spec/components/auth/default_current_user_provider_spec.rb b/spec/components/auth/default_current_user_provider_spec.rb index 3fecd09e56..dd69a375b3 100644 --- a/spec/components/auth/default_current_user_provider_spec.rb +++ b/spec/components/auth/default_current_user_provider_spec.rb @@ -86,6 +86,10 @@ describe Auth::DefaultCurrentUserProvider do end it "can only try 10 bad cookies a minute" do + + user = Fabricate(:user) + provider('/').log_on_user(user, {}, {}) + RateLimiter.stubs(:disabled?).returns(false) RateLimiter.new(nil, "cookie_auth_10.0.0.1", 10, 60).clear! @@ -97,10 +101,16 @@ describe Auth::DefaultCurrentUserProvider do 10.times do provider('/', env).current_user end + expect { provider('/', env).current_user }.to raise_error(Discourse::InvalidAccess) + expect { + env["HTTP_COOKIE"] = "_t=#{user.auth_token}" + provider("/", env).current_user + }.to raise_error(Discourse::InvalidAccess) + env["REMOTE_ADDR"] = "10.0.0.2" provider('/', env).current_user end From 73b6a22f615f965a93f5dcf863305096911b4973 Mon Sep 17 00:00:00 2001 From: Sam Date: Tue, 9 Aug 2016 10:12:56 +1000 Subject: [PATCH 22/69] UX: link to PM list from pm suggested topics --- app/assets/javascripts/discourse/controllers/topic.js.es6 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index a21d55ca32..71a8ca8fc1 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -144,7 +144,7 @@ export default Ember.Controller.extend(SelectedPostsCount, BufferedContent, { @computed('model') suggestedTitle(model) { return model.get('isPrivateMessage') ? - ` ${I18n.t("suggested_topics.pm_title")}` : + ` ${I18n.t("suggested_topics.pm_title")}` : I18n.t("suggested_topics.title"); }, From 282f9948cbf17f9e713f2d385e20f0b941fddda4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Tue, 9 Aug 2016 20:14:49 +0200 Subject: [PATCH 23/69] FIX: wasn't able to update category's settings --- lib/validators/upload_url_validator.rb | 2 +- spec/models/category_spec.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/validators/upload_url_validator.rb b/lib/validators/upload_url_validator.rb index 527ad2d4e9..da9c27d1fd 100644 --- a/lib/validators/upload_url_validator.rb +++ b/lib/validators/upload_url_validator.rb @@ -3,7 +3,7 @@ class UploadUrlValidator < ActiveModel::EachValidator if value.present? uri = URI.parse(value) rescue nil - unless uri && Upload.exists?(url: value) + unless uri && Discourse.store.has_been_uploaded?(value) record.errors[attribute] << (options[:message] || I18n.t('errors.messages.invalid')) end end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index d603077ebf..ac5689578a 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -14,19 +14,20 @@ describe Category do context "url validation" do let(:user) { Fabricate(:user) } - let(:upload) { Fabricate(:upload) } it "ensures logo_url is valid" do expect(Fabricate.build(:category, user: user, logo_url: "---%")).not_to be_valid expect(Fabricate.build(:category, user: user, logo_url: "http://example.com/made-up.jpg")).not_to be_valid expect(Fabricate.build(:category, user: user, logo_url: upload.url)).to be_valid + expect(Fabricate.build(:category, user: user, logo_url: UrlHelper.schemaless(UrlHelper.absolute(upload.url)))).to be_valid end it "ensures background_url is valid" do expect(Fabricate.build(:category, user: user, background_url: ";test")).not_to be_valid expect(Fabricate.build(:category, user: user, background_url: "http://example.com/no.jpg")).not_to be_valid expect(Fabricate.build(:category, user: user, background_url: upload.url)).to be_valid + expect(Fabricate.build(:category, user: user, background_url: UrlHelper.schemaless(UrlHelper.absolute(upload.url)))).to be_valid end end From d8808aa9abde8d4dfa883f6b4d5ba92890fe274f Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 9 Aug 2016 12:16:29 -0400 Subject: [PATCH 24/69] Add back acceptance tests for full page search --- .../javascripts/discourse/lib/search.js.es6 | 34 ++++++++++--------- .../acceptance/search-full-test.js.es6 | 25 +++++++++----- .../helpers/create-pretender.js.es6 | 13 +++++++ 3 files changed, 48 insertions(+), 24 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/search.js.es6 b/app/assets/javascripts/discourse/lib/search.js.es6 index b0929ca60a..348fe0f191 100644 --- a/app/assets/javascripts/discourse/lib/search.js.es6 +++ b/app/assets/javascripts/discourse/lib/search.js.es6 @@ -41,24 +41,26 @@ export function translateResults(results, opts) { results.resultTypes = []; // TODO: consider refactoring front end to take a better structure - [['topic','posts'],['user','users'],['category','categories']].forEach(function(pair){ - const type = pair[0], name = pair[1]; - if (results[name].length > 0) { - var result = { - results: results[name], - componentName: "search-result-" + ((opts.searchContext && opts.searchContext.type === 'topic' && type === 'topic') ? 'post' : type), - type, - more: r['more_' + name] - }; + if (r) { + [['topic','posts'],['user','users'],['category','categories']].forEach(function(pair){ + const type = pair[0], name = pair[1]; + if (results[name].length > 0) { + var result = { + results: results[name], + componentName: "search-result-" + ((opts.searchContext && opts.searchContext.type === 'topic' && type === 'topic') ? 'post' : type), + type, + more: r['more_' + name] + }; - if (result.more && name === "posts" && opts.fullSearchUrl) { - result.more = false; - result.moreUrl = opts.fullSearchUrl; + if (result.more && name === "posts" && opts.fullSearchUrl) { + result.more = false; + result.moreUrl = opts.fullSearchUrl; + } + + results.resultTypes.push(result); } - - results.resultTypes.push(result); - } - }); + }); + } const noResults = !!(results.topics.length === 0 && results.posts.length === 0 && diff --git a/test/javascripts/acceptance/search-full-test.js.es6 b/test/javascripts/acceptance/search-full-test.js.es6 index 96320ff0ce..005ea036f3 100644 --- a/test/javascripts/acceptance/search-full-test.js.es6 +++ b/test/javascripts/acceptance/search-full-test.js.es6 @@ -1,12 +1,21 @@ import { acceptance } from "helpers/qunit-helpers"; acceptance("Search - Full Page"); -// TODO: needs fixing (cc @sam) -// test("search", (assert) => { -// visit("/search?q=trout"); +test("perform various searches", assert => { + visit("/search"); -// andThen(() => { -// assert.ok(find('input.search').length > 0); -// assert.ok(find('.topic-list-item').length > 0); -// }); -// }); + andThen(() => { + assert.ok(find('input.search').length > 0); + assert.ok(find('.topic').length === 0); + }); + + fillIn('.search input', 'none'); + click('.search .btn-primary'); + + andThen(() => assert.ok(find('.topic').length === 0), 'has no results'); + + fillIn('.search input', 'posts'); + click('.search .btn-primary'); + + andThen(() => assert.ok(find('.topic').length === 1, 'has one post')); +}); diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index fc2c537c1a..b1e340058d 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -78,6 +78,19 @@ export default function() { this.get('/clicks/track', success); + this.get('/search', request => { + if (request.queryParams.q === 'posts') { + return response({ + posts: [{ + id: 1234 + }] + }); + } + + return response({}); + }); + + this.put('/users/eviltrout', () => response({ user: {} })); this.get("/t/280.json", () => response(fixturesByUrl['/t/280/1.json'])); From b2134aa1731fd0346969969cac55b56a7a603253 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 9 Aug 2016 13:22:14 -0400 Subject: [PATCH 25/69] Refactor full page search for style, remove lookups --- .../controllers/full-page-search.js.es6 | 36 ++++++--- .../initializers/page-tracking.js.es6 | 17 ----- .../discourse/lib/page-tracker.js.es6 | 21 +++++- .../discourse/routes/full-page-search.js.es6 | 8 +- .../discourse/templates/full-page-search.hbs | 74 +++++++++---------- .../acceptance/search-full-test.js.es6 | 6 +- 6 files changed, 85 insertions(+), 77 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 b/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 index 27ee3f0309..b2a5aeaeca 100644 --- a/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 +++ b/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 @@ -4,6 +4,7 @@ import showModal from 'discourse/lib/show-modal'; import { default as computed, observes } from 'ember-addons/ember-computed-decorators'; import Category from 'discourse/models/category'; import { escapeExpression } from 'discourse/lib/utilities'; +import { setTransient } from 'discourse/lib/page-tracker'; const SortOrders = [ {name: I18n.t('search.relevance'), id: 0}, @@ -14,6 +15,7 @@ const SortOrders = [ export default Ember.Controller.extend({ needs: ["application"], + bulkSelectEnabled: null, loading: Em.computed.not("model"), queryParams: ["q", "context_id", "context", "skip_context"], @@ -26,10 +28,15 @@ export default Ember.Controller.extend({ sortOrders: SortOrders, @computed('model.posts') - resultCount(posts){ + resultCount(posts) { return posts && posts.length; }, + @computed('resultCount') + hasResults(resultCount) { + return (resultCount || 0) > 0; + }, + @computed('q') hasAutofocus(q) { return Em.isEmpty(q); @@ -85,7 +92,7 @@ export default Ember.Controller.extend({ setSearchTerm(term) { this._searchOnSortChange = false; if (term) { - SortOrders.forEach((order) => { + SortOrders.forEach(order => { if (term.indexOf(order.term) > -1){ this.set('sortOrder', order.id); term = term.replace(order.term, ""); @@ -98,7 +105,7 @@ export default Ember.Controller.extend({ }, @observes('sortOrder') - triggerSearch(){ + triggerSearch() { if (this._searchOnSortChange) { this.search(); } @@ -130,13 +137,22 @@ export default Ember.Controller.extend({ this.set("controllers.application.showFooter", !this.get("loading")); }, - canBulkSelect: Em.computed.alias('currentUser.staff'), + @computed('hasResults') + canBulkSelect(hasResults) { + return this.currentUser && this.currentUser.staff && hasResults; + }, + + @computed + canCreateTopic() { + return this.currentUser && !this.site.mobileView; + }, search(){ if (this.get("searching")) return; this.set("searching", true); - const router = Discourse.__container__.lookup('router:main'); + this.set('bulkSelectEnabled', false); + this.get('selected').clear(); var args = { q: this.get("searchTerm") }; @@ -160,9 +176,9 @@ export default Ember.Controller.extend({ ajax("/search", { data: args }).then(results => { const model = translateResults(results) || {}; - router.transientCache('lastSearch', { searchKey, model }, 5); + setTransient('lastSearch', { searchKey, model }, 5); this.set("model", model); - }).finally(() => { this.set("searching",false); }); + }).finally(() => this.set("searching", false)); }, actions: { @@ -188,16 +204,12 @@ export default Ember.Controller.extend({ }, refresh() { - this.set('bulkSelectEnabled', false); - this.get('selected').clear(); this.search(); }, showSearchHelp() { // TODO: dupe code should be centralized - ajax("/static/search_help.html", { dataType: 'html' }).then((model) => { - showModal('searchHelp', { model }); - }); + ajax("/static/search_help.html", { dataType: 'html' }).then(model => showModal('searchHelp', { model })); }, search() { diff --git a/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 b/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 index 860ddaf0f8..7e744a7de9 100644 --- a/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 +++ b/app/assets/javascripts/discourse/initializers/page-tracking.js.es6 @@ -7,31 +7,14 @@ export default { initialize(container) { - const cache = {}; - var transitionCount = 0; - // Tell our AJAX system to track a page transition const router = container.lookup('router:main'); router.on('willTransition', viewTrackingRequired); router.on('didTransition', function() { Em.run.scheduleOnce('afterRender', Ember.Route, cleanDOM); - transitionCount++; - _.each(cache, (v,k) => { - if (v && v.target && v.target < transitionCount) { - delete cache[k]; - } - }); }); - router.transientCache = function(key, data, count) { - if (data === undefined) { - return cache[key]; - } else { - return cache[key] = {data, target: transitionCount + count}; - } - }; - startPageTracking(router); // Out of the box, Discourse tries to track google analytics diff --git a/app/assets/javascripts/discourse/lib/page-tracker.js.es6 b/app/assets/javascripts/discourse/lib/page-tracker.js.es6 index 7e63b5c731..89deaafe03 100644 --- a/app/assets/javascripts/discourse/lib/page-tracker.js.es6 +++ b/app/assets/javascripts/discourse/lib/page-tracker.js.es6 @@ -2,6 +2,18 @@ const PageTracker = Ember.Object.extend(Ember.Evented); let _pageTracker = PageTracker.create(); let _started = false; + +const cache = {}; +let transitionCount = 0; + +export function setTransient(key, data, count) { + cache[key] = {data, target: transitionCount + count}; +} + +export function getTransient(key) { + return cache[key]; +} + export function startPageTracking(router) { if (_started) { return; } @@ -11,8 +23,13 @@ export function startPageTracking(router) { // Refreshing the title is debounced, so we need to trigger this in the // next runloop to have the correct title. - Em.run.next(() => { - _pageTracker.trigger('change', url, Discourse.get('_docTitle')); + Em.run.next(() => _pageTracker.trigger('change', url, Discourse.get('_docTitle'))); + + transitionCount++; + _.each(cache, (v,k) => { + if (v && v.target && v.target < transitionCount) { + delete cache[k]; + } }); }); _started = true; diff --git a/app/assets/javascripts/discourse/routes/full-page-search.js.es6 b/app/assets/javascripts/discourse/routes/full-page-search.js.es6 index 8507c0dce4..10cd876555 100644 --- a/app/assets/javascripts/discourse/routes/full-page-search.js.es6 +++ b/app/assets/javascripts/discourse/routes/full-page-search.js.es6 @@ -2,13 +2,13 @@ import { ajax } from 'discourse/lib/ajax'; import { translateResults, getSearchKey, isValidSearchTerm } from "discourse/lib/search"; import Composer from 'discourse/models/composer'; import PreloadStore from 'preload-store'; +import { getTransient, setTransient } from 'discourse/lib/page-tracker'; export default Discourse.Route.extend({ queryParams: { q: {}, context_id: {}, context: {}, skip_context: {} }, model(params) { - const router = Discourse.__container__.lookup('router:main'); - var cached = router.transientCache('lastSearch'); + const cached = getTransient('lastSearch'); var args = { q: params.q }; if (params.context_id && !args.skip_context) { args.search_context = { @@ -21,7 +21,7 @@ export default Discourse.Route.extend({ if (cached && cached.data.searchKey === searchKey) { // extend expiry - router.transientCache('lastSearch', { searchKey, model: cached.data.model }, 5); + setTransient('lastSearch', { searchKey, model: cached.data.model }, 5); return cached.data.model; } @@ -33,7 +33,7 @@ export default Discourse.Route.extend({ } }).then(results => { const model = (results && translateResults(results)) || {}; - router.transientCache('lastSearch', { searchKey, model }, 5); + setTransient('lastSearch', { searchKey, model }, 5); return model; }); }, diff --git a/app/assets/javascripts/discourse/templates/full-page-search.hbs b/app/assets/javascripts/discourse/templates/full-page-search.hbs index b869f0a724..d440956315 100644 --- a/app/assets/javascripts/discourse/templates/full-page-search.hbs +++ b/app/assets/javascripts/discourse/templates/full-page-search.hbs @@ -1,61 +1,57 @@
{{search-text-field value=searchTerm class="full-page-search input-xxlarge search no-blur" action="search" hasAutofocus=hasAutofocus}} {{d-button action="search" icon="search" class="btn-primary" disabled=searchButtonDisabled}} - {{#if currentUser}} - {{#unless site.mobileView}} - {{d-button id="create-topic" class="btn-default" action="createTopic" actionParam=searchTerm icon="plus" label="topic.create"}} - {{/unless}} + + {{#if canCreateTopic}} + {{d-button id="create-topic" class="btn-default" action="createTopic" actionParam=searchTerm icon="plus" label="topic.create"}} {{/if}} + {{#if canBulkSelect}} - {{#if model.posts}} - {{d-button icon="list" class="bulk-select" title="topics.bulk.toggle" action="toggleBulkSelect"}} - {{bulk-select-button selected=selected action="refresh"}} - {{/if}} + {{d-button icon="list" class="bulk-select" title="topics.bulk.toggle" action="toggleBulkSelect"}} + {{bulk-select-button selected=selected action="refresh"}} {{/if}}
-{{#if model.posts}} - {{#if bulkSelectEnabled}} +{{#if bulkSelectEnabled}}
- {{i18n "search.select_all"}} - {{i18n "search.clear_all"}} + {{d-link action="selectAll" label="search.select_all"}} + {{d-link action="clearAll" label="search.clear_all"}}
- {{/if}} {{/if}} {{#if context}} -
- -
+
+ +
{{/if}} {{#conditional-loading-spinner condition=loading}} - {{#unless model.posts}} + {{#unless hasResults}}

{{#if searchActive}} - {{i18n "search.no_results"}} + {{i18n "search.no_results"}} {{/if}} {{i18n "search.search_help"}}

{{/unless}} - {{#if model.posts}} -
-
- - {{{i18n "search.result_count" count=resultCount term=noSortQ}}} - + {{#if hasResults}} +
+
+ + {{{i18n "search.result_count" count=resultCount term=noSortQ}}} + +
+
+ + {{i18n "search.sort_by"}} + + {{combo-box value=sortOrder content=sortOrders castInteger="true"}} +
-
- - {{i18n "search.sort_by"}} - - {{combo-box value=sortOrder content=sortOrders castInteger="true"}} -
-
{{/if}} {{#each model.posts as |result|}} @@ -101,7 +97,7 @@ {{#if showLikeCount}} {{#if result.like_count}} {{/if}} {{/if}} @@ -109,11 +105,11 @@
{{/each}} - {{#if model.posts}} -

- {{i18n "search.no_more_results"}} - {{i18n "search.search_help"}} -

+ {{#if hasResults}} +

+ {{i18n "search.no_more_results"}} + {{i18n "search.search_help"}} +

{{/if}} {{/conditional-loading-spinner}} diff --git a/test/javascripts/acceptance/search-full-test.js.es6 b/test/javascripts/acceptance/search-full-test.js.es6 index 005ea036f3..889b066ee6 100644 --- a/test/javascripts/acceptance/search-full-test.js.es6 +++ b/test/javascripts/acceptance/search-full-test.js.es6 @@ -6,16 +6,16 @@ test("perform various searches", assert => { andThen(() => { assert.ok(find('input.search').length > 0); - assert.ok(find('.topic').length === 0); + assert.ok(find('.fps-topic').length === 0); }); fillIn('.search input', 'none'); click('.search .btn-primary'); - andThen(() => assert.ok(find('.topic').length === 0), 'has no results'); + andThen(() => assert.ok(find('.fps-topic').length === 0), 'has no results'); fillIn('.search input', 'posts'); click('.search .btn-primary'); - andThen(() => assert.ok(find('.topic').length === 1, 'has one post')); + andThen(() => assert.ok(find('.fps-topic').length === 1, 'has one post')); }); From c1125c8649610a72e179416fae17d54104ebfef8 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 9 Aug 2016 13:41:25 -0400 Subject: [PATCH 26/69] PERF: Use simpler serializer for search, eager load post users --- app/serializers/search_post_serializer.rb | 5 ++--- lib/search.rb | 5 ++++- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/serializers/search_post_serializer.rb b/app/serializers/search_post_serializer.rb index 1e92d044be..098aa60dff 100644 --- a/app/serializers/search_post_serializer.rb +++ b/app/serializers/search_post_serializer.rb @@ -1,9 +1,8 @@ -class SearchPostSerializer < PostSerializer +class SearchPostSerializer < BasicPostSerializer has_one :topic, serializer: TopicListItemSerializer - attributes :like_count + attributes :like_count, :blurb - attributes :blurb def blurb options[:result].blurb(object) end diff --git a/lib/search.rb b/lib/search.rb index 4cb1711c4e..84b29700b9 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -663,6 +663,7 @@ class Search post_sql = "SELECT *, row_number() over() row_number FROM (#{post_sql}) xxx" posts = Post.includes(:topic => :category) + .includes(:user) .joins("JOIN (#{post_sql}) x ON x.id = posts.topic_id AND x.post_number = posts.post_number") .order('row_number') @@ -679,7 +680,9 @@ class Search def topic_search if @search_context.is_a?(Topic) - posts = posts_query(@limit).where('posts.topic_id = ?', @search_context.id).includes(:topic => :category) + posts = posts_query(@limit).where('posts.topic_id = ?', @search_context.id) + .includes(:topic => :category) + .includes(:user) posts.each do |post| @results.add(post) end From 28436a604a65081f81ae8abfe0fc068e9d24f29c Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 9 Aug 2016 14:48:39 -0400 Subject: [PATCH 27/69] FIX: Prevent tricking the search from ignoring minimum lengths --- .../javascripts/discourse/lib/search.js.es6 | 10 +++---- lib/search.rb | 20 ++++++++++++-- spec/components/search_spec.rb | 27 +++++++++++++++++-- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/search.js.es6 b/app/assets/javascripts/discourse/lib/search.js.es6 index 348fe0f191..0598515b22 100644 --- a/app/assets/javascripts/discourse/lib/search.js.es6 +++ b/app/assets/javascripts/discourse/lib/search.js.es6 @@ -70,7 +70,7 @@ export function translateResults(results, opts) { return noResults ? null : Em.Object.create(results); } -function searchForTerm(term, opts) { +export function searchForTerm(term, opts) { if (!opts) opts = {}; // Only include the data we have @@ -94,7 +94,7 @@ function searchForTerm(term, opts) { return promise; } -const searchContextDescription = function(type, name){ +export function searchContextDescription(type, name) { if (type) { switch(type) { case 'topic': @@ -109,17 +109,15 @@ const searchContextDescription = function(type, name){ } }; -const getSearchKey = function(args){ +export function getSearchKey(args) { return args.q + "|" + ((args.searchContext && args.searchContext.type) || "") + "|" + ((args.searchContext && args.searchContext.id) || ""); }; -const isValidSearchTerm = function(searchTerm) { +export function isValidSearchTerm(searchTerm) { if (searchTerm) { return searchTerm.trim().length >= Discourse.SiteSettings.min_search_term_length; } else { return false; } }; - -export { searchForTerm, searchContextDescription, getSearchKey, isValidSearchTerm }; diff --git a/lib/search.rb b/lib/search.rb index 84b29700b9..38024cfcde 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -128,6 +128,8 @@ class Search end end + attr_accessor :term + def initialize(term, opts=nil) @opts = opts || {} @guardian = @opts[:guardian] || Guardian.new @@ -135,6 +137,7 @@ class Search @include_blurbs = @opts[:include_blurbs] || false @blurb_length = @opts[:blurb_length] @limit = Search.per_facet + @valid = true term = process_advanced_search!(term) @@ -155,14 +158,27 @@ class Search @results = GroupedSearchResults.new(@opts[:type_filter], term, @search_context, @include_blurbs, @blurb_length) end + def valid? + @valid + end + def self.execute(term, opts=nil) self.new(term, opts).execute end # Query a term def execute - if @term.blank? || @term.length < (@opts[:min_search_term_length] || SiteSetting.min_search_term_length) - return nil unless @filters.present? + + unless @filters.present? + min_length = @opts[:min_search_term_length] || SiteSetting.min_search_term_length + terms = (@term || '').split(/\s(?=(?:[^"]|"[^"]*")*$)/).reject {|t| t.length < min_length } + + if terms.blank? + @term = '' + @valid = false + return + end + @term = terms.join(' ') end # If the term is a number or url to a topic, just include that topic diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index 91a88719f5..f7dac71670 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -61,8 +61,31 @@ describe Search do end it 'does not search when the search term is too small' do - ActiveRecord::Base.expects(:exec_sql).never - Search.execute('evil', min_search_term_length: 5) + search = Search.new('evil', min_search_term_length: 5) + search.execute + expect(search.valid?).to eq(false) + expect(search.term).to eq('') + end + + it 'does not search for multiple small terms' do + search = Search.new('a b c d', min_search_term_length: 5) + search.execute + expect(search.valid?).to eq(false) + expect(search.term).to eq('') + end + + it 'searches for quoted short terms' do + search = Search.new('"a b c d"', min_search_term_length: 5) + search.execute + expect(search.valid?).to eq(true) + expect(search.term).to eq('"a b c d"') + end + + it 'discards short terms' do + search = Search.new('a b c okaylength', min_search_term_length: 5) + search.execute + expect(search.valid?).to eq(true) + expect(search.term).to eq('okaylength') end it 'escapes non alphanumeric characters' do From fd3a8583dd5c5f18c3f91839555744fa251586a6 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Tue, 9 Aug 2016 15:11:58 -0400 Subject: [PATCH 28/69] UX: Display a message when the search term is too short on full page --- .../controllers/full-page-search.js.es6 | 30 +++++++++---------- .../discourse/templates/full-page-search.hbs | 10 +++++-- .../stylesheets/common/base/search.scss | 4 +++ config/locales/client.en.yml | 1 + 4 files changed, 27 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 b/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 index b2a5aeaeca..b33f338f6f 100644 --- a/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 +++ b/app/assets/javascripts/discourse/controllers/full-page-search.js.es6 @@ -26,6 +26,7 @@ export default Ember.Controller.extend({ searching: false, sortOrder: 0, sortOrders: SortOrders, + invalidSearch: false, @computed('model.posts') resultCount(posts) { @@ -69,11 +70,6 @@ export default Ember.Controller.extend({ return isValidSearchTerm(q); }, - @computed('searchTerm', 'searching') - searchButtonDisabled(searchTerm, searching) { - return !!(searching || !isValidSearchTerm(searchTerm)); - }, - @computed('q') noSortQ(q) { if (q) { @@ -107,7 +103,7 @@ export default Ember.Controller.extend({ @observes('sortOrder') triggerSearch() { if (this._searchOnSortChange) { - this.search(); + this._search(); } }, @@ -147,14 +143,21 @@ export default Ember.Controller.extend({ return this.currentUser && !this.site.mobileView; }, - search(){ - if (this.get("searching")) return; - this.set("searching", true); + _search() { + if (this.get("searching")) { return; } + this.set('invalidSearch', false); + const searchTerm = this.get('searchTerm'); + if (!isValidSearchTerm(searchTerm)) { + this.set('invalidSearch', true); + return; + } + + this.set("searching", true); this.set('bulkSelectEnabled', false); this.get('selected').clear(); - var args = { q: this.get("searchTerm") }; + var args = { q: searchTerm }; const sortOrder = this.get("sortOrder"); if (sortOrder && SortOrders[sortOrder].term) { @@ -203,18 +206,13 @@ export default Ember.Controller.extend({ this.get('selected').clear(); }, - refresh() { - this.search(); - }, - showSearchHelp() { // TODO: dupe code should be centralized ajax("/static/search_help.html", { dataType: 'html' }).then(model => showModal('searchHelp', { model })); }, search() { - if (this.get("searchButtonDisabled")) return; - this.search(); + this._search(); } } }); diff --git a/app/assets/javascripts/discourse/templates/full-page-search.hbs b/app/assets/javascripts/discourse/templates/full-page-search.hbs index d440956315..5a57cfa87b 100644 --- a/app/assets/javascripts/discourse/templates/full-page-search.hbs +++ b/app/assets/javascripts/discourse/templates/full-page-search.hbs @@ -1,6 +1,6 @@
{{search-text-field value=searchTerm class="full-page-search input-xxlarge search no-blur" action="search" hasAutofocus=hasAutofocus}} - {{d-button action="search" icon="search" class="btn-primary" disabled=searchButtonDisabled}} + {{d-button action="search" icon="search" class="btn-primary" disabled=searching}} {{#if canCreateTopic}} {{d-button id="create-topic" class="btn-default" action="createTopic" actionParam=searchTerm icon="plus" label="topic.create"}} @@ -8,7 +8,7 @@ {{#if canBulkSelect}} {{d-button icon="list" class="bulk-select" title="topics.bulk.toggle" action="toggleBulkSelect"}} - {{bulk-select-button selected=selected action="refresh"}} + {{bulk-select-button selected=selected action="search"}} {{/if}}
@@ -19,6 +19,12 @@ {{/if}} +{{#if invalidSearch}} +
+ {{i18n "search.too_short"}} +
+{{/if}} + {{#if context}}