From 6ea811c923c2d96e4d71332c300422653909d890 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Wed, 8 May 2019 07:54:21 +0200 Subject: [PATCH 1/9] Revert "PERF: Skip compressing locales for faster rebuilds (#7501)" (#7502) --- config/environments/production.rb | 3 +-- lib/tasks/assets.rake | 29 ++++++----------------------- 2 files changed, 7 insertions(+), 25 deletions(-) diff --git a/config/environments/production.rb b/config/environments/production.rb index fd344fab4c4..c12ed4eaa85 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -12,8 +12,7 @@ Discourse::Application.configure do # Disable Rails's static asset server (Apache or nginx will already do this) config.public_file_server.enabled = GlobalSetting.serve_static_assets || false - require 'uglifier' - config.assets.js_compressor = Uglifier.new(harmony: true) + config.assets.js_compressor = :uglifier # stuff should be pre-compiled config.assets.compile = false diff --git a/lib/tasks/assets.rake b/lib/tasks/assets.rake index 47ee8f545ac..fe374482dd0 100644 --- a/lib/tasks/assets.rake +++ b/lib/tasks/assets.rake @@ -16,7 +16,7 @@ task 'assets:precompile:before' do # is recompiled Emoji.clear_cache - if !`which uglifyjs`.empty? && !ENV['SKIP_NODE_UGLIFY'] + if Rails.configuration.assets.js_compressor == :uglifier && !`which uglifyjs`.empty? && !ENV['SKIP_NODE_UGLIFY'] $node_uglify = true end @@ -79,7 +79,7 @@ def compress_node(from, to) source_map_root = assets + ((d = File.dirname(from)) == "." ? "" : "/#{d}") source_map_url = cdn_path "/assets/#{to}.map" - cmd = "uglifyjs '#{assets_path}/#{from}' -p relative -m -c -o '#{to_path}' --source-map-root '#{source_map_root}' --source-map '#{assets_path}/#{to}.map' --source-map-url '#{source_map_url}'" + cmd = "uglifyjs '#{assets_path}/#{from}' -p relative -c -m -o '#{to_path}' --source-map-root '#{source_map_root}' --source-map '#{assets_path}/#{to}.map' --source-map-url '#{source_map_url}'" STDERR.puts cmd result = `#{cmd} 2>&1` @@ -128,16 +128,6 @@ def brotli(path) raise "chmod failed: exit code #{$?.exitstatus}" if $?.exitstatus != 0 end -def should_compress?(path, locales) - return false if Rails.configuration.assets.skip_minification.include? path - return true unless path.include? "locales/" - - path_locale = path.delete_prefix("locales/").delete_suffix(".js") - return true if locales.include? path_locale - - false -end - def compress(from, to) if $node_uglify compress_node(from, to) @@ -173,15 +163,10 @@ task 'assets:precompile' => 'assets:precompile:before' do if $bypass_sprockets_uglify puts "Compressing Javascript and Generating Source Maps" - startAll = Process.clock_gettime(Process::CLOCK_MONOTONIC) manifest = Sprockets::Manifest.new(assets_path) - locales = Set.new(["en"]) - - RailsMultisite::ConnectionManagement.each_connection do |db| - locales.add(SiteSetting.default_locale) - end concurrent? do |proc| + to_skip = Rails.configuration.assets.skip_minification || [] manifest.files .select { |k, v| k =~ /\.js$/ } .each do |file, info| @@ -197,7 +182,8 @@ task 'assets:precompile' => 'assets:precompile:before' do start = Process.clock_gettime(Process::CLOCK_MONOTONIC) STDERR.puts "#{start} Compressing: #{file}" - if should_compress?(info["logical_path"], locales) + # We can specify some files to never minify + unless (ENV["DONT_MINIFY"] == "1") || to_skip.include?(info['logical_path']) FileUtils.mv(path, _path) compress(_file, file) end @@ -205,7 +191,7 @@ task 'assets:precompile' => 'assets:precompile:before' do info["size"] = File.size(path) info["mtime"] = File.mtime(path).iso8601 gzip(path) - brotli(path) if should_compress?(info["logical_path"], locales) + brotli(path) STDERR.puts "Done compressing #{file} : #{(Process.clock_gettime(Process::CLOCK_MONOTONIC) - start).round(2)} secs" STDERR.puts @@ -214,9 +200,6 @@ task 'assets:precompile' => 'assets:precompile:before' do end end - STDERR.puts "Done compressing all JS files : #{(Process.clock_gettime(Process::CLOCK_MONOTONIC) - startAll).round(2)} secs" - STDERR.puts - # protected manifest.send :save From 46ea69a73c64303991cb812d742396a4f8e4ca4e Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 8 May 2019 14:15:13 +0800 Subject: [PATCH 2/9] UX: Initialize client side category model with right search_priority. This is strictly a UX change since the default value of the column is 0. --- .../javascripts/discourse/routes/discovery-categories.js.es6 | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 b/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 index 2b77d5bc25b..2b390dab919 100644 --- a/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 +++ b/app/assets/javascripts/discourse/routes/discovery-categories.js.es6 @@ -5,6 +5,7 @@ import { defaultHomepage } from "discourse/lib/utilities"; import TopicList from "discourse/models/topic-list"; import { ajax } from "discourse/lib/ajax"; import PreloadStore from "preload-store"; +import { searchPriorities } from "discourse/components/concerns/category_search_priorities"; const DiscoveryCategoriesRoute = Discourse.Route.extend(OpenComposer, { renderTemplate() { @@ -109,7 +110,8 @@ const DiscoveryCategoriesRoute = Discourse.Route.extend(OpenComposer, { available_groups: groups.map(g => g.name), allow_badges: true, topic_featured_link_allowed: true, - custom_fields: {} + custom_fields: {}, + search_priority: searchPriorities.normal }); showModal("edit-category", { model }); From 405ba00c08a86778a7baf28e93388201162a5347 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 8 May 2019 15:18:56 +0800 Subject: [PATCH 3/9] FEATURE: Create notifications on wiki edits for watching users. * Moves creation of notification into background job. --- app/jobs/regular/notify_post_revision.rb | 20 ++++++++ app/models/topic_user.rb | 14 ++++-- app/services/post_action_notifier.rb | 28 +++++++---- notify_post_revision.rb | 20 ++++++++ spec/services/post_action_notifier_spec.rb | 58 +++++++++++++++++++++- 5 files changed, 127 insertions(+), 13 deletions(-) create mode 100644 app/jobs/regular/notify_post_revision.rb create mode 100644 notify_post_revision.rb diff --git a/app/jobs/regular/notify_post_revision.rb b/app/jobs/regular/notify_post_revision.rb new file mode 100644 index 00000000000..d7e38a841da --- /dev/null +++ b/app/jobs/regular/notify_post_revision.rb @@ -0,0 +1,20 @@ +module Jobs + class NotifyPostRevision < Jobs::Base + def execute(args) + user = User.find_by(id: args[:user_id]) + raise Discourse::InvalidParameters.new(:user_id) unless user + + post_revision = PostRevision.find_by(id: args[:post_revision_id]) + raise Discourse::InvalidParameters.new(:post_revision_id) unless post_revision + + PostActionNotifier.alerter.create_notification( + user, + Notification.types[:edited], + post_revision.post, + display_username: post_revision.user.username, + acting_user_id: post_revision.try(:user_id), + revision_number: post_revision.number + ) + end + end +end diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index 8122286406b..8a7125340de 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -7,11 +7,19 @@ class TopicUser < ActiveRecord::Base # used for serialization attr_accessor :post_action_data - scope :tracking, lambda { |topic_id| + scope :level, lambda { |topic_id, level| where(topic_id: topic_id) - .where("COALESCE(topic_users.notification_level, :regular) >= :tracking", + .where("COALESCE(topic_users.notification_level, :regular) >= :level", regular: TopicUser.notification_levels[:regular], - tracking: TopicUser.notification_levels[:tracking]) + level: TopicUser.notification_levels[level]) + } + + scope :tracking, lambda { |topic_id| + level(topic_id, :tracking) + } + + scope :watching, lambda { |topic_id| + level(topic_id, :watching) } # Class methods diff --git a/app/services/post_action_notifier.rb b/app/services/post_action_notifier.rb index c4d988f25dc..9d8bb923f45 100644 --- a/app/services/post_action_notifier.rb +++ b/app/services/post_action_notifier.rb @@ -93,19 +93,29 @@ class PostActionNotifier return unless post return if post_revision.user.blank? - return if post_revision.user_id == post.user_id return if post.topic.blank? return if post.topic.private_message? return if SiteSetting.disable_edit_notifications && post_revision.user_id == Discourse::SYSTEM_USER_ID - alerter.create_notification( - post.user, - Notification.types[:edited], - post, - display_username: post_revision.user.username, - acting_user_id: post_revision.try(:user_id), - revision_number: post_revision.number - ) + if post_revision.user_id != post.user_id + Jobs.enqueue(:notify_post_revision, + user_id: post.user_id, + post_revision_id: post_revision.id + ) + end + + if post.wiki && post.is_first_post? + TopicUser.watching(post.topic_id) + .where.not(user_id: post_revision.user_id) + .where(topic: post.topic) + .find_each do |topic_user| + + Jobs.enqueue(:notify_post_revision, + user_id: topic_user.user_id, + post_revision_id: post_revision.id + ) + end + end end def self.after_post_unhide(post, flaggers) diff --git a/notify_post_revision.rb b/notify_post_revision.rb new file mode 100644 index 00000000000..d94d193533c --- /dev/null +++ b/notify_post_revision.rb @@ -0,0 +1,20 @@ +module Jobs + class NotifyPostRevision < Jobs::Base + def execute(args) + user = User.find_by(id: args[:user_id]) + raise Discourse::InvalidParameters.new(:user_id) unless user + + post_revision = PostRevision.find_by(id: args[:post_revision_id]) + raise Discourse::InvalidParameters.new(:post_revision_id) unless post_revision + + PostActionNotifier.alerter.create_notification( + user, + Notification.types[:edited], + post_revision.post, + display_username: post_revision.user.username, + acting_user_id: post_revision&.user_id, + revision_number: post_revision.number + ) + end + end +end diff --git a/spec/services/post_action_notifier_spec.rb b/spec/services/post_action_notifier_spec.rb index 845ab88313e..592372c2c5e 100644 --- a/spec/services/post_action_notifier_spec.rb +++ b/spec/services/post_action_notifier_spec.rb @@ -6,6 +6,7 @@ describe PostActionNotifier do before do PostActionNotifier.enable + Jobs.run_immediately! end fab!(:evil_trout) { Fabricate(:evil_trout) } @@ -15,7 +16,62 @@ describe PostActionNotifier do it 'notifies a user of the revision' do expect { post.revise(evil_trout, raw: "world is the new body of the message") - }.to change(post.user.notifications, :count).by(1) + }.to change { post.reload.user.notifications.count }.by(1) + end + + it 'notifies watching users of revision when post is wiki-ed and first post in topic' do + SiteSetting.editing_grace_period_max_diff = 1 + + post.update!(wiki: true) + user = post.user + user2 = Fabricate(:user) + user3 = Fabricate(:user) + + TopicUser.change(user2.id, post.topic, + notification_level: TopicUser.notification_levels[:watching] + ) + + TopicUser.change(user3.id, post.topic, + notification_level: TopicUser.notification_levels[:tracking] + ) + + expect do + post.revise(Fabricate(:user), raw: "I made some changes to the wiki!") + end.to change { Notification.count }.by(2) + + edited_notification_type = Notification.types[:edited] + + expect(Notification.exists?( + user: user, + notification_type: edited_notification_type + )).to eq(true) + + expect(Notification.exists?( + user: user2, + notification_type: edited_notification_type + )).to eq(true) + + expect do + post.revise(user, raw: "I made some changes to the wiki again!") + end.to change { + Notification.where(notification_type: edited_notification_type).count + }.by(1) + + expect(Notification.where( + user: user2, + notification_type: edited_notification_type + ).count).to eq(2) + + expect do + post.revise(user2, raw: "I changed the wiki totally") + end.to change { + Notification.where(notification_type: edited_notification_type).count + }.by(1) + + expect(Notification.where( + user: user, + notification_type: edited_notification_type + ).count).to eq(2) end it 'stores the revision number with the notification' do From c72f16d927dcf2b43af003c1260d8de9493d9182 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 8 May 2019 15:36:12 +0800 Subject: [PATCH 4/9] Follow up to 329969ea20296b74b77e8e568e63f635cefa7b3a. --- .../lib/discourse_narrative_bot/actions.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb index ca75d080891..c0832fdeca7 100644 --- a/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb +++ b/plugins/discourse-narrative-bot/lib/discourse_narrative_bot/actions.rb @@ -69,7 +69,7 @@ module DiscourseNarrativeBot valid = false doc.css(".mention").each do |mention| - if mention.text.downcase == "@#{self.discobot_user.username}".downcase + if User.normalize_username(mention.text) == "@#{self.discobot_user.username_lower}" valid = true break end From ba6904bb279fc3b5539df3f24a796bc2efaa564b Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 8 May 2019 15:45:25 +0800 Subject: [PATCH 5/9] Fix broken spec in 405ba00c08a86778a7baf28e93388201162a5347. --- spec/models/user_action_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index 12ecf3d917a..de0e3f1cdc2 100644 --- a/spec/models/user_action_spec.rb +++ b/spec/models/user_action_spec.rb @@ -53,6 +53,7 @@ describe UserAction do end it 'includes the events correctly' do + Jobs.run_immediately! PostActionNotifier.enable mystats = stats_for_user(user) From f04518fdf9e0a2f08ef28cc14fd4916dc43d8dd8 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 8 May 2019 15:58:08 +0800 Subject: [PATCH 6/9] DEV: Reduce number of jobs enqueued. Apply code review suggestion from 405ba00c08a86778a7baf28e93388201162a5347. --- app/jobs/regular/notify_post_revision.rb | 23 ++++++++++--------- app/services/post_action_notifier.rb | 28 +++++++++++++----------- notify_post_revision.rb | 20 ----------------- 3 files changed, 28 insertions(+), 43 deletions(-) delete mode 100644 notify_post_revision.rb diff --git a/app/jobs/regular/notify_post_revision.rb b/app/jobs/regular/notify_post_revision.rb index d7e38a841da..b0b0794685f 100644 --- a/app/jobs/regular/notify_post_revision.rb +++ b/app/jobs/regular/notify_post_revision.rb @@ -1,20 +1,23 @@ module Jobs class NotifyPostRevision < Jobs::Base def execute(args) - user = User.find_by(id: args[:user_id]) - raise Discourse::InvalidParameters.new(:user_id) unless user + raise Discourse::InvalidParameters.new(:user_ids) unless args[:user_ids] post_revision = PostRevision.find_by(id: args[:post_revision_id]) raise Discourse::InvalidParameters.new(:post_revision_id) unless post_revision - PostActionNotifier.alerter.create_notification( - user, - Notification.types[:edited], - post_revision.post, - display_username: post_revision.user.username, - acting_user_id: post_revision.try(:user_id), - revision_number: post_revision.number - ) + ActiveRecord::Base.transaction do + User.where(id: args[:user_ids]).find_each do |user| + PostActionNotifier.alerter.create_notification( + user, + Notification.types[:edited], + post_revision.post, + display_username: post_revision.user.username, + acting_user_id: post_revision&.user_id, + revision_number: post_revision.number + ) + end + end end end end diff --git a/app/services/post_action_notifier.rb b/app/services/post_action_notifier.rb index 9d8bb923f45..3e3fd46fbb9 100644 --- a/app/services/post_action_notifier.rb +++ b/app/services/post_action_notifier.rb @@ -97,24 +97,26 @@ class PostActionNotifier return if post.topic.private_message? return if SiteSetting.disable_edit_notifications && post_revision.user_id == Discourse::SYSTEM_USER_ID + user_ids = [] + if post_revision.user_id != post.user_id - Jobs.enqueue(:notify_post_revision, - user_id: post.user_id, - post_revision_id: post_revision.id - ) + user_ids << post.user_id end if post.wiki && post.is_first_post? - TopicUser.watching(post.topic_id) - .where.not(user_id: post_revision.user_id) - .where(topic: post.topic) - .find_each do |topic_user| + user_ids.concat( + TopicUser.watching(post.topic_id) + .where.not(user_id: post_revision.user_id) + .where(topic: post.topic) + .pluck(:user_id) + ) + end - Jobs.enqueue(:notify_post_revision, - user_id: topic_user.user_id, - post_revision_id: post_revision.id - ) - end + if user_ids.present? + Jobs.enqueue(:notify_post_revision, + user_ids: user_ids, + post_revision_id: post_revision.id + ) end end diff --git a/notify_post_revision.rb b/notify_post_revision.rb deleted file mode 100644 index d94d193533c..00000000000 --- a/notify_post_revision.rb +++ /dev/null @@ -1,20 +0,0 @@ -module Jobs - class NotifyPostRevision < Jobs::Base - def execute(args) - user = User.find_by(id: args[:user_id]) - raise Discourse::InvalidParameters.new(:user_id) unless user - - post_revision = PostRevision.find_by(id: args[:post_revision_id]) - raise Discourse::InvalidParameters.new(:post_revision_id) unless post_revision - - PostActionNotifier.alerter.create_notification( - user, - Notification.types[:edited], - post_revision.post, - display_username: post_revision.user.username, - acting_user_id: post_revision&.user_id, - revision_number: post_revision.number - ) - end - end -end From f5300489735b9d97c0efb706dd0d0906a4407205 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 8 May 2019 16:07:14 +0800 Subject: [PATCH 7/9] Fix broken spec in 405ba00 take 2. --- spec/services/post_alerter_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 4579d2f4d47..c91379d992d 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -102,6 +102,7 @@ describe PostAlerter do context 'edits' do it 'notifies correctly on edits' do + Jobs.run_immediately! PostActionNotifier.enable post = Fabricate(:post, raw: 'I love waffles') From 861023f0d64d1ba26c07d42d24ad55315e50ed1b Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Wed, 8 May 2019 11:46:35 +0300 Subject: [PATCH 8/9] FIX: Skip attachments in click track. --- .../discourse/lib/click-track.js.es6 | 20 +++++++++++-------- test/javascripts/lib/click-track-test.js.es6 | 4 ++-- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/click-track.js.es6 b/app/assets/javascripts/discourse/lib/click-track.js.es6 index d744929fcb6..438915c13ff 100644 --- a/app/assets/javascripts/discourse/lib/click-track.js.es6 +++ b/app/assets/javascripts/discourse/lib/click-track.js.es6 @@ -52,6 +52,11 @@ export default { return true; } + let href = ($link.attr("href") || $link.data("href") || "").trim(); + if (!href || href.indexOf("mailto:") === 0) { + return true; + } + if ($link.hasClass("attachment")) { // Warn the user if they cannot download the file. if ( @@ -59,15 +64,14 @@ export default { !Discourse.User.current() ) { bootbox.alert(I18n.t("post.errors.attachment_download_requires_login")); - return false; + } else if (wantsNewWindow(e)) { + const newWindow = window.open(href, "_blank"); + newWindow.opener = null; + newWindow.focus(); + } else { + DiscourseURL.redirectTo(href); } - - return true; - } - - let href = ($link.attr("href") || $link.data("href") || "").trim(); - if (!href || href.indexOf("mailto:") === 0) { - return true; + return false; } const $article = $link.closest( diff --git a/test/javascripts/lib/click-track-test.js.es6 b/test/javascripts/lib/click-track-test.js.es6 index d69b2332cca..20e41e560fd 100644 --- a/test/javascripts/lib/click-track-test.js.es6 +++ b/test/javascripts/lib/click-track-test.js.es6 @@ -70,13 +70,13 @@ QUnit.test("does not track elements with no href", async assert => { }); QUnit.test("does not track attachments", async assert => { - assert.expect(1); sandbox.stub(DiscourseURL, "origin").returns("http://discuss.domain.com"); /* global server */ server.post("/clicks/track", () => assert.ok(false)); - assert.ok(track(generateClickEventOn(".attachment"))); + assert.notOk(track(generateClickEventOn(".attachment"))); + assert.ok(DiscourseURL.redirectTo.calledWith("http://discuss.domain.com/uploads/default/1234/1532357280.txt")); }); QUnit.test("tracks external URLs", async assert => { From 6efc5dd2024f5dee65b8af2707d92c92dcbcd61e Mon Sep 17 00:00:00 2001 From: Bianca Nenciu Date: Wed, 8 May 2019 12:07:52 +0300 Subject: [PATCH 9/9] DEV: Fix lint. --- test/javascripts/lib/click-track-test.js.es6 | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/javascripts/lib/click-track-test.js.es6 b/test/javascripts/lib/click-track-test.js.es6 index 20e41e560fd..216ed3708eb 100644 --- a/test/javascripts/lib/click-track-test.js.es6 +++ b/test/javascripts/lib/click-track-test.js.es6 @@ -76,7 +76,11 @@ QUnit.test("does not track attachments", async assert => { server.post("/clicks/track", () => assert.ok(false)); assert.notOk(track(generateClickEventOn(".attachment"))); - assert.ok(DiscourseURL.redirectTo.calledWith("http://discuss.domain.com/uploads/default/1234/1532357280.txt")); + assert.ok( + DiscourseURL.redirectTo.calledWith( + "http://discuss.domain.com/uploads/default/1234/1532357280.txt" + ) + ); }); QUnit.test("tracks external URLs", async assert => {