From 5ee9b3cd19d4a13fcec155deb6c01a062d8e7012 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Wed, 18 May 2016 10:33:21 +0800 Subject: [PATCH 01/51] FIX: Don't throw uncaught error warning when in readonly mode. --- app/assets/javascripts/discourse/lib/screen-track.js.es6 | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/assets/javascripts/discourse/lib/screen-track.js.es6 b/app/assets/javascripts/discourse/lib/screen-track.js.es6 index 60704ac4bb..fa52eab6fc 100644 --- a/app/assets/javascripts/discourse/lib/screen-track.js.es6 +++ b/app/assets/javascripts/discourse/lib/screen-track.js.es6 @@ -124,6 +124,9 @@ export default class { const postNumbers = Object.keys(newTimings).map(v => parseInt(v, 10)); controller.readPosts(topicId, postNumbers); } + }).catch(e => { + const error = e.jqXHR; + if (error.status === 405 && error.responseJSON.error_type === "read_only") return; }); } else if (this._anonCallback) { // Anonymous viewer - save to localStorage From 2c2c47fe4e457dcef985510fcc3664b75e7f1a13 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Thu, 19 May 2016 22:16:23 +0200 Subject: [PATCH 02/51] add support for 'command + up/down' to go to first/last post --- app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 index f44bc3c40a..e186bac804 100644 --- a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 +++ b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 @@ -15,6 +15,7 @@ const bindings = { 'd': {postAction: 'deletePost'}, 'e': {postAction: 'editPost'}, 'end': {handler: 'goToLastPost', anonymous: true}, + 'command+down': {handler: 'goToLastPost', anonymous: true}, 'f': {handler: 'toggleBookmarkTopic'}, 'g h': {path: '/', anonymous: true}, 'g l': {path: '/latest', anonymous: true}, @@ -26,6 +27,7 @@ const bindings = { 'g p': {path: '/my/activity'}, 'g m': {path: '/my/messages'}, 'home': {handler: 'goToFirstPost', anonymous: true}, + 'command+up': {handler: 'goToFirstPost', anonymous: true}, 'j': {handler: 'selectDown', anonymous: true}, 'k': {handler: 'selectUp', anonymous: true}, 'l': {click: '.topic-post.selected button.toggle-like'}, From 0cf5a1705a3b498a88212b66211650a2ea7e1ff2 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 20 May 2016 12:35:09 +1000 Subject: [PATCH 03/51] FIX: custom field index must only index short values --- ...160520022627_shorten_topic_custom_fields_index.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 db/migrate/20160520022627_shorten_topic_custom_fields_index.rb diff --git a/db/migrate/20160520022627_shorten_topic_custom_fields_index.rb b/db/migrate/20160520022627_shorten_topic_custom_fields_index.rb new file mode 100644 index 0000000000..8226c7556f --- /dev/null +++ b/db/migrate/20160520022627_shorten_topic_custom_fields_index.rb @@ -0,0 +1,12 @@ +class ShortenTopicCustomFieldsIndex < ActiveRecord::Migration + def up + remove_index :topic_custom_fields, :value + add_index :topic_custom_fields, [:value, :name], + name: 'topic_custom_fields_value_key_idx', + where: 'value IS NOT NULL AND char_length(value) < 400' + end + def down + remove_index :topic_custom_fields, :value, name: 'topic_custom_fields_value_key_idx' + add_index :topic_custom_fields, :value + end +end From 063483400942819287979e4204515cc2d39ae9cf Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Fri, 20 May 2016 15:12:25 +0800 Subject: [PATCH 04/51] Some fixes related to optimized images (#4233) * FIX: No need to manually include relation. * FIX: OR instead of chaining relation. --- app/models/upload.rb | 1 - lib/tasks/avatars.rake | 6 +++--- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/upload.rb b/app/models/upload.rb index 5783a94d2d..dec7de8f2f 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -41,7 +41,6 @@ class Upload < ActiveRecord::Base ) if thumbnail - optimized_images << thumbnail self.width = width self.height = height save(validate: false) diff --git a/lib/tasks/avatars.rake b/lib/tasks/avatars.rake index c27dac792f..4c1da8fd54 100644 --- a/lib/tasks/avatars.rake +++ b/lib/tasks/avatars.rake @@ -21,9 +21,9 @@ task "avatars:clean" => :environment do puts "Cleaning up avatar thumbnails" puts - OptimizedImage.where("upload_id IN (SELECT custom_upload_id FROM user_avatars)") - .where("upload_id IN (SELECT gravatar_upload_id FROM user_avatars)") - .where("upload_id IN (SELECT uploaded_avatar_id FROM users)") + OptimizedImage.where("upload_id IN (SELECT custom_upload_id FROM user_avatars) OR + upload_id IN (SELECT gravatar_upload_id FROM user_avatars) OR + upload_id IN (SELECT uploaded_avatar_id FROM users)") .find_each do |optimized_image| optimized_image.destroy! putc "." if (i += 1) % 10 == 0 From c4f93846f95cb37acbe2fa725614494a2ecbee36 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 20 May 2016 11:35:47 +0200 Subject: [PATCH 05/51] add linebreaks when uploading a file --- app/assets/javascripts/discourse/lib/utilities.js | 2 +- test/javascripts/lib/utilities-test.js.es6 | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/discourse/lib/utilities.js b/app/assets/javascripts/discourse/lib/utilities.js index 16fb66b631..491e2b6a2b 100644 --- a/app/assets/javascripts/discourse/lib/utilities.js +++ b/app/assets/javascripts/discourse/lib/utilities.js @@ -269,7 +269,7 @@ Discourse.Utilities = { // is Audio/Video return Discourse.Utilities.uploadLocation(upload.url); } else { - return '' + upload.original_filename + ' (' + I18n.toHumanSize(upload.filesize) + ')'; + return '' + upload.original_filename + ' (' + I18n.toHumanSize(upload.filesize) + ')\n'; } }, diff --git a/test/javascripts/lib/utilities-test.js.es6 b/test/javascripts/lib/utilities-test.js.es6 index 3cea33cbed..c9f35a948b 100644 --- a/test/javascripts/lib/utilities-test.js.es6 +++ b/test/javascripts/lib/utilities-test.js.es6 @@ -92,7 +92,7 @@ var getUploadMarkdown = function(filename) { test("getUploadMarkdown", function() { ok(getUploadMarkdown("lolcat.gif") === ''); - ok(getUploadMarkdown("important.txt") === 'important.txt (42 Bytes)'); + ok(getUploadMarkdown("important.txt") === 'important.txt (42 Bytes)\n'); }); test("isAnImage", function() { From e2df79ee9e8d93dd1af25a0b360ddbd352ef3232 Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Fri, 20 May 2016 15:44:19 +0530 Subject: [PATCH 06/51] FIX: handle posts with no user on needs approval page --- app/serializers/queued_post_serializer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/serializers/queued_post_serializer.rb b/app/serializers/queued_post_serializer.rb index 43892fa292..b2d215cf5e 100644 --- a/app/serializers/queued_post_serializer.rb +++ b/app/serializers/queued_post_serializer.rb @@ -30,7 +30,7 @@ class QueuedPostSerializer < ApplicationSerializer end def include_can_delete_user? - user.trust_level == TrustLevel[0] + user && user.trust_level == TrustLevel[0] end end From 4eeae880b6c20d0770ab5b6578727da18fa1dcbe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Sat, 21 May 2016 12:49:29 +0200 Subject: [PATCH 07/51] fix deprecation comment in momentjs --- lib/javascripts/moment.js | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/javascripts/moment.js b/lib/javascripts/moment.js index d5e925c1d5..7f1d25a4c5 100644 --- a/lib/javascripts/moment.js +++ b/lib/javascripts/moment.js @@ -1171,10 +1171,7 @@ } utils_hooks__hooks.createFromInputFallback = deprecate( - 'moment construction falls back to js Date. This is ' + - 'discouraged and will be removed in upcoming major ' + - 'release. Please refer to ' + - 'https://github.com/moment/moment/issues/1407 for more info.', + 'moment construction falls back to js Date. This is discouraged and will be removed in upcoming major release. Please refer to https://github.com/moment/moment/issues/1407 for more info.', function (config) { config._d = new Date(config._i + (config._useUTC ? ' UTC' : '')); } From feffe23cc5fd7dffa68137b156910ee831ad6aa2 Mon Sep 17 00:00:00 2001 From: James Kiesel Date: Sat, 21 May 2016 06:17:54 -0700 Subject: [PATCH 08/51] FEATURE: More granular mailing list mode (#4068) * Rearrange frontend to account for mailing list mode * Allow update of user preference for mailing list frequency * Add mailing list frequency estimate * Simplify frequency estimate; disable activity summary for mailing list mode * Remove combined updates * Add specs for enqueue mailing list mode job * Write mailing list method for mailer * Fix linting error * Account for stale topics * Add translations for default mailing list setting * One query for mailing list topics * Fix failing spec * WIP * Flesh out html template * First pass at text-based mailing list summary * Add user avatar * Properly format posts for mailing list * Move make_all_links_absolute into Email::Styles * Apply first_seen_at to user * Send mailing list email summary hourly based on first_seen_at * Branch and test cleanup * Use existing mailing list mode estimate * Fix failing specs --- .../discourse/controllers/preferences.js.es6 | 18 +++ .../components/preference-checkbox.hbs | 2 +- .../discourse/templates/user/preferences.hbs | 31 ++++- app/helpers/user_notifications_helper.rb | 4 +- .../notify_mailing_list_subscribers.rb | 2 +- .../scheduled/enqueue_mailing_list_emails.rb | 27 ++++ app/mailers/user_notifications.rb | 42 ++++-- app/models/mailing_list_mode_site_setting.rb | 19 +++ app/models/post.rb | 8 ++ app/models/user.rb | 1 + app/models/user_option.rb | 1 + app/serializers/user_option_serializer.rb | 1 + app/services/user_updater.rb | 1 + .../user_notifications/mailing_list.html.erb | 76 +++++++++++ .../user_notifications/mailing_list.text.erb | 31 +++++ config/locales/client.en.yml | 12 +- config/locales/server.en.yml | 13 ++ config/site_settings.yml | 3 + ...9073132_add_mailing_list_mode_frequency.rb | 5 + .../20160326001747_add_user_first_visit.rb | 5 + lib/email/styles.rb | 12 ++ lib/pretty_text.rb | 27 +--- spec/components/pretty_text_spec.rb | 65 ++++----- spec/jobs/enqueue_mailing_list_emails_spec.rb | 128 ++++++++++++++++++ .../notify_mailing_list_subscribers_spec.rb | 9 +- spec/mailers/user_notifications_spec.rb | 63 +++++++++ .../mailing_list_mode_site_setting_spec.rb | 17 +++ spec/models/user_spec.rb | 23 ++++ 28 files changed, 567 insertions(+), 79 deletions(-) create mode 100644 app/jobs/scheduled/enqueue_mailing_list_emails.rb create mode 100644 app/models/mailing_list_mode_site_setting.rb create mode 100644 app/views/user_notifications/mailing_list.html.erb create mode 100644 app/views/user_notifications/mailing_list.text.erb create mode 100644 db/migrate/20160309073132_add_mailing_list_mode_frequency.rb create mode 100644 db/migrate/20160326001747_add_user_first_visit.rb create mode 100644 spec/jobs/enqueue_mailing_list_emails_spec.rb create mode 100644 spec/models/mailing_list_mode_site_setting_spec.rb diff --git a/app/assets/javascripts/discourse/controllers/preferences.js.es6 b/app/assets/javascripts/discourse/controllers/preferences.js.es6 index 6ccc5bb22b..733268a168 100644 --- a/app/assets/javascripts/discourse/controllers/preferences.js.es6 +++ b/app/assets/javascripts/discourse/controllers/preferences.js.es6 @@ -62,6 +62,24 @@ export default Ember.Controller.extend(CanCheckEmails, { return this.siteSettings.available_locales.split('|').map(s => ({ name: s, value: s })); }, + @computed() + frequencyEstimate() { + var estimate = this.get('model.mailing_list_posts_per_day'); + if (!estimate || estimate < 2) { + return I18n.t('user.mailing_list_mode.few_per_day'); + } else { + return I18n.t('user.mailing_list_mode.many_per_day', { dailyEmailEstimate: estimate }); + } + }, + + @computed() + mailingListModeOptions() { + return [ + {name: I18n.t('user.mailing_list_mode.daily'), value: 0}, + {name: this.get('frequencyEstimate'), value: 1} + ]; + }, + previousRepliesOptions: [ {name: I18n.t('user.email_previous_replies.always'), value: 0}, {name: I18n.t('user.email_previous_replies.unless_emailed'), value: 1}, diff --git a/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs b/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs index b47200bf5a..f7931d5918 100644 --- a/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs +++ b/app/assets/javascripts/discourse/templates/components/preference-checkbox.hbs @@ -1,4 +1,4 @@ diff --git a/app/assets/javascripts/discourse/templates/user/preferences.hbs b/app/assets/javascripts/discourse/templates/user/preferences.hbs index 644b9a84b7..9c14f6b457 100644 --- a/app/assets/javascripts/discourse/templates/user/preferences.hbs +++ b/app/assets/javascripts/discourse/templates/user/preferences.hbs @@ -185,9 +185,6 @@ {{preference-checkbox labelKey="user.email_in_reply_to" checked=model.user_option.email_in_reply_to}} {{preference-checkbox labelKey="user.email_private_messages" checked=model.user_option.email_private_messages}} {{preference-checkbox labelKey="user.email_direct" checked=model.user_option.email_direct}} - {{#unless siteSettings.disable_mailing_list_mode}} - {{preference-checkbox warning="checkMailingList" labelKey="user.mailing_list_mode" checked=model.user_option.mailing_list_mode}} - {{/unless}} {{preference-checkbox labelKey="user.email_always" checked=model.user_option.email_always}} {{#unless model.user_option.email_always}}
@@ -198,11 +195,33 @@ {{/if}}
{{/unless}} - - - +
+ + {{#if canReceiveDigest}} + {{preference-checkbox labelKey="user.email_digests.title" disabled=model.user_option.mailing_list_mode checked=model.user_option.email_digests}} + {{#if model.user_option.email_digests}} +
+ {{combo-box valueAttribute="value" content=digestFrequencies value=model.user_option.digest_after_minutes}} +
+ {{/if}} + {{/if}} +
+ + {{#unless siteSettings.disable_mailing_list_mode}} +
+ + {{preference-checkbox labelKey="user.mailing_list_mode.enabled" warning="checkMailingList" checked=model.user_option.mailing_list_mode}} +
{{{i18n 'user.mailing_list_mode.instructions'}}}
+ {{#if model.user_option.mailing_list_mode}} +
+ {{combo-box valueAttribute="value" content=mailingListModeOptions value=model.user_option.mailing_list_mode_frequency}} +
+ {{/if}} +
+ {{/unless}} +
{{desktop-notification-config}} diff --git a/app/helpers/user_notifications_helper.rb b/app/helpers/user_notifications_helper.rb index 4c088836d1..39553ffcab 100644 --- a/app/helpers/user_notifications_helper.rb +++ b/app/helpers/user_notifications_helper.rb @@ -65,9 +65,9 @@ module UserNotificationsHelper normalize_name(post.user.name) != normalize_name(post.user.username) end - def format_for_email(post, use_excerpt) + def format_for_email(post, use_excerpt, style: nil) html = use_excerpt ? post.excerpt : post.cooked - PrettyText.format_for_email(html, post).html_safe + PrettyText.format_for_email(html, post, style: style).html_safe end end diff --git a/app/jobs/regular/notify_mailing_list_subscribers.rb b/app/jobs/regular/notify_mailing_list_subscribers.rb index 38cb079a3d..bbae4bb8b3 100644 --- a/app/jobs/regular/notify_mailing_list_subscribers.rb +++ b/app/jobs/regular/notify_mailing_list_subscribers.rb @@ -16,7 +16,7 @@ module Jobs users = User.activated.not_blocked.not_suspended.real .joins(:user_option) - .where(user_options: {mailing_list_mode: true}) + .where(user_options: {mailing_list_mode: true, mailing_list_mode_frequency: 1}) .where('NOT EXISTS( SELECT 1 FROM topic_users tu diff --git a/app/jobs/scheduled/enqueue_mailing_list_emails.rb b/app/jobs/scheduled/enqueue_mailing_list_emails.rb new file mode 100644 index 0000000000..2417264928 --- /dev/null +++ b/app/jobs/scheduled/enqueue_mailing_list_emails.rb @@ -0,0 +1,27 @@ +module Jobs + + class EnqueueMailingListEmails < Jobs::Scheduled + every 1.hour + + def execute(args) + return if SiteSetting.disable_mailing_list_mode? + target_user_ids.each do |user_id| + Jobs.enqueue(:user_email, type: :mailing_list, user_id: user_id) + end + end + + def target_user_ids + # Users who want to receive daily mailing list emails + User.real + .not_suspended + .joins(:user_option) + .where(active: true, staged: false, user_options: {mailing_list_mode: true, mailing_list_mode_frequency: 0}) + .where("#{!SiteSetting.must_approve_users?} OR approved OR moderator OR admin") + .where("date_part('hour', first_seen_at) = date_part('hour', CURRENT_TIMESTAMP)") # where the hour of first_seen_at is the same as the current hour + .where("COALESCE(first_seen_at, '2010-01-01') <= CURRENT_TIMESTAMP - '23 HOURS'::INTERVAL") # don't send unless you've been around for a day already + .pluck(:id) + end + + end + +end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index 4b9ffa2725..a1b5361f03 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -73,16 +73,27 @@ class UserNotifications < ActionMailer::Base end end - def digest(user, opts={}) - @user = user - @base_url = Discourse.base_url + def mailing_list(user, opts={}) + @since = opts[:since] || 1.day.ago + @since_formatted = short_date(@since) + @new_topic_posts = Post.mailing_list_new_topics(user, @since).group_by(&:topic) || {} + @existing_topic_posts = Post.mailing_list_updates(user, @since).group_by(&:topic) || {} + @posts_by_topic = @new_topic_posts.merge @existing_topic_posts + return unless @posts_by_topic.present? + + build_summary_for(user) + build_email @user.email, + from_alias: I18n.t('user_notifications.mailing_list.from', site_name: SiteSetting.title), + subject: I18n.t('user_notifications.mailing_list.subject_template', + site_name: @site_name, + date: @date) + end + + def digest(user, opts={}) + build_summary_for(user) min_date = opts[:since] || @user.last_emailed_at || @user.last_seen_at || 1.month.ago - @site_name = SiteSetting.email_prefix.presence || SiteSetting.title - - @header_color = ColorScheme.hex_for_name('header_background') - @anchor_color = ColorScheme.hex_for_name('tertiary') @last_seen_at = short_date(@user.last_seen_at || @user.created_at) # A list of topics to show the user @@ -106,10 +117,8 @@ class UserNotifications < ActionMailer::Base end @featured_topics, @new_topics = @featured_topics[0..4], @featured_topics[5..-1] - @markdown_linker = MarkdownLinker.new(Discourse.base_url) - @unsubscribe_key = DigestUnsubscribeKey.create_key_for(@user) - build_email user.email, + build_email @user.email, from_alias: I18n.t('user_notifications.digest.from', site_name: SiteSetting.title), subject: I18n.t('user_notifications.digest.subject_template', site_name: @site_name, @@ -396,4 +405,17 @@ class UserNotifications < ActionMailer::Base build_email(user.email, email_opts) end + + private + + def build_summary_for(user) + @user = user + @date = short_date(Time.now) + @base_url = Discourse.base_url + @site_name = SiteSetting.email_prefix.presence || SiteSetting.title + @header_color = ColorScheme.hex_for_name('header_background') + @anchor_color = ColorScheme.hex_for_name('tertiary') + @markdown_linker = MarkdownLinker.new(@base_url) + @unsubscribe_key = DigestUnsubscribeKey.create_key_for(@user) + end end diff --git a/app/models/mailing_list_mode_site_setting.rb b/app/models/mailing_list_mode_site_setting.rb new file mode 100644 index 0000000000..b2a8291d00 --- /dev/null +++ b/app/models/mailing_list_mode_site_setting.rb @@ -0,0 +1,19 @@ +require_dependency 'enum_site_setting' + +class MailingListModeSiteSetting < EnumSiteSetting + def self.valid_value?(val) + val.to_i.to_s == val.to_s && + values.any? { |v| v[:value] == val.to_i } + end + + def self.values + @values ||= [ + { name: 'user.mailing_list_mode.daily', value: 0 }, + { name: 'user.mailing_list_mode.individual', value: 1 } + ] + end + + def self.translate_names? + true + end +end diff --git a/app/models/post.rb b/app/models/post.rb index 981a0ba924..65a1e7fcbd 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -65,6 +65,14 @@ class Post < ActiveRecord::Base scope :with_topic_subtype, ->(subtype) { joins(:topic).where('topics.subtype = ?', subtype) } scope :visible, -> { joins(:topic).where('topics.visible = true').where(hidden: false) } scope :secured, lambda { |guardian| where('posts.post_type in (?)', Topic.visible_post_types(guardian && guardian.user))} + scope :for_mailing_list, ->(user, since) { + created_since(since) + .joins(:topic) + .where(topic: Topic.for_digest(user, 100.years.ago)) # we want all topics with new content, regardless when they were created + .order('posts.created_at ASC') + } + scope :mailing_list_new_topics, ->(user, since) { for_mailing_list(user, since).where('topics.created_at > ?', since) } + scope :mailing_list_updates, ->(user, since) { for_mailing_list(user, since).where('topics.created_at <= ?', since) } delegate :username, to: :user diff --git a/app/models/user.rb b/app/models/user.rb index 8d68c337b0..a410a71161 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -476,6 +476,7 @@ class User < ActiveRecord::Base update_previous_visit(now) # using update_column to avoid the AR transaction update_column(:last_seen_at, now) + update_column(:first_seen_at, now) unless self.first_seen_at end def self.gravatar_template(email) diff --git a/app/models/user_option.rb b/app/models/user_option.rb index d986a2885b..ad4ac23a49 100644 --- a/app/models/user_option.rb +++ b/app/models/user_option.rb @@ -24,6 +24,7 @@ class UserOption < ActiveRecord::Base def set_defaults self.email_always = SiteSetting.default_email_always self.mailing_list_mode = SiteSetting.default_email_mailing_list_mode + self.mailing_list_mode_frequency = SiteSetting.default_email_mailing_list_mode_frequency self.email_direct = SiteSetting.default_email_direct self.automatically_unpin_topics = SiteSetting.default_topics_automatic_unpin self.email_private_messages = SiteSetting.default_email_private_messages diff --git a/app/serializers/user_option_serializer.rb b/app/serializers/user_option_serializer.rb index 591b07e457..a6ea26da13 100644 --- a/app/serializers/user_option_serializer.rb +++ b/app/serializers/user_option_serializer.rb @@ -2,6 +2,7 @@ class UserOptionSerializer < ApplicationSerializer attributes :user_id, :email_always, :mailing_list_mode, + :mailing_list_mode_frequency, :email_digests, :email_private_messages, :email_direct, diff --git a/app/services/user_updater.rb b/app/services/user_updater.rb index 747ce7d15b..fa9c57736f 100644 --- a/app/services/user_updater.rb +++ b/app/services/user_updater.rb @@ -9,6 +9,7 @@ class UserUpdater OPTION_ATTR = [ :email_always, :mailing_list_mode, + :mailing_list_mode_frequency, :email_digests, :email_direct, :email_private_messages, diff --git a/app/views/user_notifications/mailing_list.html.erb b/app/views/user_notifications/mailing_list.html.erb new file mode 100644 index 0000000000..efd6ac3df8 --- /dev/null +++ b/app/views/user_notifications/mailing_list.html.erb @@ -0,0 +1,76 @@ +
+ + + + + + + +
+ + <% if logo_url.blank? %> + <%= SiteSetting.title %> + <% else %> + + <% end %> +
+
+ <%= raw(t 'user_notifications.mailing_list.why', site_link: html_site_link(@anchor_color), date: @since_formatted) %> +
+
+ +
+

<%= t('user_notifications.mailing_list.new_topics') %>

+
    + <% @new_topic_posts.keys.each do |topic| %> +
  • + <%= raw format_topic_title(topic.title) %> + <%= @new_topic_posts[topic].length %> + <%= category_badge(topic.category, inline_style: true, absolute_url: true) %> +
  • + <% end %> +
+

<%= t('user_notifications.mailing_list.topic_updates') %>

+
    + <% @existing_topic_posts.keys.each do |topic| %> +
  • + <%= raw format_topic_title(topic.title) %> + <%= @existing_topic_posts[topic].length %> + <%= category_badge(topic.category, inline_style: true, absolute_url: true) %> +
  • + <% end %> +
+
+ + <% @posts_by_topic.keys.each do |topic| %> + + + + + + + + + + <% end %> +
+

+ <%= raw format_topic_title(topic.title) %> +

+
+ <% @posts_by_topic[topic].each do |post| %> +
+ +

+ '><%= post.user.name || post.user.username %> + - + <%= I18n.l(post.created_at, format: :long) %> +

+ <%= raw format_for_email(post, false, style: :notification) %> +
+
+ <% end %> + <%= t('user_notifications.mailing_list.view_this_topic') %> +
+ <%= t('user_notifications.mailing_list.back_to_top') %> +
diff --git a/app/views/user_notifications/mailing_list.text.erb b/app/views/user_notifications/mailing_list.text.erb new file mode 100644 index 0000000000..5208a9f085 --- /dev/null +++ b/app/views/user_notifications/mailing_list.text.erb @@ -0,0 +1,31 @@ +<%- site_link = raw(@markdown_linker.create(@site_name, '/')) %> +<%= raw(t 'user_notifications.mailing_list.why', site_link: site_link, date: @since_formatted) %> + +<%- if @new_topic_posts.keys.present? -%> + ### <%= t 'user_notifications.mailing_list.new_topics' %> + <%- @new_topic_posts.keys.each do |topic| %> + <%= raw(@markdown_linker.create(topic.title, topic.relative_url)) %> + <%- end -%> +<%- end -%> + +<%- if @existing_topic_posts.keys.present? -%> + ### <%= t 'user_notifications.mailing_list.new_topics' %> + <%- @existing_topic_posts.keys.each do |topic| -%> + <%= raw(@markdown_linker.create(topic.title, topic.relative_url)) %> + <%= @existing_topic_posts[topic].length %> + <%- end -%> +<%- end -%> + +-------------------------------------------------------------------------------- + +<%- @posts_by_topic.keys.each do |topic| %> + ### <%= raw(@markdown_linker.create(topic.title, topic.relative_url)) %> + + <%- @posts_by_topic[topic].each do |post| -%> + <%= post.user.name || post.user.username %> - <%= post.created_at %> +-------------------------------------------------------------------------------- + <%= post.raw %> + + + <%- end -%> +<%- end %> diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 80c13dabd3..5a4c386f43 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -504,7 +504,17 @@ en: suspended_notice: "This user is suspended until {{date}}." suspended_reason: "Reason: " github_profile: "Github" - mailing_list_mode: "Send me an email for every new post (unless I mute the topic or category)" + email_activity_summary: "Activity Summary" + mailing_list_mode: + label: "Mailing list mode" + enabled: "Enable mailing list mode." + instructions: | + This setting overrides the activity summary.
+ Muted topics and categories are not included in these emails. + daily: "Send daily updates." + individual: "Send an email for every new post." + many_per_day: "Send me an email for every new post (about {{dailyEmailEstimate}} per day)." + few_per_day: "Send me an email for every new post (less than 2 per day)." watched_categories: "Watched" watched_categories_instructions: "You will automatically watch all new topics in these categories. You will be notified of all new posts and topics, and a count of new posts will also appear next to the topic." tracked_categories: "Tracked" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 0c73c7d857..663ea6a9b4 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -28,6 +28,7 @@ en: short_no_year: "%B %-d" # Format directives: http://ruby-doc.org/core-2.2.0/Time.html#method-i-strftime date_only: "%B %-d, %Y" + long: "%B %-d, %Y, %l:%M%P" date: # Do not remove the brackets and commas and do not translate the first month name. It should be "null". month_names: @@ -35,6 +36,8 @@ en: <<: *datetime_formats time: <<: *datetime_formats + am: "am" + pm: "pm" title: "Discourse" topics: "Topics" @@ -1300,6 +1303,7 @@ en: default_email_private_messages: "Send an email when someone messages the user by default." default_email_direct: "Send an email when someone quotes/replies to/mentions or invites the user by default." default_email_mailing_list_mode: "Send an email for every new post by default." + default_email_mailing_list_mode_frequency: "Users who enable mailing list mode will receive emails this often by default." disable_mailing_list_mode: "Disallow users from enabling mailing list mode." default_email_always: "Send an email notification even when the user is active by default." default_email_previous_replies: "Include previous replies in emails by default." @@ -2278,6 +2282,15 @@ en: more_topics: "There were %{new_topics_since_seen} other new topics." more_topics_category: "More new topics:" + mailing_list: + why: "All activity on %{site_link} for %{date}" + subject_template: "[%{site_name}] Summary for %{date}" + unsubscribe: "This summary is sent daily due to mailing list mode being enabled. To unsubscribe %{unsubscribe_link}." + from: "%{site_name} summary" + new_topics: "New topics" + topic_updates: "Topic updates" + view_this_topic: "View this topic" + back_to_top: "Back to top" forgot_password: subject_template: "[%{site_name}] Password reset" text_body_template: | diff --git a/config/site_settings.yml b/config/site_settings.yml index fc59c05897..3f993bb21d 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -1128,6 +1128,9 @@ user_preferences: default_email_private_messages: true default_email_direct: true default_email_mailing_list_mode: false + default_email_mailing_list_mode_frequency: + enum: 'MailingListModeSiteSetting' + default: 1 disable_mailing_list_mode: default: false client: true diff --git a/db/migrate/20160309073132_add_mailing_list_mode_frequency.rb b/db/migrate/20160309073132_add_mailing_list_mode_frequency.rb new file mode 100644 index 0000000000..dcdce07c40 --- /dev/null +++ b/db/migrate/20160309073132_add_mailing_list_mode_frequency.rb @@ -0,0 +1,5 @@ +class AddMailingListModeFrequency < ActiveRecord::Migration + def change + add_column :user_options, :mailing_list_mode_frequency, :integer, default: 0, null: false + end +end diff --git a/db/migrate/20160326001747_add_user_first_visit.rb b/db/migrate/20160326001747_add_user_first_visit.rb new file mode 100644 index 0000000000..50e000ab00 --- /dev/null +++ b/db/migrate/20160326001747_add_user_first_visit.rb @@ -0,0 +1,5 @@ +class AddUserFirstVisit < ActiveRecord::Migration + def change + add_column :users, :first_seen_at, :datetime + end +end diff --git a/lib/email/styles.rb b/lib/email/styles.rb index a82c8459f3..25076ab72f 100644 --- a/lib/email/styles.rb +++ b/lib/email/styles.rb @@ -90,6 +90,7 @@ module Email style('.rtl', 'direction: rtl;') style('td.body', 'padding-top:5px;', colspan: "2") style('.whisper td.body', 'font-style: italic; color: #9c9c9c;') + style('.lightbox-wrapper .meta', 'display: none') correct_first_body_margin correct_footer_style reset_tables @@ -186,6 +187,17 @@ module Email @fragment.to_s end + def make_all_links_absolute + site_uri = URI(Discourse.base_url) + @fragment.css("a").each do |link| + begin + link["href"] = "#{site_uri}#{link['href']}" unless URI(link["href"].to_s).host.present? + rescue URI::InvalidURIError, URI::InvalidComponentError + # leave it + end + end + end + private def replace_relative_urls diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index bfdcbbcd92..5ff3c054f1 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -386,31 +386,16 @@ module PrettyText fragment.to_html end - # Given a Nokogiri doc, convert all links to absolute - def self.make_all_links_absolute(doc) - site_uri = nil - doc.css("a").each do |link| - href = link["href"].to_s - begin - uri = URI(href) - site_uri ||= URI(Discourse.base_url) - link["href"] = "#{site_uri}#{link['href']}" unless uri.host.present? - rescue URI::InvalidURIError, URI::InvalidComponentError - # leave it - end - end - end - def self.strip_image_wrapping(doc) doc.css(".lightbox-wrapper .meta").remove end - def self.format_for_email(html, post = nil) - doc = Nokogiri::HTML.fragment(html) - DiscourseEvent.trigger(:reduce_cooked, doc, post) - make_all_links_absolute(doc) - strip_image_wrapping(doc) - doc.to_html + def self.format_for_email(html, post = nil, style: nil) + Email::Styles.new(html, style: style).tap do |doc| + doc.make_all_links_absolute + doc.send :"format_#{style}" if style + DiscourseEvent.trigger(:reduce_cooked, doc, post) + end.to_html end protected diff --git a/spec/components/pretty_text_spec.rb b/spec/components/pretty_text_spec.rb index 63e2eca471..d66de21387 100644 --- a/spec/components/pretty_text_spec.rb +++ b/spec/components/pretty_text_spec.rb @@ -286,41 +286,6 @@ HTML end end - describe "make_all_links_absolute" do - let(:base_url) { "http://baseurl.net" } - - def make_abs_string(html) - doc = Nokogiri::HTML.fragment(html) - described_class.make_all_links_absolute(doc) - doc.to_html - end - - before do - Discourse.stubs(:base_url).returns(base_url) - end - - it "adds base url to relative links" do - html = "

@wiseguy, @trollol what do you guys think?

" - output = make_abs_string(html) - expect(output).to eq("

@wiseguy, @trollol what do you guys think?

") - end - - it "doesn't change external absolute links" do - html = "

Check out this guy.

" - expect(make_abs_string(html)).to eq(html) - end - - it "doesn't change internal absolute links" do - html = "

Check out this guy.

" - expect(make_abs_string(html)).to eq(html) - end - - it "can tolerate invalid URLs" do - html = "

Check out this guy.

" - expect { make_abs_string(html) }.to_not raise_error - end - end - describe "strip_image_wrapping" do def strip_image_wrapping(html) doc = Nokogiri::HTML.fragment(html) @@ -339,8 +304,36 @@ HTML end describe 'format_for_email' do + let(:base_url) { "http://baseurl.net" } + let(:post) { Fabricate(:post) } + + before do + Discourse.stubs(:base_url).returns(base_url) + end + it 'does not crash' do - PrettyText.format_for_email('test') + PrettyText.format_for_email('test', post) + end + + it "adds base url to relative links" do + html = "

@wiseguy, @trollol what do you guys think?

" + output = described_class.format_for_email(html, post) + expect(output).to eq("

@wiseguy, @trollol what do you guys think?

") + end + + it "doesn't change external absolute links" do + html = "

Check out this guy.

" + expect(described_class.format_for_email(html, post)).to eq(html) + end + + it "doesn't change internal absolute links" do + html = "

Check out this guy.

" + expect(described_class.format_for_email(html, post)).to eq(html) + end + + it "can tolerate invalid URLs" do + html = "

Check out this guy.

" + expect { described_class.format_for_email(html, post) }.to_not raise_error end end diff --git a/spec/jobs/enqueue_mailing_list_emails_spec.rb b/spec/jobs/enqueue_mailing_list_emails_spec.rb new file mode 100644 index 0000000000..820ed1e812 --- /dev/null +++ b/spec/jobs/enqueue_mailing_list_emails_spec.rb @@ -0,0 +1,128 @@ +require 'rails_helper' +require_dependency 'jobs/base' + +describe Jobs::EnqueueMailingListEmails do + + describe '#target_users' do + + context 'unapproved users' do + Given!(:unapproved_user) { Fabricate(:active_user, approved: false, first_seen_at: 24.hours.ago) } + When do + SiteSetting.must_approve_users = true + unapproved_user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0) + end + Then { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(false) } + + # As a moderator + And { unapproved_user.update_column(:moderator, true) } + And { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true) } + + # As an admin + And { unapproved_user.update_attributes(admin: true, moderator: false) } + And { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true) } + + # As an approved user + And { unapproved_user.update_attributes(admin: false, moderator: false, approved: true ) } + And { expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(true) } + end + + context 'staged users' do + let!(:staged_user) { Fabricate(:active_user, staged: true, last_emailed_at: 1.year.ago, last_seen_at: 1.year.ago) } + + it "doesn't return staged users" do + expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(staged_user.id)).to eq(false) + end + end + + context "inactive user" do + let!(:inactive_user) { Fabricate(:user, active: false) } + + it "doesn't return users who have been emailed recently" do + expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(inactive_user.id)).to eq(false) + end + end + + context "suspended user" do + let!(:suspended_user) { Fabricate(:user, suspended_till: 1.week.from_now, suspended_at: 1.day.ago) } + + it "doesn't return users who are suspended" do + expect(Jobs::EnqueueMailingListEmails.new.target_user_ids.include?(suspended_user.id)).to eq(false) + end + end + + context 'users with mailing list mode on' do + let(:user) { Fabricate(:active_user, first_seen_at: 24.hours.ago) } + let(:user_option) { user.user_option } + subject { Jobs::EnqueueMailingListEmails.new.target_user_ids } + before do + user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0) + end + + it "returns a user whose first_seen_at matches the current hour" do + expect(subject).to include user.id + end + + it "returns a user seen multiple days ago" do + user.update(first_seen_at: 72.hours.ago) + expect(subject).to include user.id + end + + it "doesn't return a user who has never been seen" do + user.update(first_seen_at: nil) + expect(subject).to_not include user.id + end + + it "doesn't return users with mailing list mode off" do + user_option.update(mailing_list_mode: false) + expect(subject).to_not include user.id + end + + it "doesn't return users with mailing list mode set to 'individual'" do + user_option.update(mailing_list_mode_frequency: 1) + expect(subject).to_not include user.id + end + + it "doesn't return a user who has received the mailing list summary earlier" do + user.update(first_seen_at: 5.hours.ago) + expect(subject).to_not include user.id + end + + it "doesn't return a user who was first seen today" do + user.update(first_seen_at: 2.minutes.ago) + expect(subject).to_not include user.id + end + end + + end + + describe '#execute' do + + let(:user) { Fabricate(:user) } + + context "mailing list emails are enabled" do + before do + Jobs::EnqueueMailingListEmails.any_instance.expects(:target_user_ids).returns([user.id]) + end + + it "enqueues the mailing list email job" do + Jobs.expects(:enqueue).with(:user_email, type: :mailing_list, user_id: user.id) + Jobs::EnqueueMailingListEmails.new.execute({}) + end + end + + context "mailing list emails are disabled" do + before do + Jobs::EnqueueMailingListEmails.any_instance.expects(:target_user_ids).never + end + + it "does not enqueue the mailing list email job" do + SiteSetting.disable_mailing_list_mode = true + Jobs.expects(:enqueue).with(:user_email, type: :mailing_list, user_id: user.id).never + Jobs::EnqueueMailingListEmails.new.execute({}) + end + end + + end + + +end diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index f63b4225cc..75e32073d9 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -34,10 +34,17 @@ describe Jobs::NotifyMailingListSubscribers do context "with a valid post" do let!(:post) { Fabricate(:post, user: user) } - it "sends the email to the user" do + it "sends the email to the user if the frequency is set to 'always'" do + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 1) UserNotifications.expects(:mailing_list_notify).with(user, post).once Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) end + + it "does not send the email to the user if the frequency is set to 'daily'" do + user.user_option.update(mailing_list_mode: true, mailing_list_mode_frequency: 0) + UserNotifications.expects(:mailing_list_notify).never + Jobs::NotifyMailingListSubscribers.new.execute(post_id: post.id) + end end context "with a deleted post" do diff --git a/spec/mailers/user_notifications_spec.rb b/spec/mailers/user_notifications_spec.rb index 00ceb74a3a..c1f0c6f065 100644 --- a/spec/mailers/user_notifications_spec.rb +++ b/spec/mailers/user_notifications_spec.rb @@ -76,6 +76,69 @@ describe UserNotifications do end + describe '.mailing_list' do + subject { UserNotifications.mailing_list(user) } + + context "without new posts" do + it "doesn't send the email" do + expect(subject.to).to be_blank + end + end + + context "with new posts" do + let(:user) { Fabricate(:user) } + let(:topic) { Fabricate(:topic, user: user) } + let!(:new_post) { Fabricate(:post, topic: topic, created_at: 2.hours.ago, raw: "Feel the Bern") } + let!(:old_post) { Fabricate(:post, topic: topic, created_at: 25.hours.ago, raw: "Make America Great Again") } + let(:old_topic) { Fabricate(:topic, user: user, created_at: 10.days.ago) } + let(:new_post_in_old_topic) { Fabricate(:post, topic: old_topic, created_at: 2.hours.ago, raw: "Yes We Can") } + let(:stale_post) { Fabricate(:post, topic: old_topic, created_at: 2.days.ago, raw: "A New American Century") } + + it "works" do + expect(subject.to).to eq([user.email]) + expect(subject.subject).to be_present + expect(subject.from).to eq([SiteSetting.notification_email]) + expect(subject.html_part.body.to_s).to include topic.title + expect(subject.text_part.body.to_s).to be_present + end + + it "includes posts less than 24 hours old" do + expect(subject.html_part.body.to_s).to include new_post.cooked + end + + it "does not include posts older than 24 hours old" do + expect(subject.html_part.body.to_s).to_not include old_post.cooked + end + + it "includes topics created over 24 hours ago which have new posts" do + new_post_in_old_topic + expect(subject.html_part.body.to_s).to include old_topic.title + expect(subject.html_part.body.to_s).to include new_post_in_old_topic.cooked + expect(subject.html_part.body.to_s).to_not include stale_post.cooked + end + + it "includes multiple topics" do + new_post_in_old_topic + expect(subject.html_part.body.to_s).to include topic.title + expect(subject.html_part.body.to_s).to include old_topic.title + end + + it "does not include topics not updated for the past 24 hours" do + stale_post + expect(subject.html_part.body.to_s).to_not include old_topic.title + expect(subject.html_part.body.to_s).to_not include stale_post.cooked + end + + it "includes email_prefix in email subject instead of site title" do + SiteSetting.email_prefix = "Try Discourse" + SiteSetting.title = "Discourse Meta" + + expect(subject.subject).to match(/Try Discourse/) + expect(subject.subject).not_to match(/Discourse Meta/) + end + end + end + describe '.digest' do subject { UserNotifications.digest(user) } diff --git a/spec/models/mailing_list_mode_site_setting_spec.rb b/spec/models/mailing_list_mode_site_setting_spec.rb new file mode 100644 index 0000000000..d8ccc7e4aa --- /dev/null +++ b/spec/models/mailing_list_mode_site_setting_spec.rb @@ -0,0 +1,17 @@ +require 'rails_helper' + +describe MailingListModeSiteSetting do + describe 'valid_value?' do + it 'returns true for a valid value as an int' do + expect(DigestEmailSiteSetting.valid_value?(0)).to eq true + end + + it 'returns false for a valid value as a string' do + expect(DigestEmailSiteSetting.valid_value?('0')).to eq true + end + + it 'returns false for an out of range value' do + expect(DigestEmailSiteSetting.valid_value?(3)).to eq false + end + end +end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 92e12e9e71..63a0dcfb2a 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -596,6 +596,29 @@ describe User do end + describe "update_last_seen!" do + let (:user) { Fabricate(:user) } + let!(:first_visit_date) { Time.zone.now } + let!(:second_visit_date) { 2.hours.from_now } + + it "should update the last seen value" do + expect(user.last_seen_at).to eq nil + user.update_last_seen!(first_visit_date) + expect(user.reload.last_seen_at).to be_within_one_second_of(first_visit_date) + end + + it "should update the first seen value if it doesn't exist" do + user.update_last_seen!(first_visit_date) + expect(user.reload.first_seen_at).to be_within_one_second_of(first_visit_date) + end + + it "should not update the first seen value if it doesn't exist" do + user.update_last_seen!(first_visit_date) + user.update_last_seen!(second_visit_date) + expect(user.reload.first_seen_at).to be_within_one_second_of(first_visit_date) + end + end + describe "last_seen_at" do let(:user) { Fabricate(:user) } From c012b18601e9b79ed64e713a043bdfd707985727 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Sat, 21 May 2016 20:13:00 +0200 Subject: [PATCH 09/51] FIX: sending email wasn't working anymore when a plugin used 'reduce_cooked' (cc @gdpelican) --- app/helpers/user_notifications_helper.rb | 4 ++-- app/views/user_notifications/mailing_list.html.erb | 2 +- lib/email/styles.rb | 4 ++++ lib/pretty_text.rb | 4 ++-- 4 files changed, 9 insertions(+), 5 deletions(-) diff --git a/app/helpers/user_notifications_helper.rb b/app/helpers/user_notifications_helper.rb index 39553ffcab..afa80d832a 100644 --- a/app/helpers/user_notifications_helper.rb +++ b/app/helpers/user_notifications_helper.rb @@ -65,9 +65,9 @@ module UserNotificationsHelper normalize_name(post.user.name) != normalize_name(post.user.username) end - def format_for_email(post, use_excerpt, style: nil) + def format_for_email(post, use_excerpt, style = nil) html = use_excerpt ? post.excerpt : post.cooked - PrettyText.format_for_email(html, post, style: style).html_safe + PrettyText.format_for_email(html, post, style).html_safe end end diff --git a/app/views/user_notifications/mailing_list.html.erb b/app/views/user_notifications/mailing_list.html.erb index efd6ac3df8..2cd2116450 100644 --- a/app/views/user_notifications/mailing_list.html.erb +++ b/app/views/user_notifications/mailing_list.html.erb @@ -60,7 +60,7 @@ - <%= I18n.l(post.created_at, format: :long) %>

- <%= raw format_for_email(post, false, style: :notification) %> + <%= raw format_for_email(post, false, :notification) %>
<% end %> diff --git a/lib/email/styles.rb b/lib/email/styles.rb index 25076ab72f..0156fe0643 100644 --- a/lib/email/styles.rb +++ b/lib/email/styles.rb @@ -6,6 +6,10 @@ module Email class Styles @@plugin_callbacks = [] + attr_accessor :fragment + + delegate :css, to: :fragment + def initialize(html, opts=nil) @html = html @opts = opts || {} diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index 5ff3c054f1..e3e5cc756b 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -390,11 +390,11 @@ module PrettyText doc.css(".lightbox-wrapper .meta").remove end - def self.format_for_email(html, post = nil, style: nil) + def self.format_for_email(html, post = nil, style = nil) Email::Styles.new(html, style: style).tap do |doc| + DiscourseEvent.trigger(:reduce_cooked, doc, post) doc.make_all_links_absolute doc.send :"format_#{style}" if style - DiscourseEvent.trigger(:reduce_cooked, doc, post) end.to_html end From 8f8ad3fe4afc294ce9088151a7ffc53c03415d36 Mon Sep 17 00:00:00 2001 From: Steve Kemp Date: Sun, 22 May 2016 11:54:03 +0300 Subject: [PATCH 10/51] Allow an (optional) post-creation time to be submitted. (#4205) * Allow an (optional) post-creation time to be submitted. This should allow a new post to be created with an initial date/time specified by the caller, which will be useful for people writing importers.. * Only allow `created_at` to be submitted via the API. This addresses the previous concern. --- app/controllers/posts_controller.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 2a21e8607d..e1d0831b26 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -544,7 +544,7 @@ class PostsController < ApplicationController :reply_to_post_number, :auto_track, :typing_duration_msecs, - :composer_open_duration_msecs + :composer_open_duration_msecs, ] # param munging for WordPress @@ -557,6 +557,10 @@ class PostsController < ApplicationController # We allow `embed_url` via the API permitted << :embed_url + + # We allow `created_at` via the API + permitted << :created_at + end params.require(:raw) From f387dfe2263ef198701d8876977881ce99fe385c Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Sun, 22 May 2016 18:31:46 +0530 Subject: [PATCH 11/51] FIX: mixed case group mentions were not getting highligted in composer --- app/controllers/users_controller.rb | 2 +- spec/controllers/users_controller_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 719dcf37aa..af4708fb7e 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -211,7 +211,6 @@ class UsersController < ApplicationController def is_local_username usernames = params[:usernames] usernames = [params[:username]] if usernames.blank? - usernames.each(&:downcase!) groups = Group.where(name: usernames).pluck(:name) mentionable_groups = @@ -223,6 +222,7 @@ class UsersController < ApplicationController end usernames -= groups + usernames.each(&:downcase!) result = User.where(staged: false) .where(username_lower: usernames) diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index 43887f1ea5..9edaa64734 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -1516,6 +1516,7 @@ describe UsersController do describe ".is_local_username" do let(:user) { Fabricate(:user) } + let(:group) { Fabricate(:group, name: "Discourse") } it "finds the user" do xhr :get, :is_local_username, username: user.username @@ -1524,6 +1525,13 @@ describe UsersController do expect(json["valid"][0]).to eq(user.username) end + it "finds the group" do + xhr :get, :is_local_username, username: group.name + expect(response).to be_success + json = JSON.parse(response.body) + expect(json["valid_groups"][0]).to eq(group.name) + end + it "supports multiples usernames" do xhr :get, :is_local_username, usernames: [user.username, "system"] expect(response).to be_success From 695773db1c3af0142a92cc25d4fe6e852b71e698 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 19 May 2016 22:25:08 +1000 Subject: [PATCH 12/51] FEATURE: upgrade from therubyracer to mini_racer This pushes our internal V8 JavaScript engine from Chrome 32 to 50. It also resolves some long standing issues we had with the old wrapper. --- Gemfile | 3 +- Gemfile.lock | 18 ++++---- app/models/site_customization.rb | 2 +- .../tilt/es6_module_transpiler_template.rb | 42 +++++++------------ lib/js_locale_helper.rb | 26 ++++++++---- lib/pretty_text.rb | 38 ++++++++--------- spec/components/js_locale_helper_spec.rb | 9 ++-- 7 files changed, 70 insertions(+), 68 deletions(-) diff --git a/Gemfile b/Gemfile index 178f640533..d96c77701f 100644 --- a/Gemfile +++ b/Gemfile @@ -106,7 +106,8 @@ gem 'sidekiq-statistic' # for sidekiq web gem 'sinatra', require: false -gem 'therubyracer' +gem 'execjs', github: 'rails/execjs', require: false +gem 'mini_racer' gem 'thin', require: false gem 'highline', require: false gem 'rack-protection' # security diff --git a/Gemfile.lock b/Gemfile.lock index 5ed3677067..3491b735a1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,3 +1,9 @@ +GIT + remote: git://github.com/rails/execjs.git + revision: 22476229323cbec3befa8b690cc6c7c3957a0044 + specs: + execjs (2.6.0) + GEM remote: https://rubygems.org/ specs: @@ -94,7 +100,6 @@ GEM erubis (2.7.0) eventmachine (1.2.0.1) excon (0.45.4) - execjs (2.6.0) exifr (1.2.4) fabrication (2.9.8) fakeweb (1.3.0) @@ -144,7 +149,7 @@ GEM librarian (0.1.2) highline thor (~> 0.15) - libv8 (3.16.14.13) + libv8 (5.0.71.48.3) listen (0.7.3) logster (1.2.3) loofah (2.0.3) @@ -159,6 +164,8 @@ GEM method_source (0.8.2) mime-types (2.99.1) mini_portile2 (2.1.0) + mini_racer (0.1.3) + libv8 (~> 5.0) minitest (5.8.4) mocha (1.1.0) metaclass (~> 0.0.1) @@ -284,7 +291,6 @@ GEM redis (3.3.0) redis-namespace (1.5.2) redis (~> 3.0, >= 3.0.4) - ref (2.0.0) rest-client (1.8.0) http-cookie (>= 1.0.2, < 2.0) mime-types (>= 1.16, < 3.0) @@ -373,9 +379,6 @@ GEM activesupport (>= 4.0) sprockets (>= 3.0.0) stackprof (0.2.9) - therubyracer (0.12.2) - libv8 (~> 3.16.14.0) - ref thin (1.6.4) daemons (~> 1.0, >= 1.0.9) eventmachine (~> 1.0, >= 1.0.4) @@ -417,6 +420,7 @@ DEPENDENCIES ember-rails (= 0.18.5) ember-source (= 1.12.2) excon + execjs! fabrication (= 2.9.8) fakeweb (~> 1.3.0) fast_blank @@ -439,6 +443,7 @@ DEPENDENCIES memory_profiler message_bus (= 2.0.0.beta.11) mime-types + mini_racer minitest mocha mock_redis @@ -493,7 +498,6 @@ DEPENDENCIES sinatra spork-rails stackprof - therubyracer thin timecop uglifier diff --git a/app/models/site_customization.rb b/app/models/site_customization.rb index a74e188a23..562c544f40 100644 --- a/app/models/site_customization.rb +++ b/app/models/site_customization.rb @@ -59,7 +59,7 @@ SCRIPT begin code = transpile(node.inner_html, node['version']) node.replace("") - rescue Tilt::ES6ModuleTranspilerTemplate::JavaScriptError => ex + rescue MiniRacer::RuntimeError => ex node.replace("") end end diff --git a/lib/es6_module_transpiler/tilt/es6_module_transpiler_template.rb b/lib/es6_module_transpiler/tilt/es6_module_transpiler_template.rb index 58eb0e678c..68b2ca990b 100644 --- a/lib/es6_module_transpiler/tilt/es6_module_transpiler_template.rb +++ b/lib/es6_module_transpiler/tilt/es6_module_transpiler_template.rb @@ -1,22 +1,9 @@ require 'execjs' require 'babel/transpiler' +require 'mini_racer' module Tilt - class Console - def initialize(prefix=nil) - @prefix = prefix || '' - end - - def log(msg) - Rails.logger.info("#{@prefix}#{msg}") - end - - def error(msg) - Rails.logger.error("#{@prefix}#{msg}") - end - end - class ES6ModuleTranspilerTemplate < Tilt::Template self.default_mime_type = 'application/javascript' @@ -30,10 +17,19 @@ module Tilt def self.create_new_context # timeout any eval that takes longer than 15 seconds - ctx = V8::Context.new(timeout: 15000) + ctx = MiniRacer::Context.new(timeout: 15000) ctx.eval("var self = this; #{File.read(Babel::Transpiler.script_path)}") ctx.eval("module = {}; exports = {};"); ctx.load("#{Rails.root}/lib/es6_module_transpiler/support/es6-module-transpiler.js") + ctx.attach("rails.logger.info", proc{|err| Rails.logger.info(err.to_s)}) + ctx.attach("rails.logger.error", proc{|err| Rails.logger.error(err.to_s)}) + ctx.eval < e - raise JavaScriptError.new(e.message, e.backtrace) - end + yield end - rval end def whitelisted?(path) @@ -98,7 +84,7 @@ module Tilt def babel_transpile(source) klass = self.class klass.protect do - klass.v8['console'] = Console.new("BABEL: babel-eval: ") + klass.v8.eval("console.prefix = 'BABEL: babel-eval: ';") @output = klass.v8.eval(babel_source(source)) end end @@ -108,7 +94,7 @@ module Tilt klass = self.class klass.protect do - klass.v8['console'] = Console.new("BABEL: #{scope.logical_path}: ") + klass.v8.eval("console.prefix = 'BABEL: #{scope.logical_path}: ';") @output = klass.v8.eval(generate_source(scope)) end diff --git a/lib/js_locale_helper.rb b/lib/js_locale_helper.rb index db4821a37b..eb2f1a19d3 100644 --- a/lib/js_locale_helper.rb +++ b/lib/js_locale_helper.rb @@ -154,15 +154,25 @@ module JsLocaleHelper result end - def self.compile_message_format(locale, format) - ctx = V8::Context.new - ctx.load(Rails.root + 'lib/javascripts/messageformat.js') - path = Rails.root + "lib/javascripts/locale/#{locale}.js" - ctx.load(path) if File.exists?(path) - ctx.eval("mf = new MessageFormat('#{locale}');") - ctx.eval("mf.precompile(mf.parse(#{format.inspect}))") + @mutex = Mutex.new + def self.with_context + @mutex.synchronize do + yield @ctx ||= begin + ctx = MiniRacer::Context.new + ctx.load(Rails.root + 'lib/javascripts/messageformat.js') + ctx + end + end + end - rescue V8::Error => e + def self.compile_message_format(locale, format) + with_context do |ctx| + path = Rails.root + "lib/javascripts/locale/#{locale}.js" + ctx.load(path) if File.exists?(path) + ctx.eval("mf = new MessageFormat('#{locale}');") + ctx.eval("mf.precompile(mf.parse(#{format.inspect}))") + end + rescue MiniRacer::EvalError => e message = "Invalid Format: " << e.message "function(){ return #{message.inspect};}" end diff --git a/lib/pretty_text.rb b/lib/pretty_text.rb index e3e5cc756b..b328f230f3 100644 --- a/lib/pretty_text.rb +++ b/lib/pretty_text.rb @@ -1,4 +1,4 @@ -require 'v8' +require 'mini_racer' require 'nokogiri' require_dependency 'url_helper' require_dependency 'excerpt_parser' @@ -7,7 +7,9 @@ require_dependency 'discourse_tagging' module PrettyText - class Helpers + module Helpers + extend self + def t(key, opts) key = "js." + key unless opts @@ -81,6 +83,7 @@ module PrettyText nil end end + end @mutex = Mutex.new @@ -92,9 +95,11 @@ module PrettyText def self.create_new_context # timeout any eval that takes longer than 15 seconds - ctx = V8::Context.new(timeout: 15000) + ctx = MiniRacer::Context.new(timeout: 15000) - ctx["helpers"] = Helpers.new + Helpers.instance_methods.each do |method| + ctx.attach("helpers.#{method}", Helpers.method(method)) + end ctx_load(ctx, "vendor/assets/javascripts/md5.js", @@ -198,6 +203,7 @@ module PrettyText # we use the exact same markdown converter as the client # TODO: use the same extensions on both client and server (in particular the template for mentions) baked = nil + text = text || "" protect do context = v8 @@ -206,8 +212,9 @@ module PrettyText context_opts = opts || {} context_opts[:sanitize] = true unless context_opts[:sanitize] == false - context['opts'] = context_opts - context['raw'] = text + + context.eval("opts = #{context_opts.to_json};") + context.eval("raw = #{text.inspect};") if Post.white_listed_image_classes.present? Post.white_listed_image_classes.each do |klass| @@ -258,8 +265,10 @@ module PrettyText # leaving this here, cause it invokes v8, don't want to implement twice def self.avatar_img(avatar_template, size) protect do - v8['avatarTemplate'] = avatar_template - v8['size'] = size + v8.eval < e - raise JavaScriptError.new(e.message, e.backtrace) - end + rval = yield end rval end diff --git a/spec/components/js_locale_helper_spec.rb b/spec/components/js_locale_helper_spec.rb index 1b26efe88c..125474f93d 100644 --- a/spec/components/js_locale_helper_spec.rb +++ b/spec/components/js_locale_helper_spec.rb @@ -1,5 +1,6 @@ require 'rails_helper' require_dependency 'js_locale_helper' +require 'mini_racer' describe JsLocaleHelper do @@ -25,7 +26,7 @@ describe JsLocaleHelper do end def setup_message_format(format) - @ctx = V8::Context.new + @ctx = MiniRacer::Context.new @ctx.eval('MessageFormat = {locale: {}};') @ctx.load(Rails.root + 'lib/javascripts/locale/en.js') compiled = JsLocaleHelper.compile_message_format('en', format) @@ -72,7 +73,7 @@ describe JsLocaleHelper do end it 'handles message format special keys' do - ctx = V8::Context.new + ctx = MiniRacer::Context.new ctx.eval("I18n = {};") JsLocaleHelper.set_translations 'en', { @@ -149,7 +150,7 @@ describe JsLocaleHelper do SiteSetting.default_locale = 'ru' I18n.locale = :uk - ctx = V8::Context.new + ctx = MiniRacer::Context.new ctx.eval('var window = this;') ctx.load(Rails.root + 'app/assets/javascripts/locales/i18n.js') ctx.eval(JsLocaleHelper.output_locale(I18n.locale)) @@ -167,7 +168,7 @@ describe JsLocaleHelper do LocaleSiteSetting.values.each do |locale| it "generates valid date helpers for #{locale[:value]} locale" do js = JsLocaleHelper.output_locale(locale[:value]) - ctx = V8::Context.new + ctx = MiniRacer::Context.new ctx.eval('var window = this;') ctx.load(Rails.root + 'app/assets/javascripts/locales/i18n.js') ctx.eval(js) From f06266f31e1293363d04bb3647ad5b99939af53a Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 20 May 2016 17:06:44 +1000 Subject: [PATCH 13/51] execjs 2.7 was just released to support mini_racer --- Gemfile | 3 +-- Gemfile.lock | 9 ++------- 2 files changed, 3 insertions(+), 9 deletions(-) diff --git a/Gemfile b/Gemfile index d96c77701f..29c9292d08 100644 --- a/Gemfile +++ b/Gemfile @@ -105,8 +105,7 @@ gem 'sidekiq-statistic' # for sidekiq web gem 'sinatra', require: false - -gem 'execjs', github: 'rails/execjs', require: false +gem 'execjs', require: false gem 'mini_racer' gem 'thin', require: false gem 'highline', require: false diff --git a/Gemfile.lock b/Gemfile.lock index 3491b735a1..8ebfd949b7 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,9 +1,3 @@ -GIT - remote: git://github.com/rails/execjs.git - revision: 22476229323cbec3befa8b690cc6c7c3957a0044 - specs: - execjs (2.6.0) - GEM remote: https://rubygems.org/ specs: @@ -100,6 +94,7 @@ GEM erubis (2.7.0) eventmachine (1.2.0.1) excon (0.45.4) + execjs (2.7.0) exifr (1.2.4) fabrication (2.9.8) fakeweb (1.3.0) @@ -420,7 +415,7 @@ DEPENDENCIES ember-rails (= 0.18.5) ember-source (= 1.12.2) excon - execjs! + execjs fabrication (= 2.9.8) fakeweb (~> 1.3.0) fast_blank From 9285168aa433c24db86a4c89e69d2c08a127c121 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 23 May 2016 11:58:20 +1000 Subject: [PATCH 14/51] use trusty --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index dc5a82fc36..03b1f5f1b3 100644 --- a/.travis.yml +++ b/.travis.yml @@ -33,6 +33,7 @@ services: - redis-server sudo: false +dist: trusty cache: directories: From 1adf24656c0035bd8741a697e211876932e9c415 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 23 May 2016 12:04:44 +1000 Subject: [PATCH 15/51] get trusty working on travis --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 03b1f5f1b3..43ef9e8166 100644 --- a/.travis.yml +++ b/.travis.yml @@ -32,7 +32,7 @@ rvm: services: - redis-server -sudo: false +sudo: required dist: trusty cache: From 64e59564dd41cd739ef53e3dcc79b197557a9e6a Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 23 May 2016 12:23:15 +1000 Subject: [PATCH 16/51] update gems --- Gemfile.lock | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 8ebfd949b7..deae609a31 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -38,16 +38,16 @@ GEM minitest (~> 5.1) thread_safe (~> 0.3, >= 0.3.4) tzinfo (~> 1.1) - annotate (2.7.0) + annotate (2.7.1) activerecord (>= 3.2, < 6.0) - rake (~> 10.4) + rake (>= 10.4, < 12.0) arel (6.0.3) - aws-sdk (2.2.9) - aws-sdk-resources (= 2.2.9) - aws-sdk-core (2.2.9) + aws-sdk (2.3.7) + aws-sdk-resources (= 2.3.7) + aws-sdk-core (2.3.7) jmespath (~> 1.0) - aws-sdk-resources (2.2.9) - aws-sdk-core (= 2.2.9) + aws-sdk-resources (2.3.7) + aws-sdk-core (= 2.3.7) babel-source (5.8.34) babel-transpiler (0.7.0) babel-source (>= 4.0, < 6) @@ -108,7 +108,7 @@ GEM fast_xs (0.8.0) ffi (1.9.10) flamegraph (0.9.5) - foreman (0.78.0) + foreman (0.82.0) thor (~> 0.19.1) fspath (2.1.1) gctools (0.2.3) @@ -117,7 +117,7 @@ GEM globalid (0.3.6) activesupport (>= 4.1.0) guess_html_encoding (0.0.11) - hashie (3.4.3) + hashie (3.4.4) highline (1.7.8) hiredis (0.6.1) htmlentities (4.3.4) @@ -133,12 +133,14 @@ GEM progress (~> 3.0, >= 3.0.1) image_size (1.4.1) in_threads (1.3.1) - jmespath (1.1.3) + jmespath (1.2.4) + json_pure (>= 1.8.1) jquery-rails (4.0.5) rails-dom-testing (~> 1.0) railties (>= 4.2.0) thor (>= 0.14, < 2.0) json (1.8.3) + json_pure (1.8.3) jwt (1.5.2) kgio (2.10.0) librarian (0.1.2) @@ -161,12 +163,12 @@ GEM mini_portile2 (2.1.0) mini_racer (0.1.3) libv8 (~> 5.0) - minitest (5.8.4) + minitest (5.9.0) mocha (1.1.0) metaclass (~> 0.0.1) mock_redis (0.15.4) moneta (0.8.0) - msgpack (0.7.4) + msgpack (0.7.6) multi_json (1.11.2) multi_xml (0.5.5) multipart-post (2.0.0) @@ -272,14 +274,14 @@ GEM activesupport (= 4.2.6) rake (>= 0.8.7) thor (>= 0.18.1, < 2.0) - raindrops (0.15.0) - rake (10.5.0) + raindrops (0.16.0) + rake (11.1.2) rake-compiler (0.9.9) rake rb-fsevent (0.9.7) - rb-inotify (0.9.5) + rb-inotify (0.9.7) ffi (>= 0.5.0) - rbtrace (0.4.7) + rbtrace (0.4.8) ffi (>= 1.0.6) msgpack (>= 0.4.3) trollop (>= 1.16.2) @@ -290,7 +292,7 @@ GEM http-cookie (>= 1.0.2, < 2.0) mime-types (>= 1.16, < 3.0) netrc (~> 0.7) - rinku (1.7.3) + rinku (2.0.0) rmmseg-cpp (0.2.9) rspec (3.2.0) rspec-core (~> 3.2.0) @@ -385,15 +387,13 @@ GEM trollop (2.1.2) tzinfo (1.2.2) thread_safe (~> 0.1) - uglifier (2.7.2) - execjs (>= 0.3.0) - json (>= 1.8.0) + uglifier (3.0.0) + execjs (>= 0.3.0, < 3) unf (0.1.4) unf_ext unf_ext (0.0.7.1) - unicorn (5.0.1) + unicorn (5.1.0) kgio (~> 2.6) - rack raindrops (~> 0.7) PLATFORMS From 26084397c1219040dc9d8810e73d985b53dd80c6 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 23 May 2016 10:25:25 +0800 Subject: [PATCH 17/51] FIX: Check if file exists upfront. --- lib/file_store/local_store.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/file_store/local_store.rb b/lib/file_store/local_store.rb index a4068e57d1..e9bbdab6a1 100644 --- a/lib/file_store/local_store.rb +++ b/lib/file_store/local_store.rb @@ -12,11 +12,10 @@ module FileStore def remove_file(url) return unless is_relative?(url) path = public_dir + url + return if !File.exists?(path) tombstone = public_dir + url.sub("/uploads/", "/tombstone/") FileUtils.mkdir_p(Pathname.new(tombstone).dirname) FileUtils.move(path, tombstone, :force => true) - rescue Errno::ENOENT - # don't care if the file isn't there end def has_been_uploaded?(url) From 3a140a982fd1d55b9f2e7e6a2416a6a57423c774 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 23 May 2016 11:22:25 +0800 Subject: [PATCH 18/51] Fix build. --- spec/components/file_store/local_store_spec.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/components/file_store/local_store_spec.rb b/spec/components/file_store/local_store_spec.rb index 28f741f1aa..68724c5777 100644 --- a/spec/components/file_store/local_store_spec.rb +++ b/spec/components/file_store/local_store_spec.rb @@ -40,6 +40,7 @@ describe FileStore::LocalStore do it "moves the file to the tombstone" do FileUtils.expects(:mkdir_p) FileUtils.expects(:move) + File.expects(:exists?).returns(true) upload = Upload.new upload.stubs(:url).returns("/uploads/default/42/253dc8edf9d4ada1.png") store.remove_upload(upload) @@ -52,6 +53,7 @@ describe FileStore::LocalStore do it "moves the file to the tombstone" do FileUtils.expects(:mkdir_p) FileUtils.expects(:move) + File.expects(:exists?).returns(true) oi = OptimizedImage.new oi.stubs(:url).returns("/uploads/default/_optimized/42/253dc8edf9d4ada1.png") store.remove_optimized_image(upload) From b2d59a8de3cdc32deca1736a163b8507e1e11dc4 Mon Sep 17 00:00:00 2001 From: Sam Date: Mon, 23 May 2016 15:29:23 +1000 Subject: [PATCH 19/51] update docker test --- lib/tasks/docker.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/docker.rake b/lib/tasks/docker.rake index 0045fcee42..8e847c698f 100644 --- a/lib/tasks/docker.rake +++ b/lib/tasks/docker.rake @@ -15,7 +15,7 @@ task 'docker:test' do puts "Starting background redis" @redis_pid = Process.spawn('redis-server --dir tmp/test_data/redis') - @postgres_bin = "/usr/lib/postgresql/9.3/bin/" + @postgres_bin = "/usr/lib/postgresql/9.5/bin/" `#{@postgres_bin}initdb -D tmp/test_data/pg` # speed up db, never do this in production mmmmk From ab5337b79ca4e5dab3a0028dd54f2857023938a9 Mon Sep 17 00:00:00 2001 From: David Keller Date: Mon, 23 May 2016 08:12:02 +0200 Subject: [PATCH 20/51] Correct typo preventing script from running. (#4234) Signed-off-by: David Keller --- script/import_scripts/punbb.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/script/import_scripts/punbb.rb b/script/import_scripts/punbb.rb index a5d54372f1..6b8fad7f50 100644 --- a/script/import_scripts/punbb.rb +++ b/script/import_scripts/punbb.rb @@ -43,7 +43,7 @@ class ImportScripts::PunBB < ImportScripts::Base break if results.size < 1 - next if all_records_exist? :users, users.map {|u| u["id"].to_i} + next if all_records_exist? :users, results.map {|u| u["id"].to_i} create_users(results, total: total_count, offset: offset) do |user| { id: user['id'], From 8c525080076e31b003f8a0152490cf32de9da53f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 23 May 2016 09:33:29 +0200 Subject: [PATCH 21/51] warn users it may take a while to un/zip backup --- lib/backup_restore/backuper.rb | 4 ++-- lib/backup_restore/restorer.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/backup_restore/backuper.rb b/lib/backup_restore/backuper.rb index b55d735907..163bc84508 100644 --- a/lib/backup_restore/backuper.rb +++ b/lib/backup_restore/backuper.rb @@ -257,12 +257,12 @@ module BackupRestore end end - log "Gzipping archive..." + log "Gzipping archive, this may take a while..." `gzip -5 #{tar_filename}` end def after_create_hook - log "Executing the after_create_hook for the backup" + log "Executing the after_create_hook for the backup..." backup = Backup.create_from_filename("#{File.basename(@archive_basename)}.tar.gz") backup.after_create_hook end diff --git a/lib/backup_restore/restorer.rb b/lib/backup_restore/restorer.rb index 15c177240e..3b843998ab 100644 --- a/lib/backup_restore/restorer.rb +++ b/lib/backup_restore/restorer.rb @@ -161,7 +161,7 @@ module BackupRestore end def unzip_archive - log "Unzipping archive..." + log "Unzipping archive, this may take a while..." FileUtils.cd(@tmp_directory) { `gzip --decompress '#{@archive_filename}'` } end From b6db022051ccadfcc53948c6eda9a198b6be0a90 Mon Sep 17 00:00:00 2001 From: Jeff Atwood Date: Mon, 23 May 2016 03:36:26 -0700 Subject: [PATCH 22/51] de-emphasize notification state text slightly --- app/assets/stylesheets/desktop/topic-post.scss | 14 +++++++++++--- app/assets/stylesheets/mobile/topic-post.scss | 11 +++++++++-- 2 files changed, 20 insertions(+), 5 deletions(-) diff --git a/app/assets/stylesheets/desktop/topic-post.scss b/app/assets/stylesheets/desktop/topic-post.scss index 079020e04b..f417858dd6 100644 --- a/app/assets/stylesheets/desktop/topic-post.scss +++ b/app/assets/stylesheets/desktop/topic-post.scss @@ -448,7 +448,7 @@ a.star { padding: 10px 10px 0 0; p { line-height: 32px; - color: $primary; + color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); } .btn { margin-bottom: 5px; @@ -757,8 +757,16 @@ $topic-avatar-width: 45px; border: 1px solid dark-light-diff($primary, $secondary, 90%, -60%); box-shadow: 0 1px 5px rgba(0,0,0, .4); background-clip: padding-box; - span {font-size: 0.857em;} - .title {font-weight: bold; display: block; font-size: 1em;} + span { + font-size: 0.857em; + color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); + } + span.title { + font-weight: bold; + display: block; + font-size: 1em; + color: $primary; + } } .dropdown-menu a { display: block; diff --git a/app/assets/stylesheets/mobile/topic-post.scss b/app/assets/stylesheets/mobile/topic-post.scss index 6fba9b16a6..b7b820a3c0 100644 --- a/app/assets/stylesheets/mobile/topic-post.scss +++ b/app/assets/stylesheets/mobile/topic-post.scss @@ -327,10 +327,11 @@ a.star { } } -/* this is to force the drop-down notification state description para below the button */ + #topic-footer-buttons p { - clear: both; + clear: both; /* this is to force the drop-down notification state description para below the button */ margin: 0; + color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); } #suggested-topics { @@ -408,6 +409,12 @@ iframe { margin-right: 5px; padding-top: 1px; } + span { + color: dark-light-choose(scale-color($primary, $lightness: 50%), scale-color($secondary, $lightness: 50%)); + } + span.title { + color: $primary; + } } .btn-group { From a3672621258bb17d67b77c4d2ef42a6a7517460d Mon Sep 17 00:00:00 2001 From: Jeff Atwood Date: Mon, 23 May 2016 05:02:24 -0700 Subject: [PATCH 23/51] UX: simpler "white box" for oneboxes --- app/assets/stylesheets/common/base/onebox.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/stylesheets/common/base/onebox.scss b/app/assets/stylesheets/common/base/onebox.scss index 46052351fb..5eb272d11c 100644 --- a/app/assets/stylesheets/common/base/onebox.scss +++ b/app/assets/stylesheets/common/base/onebox.scss @@ -88,7 +88,7 @@ a.loading-onebox { } aside.onebox { - @include post-aside; + border: 5px solid dark-light-diff($primary, $secondary, 90%, -85%); padding: 12px 25px 12px 12px; font-size: 1em; From 667dd54a23b705d4286ac0b2a8d398c13675963b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Mon, 23 May 2016 16:18:30 +0200 Subject: [PATCH 24/51] FEATURE: new 'crop_tall_images' site setting --- app/models/optimized_image.rb | 26 +++++++++++++++++++ app/models/upload.rb | 14 +++++----- config/locales/server.en.yml | 1 + config/site_settings.yml | 1 + lib/cooked_post_processor.rb | 15 +++++++++-- lib/image_sizer.rb | 15 +++++++++++ spec/components/cooked_post_processor_spec.rb | 18 ++++++------- 7 files changed, 71 insertions(+), 19 deletions(-) diff --git a/app/models/optimized_image.rb b/app/models/optimized_image.rb index 590724f21e..d228ce0598 100644 --- a/app/models/optimized_image.rb +++ b/app/models/optimized_image.rb @@ -45,6 +45,8 @@ class OptimizedImage < ActiveRecord::Base if extension =~ /\.svg$/i FileUtils.cp(original_path, temp_path) resized = true + elsif opts[:crop] + resized = crop(original_path, temp_path, width, height, opts) else resized = resize(original_path, temp_path, width, height, opts) end @@ -124,6 +126,25 @@ class OptimizedImage < ActiveRecord::Base } end + def self.crop_instructions(from, to, dimensions, opts={}) + %W{ + convert + #{from}[0] + -gravity north + -background transparent + -thumbnail #{opts[:width]} + -crop #{dimensions}+0+0 + -unsharp 2x0.5+0.7+0 + -quality 98 + -profile #{File.join(Rails.root, 'vendor', 'data', 'RT_sRGB.icm')} + #{to} + } + end + + def self.crop_instructions_animated(from, to, dimensions, opts={}) + resize_instructions_animated(from, to, dimensions, opts) + end + def self.downsize_instructions(from, to, dimensions, opts={}) %W{ convert @@ -144,6 +165,11 @@ class OptimizedImage < ActiveRecord::Base optimize("resize", from, to, "#{width}x#{height}", opts) end + def self.crop(from, to, width, height, opts={}) + opts[:width] = width + optimize("crop", from, to, "#{width}x#{height}", opts) + end + def self.downsize(from, to, dimensions, opts={}) optimize("downsize", from, to, dimensions, opts) end diff --git a/app/models/upload.rb b/app/models/upload.rb index dec7de8f2f..e2512efc4c 100644 --- a/app/models/upload.rb +++ b/app/models/upload.rb @@ -29,18 +29,16 @@ class Upload < ActiveRecord::Base thumbnail(width, height).present? end - def create_thumbnail!(width, height) + def create_thumbnail!(width, height, crop=false) return unless SiteSetting.create_thumbnails? - thumbnail = OptimizedImage.create_for( - self, - width, - height, + opts = { filename: self.original_filename, - allow_animation: SiteSetting.allow_animated_thumbnails - ) + allow_animation: SiteSetting.allow_animated_thumbnails, + crop: crop && SiteSetting.crop_tall_images + } - if thumbnail + if thumbnail = OptimizedImage.create_for(self, width, height, opts) self.width = width self.height = height save(validate: false) diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 663ea6a9b4..c486de15de 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1094,6 +1094,7 @@ en: max_users_notified_per_group_mention: "Maximum number of users that may recieve a notification if a group is mentioned (if threshold is met no notifications will be raised)" create_thumbnails: "Create thumbnails and lightbox images that are too large to fit in a post." + crop_tall_images: "When creating a thumbnail of a tall image, crop it instead of generating a thin thumbnail." email_time_window_mins: "Wait (n) minutes before sending any notification emails, to give users a chance to edit and finalize their posts." private_email_time_window_seconds: "Wait (n) seconds before sending any private notification emails, to give users a chance to edit and finalize their messages." diff --git a/config/site_settings.yml b/config/site_settings.yml index 3f993bb21d..2a32d00c72 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -613,6 +613,7 @@ files: type: list default: '' create_thumbnails: true + crop_tall_images: true clean_up_uploads: true clean_orphan_uploads_grace_period_hours: 1 purge_deleted_uploads_grace_period_days: 30 diff --git a/lib/cooked_post_processor.rb b/lib/cooked_post_processor.rb index 9bb6b83b04..5234ad70e9 100644 --- a/lib/cooked_post_processor.rb +++ b/lib/cooked_post_processor.rb @@ -194,7 +194,10 @@ class CookedPostProcessor original_width, original_height = get_size(src) # can't reach the image... - if original_width.nil? || original_height.nil? + if original_width.nil? || + original_height.nil? || + original_width == 0 || + original_height == 0 Rails.logger.info "Can't reach '#{src}' to get its dimension." return end @@ -204,8 +207,16 @@ class CookedPostProcessor return if is_a_hyperlink?(img) + crop = false + if SiteSetting.crop_tall_images && (original_width.to_f / original_height.to_f < 0.75) + crop = true + width, height = ImageSizer.crop(original_width, original_height) + img["width"] = width + img["height"] = height + end + if upload = Upload.get_from_url(src) - upload.create_thumbnail!(width, height) + upload.create_thumbnail!(width, height, crop) end add_lightbox!(img, original_width, original_height, upload) diff --git a/lib/image_sizer.rb b/lib/image_sizer.rb index cc4ee8a6b5..3cf9f126fb 100644 --- a/lib/image_sizer.rb +++ b/lib/image_sizer.rb @@ -16,4 +16,19 @@ module ImageSizer [(w * ratio).floor, (h * ratio).floor] end + def self.crop(width, height, opts = {}) + return if width.blank? || height.blank? + + max_width = (opts[:max_width] || SiteSetting.max_image_width).to_f + max_height = (opts[:max_height] || SiteSetting.max_image_height).to_f + + w = width.to_f + h = height.to_f + + return [w.floor, h.floor] if w <= max_width && h <= max_height + + ratio = max_width / w + [max_width.floor, [max_height, (h * ratio)].min.floor] + end + end diff --git a/spec/components/cooked_post_processor_spec.rb b/spec/components/cooked_post_processor_spec.rb index 27e6e3f823..02e36bc8a5 100644 --- a/spec/components/cooked_post_processor_spec.rb +++ b/spec/components/cooked_post_processor_spec.rb @@ -110,7 +110,7 @@ describe CookedPostProcessor do SiteSetting.create_thumbnails = true Upload.expects(:get_from_url).returns(upload) - FastImage.stubs(:size).returns([1000, 2000]) + FastImage.stubs(:size).returns([1750, 2000]) # hmmm this should be done in a cleaner way OptimizedImage.expects(:resize).returns(true) @@ -120,8 +120,8 @@ describe CookedPostProcessor do it "generates overlay information" do cpp.post_process_images - expect(cpp.html).to match_html '