diff --git a/app/assets/javascripts/discourse/controllers/history.js.es6 b/app/assets/javascripts/discourse/controllers/history.js.es6 index 7b8cb412db5..8b090126215 100644 --- a/app/assets/javascripts/discourse/controllers/history.js.es6 +++ b/app/assets/javascripts/discourse/controllers/history.js.es6 @@ -12,8 +12,8 @@ import ObjectController from 'discourse/controllers/object'; @module Discourse **/ export default ObjectController.extend(ModalFunctionality, { - loading: false, - viewMode: "side_by_side", + loading: true, + viewMode: Discourse.Mobile.mobileView ? "inline" : "side_by_side", revisionsTextKey: "post.revisions.controls.comparing_previous_to_current_out_of_total", refresh: function(postId, postVersion) { @@ -41,87 +41,72 @@ export default ObjectController.extend(ModalFunctionality, { createdAtDate: function() { return moment(this.get("created_at")).format("LLLL"); }.property("created_at"), - previousVersion: function() { return this.get("version") - 1; }.property("version"), + previousVersion: function() { return this.get("current_version") - 1; }.property("current_version"), - displayGoToFirst: Em.computed.gt("version", 3), - displayGoToPrevious: Em.computed.gt("version", 2), - displayRevisions: Em.computed.gt("revisions_count", 2), - displayGoToNext: function() { return this.get("version") < this.get("revisions_count"); }.property("version", "revisions_count"), - displayGoToLast: function() { return this.get("version") < this.get("revisions_count") - 1; }.property("version", "revisions_count"), + displayGoToFirst: function() { return this.get("current_revision") > this.get("first_revision"); }.property("current_revision", "first_revision"), + displayGoToPrevious: function() { return this.get("previous_revision") && this.get("current_revision") > this.get("previous_revision"); }.property("current_revision", "previous_revision"), + displayRevisions: Em.computed.gt("version_count", 2), + displayGoToNext: function() { return this.get("next_revision") && this.get("current_revision") < this.get("next_revision"); }.property("current_revision", "next_revision"), + displayGoToLast: function() { return this.get("current_revision") < this.get("last_revision"); }.property("current_revision", "last_revision"), - displayShow: function() { return this.get("hidden") && Discourse.User.currentProp('staff'); }.property("hidden"), - displayHide: function() { return !this.get("hidden") && Discourse.User.currentProp('staff'); }.property("hidden"), + displayShow: function() { return !Discourse.Mobile.mobileView && this.get("previous_hidden") && Discourse.User.currentProp('staff'); }.property("previous_hidden"), + displayHide: function() { return !Discourse.Mobile.mobileView && !this.get("previous_hidden") && Discourse.User.currentProp('staff'); }.property("previous_hidden"), + + isEitherRevisionHidden: Em.computed.or("previous_hidden", "current_hidden"), + + hiddenClasses: function() { + if (this.get("displayingInline")) { + return this.get("isEitherRevisionHidden") ? "hidden-revision-either" : null; + } else { + var result = []; + if (this.get("previous_hidden")) { result.push("hidden-revision-previous"); } + if (this.get("current_hidden")) { result.push("hidden-revision-current"); } + return result.join(" "); + } + }.property("previous_hidden", "current_hidden", "displayingInline"), displayingInline: Em.computed.equal("viewMode", "inline"), displayingSideBySide: Em.computed.equal("viewMode", "side_by_side"), displayingSideBySideMarkdown: Em.computed.equal("viewMode", "side_by_side_markdown"), - category_diff: function() { - var viewMode = this.get("viewMode"); + previousCategory: function() { var changes = this.get("category_changes"); - - if (changes === null) { return; } - - var prevCategory = Discourse.Category.findById(changes.previous_category_id); - var curCategory = Discourse.Category.findById(changes.current_category_id); - - var raw = ""; - var opts = { allowUncategorized: true }; - prevCategory = Discourse.HTML.categoryBadge(prevCategory, opts); - curCategory = Discourse.HTML.categoryBadge(curCategory, opts); - - if(viewMode === "side_by_side_markdown" || viewMode === "side_by_side") { - raw = "
" + prevCategory + "
" + curCategory + "
"; - } else { - var diff = "" + prevCategory + " " + "" + curCategory + ""; - raw = "
" + diff + "
"; + if (changes) { + var category = Discourse.Category.findById(changes["previous"]); + return Discourse.HTML.categoryBadge(category, { allowUncategorized: true }); } + }.property("category_changes"), - return raw; - - }.property("viewMode", "category_changes"), + currentCategory: function() { + var changes = this.get("category_changes"); + if (changes) { + var category = Discourse.Category.findById(changes["current"]); + return Discourse.HTML.categoryBadge(category, { allowUncategorized: true }); + } + }.property("category_changes"), wiki_diff: function() { - var viewMode = this.get("viewMode"); - var changes = this.get("wiki_changes"); - if (changes === null) { return; } - - if (viewMode === "inline") { - var diff = changes["current_wiki"] ? '' : ''; - return "
" + diff + "
"; - } else { - var prev = changes["previous_wiki"] ? '' : " "; - var curr = changes["current_wiki"] ? '' : ''; - return "
" + prev + "
" + curr + "
"; + var changes = this.get("wiki_changes") + if (changes) { + return changes["current"] ? + '' : + ''; } - }.property("viewMode", "wiki_changes"), + }.property("wiki_changes"), post_type_diff: function () { - var viewMode = this.get("viewMode"); - var changes = this.get("post_type_changes"); - if (changes === null) { return; } - var moderator = Discourse.Site.currentProp('post_types.moderator_action'); - - if (viewMode === "inline") { - var diff = changes["current_post_type"] === moderator ? - '' : - ''; - return "
" + diff + "
"; - } else { - var prev = changes["previous_post_type"] === moderator ? '' : " "; - var curr = changes["current_post_type"] === moderator ? - '' : - ''; - return "
" + prev + "
" + curr + "
"; + var changes = this.get("post_type_changes"); + if (changes) { + return changes["current"] == moderator ? + '' : + ''; } - }.property("viewMode", "post_type_changes"), + }.property("post_type_changes"), title_diff: function() { var viewMode = this.get("viewMode"); - if(viewMode === "side_by_side_markdown") { - viewMode = "side_by_side"; - } + if (viewMode === "side_by_side_markdown") { viewMode = "side_by_side"; } return this.get("title_changes." + viewMode); }.property("viewMode", "title_changes"), @@ -130,13 +115,13 @@ export default ObjectController.extend(ModalFunctionality, { }.property("viewMode", "body_changes"), actions: { - loadFirstVersion: function() { this.refresh(this.get("post_id"), 2); }, - loadPreviousVersion: function() { this.refresh(this.get("post_id"), this.get("version") - 1); }, - loadNextVersion: function() { this.refresh(this.get("post_id"), this.get("version") + 1); }, - loadLastVersion: function() { this.refresh(this.get("post_id"), this.get("revisions_count")); }, + loadFirstVersion: function() { this.refresh(this.get("post_id"), this.get("first_revision")); }, + loadPreviousVersion: function() { this.refresh(this.get("post_id"), this.get("previous_revision")); }, + loadNextVersion: function() { this.refresh(this.get("post_id"), this.get("next_revision")); }, + loadLastVersion: function() { this.refresh(this.get("post_id"), this.get("last_revision")); }, - hideVersion: function() { this.hide(this.get("post_id"), this.get("version")); }, - showVersion: function() { this.show(this.get("post_id"), this.get("version")); }, + hideVersion: function() { this.hide(this.get("post_id"), this.get("current_revision")); }, + showVersion: function() { this.show(this.get("post_id"), this.get("current_revision")); }, displayInline: function() { this.set("viewMode", "inline"); }, displaySideBySide: function() { this.set("viewMode", "side_by_side"); }, diff --git a/app/assets/javascripts/discourse/models/_post.js b/app/assets/javascripts/discourse/models/_post.js index 118afce2997..9e865ea1892 100644 --- a/app/assets/javascripts/discourse/models/_post.js +++ b/app/assets/javascripts/discourse/models/_post.js @@ -111,9 +111,7 @@ Discourse.Post = Discourse.Model.extend({ }.property('link_counts.@each.internal'), // Edits are the version - 1, so version 2 = 1 edit - editCount: function() { - return this.get('version') - 1; - }.property('version'), + editCount: function() { return this.get('version') - 1; }.property('version'), flagsAvailable: function() { var post = this; diff --git a/app/assets/javascripts/discourse/routes/topic_route.js b/app/assets/javascripts/discourse/routes/topic_route.js index 2d0618327d0..38a4ffcd53a 100644 --- a/app/assets/javascripts/discourse/routes/topic_route.js +++ b/app/assets/javascripts/discourse/routes/topic_route.js @@ -82,7 +82,7 @@ Discourse.TopicRoute = Discourse.Route.extend({ showHistory: function(post) { Discourse.Route.showModal(this, 'history', post); - this.controllerFor('history').refresh(post.get("id"), post.get("version")); + this.controllerFor('history').refresh(post.get("id"), "latest"); this.controllerFor('modal').set('modalClass', 'history-modal'); }, diff --git a/app/assets/javascripts/discourse/templates/modal/history.hbs b/app/assets/javascripts/discourse/templates/modal/history.hbs index 94b2376fa61..20c513e94bc 100644 --- a/app/assets/javascripts/discourse/templates/modal/history.hbs +++ b/app/assets/javascripts/discourse/templates/modal/history.hbs @@ -7,13 +7,13 @@ {{#if loading}}
{{i18n loading}}
{{else}} - {{boundI18n revisionsTextKey previousBinding="previousVersion" currentBinding="version" totalBinding="revisions_count"}} + {{boundI18n revisionsTextKey previousBinding="previousVersion" currentBinding="current_version" totalBinding="version_count"}} {{/if}} {{#if displayHide}} - + {{/if}} {{#if displayShow}} @@ -28,34 +28,61 @@
- {{i18n post.revisions.details.edited_by}} {{#link-to 'user' username}}{{bound-avatar-template content.avatar_template "small"}} {{username}}{{/link-to}} {{bound-date created_at}} {{#if edit_reason}} — {{edit_reason}}{{/if}} + {{i18n post.revisions.details.edited_by}} + {{#link-to 'user' username}} + {{bound-avatar-template content.avatar_template "small"}} {{username}} + {{/link-to}} + {{bound-date created_at}} + {{#if edit_reason}} + — {{edit_reason}} + {{/if}} + {{#unless site.mobileView}} + {{#if user_changes}} + — {{bound-avatar-template user_changes.previous.avatar_template "small"}} {{user_changes.previous.username}} + → {{bound-avatar-template user_changes.current.avatar_template "small"}} {{user_changes.current.username}} + {{/if}} + {{#if wiki_changes}} + — {{{wiki_diff}}} + {{/if}} + {{#if post_type_changes}} + — {{{post_type_diff}}} + {{/if}} + {{#if category_changes}} + — {{{previousCategory}}} → {{{currentCategory}}} + {{/if}} + {{/unless}}
-
+
{{#if title_changes}}

{{{title_diff}}}

{{/if}} - {{#if category_changes}} -
- {{{category_diff}}} -
+ {{#if site.mobileView}} + {{#if user_changes}} +
+ {{bound-avatar-template user_changes.previous.avatar_template "small"}} {{user_changes.previous.username}} + → {{bound-avatar-template user_changes.current.avatar_template "small"}} {{user_changes.current.username}} +
+ {{/if}} + {{#if wiki_changes}} +
+ {{{wiki_diff}}} +
+ {{/if}} + {{#if post_type_changes}} +
+ {{{post_type_diff}}} +
+ {{/if}} + {{#if category_changes}} +
+ {{{previousCategory}}} → {{{currentCategory}}} +
+ {{/if}} {{/if}} - {{#if user_changes}} -
- {{bound-avatar-template user_changes.previous.avatar_template "small"}} {{user_changes.previous.username}} → {{bound-avatar-template user_changes.current.avatar_template "small"}} {{user_changes.current.username}} -
- {{/if}} - {{#if wiki_changes}} -
- {{{wiki_diff}}} -
- {{/if}} - {{#if post_type_changes}} -
- {{{post_type_diff}}} -
- {{/if}} - {{{body_diff}}} +
+ {{{body_diff}}} +
diff --git a/app/assets/stylesheets/common/base/history.scss b/app/assets/stylesheets/common/base/history.scss index 49cddbfaa99..cace79e7c30 100644 --- a/app/assets/stylesheets/common/base/history.scss +++ b/app/assets/stylesheets/common/base/history.scss @@ -11,6 +11,9 @@ margin-right: 7px; } } + #revisions .row:first-of-type { + margin-top: 10px; + } ins, .diff-ins { code, img { border: 2px solid $success; @@ -75,7 +78,17 @@ .fa-ban { color: #f00; } - .hidden-revision { - opacity: 0.5; + .hidden-revision-either { + opacity: .5; + } + .hidden-revision-previous .row { + div:nth-of-type(1), td:nth-of-type(1) { + opacity: .5; + } + } + .hidden-revision-current .row { + div:nth-of-type(2), td:nth-of-type(2) { + opacity: .5; + } } } diff --git a/app/assets/stylesheets/desktop/history.scss b/app/assets/stylesheets/desktop/history.scss index 75aa5bbbdeb..83c021445fc 100644 --- a/app/assets/stylesheets/desktop/history.scss +++ b/app/assets/stylesheets/desktop/history.scss @@ -1,6 +1,10 @@ // styles that apply to the popup that appears when you show the edit history of a post .modal.history-modal { + .btn { + // remove transitions on the buttons in the history modal + transition: none; + } .modal-inner-container { min-width: 960px; min-height: 500px; @@ -18,13 +22,13 @@ background-color: scale-color-diff(); padding: 5px; margin-top: 10px; + line-height: 2em; + height: 30px; } #revisions { word-wrap: break-word; - .row, table { + table { margin-top: 10px; - padding-bottom: 10px; - border-bottom: 1px dotted; } } img { diff --git a/app/assets/stylesheets/mobile/modal.scss b/app/assets/stylesheets/mobile/modal.scss index 13183abe172..fc5e991f36a 100644 --- a/app/assets/stylesheets/mobile/modal.scss +++ b/app/assets/stylesheets/mobile/modal.scss @@ -35,7 +35,7 @@ .modal-body { overflow-y: auto; max-height: 400px; - padding: 10px 0 10px 15px; + padding: 10px 0 10px 10px; width: 95%; } diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index bd840b72cb8..5f87364db32 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -5,7 +5,7 @@ require_dependency 'distributed_memoizer' class PostsController < ApplicationController # Need to be logged in for all actions here - before_filter :ensure_logged_in, except: [:show, :replies, :by_number, :short_link, :reply_history, :revisions, :expand_embed, :markdown, :raw, :cooked] + before_filter :ensure_logged_in, except: [:show, :replies, :by_number, :short_link, :reply_history, :revisions, :latest_revision, :expand_embed, :markdown, :raw, :cooked] skip_before_filter :check_xhr, only: [:markdown_id, :markdown_num, :short_link] @@ -99,39 +99,30 @@ class PostsController < ApplicationController post.image_sizes = params[:image_sizes] if params[:image_sizes].present? if too_late_to(:edit, post) - render json: {errors: [I18n.t('too_late_to_edit')]}, status: 422 - return + return render json: { errors: [I18n.t('too_late_to_edit')] }, status: 422 end guardian.ensure_can_edit!(post) - # to stay consistent with the create api, - # we should allow for title changes and category changes here - # we should also move all of this to a post updater. - if post.post_number == 1 && (params[:title] || params[:post][:category_id]) - post.topic.acting_user = current_user - post.topic.title = params[:title] if params[:title] - Topic.transaction do - post.topic.change_category_to_id(params[:post][:category_id].to_i) - post.topic.save - end + changes = { + raw: params[:post][:raw], + edit_reason: params[:post][:edit_reason] + } - if post.topic.errors.present? - render_json_error(post.topic) - return - end + # to stay consistent with the create api, we allow for title & category changes here + if post.post_number == 1 + changes[:title] = params[:title] if params[:title] + changes[:category_id] = params[:post][:category_id] if params[:post][:category_id] end revisor = PostRevisor.new(post) - if revisor.revise!(current_user, params[:post][:raw], edit_reason: params[:post][:edit_reason]) + if revisor.revise!(current_user, changes) TopicLink.extract_from(post) QuotedPost.extract_from(post) end - if post.errors.present? - render_json_error(post) - return - end + return render_json_error(post) if post.errors.present? + return render_json_error(post.topic) if post.topic.errors.present? post_serializer = PostSerializer.new(post, scope: guardian, root: false) post_serializer.draft_sequence = DraftSequence.current(current_user, post.topic.draft_key) @@ -194,7 +185,6 @@ class PostsController < ApplicationController end def destroy_many - params.require(:post_ids) posts = Post.where(id: post_ids_including_replies) @@ -222,17 +212,35 @@ class PostsController < ApplicationController render_json_dump(post_revision_serializer) end + def latest_revision + post_revision = find_latest_post_revision_from_params + post_revision_serializer = PostRevisionSerializer.new(post_revision, scope: guardian, root: false) + render_json_dump(post_revision_serializer) + end + def hide_revision post_revision = find_post_revision_from_params - guardian.ensure_can_hide_post_revision! post_revision + guardian.ensure_can_hide_post_revision!(post_revision) + post_revision.hide! + + post = find_post_from_params + post.public_version -= 1 + post.save + render nothing: true end def show_revision post_revision = find_post_revision_from_params - guardian.ensure_can_show_post_revision! post_revision + guardian.ensure_can_show_post_revision!(post_revision) + post_revision.show! + + post = find_post_from_params + post.public_version += 1 + post.save + render nothing: true end @@ -252,9 +260,7 @@ class PostsController < ApplicationController guardian.ensure_can_wiki! post = find_post_from_params - post.wiki = params[:wiki] - post.version += 1 - post.save + post.revise(current_user, { wiki: params[:wiki] }) render nothing: true end @@ -263,9 +269,7 @@ class PostsController < ApplicationController guardian.ensure_can_change_post_type! post = find_post_from_params - post.post_type = params[:post_type].to_i - post.version += 1 - post.save + post.revise(current_user, { post_type: params[:post_type].to_i }) render nothing: true end @@ -329,9 +333,26 @@ class PostsController < ApplicationController raise Discourse::InvalidParameters.new(:revision) if revision < 2 post_revision = PostRevision.find_by(post_id: post_id, number: revision) - post_revision.post = find_post_from_params + raise Discourse::NotFound unless post_revision + post_revision.post = find_post_from_params guardian.ensure_can_see!(post_revision) + + post_revision + end + + def find_latest_post_revision_from_params + post_id = params[:id] || params[:post_id] + + finder = PostRevision.where(post_id: post_id).order(:number) + finder = finder.where(hidden: false) unless guardian.is_staff? + post_revision = finder.last + + raise Discourse::NotFound unless post_revision + + post_revision.post = find_post_from_params + guardian.ensure_can_see!(post_revision) + post_revision end diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 1069af4deef..a527086354b 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -122,14 +122,15 @@ class TopicsController < ApplicationController topic = Topic.find_by(id: params[:topic_id]) guardian.ensure_can_edit!(topic) - topic.title = params[:title] if params[:title].present? - topic.acting_user = current_user + changes = {} + changes[:title] = params[:title] if params[:title] + changes[:category_id] = params[:category_id] if params[:category_id] - success = false - Topic.transaction do - success = topic.save - success &= topic.change_category_to_id(params[:category_id].to_i) unless topic.private_message? - EditRateLimiter.new(current_user).performed! + success = true + + if changes.length > 0 + first_post = topic.ordered_posts.first + success = PostRevisor.new(first_post, topic).revise!(current_user, changes) end # this is used to return the title to the client as it may have been changed by "TextCleaner" @@ -308,21 +309,17 @@ class TopicsController < ApplicationController guardian.ensure_can_change_post_owner! - topic = Topic.find(params[:topic_id].to_i) - new_user = User.find_by_username(params[:username]) - ids = params[:post_ids].to_a + post_ids = params[:post_ids].to_a + topic = Topic.find_by(id: params[:topic_id].to_i) + new_user = User.find_by(username: params[:username]) - unless new_user && topic && ids - render json: failed_json, status: 422 - return - end + return render json: failed_json, status: 422 unless post_ids && topic && new_user ActiveRecord::Base.transaction do - ids.each do |id| - post = Post.find(id) - if post.is_first_post? - topic.user = new_user # Update topic owner (first avatar) - end + post_ids.each do |post_id| + post = Post.find(post_id) + # update topic owner (first avatar) + topic.user = new_user if post.is_first_post? post.set_owner(new_user, current_user) end end diff --git a/app/jobs/regular/pull_hotlinked_images.rb b/app/jobs/regular/pull_hotlinked_images.rb index 9ad4e8cf4ad..e9a35d8fc67 100644 --- a/app/jobs/regular/pull_hotlinked_images.rb +++ b/app/jobs/regular/pull_hotlinked_images.rb @@ -84,11 +84,10 @@ module Jobs delay = SiteSetting.ninja_edit_window * args[:backoff] Jobs.enqueue_in(delay.seconds.to_i, :pull_hotlinked_images, args.merge!(backoff: backoff)) elsif raw != post.raw - options = { - edit_reason: I18n.t("upload.edit_reason"), - bypass_bump: true # we never want that job to bump the topic - } - post.revise(Discourse.system_user, raw, options) + changes = { raw: raw, edit_reason: I18n.t("upload.edit_reason") } + # we never want that job to bump the topic + options = { bypass_bump: true } + post.revise(Discourse.system_user, changes, options) end end diff --git a/app/models/post.rb b/app/models/post.rb index da6b98b9b0f..94e9d8789c8 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -324,8 +324,8 @@ class Post < ActiveRecord::Base end end - def revise(updated_by, new_raw, opts = {}) - PostRevisor.new(self).revise!(updated_by, new_raw, opts) + def revise(updated_by, changes={}, opts={}) + PostRevisor.new(self).revise!(updated_by, changes, opts) end def self.rebake_old(limit) @@ -364,13 +364,14 @@ class Post < ActiveRecord::Base end def set_owner(new_user, actor) - revise(actor, self.raw, { - new_user: new_user, - changed_owner: true, - edit_reason: I18n.t('change_owner.post_revision_text', - old_user: self.user.username_lower, - new_user: new_user.username_lower) - }) + return if user_id == new_user.id + + edit_reason = I18n.t('change_owner.post_revision_text', + old_user: self.user.username_lower, + new_user: new_user.username_lower + ) + + revise(actor, { raw: self.raw, user_id: new_user.id, edit_reason: edit_reason }) end before_create do @@ -414,14 +415,6 @@ class Post < ActiveRecord::Base self.baked_version = BAKED_VERSION end - after_save do - save_revision if self.version_changed? - end - - after_update do - update_revision if self.changed? - end - def advance_draft_sequence return if topic.blank? # could be deleted DraftSequence.next!(last_editor_id, topic.draft_key) @@ -537,33 +530,6 @@ class Post < ActiveRecord::Base end end - def save_revision - modifications = changes.extract!(:raw, :cooked, :edit_reason, :user_id, :wiki, :post_type) - # make sure cooked is always present (oneboxes might not change the cooked post) - modifications["cooked"] = [self.cooked, self.cooked] unless modifications["cooked"].present? - PostRevision.create!( - user_id: last_editor_id, - post_id: id, - number: version, - modifications: modifications - ) - end - - def update_revision - revision = PostRevision.find_by(post_id: id, number: version) - return unless revision - revision.user_id = last_editor_id - modifications = changes.extract!(:raw, :cooked, :edit_reason) - [:raw, :cooked, :edit_reason].each do |field| - if modifications[field].present? - old_value = revision.modifications[field].try(:[], 0) || "" - new_value = modifications[field][1] - revision.modifications[field] = [old_value, new_value] - end - end - revision.save - end - end # == Schema Information @@ -612,7 +578,7 @@ end # cook_method :integer default(1), not null # wiki :boolean default(FALSE), not null # via_email :boolean default(FALSE), not null -# raw_email :text +# raw_email :text # baked_at :datetime # baked_version :integer # hidden_at :datetime diff --git a/app/models/post_action.rb b/app/models/post_action.rb index 3e93c59f38d..6eec1ac6ff2 100644 --- a/app/models/post_action.rb +++ b/app/models/post_action.rb @@ -163,9 +163,7 @@ class PostAction < ActiveRecord::Base end def moderator_already_replied?(topic, moderator) - topic.posts - .where("user_id = :user_id OR post_type = :post_type", user_id: moderator.id, post_type: Post.types[:moderator_action]) - .exists? + topic.posts.where("user_id = :user_id OR post_type = :post_type", user_id: moderator.id, post_type: Post.types[:moderator_action]).exists? end def self.create_message_for_post_action(user, post, post_action_type_id, opts) diff --git a/app/models/post_alert_observer.rb b/app/models/post_alert_observer.rb index 221591e6c34..20fa9e67c9b 100644 --- a/app/models/post_alert_observer.rb +++ b/app/models/post_alert_observer.rb @@ -30,11 +30,13 @@ class PostAlertObserver < ActiveRecord::Observer post = post_action.post return if post_action.user.blank? - alerter.create_notification(post.user, - Notification.types[:liked], - post, - display_username: post_action.user.username, - post_action_id: post_action.id) + alerter.create_notification( + post.user, + Notification.types[:liked], + post, + display_username: post_action.user.username, + post_action_id: post_action.id + ) end def after_create_post_revision(post_revision) @@ -50,12 +52,11 @@ class PostAlertObserver < ActiveRecord::Observer post.user, Notification.types[:edited], post, - display_username: post_revision.user.username, - post_revision: post_revision + display_username: post_revision.user.username, + acting_user_id: post_revision.try(:user_id) ) end - protected def callback_for(action, model) diff --git a/app/models/post_revision.rb b/app/models/post_revision.rb index b9099942b39..1eb71926559 100644 --- a/app/models/post_revision.rb +++ b/app/models/post_revision.rb @@ -8,185 +8,30 @@ class PostRevision < ActiveRecord::Base def self.ensure_consistency! # 1 - fix the numbers - sql = <<-SQL + PostRevision.exec_sql <<-SQL UPDATE post_revisions SET number = pr.rank - FROM (SELECT id, ROW_NUMBER() OVER (PARTITION BY post_id ORDER BY number, created_at, updated_at) AS rank FROM post_revisions) AS pr + FROM (SELECT id, 1 + ROW_NUMBER() OVER (PARTITION BY post_id ORDER BY number, created_at, updated_at) AS rank FROM post_revisions) AS pr WHERE post_revisions.id = pr.id AND post_revisions.number <> pr.rank SQL - PostRevision.exec_sql(sql) - # 2 - fix the versions on the posts - sql = <<-SQL + PostRevision.exec_sql <<-SQL UPDATE posts - SET version = pv.version - FROM (SELECT post_id, MAX(number) AS version FROM post_revisions GROUP BY post_id) AS pv - WHERE posts.id = pv.post_id - AND posts.version <> pv.version + SET version = 1 + (SELECT COUNT(*) FROM post_revisions WHERE post_id = posts.id), + public_version = 1 + (SELECT COUNT(*) FROM post_revisions pr WHERE post_id = posts.id AND pr.hidden = 'f') + WHERE version <> 1 + (SELECT COUNT(*) FROM post_revisions WHERE post_id = posts.id) + OR public_version <> 1 + (SELECT COUNT(*) FROM post_revisions pr WHERE post_id = posts.id AND pr.hidden = 'f') SQL - - PostRevision.exec_sql(sql) - end - - def body_changes - cooked_diff = DiscourseDiff.new(previous("cooked"), current("cooked")) - raw_diff = DiscourseDiff.new(previous("raw"), current("raw")) - - { - inline: cooked_diff.inline_html, - side_by_side: cooked_diff.side_by_side_html, - side_by_side_markdown: raw_diff.side_by_side_markdown - } - end - - def category_changes - prev = previous("category_id") - cur = current("category_id") - return if prev == cur - - { - previous_category_id: prev, - current_category_id: cur, - } - end - - def wiki_changes - prev = previous("wiki") - cur = current("wiki") - return if prev == cur - - { - previous_wiki: prev, - current_wiki: cur, - } - end - - def post_type_changes - prev = previous("post_type") - cur = current("post_type") - return if prev == cur - - { - previous_post_type: prev, - current_post_type: cur, - } - end - - def title_changes - prev = "
#{CGI::escapeHTML(previous("title"))}
" - cur = "
#{CGI::escapeHTML(current("title"))}
" - return if prev == cur - - diff = DiscourseDiff.new(prev, cur) - - { - inline: diff.inline_html, - side_by_side: diff.side_by_side_html - } - end - - def user_changes - prev = previous("user_id") - cur = current("user_id") - return if prev == cur - - { - previous_user: User.find_by(id: prev), - current_user: User.find_by(id: cur) - } - end - - def previous(field) - val = lookup(field) - if val.nil? - val = lookup_in_previous_revisions(field) - end - - if val.nil? - val = lookup_in_post(field) - end - - val - end - - def current(field) - val = lookup_in_next_revision(field) - if val.nil? - val = lookup_in_post(field) - end - - if val.nil? - val = lookup(field) - end - - if val.nil? - val = lookup_in_previous_revisions(field) - end - - return val - end - - def previous_revisions - @previous_revs ||= PostRevision.where("post_id = ? AND number < ? AND hidden = ?", post_id, number, false) - .order("number desc") - .to_a - end - - def next_revision - @next_revision ||= PostRevision.where("post_id = ? AND number > ? AND hidden = ?", post_id, number, false) - .order("number asc") - .to_a.first - end - - def has_topic_data? - post && post.post_number == 1 - end - - def lookup_in_previous_revisions(field) - previous_revisions.each do |v| - val = v.lookup(field) - return val unless val.nil? - end - - nil - end - - def lookup_in_next_revision(field) - if next_revision - return next_revision.lookup(field) - end - end - - def lookup_in_post(field) - if !post - return - elsif ["cooked", "raw"].include?(field) - val = post.send(field) - elsif ["title", "category_id"].include?(field) - val = post.topic.send(field) - end - - val - end - - def lookup(field) - return nil if hidden - mod = modifications[field] - unless mod.nil? - mod[0] - end end def hide! - self.hidden = true - self.save! + update_column(:hidden, true) end def show! - self.hidden = false - self.save! + update_column(:hidden, false) end end diff --git a/app/models/topic.rb b/app/models/topic.rb index 5000bbf8662..4c3031c5302 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -99,7 +99,6 @@ class Topic < ActiveRecord::Base has_many :topic_invites has_many :invites, through: :topic_invites, source: :invite - has_many :revisions, foreign_key: :topic_id, class_name: 'TopicRevision' has_one :warning # When we want to temporarily attach some data to a forum topic (usually before serialization) @@ -148,70 +147,71 @@ class Topic < ActiveRecord::Base end attr_accessor :ignore_category_auto_close + attr_accessor :skip_callbacks before_create do - self.bumped_at ||= Time.now - self.last_post_user_id ||= user_id - - if !@ignore_category_auto_close and self.category and self.category.auto_close_hours and self.auto_close_at.nil? - self.auto_close_based_on_last_post = self.category.auto_close_based_on_last_post - set_auto_close(self.category.auto_close_hours) - end + initialize_default_values + inherit_auto_close_from_category end - attr_accessor :skip_callbacks - after_create do unless skip_callbacks changed_to_category(category) - if archetype == Archetype.private_message - DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE) - else - DraftSequence.next!(user, Draft::NEW_TOPIC) - end + advance_draft_sequence end end before_save do unless skip_callbacks - if (auto_close_at_changed? and !auto_close_at_was.nil?) or (auto_close_user_id_changed? and auto_close_at) - self.auto_close_started_at ||= Time.zone.now if auto_close_at - Jobs.cancel_scheduled_job(:close_topic, {topic_id: id}) - end - if category_id.nil? && (archetype.nil? || archetype == Archetype.default) - self.category_id = SiteSetting.uncategorized_category_id - end + cancel_auto_close_job + ensure_topic_has_a_category end end after_save do - save_revision if should_create_new_version? - unless skip_callbacks - if auto_close_at and (auto_close_at_changed? or auto_close_user_id_changed?) - Jobs.enqueue_at(auto_close_at, :close_topic, {topic_id: id, user_id: auto_close_user_id || user_id}) - end + schedule_auto_close_job end end - # TODO move into PostRevisor or TopicRevisor - def save_revision - if first_post_id = posts.where(post_number: 1).pluck(:id).first + def initialize_default_values + self.bumped_at ||= Time.now + self.last_post_user_id ||= user_id + end - number = PostRevision.where(post_id: first_post_id).count + 2 - PostRevision.create!( - user_id: acting_user.id, - post_id: first_post_id, - number: number, - modifications: changes.extract!(:category_id, :title) - ) - - Post.where(id: first_post_id).update_all(version: number) + def inherit_auto_close_from_category + if !@ignore_category_auto_close && self.category && self.category.auto_close_hours && self.auto_close_at.nil? + self.auto_close_based_on_last_post = self.category.auto_close_based_on_last_post + set_auto_close(self.category.auto_close_hours) end end - def should_create_new_version? - !new_record? && (category_id_changed? || title_changed?) + def advance_draft_sequence + if archetype == Archetype.private_message + DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE) + else + DraftSequence.next!(user, Draft::NEW_TOPIC) + end + end + + def cancel_auto_close_job + if (auto_close_at_changed? && !auto_close_at_was.nil?) || (auto_close_user_id_changed? && auto_close_at) + self.auto_close_started_at ||= Time.zone.now if auto_close_at + Jobs.cancel_scheduled_job(:close_topic, { topic_id: id }) + end + end + + def schedule_auto_close_job + if auto_close_at && (auto_close_at_changed? || auto_close_user_id_changed?) + options = { topic_id: id, user_id: auto_close_user_id || user_id } + Jobs.enqueue_at(auto_close_at, :close_topic, options) + end + end + + def ensure_topic_has_a_category + if category_id.nil? && (archetype.nil? || archetype == Archetype.default) + self.category_id = SiteSetting.uncategorized_category_id + end end def self.top_viewed(max = 10) @@ -266,10 +266,6 @@ class Topic < ActiveRecord::Base Redcarpet::Render::SmartyPants.render(sanitized_title) end - def new_version_required? - title_changed? || category_id_changed? - end - # Returns hot topics since a date for display in email digest. def self.for_digest(user, since, opts=nil) opts = opts || {} @@ -383,10 +379,8 @@ class Topic < ActiveRecord::Base candidate_ids = candidates.pluck(:id) - return [] unless candidate_ids.present? - similar = Topic.select(sanitize_sql_array(["topics.*, similarity(topics.title, :title) + similarity(topics.title, :raw) AS similarity", title: title, raw: raw])) .joins("JOIN posts AS p ON p.topic_id = topics.id AND p.post_number = 1") .limit(SiteSetting.max_similar_results) @@ -451,37 +445,30 @@ class Topic < ActiveRecord::Base (topics.avg_time <> x.gmean OR topics.avg_time IS NULL)") if min_topic_age - builder.where("topics.bumped_at > :bumped_at", - bumped_at: min_topic_age) + builder.where("topics.bumped_at > :bumped_at", bumped_at: min_topic_age) end builder.exec end - def changed_to_category(cat) - return true if cat.blank? || Category.find_by(topic_id: id).present? + def changed_to_category(new_category) + return true if new_category.blank? || Category.find_by(topic_id: id).present? + return false if new_category.id == SiteSetting.uncategorized_category_id && !SiteSetting.allow_uncategorized_topics Topic.transaction do old_category = category - if category_id.present? && category_id != cat.id - Category.where(['id = ?', category_id]).update_all 'topic_count = topic_count - 1' + if self.category_id != new_category.id + self.category_id = new_category.id + self.update_column(:category_id, new_category.id) + Category.where(id: old_category.id).update_all("topic_count = topic_count - 1") if old_category end - success = true - if self.category_id != cat.id - self.category_id = cat.id - success = save - end - - if success - CategoryFeaturedTopic.feature_topics_for(old_category) unless @import_mode - Category.where(id: cat.id).update_all 'topic_count = topic_count + 1' - CategoryFeaturedTopic.feature_topics_for(cat) unless @import_mode || old_category.try(:id) == cat.try(:id) - else - return false - end + Category.where(id: new_category.id).update_all("topic_count = topic_count + 1") + CategoryFeaturedTopic.feature_topics_for(old_category) unless @import_mode + CategoryFeaturedTopic.feature_topics_for(new_category) unless @import_mode || old_category.id == new_category.id end + true end @@ -513,15 +500,15 @@ class Topic < ActiveRecord::Base def change_category_to_id(category_id) return false if private_message? - # If the category name is blank, reset the attribute - if (category_id.nil? || category_id.to_i == 0) - cat = Category.find_by(id: SiteSetting.uncategorized_category_id) - else - cat = Category.where(id: category_id).first - end + new_category_id = category_id.to_i + # if the category name is blank, reset the attribute + new_category_id = SiteSetting.uncategorized_category_id if new_category_id == 0 - return true if cat == category + return true if self.category_id == new_category_id + + cat = Category.find_by(id: new_category_id) return false unless cat + changed_to_category(cat) end diff --git a/app/models/topic_embed.rb b/app/models/topic_embed.rb index 662e284cf8b..c9dcc09bf4d 100644 --- a/app/models/topic_embed.rb +++ b/app/models/topic_embed.rb @@ -50,8 +50,7 @@ class TopicEmbed < ActiveRecord::Base post = embed.post # Update the topic if it changed if post && post.topic && content_sha1 != embed.content_sha1 - revisor = PostRevisor.new(post) - revisor.revise!(user, absolutize_urls(url, contents), skip_validations: true, bypass_rate_limiter: true) + post.revise(user, { raw: absolutize_urls(url, contents) }, skip_validations: true, bypass_rate_limiter: true) embed.update_column(:content_sha1, content_sha1) end end diff --git a/app/models/user_action_observer.rb b/app/models/user_action_observer.rb index d170853e9c2..45fba906458 100644 --- a/app/models/user_action_observer.rb +++ b/app/models/user_action_observer.rb @@ -33,7 +33,7 @@ class UserActionObserver < ActiveRecord::Observer end end - def self.log_notification(post, user, notification_type, acting_user_id = nil) + def self.log_notification(post, user, notification_type, acting_user_id=nil) action = case notification_type when Notification.types[:quoted] diff --git a/app/serializers/post_revision_serializer.rb b/app/serializers/post_revision_serializer.rb index 4f50c3ebcf4..637f0745387 100644 --- a/app/serializers/post_revision_serializer.rb +++ b/app/serializers/post_revision_serializer.rb @@ -1,38 +1,70 @@ class PostRevisionSerializer < ApplicationSerializer - attributes :post_id, - :version, - :revisions_count, + + attributes :created_at, + :post_id, + # which revision is hidden + :previous_hidden, + :current_hidden, + # dynamic & based on the current scope + :first_revision, + :previous_revision, + :current_revision, + :next_revision, + :last_revision, + # used for display + :current_version, + :version_count, + # from the user :username, :display_username, :avatar_template, - :created_at, + # all the changes :edit_reason, :body_changes, :title_changes, :category_changes, :user_changes, :wiki_changes, - :post_type_changes, - :hidden + :post_type_changes - def include_title_changes? - object.has_topic_data? + def previous_hidden + previous["hidden"] end - def include_category_changes? - object.has_topic_data? + def current_hidden + current["hidden"] end - def hidden - object.hidden + def first_revision + revisions.first["revision"] end - def version + def previous_revision + @previous_revision ||= revisions.select { |r| r["revision"] >= first_revision } + .select { |r| r["revision"] < current_revision } + .last.try(:[], "revision") + end + + def current_revision object.number end - def revisions_count - object.post.version + def next_revision + @next_revision ||= revisions.select { |r| r["revision"] <= last_revision } + .select { |r| r["revision"] > current_revision } + .first.try(:[], "revision") + end + + def last_revision + @last_revision ||= revisions.select { |r| r["revision"] <= post.version }.last["revision"] + end + + def current_version + @current_version ||= revisions.select { |r| r["revision"] <= current_revision }.count + 1 + end + + def version_count + revisions.count end def username @@ -48,32 +80,163 @@ class PostRevisionSerializer < ApplicationSerializer end def edit_reason - object.current("edit_reason") + # only show 'edit_reason' when revisions are consecutive + current["edit_reason"] if scope.can_view_hidden_post_revisions? || + current["revision"] == previous["revision"] + 1 + end + + def body_changes + cooked_diff = DiscourseDiff.new(previous["cooked"], current["cooked"]) + raw_diff = DiscourseDiff.new(previous["raw"], current["raw"]) + + { + inline: cooked_diff.inline_html, + side_by_side: cooked_diff.side_by_side_html, + side_by_side_markdown: raw_diff.side_by_side_markdown + } + end + + def title_changes + prev = "
#{CGI::escapeHTML(previous["title"])}
" + cur = "
#{CGI::escapeHTML(current["title"])}
" + return if prev == cur + + diff = DiscourseDiff.new(prev, cur) + + { + inline: diff.inline_html, + side_by_side: diff.side_by_side_html + } + end + + def category_changes + prev = previous["category_id"] + cur = current["category_id"] + return if prev == cur + + { + previous: prev, + current: cur, + } end def user_changes - obj = object.user_changes - return unless obj - # same as below - if stuff is messed up, default to system - prev = obj[:previous_user] || Discourse.system_user - new = obj[:current_user] || Discourse.system_user + prev = previous["user_id"] + cur = current["user_id"] + return if prev == cur + + # if stuff is messed up, default to system + previous = User.find_by(id: prev) || Discourse.system_user + current = User.find_by(id: cur) || Discourse.system_user + { previous: { - username: prev.username_lower, - display_username: prev.username, - avatar_template: prev.avatar_template + username: previous.username_lower, + display_username: previous.username, + avatar_template: previous.avatar_template }, current: { - username: new.username_lower, - display_username: new.username, - avatar_template: new.avatar_template + username: current.username_lower, + display_username: current.username, + avatar_template: current.avatar_template } } end - def user - # if stuff goes pear shape attribute to system - object.user || Discourse.system_user + def wiki_changes + prev = previous["wiki"] + cur = current["wiki"] + return if prev == cur + + { + previous: prev, + current: cur, + } end + def post_type_changes + prev = previous["post_type"] + cur = current["post_type"] + return if prev == cur + + { + previous: prev, + current: cur, + } + end + + protected + + def post + @post ||= object.post + end + + def topic + @topic ||= object.post.topic + end + + def revisions + @revisions ||= all_revisions.select { |r| scope.can_view_hidden_post_revisions? || !r["hidden"] } + end + + def all_revisions + return @all_revisions if @all_revisions + + post_revisions = PostRevision.where(post_id: object.post_id).order(:number).to_a + post_revisions << PostRevision.new( + number: post_revisions.last.number + 1, + hidden: post.hidden, + modifications: { + "raw" => [post.raw], + "cooked" => [post.cooked], + "edit_reason" => [post.edit_reason], + "wiki" => [post.wiki], + "post_type" => [post.post_type], + "user_id" => [post.user_id], + "title" => [topic.title], + "category_id" => [topic.category_id], + } + ) + + @all_revisions = [] + + # backtrack + post_revisions.each do |pr| + revision = HashWithIndifferentAccess.new + revision[:revision] = pr.number + revision[:hidden] = pr.hidden + + pr.modifications.keys.each do |field| + revision[field] = pr.modifications[field][0] + end + + @all_revisions << revision + end + + # waterfall + (@all_revisions.count - 1).downto(1).each do |r| + cur = @all_revisions[r] + prev = @all_revisions[r - 1] + + cur.keys.each do |field| + prev[field] = prev.has_key?(field) ? prev[field] : cur[field] + end + end + + @all_revisions + end + + def previous + @previous ||= revisions.select { |r| r["revision"] <= current_revision }.last + end + + def current + @current ||= revisions.select { |r| r["revision"] > current_revision }.first + end + + def user + # if stuff goes pear shape attribute to system + object.user || Discourse.system_user + end + end diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 3ee7ba875c1..b6ca0ba9212 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -233,7 +233,7 @@ class PostSerializer < BasicPostSerializer end def can_view_edit_history - scope.can_view_post_revisions?(object) + scope.can_view_edit_history?(object) end def user_custom_fields @@ -258,6 +258,10 @@ class PostSerializer < BasicPostSerializer object.via_email? end + def version + scope.is_staff? ? object.version : object.public_version + end + private def post_actions diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index b0f178a748b..3361d0291c5 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -94,22 +94,20 @@ class PostAlerter .order("notifications.id desc") .find_by(notification_type: type, topic_id: post.topic_id, post_number: post.post_number) - if(existing_notification) + if existing_notification return unless existing_notification.notification_type == Notification.types[:edited] && existing_notification.data_hash["display_username"] = opts[:display_username] end collapsed = false - if type == Notification.types[:replied] || - type == Notification.types[:posted] - + if type == Notification.types[:replied] || type == Notification.types[:posted] destroy_notifications(user, Notification.types[:replied] , post.topic) destroy_notifications(user, Notification.types[:posted] , post.topic) collapsed = true end - if type == Notification.types[:private_message] + if type == Notification.types[:private_message] destroy_notifications(user, type, post.topic) collapsed = true end @@ -123,7 +121,7 @@ class PostAlerter opts[:display_username] = I18n.t('embed.replies', count: count) if count > 1 end - UserActionObserver.log_notification(original_post, user, type, opts[:post_revision].try(:user_id)) + UserActionObserver.log_notification(original_post, user, type, opts[:acting_user_id]) # Create the notification user.notifications.create(notification_type: type, diff --git a/config/routes.rb b/config/routes.rb index 452eda63fa0..01cfb20928c 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -282,9 +282,10 @@ Discourse::Application.routes.draw do put "rebake" put "unhide" get "replies" - get "revisions/:revision" => "posts#revisions" - put "revisions/:revision/hide" => "posts#hide_revision" - put "revisions/:revision/show" => "posts#show_revision" + get "revisions/latest" => "posts#latest_revision" + get "revisions/:revision" => "posts#revisions", constraints: { revision: /\d+/ } + put "revisions/:revision/hide" => "posts#hide_revision", constraints: { revision: /\d+/ } + put "revisions/:revision/show" => "posts#show_revision", constraints: { revision: /\d+/ } put "recover" collection do delete "destroy_many" diff --git a/db/migrate/20141020153415_add_public_version_to_posts.rb b/db/migrate/20141020153415_add_public_version_to_posts.rb new file mode 100644 index 00000000000..52cb65f41ac --- /dev/null +++ b/db/migrate/20141020153415_add_public_version_to_posts.rb @@ -0,0 +1,15 @@ +class AddPublicVersionToPosts < ActiveRecord::Migration + def up + add_column :posts, :public_version, :integer, null: false, default: 1 + + execute <<-SQL + UPDATE posts + SET public_version = 1 + (SELECT COUNT(*) FROM post_revisions pr WHERE post_id = posts.id AND pr.hidden = 'f') + WHERE public_version <> 1 + (SELECT COUNT(*) FROM post_revisions pr WHERE post_id = posts.id AND pr.hidden = 'f') + SQL + end + + def down + remove_column :posts, :public_version + end +end diff --git a/lib/discourse_observer.rb b/lib/discourse_observer.rb deleted file mode 100644 index 51161bc7d6c..00000000000 --- a/lib/discourse_observer.rb +++ /dev/null @@ -1,47 +0,0 @@ -# -# Support delegating after_create to an appropriate helper for that class name. -# For example, an observer on post will call after_create_post if that method -# is defined. -# -# It does this after_commit by default, and contains a hack to make this work -# even in test mode. -# -class DiscourseObserver < ActiveRecord::Observer - - def after_create_delegator(model) - observer_method = :"after_create_#{model.class.name.underscore}" - send(observer_method, model) if respond_to?(observer_method) - end - - def after_destroy_delegator(model) - observer_method = :"after_destroy_#{model.class.name.underscore}" - send(observer_method, model) if respond_to?(observer_method) - end - -end - -if Rails.env.test? - - # In test mode, call the delegator right away - class DiscourseObserver < ActiveRecord::Observer - alias_method :after_create, :after_create_delegator - alias_method :after_destroy, :after_destroy_delegator - end - -else - - # Outside of test mode, use after_commit - class DiscourseObserver < ActiveRecord::Observer - def after_commit(model) - if model.send(:transaction_include_any_action?, [:create]) - after_create_delegator(model) - end - - if model.send(:transaction_include_any_action?, [:destroy]) - after_destroy_delegator(model) - end - end - end - -end - diff --git a/lib/guardian.rb b/lib/guardian.rb index 0962c610889..5fab43dfd48 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -3,6 +3,7 @@ require_dependency 'guardian/ensure_magic' require_dependency 'guardian/post_guardian' require_dependency 'guardian/topic_guardian' require_dependency 'guardian/user_guardian' +require_dependency 'guardian/post_revision_guardian' # The guardian is responsible for confirming access to various site resources and operations class Guardian @@ -11,6 +12,7 @@ class Guardian include PostGuardian include TopicGuardian include UserGuardian + include PostRevisionGuardian class AnonymousUser def blank?; true; end diff --git a/lib/guardian/post_guardian.rb b/lib/guardian/post_guardian.rb index 7eae1a89f3b..02e1e8827f5 100644 --- a/lib/guardian/post_guardian.rb +++ b/lib/guardian/post_guardian.rb @@ -140,12 +140,7 @@ module PostGuardian can_see_topic?(post.topic))) end - def can_see_post_revision?(post_revision) - return false unless post_revision - can_view_post_revisions?(post_revision.post) - end - - def can_view_post_revisions?(post) + def can_view_edit_history?(post) return false unless post if !post.hidden @@ -157,14 +152,6 @@ module PostGuardian can_see_post?(post) end - def can_hide_post_revision?(post_revision) - is_staff? - end - - def can_show_post_revision?(post_revision) - is_staff? - end - def can_vote?(post, opts={}) post_can_act?(post,:vote, opts) end diff --git a/lib/guardian/post_revision_guardian.rb b/lib/guardian/post_revision_guardian.rb new file mode 100644 index 00000000000..9b9d1b9caf8 --- /dev/null +++ b/lib/guardian/post_revision_guardian.rb @@ -0,0 +1,23 @@ +# mixin for all Guardian methods dealing with post_revisions permissions +module PostRevisionGuardian + + def can_see_post_revision?(post_revision) + return false unless post_revision + return false if post_revision.hidden && !can_view_hidden_post_revisions? + + can_view_edit_history?(post_revision.post) + end + + def can_hide_post_revision?(post_revision) + is_staff? + end + + def can_show_post_revision?(post_revision) + is_staff? + end + + def can_view_hidden_post_revisions? + is_staff? + end + +end diff --git a/lib/post_destroyer.rb b/lib/post_destroyer.rb index bdcf6058e27..28357395547 100644 --- a/lib/post_destroyer.rb +++ b/lib/post_destroyer.rb @@ -96,7 +96,7 @@ class PostDestroyer # When a user 'deletes' their own post. We just change the text. def mark_for_deletion Post.transaction do - @post.revise(@user, I18n.t('js.post.deleted_by_author', count: SiteSetting.delete_removed_posts_after), force_new_version: true) + @post.revise(@user, { raw: I18n.t('js.post.deleted_by_author', count: SiteSetting.delete_removed_posts_after) }, force_new_version: true) @post.update_column(:user_deleted, true) @post.update_flagged_posts_count @post.topic_links.each(&:destroy) @@ -110,7 +110,7 @@ class PostDestroyer Post.transaction do @post.update_column(:user_deleted, false) @post.skip_unique_check = true - @post.revise(@user, @post.revisions.last.modifications["raw"][0], force_new_version: true) + @post.revise(@user, { raw: @post.revisions.last.modifications["raw"][0] }, force_new_version: true) @post.update_flagged_posts_count end diff --git a/lib/post_revisor.rb b/lib/post_revisor.rb index 33726de7c29..e100f4df0c5 100644 --- a/lib/post_revisor.rb +++ b/lib/post_revisor.rb @@ -1,138 +1,269 @@ -require 'edit_rate_limiter' +require "edit_rate_limiter" class PostRevisor + POST_TRACKED_FIELDS = %w{raw cooked edit_reason user_id wiki post_type} + TOPIC_TRACKED_FIELDS = %w{title category_id} + attr_reader :category_changed - def initialize(post) + def initialize(post, topic=nil) @post = post + @topic = topic || post.topic end - # Recognized options: - # :edit_reason User-supplied edit reason - # :new_user New owner of the post - # :revised_at changes the date of the revision - # :force_new_version bypass ninja-edit window - # :bypass_bump do not bump the topic, even if last post - # :skip_validation ask ActiveRecord to skip validations - # - def revise!(editor, new_raw, opts = {}) + # AVAILABLE OPTIONS: + # - revised_at: changes the date of the revision + # - force_new_version: bypass ninja-edit window + # - bypass_rate_limiter: + # - bypass_bump: do not bump the topic, even if last post + # - skip_validations: ask ActiveRecord to skip validations + def revise!(editor, fields, opts={}) @editor = editor + @fields = fields.with_indifferent_access @opts = opts - @new_raw = TextCleaner.normalize_whitespaces(new_raw).gsub(/\s+\z/, "") + + # some normalization + @fields[:raw] = cleanup_whitespaces(@fields[:raw]) if @fields.has_key?(:raw) + @fields[:user_id] = @fields[:user_id].to_i if @fields.has_key?(:user_id) + @fields[:category_id] = @fields[:category_id].to_i if @fields.has_key?(:category_id) + + # always reset edit_reason unless provided + @fields[:edit_reason] = nil unless @fields.has_key?(:edit_reason) return false unless should_revise? @post.acting_user = @editor + @topic.acting_user = @editor + @revised_at = @opts[:revised_at] || Time.now + @last_version_at = @post.last_version_at || Time.now + + @version_changed = false + @post_successfully_saved = true + @topic_successfully_saved = true Post.transaction do revise_post - # TODO these callbacks are being called in a transaction - # it is kind of odd, cause the callback is called before_edit - # but the post is already edited at this point - # trouble is that much of the logic of should I edit? is deeper - # down so yanking this in front of the transaction will lead to - # false positives. This system needs a review + # TODO: these callbacks are being called in a transaction + # it is kind of odd, because the callback is called "before_edit" + # but the post is already edited at this point + # Trouble is that much of the logic of should I edit? is deeper + # down so yanking this in front of the transaction will lead to + # false positive. plugin_callbacks - update_category_description - update_topic_excerpt - @post.advance_draft_sequence + revise_topic + advance_draft_sequence end - # WARNING: do not pull this into the transaction, it can fire events in - # sidekiq before the post is done saving leading to corrupt state + # WARNING: do not pull this into the transaction + # it can fire events in sidekiq before the post is done saving + # leading to corrupt state post_process_post + update_topic_word_counts + alert_users + publish_changes + grant_badge - PostAlerter.new.after_save_post(@post) - - @post.publish_change_to_clients! :revised - BadgeGranter.queue_badge_grant(Badge::Trigger::PostRevision, post: @post) - - true + @post_successfully_saved && @topic_successfully_saved end - private + def cleanup_whitespaces(raw) + TextCleaner.normalize_whitespaces(raw).gsub(/\s+\z/, "") + end def should_revise? - @post.raw != @new_raw || @opts[:changed_owner] + post_changed? || topic_changed? + end + + def post_changed? + POST_TRACKED_FIELDS.each do |field| + return true if @fields.has_key?(field) && @fields[field] != @post.send(field) + end + false + end + + def topic_changed? + TOPIC_TRACKED_FIELDS.each do |field| + return true if @fields.has_key?(field) && @fields[field] != @topic.send(field) + end + false end def revise_post if should_create_new_version? revise_and_create_new_version else - update_post + revise end end - def plugin_callbacks - DiscourseEvent.trigger :before_edit_post, @post - DiscourseEvent.trigger :validate_post, @post - end - - def get_revised_at - @opts[:revised_at] || Time.now - end - def should_create_new_version? - @post.last_editor_id != @editor.id || - get_revised_at - @post.last_version_at > SiteSetting.ninja_edit_window.to_i || - @opts[:changed_owner] == true || + edited_by_another_user? || !ninja_edit? || owner_changed? || force_new_version? + end + + def edited_by_another_user? + @post.last_editor_id != @editor.id + end + + def ninja_edit? + @revised_at - @last_version_at <= SiteSetting.ninja_edit_window.to_i + end + + def owner_changed? + @fields.has_key?(:user_id) && @fields[:user_id] != @post.user_id + end + + def force_new_version? @opts[:force_new_version] == true end def revise_and_create_new_version + @version_changed = true @post.version += 1 - @post.last_version_at = get_revised_at + @post.public_version += 1 + @post.last_version_at = @revised_at + + revise + perform_edit + bump_topic + end + + def revise update_post - EditRateLimiter.new(@editor).performed! unless @opts[:bypass_rate_limiter] == true - bump_topic unless @opts[:bypass_bump] - end - - def bump_topic - unless Post.where('post_number > ? and topic_id = ?', @post.post_number, @post.topic_id).exists? - @post.topic.update_column(:bumped_at, Time.now) - TopicTrackingState.publish_latest(@post.topic) - end - end - - def update_topic_word_counts - Topic.exec_sql("UPDATE topics SET word_count = (SELECT SUM(COALESCE(posts.word_count, 0)) - FROM posts WHERE posts.topic_id = :topic_id) - WHERE topics.id = :topic_id", topic_id: @post.topic_id) + update_topic if topic_changed? + create_or_update_revision end def update_post - @post.raw = @new_raw - @post.word_count = @new_raw.scan(/\w+/).size - @post.last_editor_id = @editor.id - @post.edit_reason = @opts[:edit_reason] if @opts[:edit_reason] - @post.user_id = @opts[:new_user].id if @opts[:new_user] - @post.self_edits += 1 if @editor == @post.user - - if @editor == @post.user && @post.hidden && @post.hidden_reason_id == Post.hidden_reasons[:flag_threshold_reached] - PostAction.clear_flags!(@post, Discourse.system_user) - @post.unhide! + POST_TRACKED_FIELDS.each do |field| + @post.send("#{field}=", @fields[field]) if @fields.has_key?(field) end - @post.extract_quoted_post_numbers - @post.save(validate: !@opts[:skip_validations]) + @post.last_editor_id = @editor.id + @post.word_count = @fields[:raw].scan(/\w+/).size if @fields.has_key?(:raw) + @post.self_edits += 1 if self_edit? + clear_flags_and_unhide_post + + @post.extract_quoted_post_numbers + @post_successfully_saved = @post.save(validate: !@opts[:skip_validations]) @post.save_reply_relationships end - def update_category_description - # If we're revising the first post, we might have to update the category description + def self_edit? + @editor == @post.user + end + + def clear_flags_and_unhide_post + return unless editing_a_flagged_and_hidden_post? + PostAction.clear_flags!(@post, Discourse.system_user) + @post.unhide! + end + + def editing_a_flagged_and_hidden_post? + self_edit? && + @post.hidden && + @post.hidden_reason_id == Post.hidden_reasons[:flag_threshold_reached] + end + + def update_topic + @topic.title = @fields[:title] if @fields.has_key?(:title) + Topic.transaction do + @topic_successfully_saved = @topic.change_category_to_id(@fields[:category_id]) if @fields.has_key?(:category_id) + @topic_successfully_saved &&= @topic.save(validate: !@opts[:skip_validations]) + end + end + + def create_or_update_revision + if @version_changed + create_revision + else + update_revision + end + end + + def create_revision + modifications = post_changes.merge(topic_changes) + PostRevision.create!( + user_id: @post.last_editor_id, + post_id: @post.id, + number: @post.version, + modifications: modifications + ) + end + + def update_revision + return unless revision = PostRevision.find_by(post_id: @post.id, number: @post.version) + revision.user_id = @post.last_editor_id + modifications = post_changes.merge(topic_changes) + modifications.keys.each do |field| + if revision.modifications.has_key?(field) + old_value = revision.modifications[field][0] + new_value = modifications[field][1] + revision.modifications[field] = [old_value, new_value] + else + revision.modifications[field] = modifications[field] + end + end + revision.save + end + + def post_changes + @post.previous_changes.slice(*POST_TRACKED_FIELDS) + end + + def topic_changes + @topic.previous_changes.slice(*TOPIC_TRACKED_FIELDS) + end + + def perform_edit + return if bypass_rate_limiter? + EditRateLimiter.new(@editor).performed! + end + + def bypass_rate_limiter? + @opts[:bypass_rate_limiter] == true + end + + def bump_topic + return if bypass_bump? || !is_last_post? + @topic.update_column(:bumped_at, Time.now) + TopicTrackingState.publish_latest(@topic) + end + + def bypass_bump? + @opts[:bypass_bump] == true + end + + def is_last_post? + !Post.where(topic_id: @topic.id) + .where("post_number > ?", @post.post_number) + .exists? + end + + def plugin_callbacks + DiscourseEvent.trigger(:before_edit_post, @post) + DiscourseEvent.trigger(:validate_post, @post) + end + + def revise_topic return unless @post.post_number == 1 - # Is there a category with our topic id? - category = Category.find_by(topic_id: @post.topic_id) - return unless category.present? + update_topic_excerpt + update_category_description + end + + def update_topic_excerpt + excerpt = @post.excerpt(220, strip_links: true) + @topic.update_column(:excerpt, excerpt) + end + + def update_category_description + return unless category = Category.find_by(topic_id: @topic.id) - # If found, update its description body = @post.cooked matches = body.scan(/\(.*)\<\/p\>/) if matches && matches[0] && matches[0][0] @@ -143,12 +274,35 @@ class PostRevisor end end - def update_topic_excerpt - @post.topic.update_column(:excerpt, @post.excerpt(220, strip_links: true)) if @post.post_number == 1 + def advance_draft_sequence + @post.advance_draft_sequence end def post_process_post @post.invalidate_oneboxes = true @post.trigger_post_process end + + def update_topic_word_counts + Topic.exec_sql("UPDATE topics + SET word_count = ( + SELECT SUM(COALESCE(posts.word_count, 0)) + FROM posts + WHERE posts.topic_id = :topic_id + ) + WHERE topics.id = :topic_id", topic_id: @topic.id) + end + + def alert_users + PostAlerter.new.after_save_post(@post) + end + + def publish_changes + @post.publish_change_to_clients!(:revised) + end + + def grant_badge + BadgeGranter.queue_badge_grant(Badge::Trigger::PostRevision, post: @post) + end + end diff --git a/lib/tasks/posts.rake b/lib/tasks/posts.rake index 8e7b2f014e8..082c9d23db3 100644 --- a/lib/tasks/posts.rake +++ b/lib/tasks/posts.rake @@ -48,7 +48,7 @@ task 'posts:normalize_code' => :environment do Post.where("raw like '%
%%'").each do |p|
     normalized = Import::Normalize.normalize_code_blocks(p.raw, lang)
     if normalized != p.raw
-      p.revise(Discourse.system_user, normalized)
+      p.revise(Discourse.system_user, { raw: normalized })
       putc "."
       i += 1
     end
diff --git a/script/import_scripts/phpbb3.rb b/script/import_scripts/phpbb3.rb
index b4fb13f1af2..234113362c3 100644
--- a/script/import_scripts/phpbb3.rb
+++ b/script/import_scripts/phpbb3.rb
@@ -101,7 +101,7 @@ class ImportScripts::PhpBB3 < ImportScripts::Base
                   puts "Could not import avatar: #{err.message}"
               end
             end
-          end 
+          end
         }
       end
     end
@@ -413,7 +413,7 @@ class ImportScripts::PhpBB3 < ImportScripts::Base
       end
 
       if new_raw != post.raw
-        PostRevisor.new(post).revise!(post.user, new_raw, {bypass_bump: true, edit_reason: 'Migrate from PHPBB3'})
+        PostRevisor.new(post).revise!(post.user, { raw: new_raw }, { bypass_bump: true, edit_reason: 'Migrate from PHPBB3' })
       end
     end
 
diff --git a/spec/components/category_list_spec.rb b/spec/components/category_list_spec.rb
index 51b1b44ec2b..92475f0748c 100644
--- a/spec/components/category_list_spec.rb
+++ b/spec/components/category_list_spec.rb
@@ -86,7 +86,7 @@ describe CategoryList do
     end
 
     context "with a topic in a category" do
-      let!(:topic) { Fabricate(:topic, category: topic_category)}
+      let!(:topic) { Fabricate(:topic, category: topic_category) }
       let(:category) { category_list.categories.first }
 
       it "should return the category" do
diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb
index 90769440171..b1d710348e7 100644
--- a/spec/components/post_revisor_spec.rb
+++ b/spec/components/post_revisor_spec.rb
@@ -16,7 +16,7 @@ describe PostRevisor do
     describe 'with the same body' do
       it "doesn't change version" do
         lambda {
-          subject.revise!(post.user, post.raw).should == false
+          subject.revise!(post.user, { raw: post.raw }).should == false
           post.reload
         }.should_not change(post, :version)
       end
@@ -25,10 +25,11 @@ describe PostRevisor do
     describe 'ninja editing' do
       it 'correctly applies edits' do
         SiteSetting.ninja_edit_window = 1.minute.to_i
-        subject.revise!(post.user, 'updated body', revised_at: post.updated_at + 10.seconds)
+        subject.revise!(post.user, { raw: 'updated body' }, revised_at: post.updated_at + 10.seconds)
         post.reload
 
         post.version.should == 1
+        post.public_version.should == 1
         post.revisions.size.should == 0
         post.last_version_at.should == first_version_at
         subject.category_changed.should be_blank
@@ -41,7 +42,7 @@ describe PostRevisor do
 
       before do
         SiteSetting.stubs(:ninja_edit_window).returns(1.minute.to_i)
-        subject.revise!(post.user, 'updated body', revised_at: revised_at)
+        subject.revise!(post.user, { raw: 'updated body' }, revised_at: revised_at)
         post.reload
       end
 
@@ -49,11 +50,12 @@ describe PostRevisor do
         subject.category_changed.should be_blank
       end
 
-      it 'updates the version' do
+      it 'updates the versions' do
         post.version.should == 2
+        post.public_version.should == 2
       end
 
-      it 'creates a new version' do
+      it 'creates a new revision' do
         post.revisions.size.should == 1
       end
 
@@ -64,12 +66,13 @@ describe PostRevisor do
       describe "new edit window" do
 
         before do
-          subject.revise!(post.user, 'yet another updated body', revised_at: revised_at)
+          subject.revise!(post.user, { raw: 'yet another updated body' }, revised_at: revised_at)
           post.reload
         end
 
         it "doesn't create a new version if you do another" do
           post.version.should == 2
+          post.public_version.should == 2
         end
 
         it "doesn't change last_version_at" do
@@ -85,12 +88,13 @@ describe PostRevisor do
           let!(:new_revised_at) {revised_at + 2.minutes}
 
           before do
-            subject.revise!(post.user, 'yet another, another updated body', revised_at: new_revised_at)
+            subject.revise!(post.user, { raw: 'yet another, another updated body' }, revised_at: new_revised_at)
             post.reload
           end
 
           it "does create a new version after the edit window" do
             post.version.should == 3
+            post.public_version.should == 3
           end
 
           it "does create a new version after the edit window" do
@@ -116,7 +120,7 @@ describe PostRevisor do
 
       context "one paragraph description" do
         before do
-          subject.revise!(post.user, new_description)
+          subject.revise!(post.user, { raw: new_description })
           category.reload
         end
 
@@ -131,7 +135,7 @@ describe PostRevisor do
 
       context "multiple paragraph description" do
         before do
-          subject.revise!(post.user, "#{new_description}\n\nOther content goes here.")
+          subject.revise!(post.user, { raw: "#{new_description}\n\nOther content goes here." })
           category.reload
         end
 
@@ -147,7 +151,7 @@ describe PostRevisor do
       context 'when updating back to the original paragraph' do
         before do
           category.update_column(:description, 'this is my description')
-          subject.revise!(post.user, Category.post_template)
+          subject.revise!(post.user, { raw: Category.post_template })
           category.reload
         end
 
@@ -167,7 +171,7 @@ describe PostRevisor do
 
       it "triggers a rate limiter" do
         EditRateLimiter.any_instance.expects(:performed!)
-        subject.revise!(changed_by, 'updated body')
+        subject.revise!(changed_by, { raw: 'updated body' })
       end
     end
 
@@ -178,7 +182,7 @@ describe PostRevisor do
         SiteSetting.stubs(:newuser_max_images).returns(0)
         url = "http://i.imgur.com/wfn7rgU.jpg"
         Oneboxer.stubs(:onebox).with(url, anything).returns("")
-        subject.revise!(changed_by, "So, post them here!\n#{url}")
+        subject.revise!(changed_by, { raw: "So, post them here!\n#{url}" })
       end
 
       it "allows an admin to insert images into a new user's post" do
@@ -196,7 +200,7 @@ describe PostRevisor do
         SiteSetting.stubs(:newuser_max_images).returns(0)
         url = "http://i.imgur.com/FGg7Vzu.gif"
         Oneboxer.stubs(:cached_onebox).with(url, anything).returns("")
-        subject.revise!(post.user, "So, post them here!\n#{url}")
+        subject.revise!(post.user, { raw: "So, post them here!\n#{url}" })
       end
 
       it "doesn't allow images to be inserted" do
@@ -208,7 +212,7 @@ describe PostRevisor do
 
     describe 'with a new body' do
       let(:changed_by) { Fabricate(:coding_horror) }
-      let!(:result) { subject.revise!(changed_by, "lets update the body") }
+      let!(:result) { subject.revise!(changed_by, { raw: "lets update the body" }) }
 
       it 'returns true' do
         result.should == true
@@ -222,8 +226,9 @@ describe PostRevisor do
         post.invalidate_oneboxes.should == true
       end
 
-      it 'increased the version' do
+      it 'increased the versions' do
         post.version.should == 2
+        post.public_version.should == 2
       end
 
       it 'has the new revision' do
@@ -242,16 +247,14 @@ describe PostRevisor do
 
       context 'second poster posts again quickly' do
         before do
-          SiteSetting.expects(:ninja_edit_window).returns(1.minute.to_i)
-          subject.revise!(changed_by, 'yet another updated body', revised_at: post.updated_at + 10.seconds)
+          SiteSetting.stubs(:ninja_edit_window).returns(1.minute.to_i)
+          subject.revise!(changed_by, { raw: 'yet another updated body' }, revised_at: post.updated_at + 10.seconds)
           post.reload
         end
 
         it 'is a ninja edit, because the second poster posted again quickly' do
           post.version.should == 2
-        end
-
-        it 'is a ninja edit, because the second poster posted again quickly' do
+          post.public_version.should == 2
           post.revisions.size.should == 1
         end
       end
@@ -262,19 +265,19 @@ describe PostRevisor do
         revisor = described_class.new(post)
         first_post = topic.posts.by_post_number.first
         expect {
-          revisor.revise!(first_post.user, 'Edit the first post', revised_at: first_post.updated_at + 10.seconds)
+          revisor.revise!(first_post.user, { raw: 'Edit the first post' }, revised_at: first_post.updated_at + 10.seconds)
           topic.reload
         }.to change { topic.excerpt }
         second_post = Fabricate(:post, post_args.merge(post_number: 2, topic_id: topic.id))
         expect {
-          described_class.new(second_post).revise!(second_post.user, 'Edit the 2nd post')
+          described_class.new(second_post).revise!(second_post.user, { raw: 'Edit the 2nd post' })
           topic.reload
         }.to_not change { topic.excerpt }
       end
     end
 
     it "doesn't strip starting whitespaces" do
-      subject.revise!(post.user, "    <-- whitespaces -->    ")
+      subject.revise!(post.user, { raw: "    <-- whitespaces -->    " })
       post.reload
       post.raw.should == "    <-- whitespaces -->"
     end
diff --git a/spec/controllers/posts_controller_spec.rb b/spec/controllers/posts_controller_spec.rb
index dd28e3af048..5305ff49b1a 100644
--- a/spec/controllers/posts_controller_spec.rb
+++ b/spec/controllers/posts_controller_spec.rb
@@ -307,7 +307,7 @@ describe PostsController do
       end
 
       it "calls revise with valid parameters" do
-        PostRevisor.any_instance.expects(:revise!).with(post.user, 'edited body', edit_reason: 'typo')
+        PostRevisor.any_instance.expects(:revise!).with(post.user, { raw: 'edited body' , edit_reason: 'typo' })
         xhr :put, :update, update_params
       end
 
@@ -605,7 +605,8 @@ describe PostsController do
 
   describe "revisions" do
 
-    let(:post_revision) { Fabricate(:post_revision) }
+    let(:post) { Fabricate(:post, version: 2) }
+    let(:post_revision) { Fabricate(:post_revision, post: post) }
 
     it "throws an exception when revision is < 2" do
       expect {
@@ -636,7 +637,7 @@ describe PostsController do
 
       it "ensures poster can see the revisions" do
         user = log_in(:active_user)
-        post = Fabricate(:post, user: user)
+        post = Fabricate(:post, user: user, version: 3)
         pr = Fabricate(:post_revision, user: user, post: post)
         xhr :get, :revisions, post_id: pr.post_id, revision: pr.number
         response.should be_success
@@ -663,7 +664,7 @@ describe PostsController do
 
     context "deleted post" do
       let(:admin) { log_in(:admin) }
-      let(:deleted_post) { Fabricate(:post, user: admin) }
+      let(:deleted_post) { Fabricate(:post, user: admin, version: 3) }
       let(:deleted_post_revision) { Fabricate(:post_revision, user: admin, post: deleted_post) }
 
       before { deleted_post.trash!(admin) }
@@ -677,7 +678,7 @@ describe PostsController do
     context "deleted topic" do
       let(:admin) { log_in(:admin) }
       let(:deleted_topic) { Fabricate(:topic, user: admin) }
-      let(:post) { Fabricate(:post, user: admin, topic: deleted_topic) }
+      let(:post) { Fabricate(:post, user: admin, topic: deleted_topic, version: 3) }
       let(:post_revision) { Fabricate(:post_revision, user: admin, post: post) }
 
       before { deleted_topic.trash!(admin) }
diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb
index 3a59449adc9..d8500e82d01 100644
--- a/spec/controllers/topics_controller_spec.rb
+++ b/spec/controllers/topics_controller_spec.rb
@@ -733,6 +733,7 @@ describe TopicsController do
     describe 'when logged in' do
       before do
         @topic = Fabricate(:topic, user: log_in)
+        Fabricate(:post, topic: @topic)
       end
 
       describe 'without permission' do
@@ -778,7 +779,7 @@ describe TopicsController do
 
         it "returns errors with invalid categories" do
           Topic.any_instance.expects(:change_category_to_id).returns(false)
-          xhr :put, :update, topic_id: @topic.id, slug: @topic.title
+          xhr :put, :update, topic_id: @topic.id, slug: @topic.title, category_id: -1
           expect(response).not_to be_success
         end
 
diff --git a/spec/fabricators/post_revision_fabricator.rb b/spec/fabricators/post_revision_fabricator.rb
index 5a977b23829..43cc8f67bd0 100644
--- a/spec/fabricators/post_revision_fabricator.rb
+++ b/spec/fabricators/post_revision_fabricator.rb
@@ -1,7 +1,7 @@
 Fabricator(:post_revision) do
   post
   user
-  number 3
+  number 2
   modifications do
     { "cooked" => ["

BEFORE

", "

AFTER

"], "raw" => ["BEFORE", "AFTER"] } end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index 192f498906c..f366b566c35 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -368,7 +368,7 @@ describe Category do post = create_post(user: @category.user, category: @category.name) SiteSetting.stubs(:ninja_edit_window).returns(1.minute.to_i) - post.revise(post.user, 'updated body', revised_at: post.updated_at + 2.minutes) + post.revise(post.user, { raw: 'updated body' }, revised_at: post.updated_at + 2.minutes) Category.update_stats @category.reload diff --git a/spec/models/draft_spec.rb b/spec/models/draft_spec.rb index e62f667dda2..4ef4f3baebe 100644 --- a/spec/models/draft_spec.rb +++ b/spec/models/draft_spec.rb @@ -73,7 +73,7 @@ describe Draft do it 'nukes the post draft when a post is revised' do p = Fabricate(:post) Draft.set(p.user, p.topic.draft_key, 0,'hello') - p.revise(p.user, 'another test') + p.revise(p.user, { raw: 'another test' }) s = DraftSequence.current(p.user, p.topic.draft_key) Draft.get(p.user, p.topic.draft_key, s).should == nil end diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb index 811694349f2..f1501538328 100644 --- a/spec/models/post_action_spec.rb +++ b/spec/models/post_action_spec.rb @@ -297,7 +297,7 @@ describe PostAction do post.hidden_reason_id.should == Post.hidden_reasons[:flag_threshold_reached] post.topic.visible.should == false - post.revise(post.user, post.raw + " ha I edited it ") + post.revise(post.user, { raw: post.raw + " ha I edited it " }) post.reload @@ -316,7 +316,7 @@ describe PostAction do post.hidden_reason_id.should == Post.hidden_reasons[:flag_threshold_reached_again] post.topic.visible.should == false - post.revise(post.user, post.raw + " ha I edited it again ") + post.revise(post.user, { raw: post.raw + " ha I edited it again " }) post.reload diff --git a/spec/models/post_alert_observer_spec.rb b/spec/models/post_alert_observer_spec.rb index 3ff4802b379..e61d84d04d3 100644 --- a/spec/models/post_alert_observer_spec.rb +++ b/spec/models/post_alert_observer_spec.rb @@ -36,7 +36,7 @@ describe PostAlertObserver do context 'when editing a post' do it 'notifies a user of the revision' do lambda { - post.revise(evil_trout, "world is the new body of the message") + post.revise(evil_trout, { raw: "world is the new body of the message" }) }.should change(post.user.notifications, :count).by(1) end @@ -47,13 +47,13 @@ describe PostAlertObserver do it 'notifies a user of the revision made by another user' do lambda { - post.revise(evil_trout, "world is the new body of the message") + post.revise(evil_trout, { raw: "world is the new body of the message" }) }.should change(post.user.notifications, :count).by(1) end it 'does not notifiy a user of the revision made by the system user' do lambda { - post.revise(Discourse.system_user, "world is the new body of the message") + post.revise(Discourse.system_user, { raw: "world is the new body of the message" }) }.should_not change(post.user.notifications, :count) end diff --git a/spec/models/post_revision_spec.rb b/spec/models/post_revision_spec.rb deleted file mode 100644 index b19bacb0639..00000000000 --- a/spec/models/post_revision_spec.rb +++ /dev/null @@ -1,119 +0,0 @@ -require 'spec_helper' -require_dependency 'post_revision' - -describe PostRevision do - - before do - @number = 1 - end - - def create_rev(modifications, post_id=1) - @number += 1 - PostRevision.create!(post_id: post_id, user_id: 1, number: @number, modifications: modifications) - end - - it "ignores deprecated current values in history" do - p = PostRevision.new(modifications: {"foo" => ["bar", "bar1"]}) - p.previous("foo").should == "bar" - p.current("foo").should == "bar" - end - - it "can fallback to previous revisions if needed" do - r1 = create_rev("foo" => ["A", "B"]) - r2 = create_rev("foo" => ["C", "D"]) - - r1.current("foo").should == "C" - r2.current("foo").should == "C" - r2.previous("foo").should == "C" - end - - it "can fallback to post if needed" do - post = Fabricate(:post) - r = create_rev({"foo" => ["A", "B"]}, post.id) - - r.current("raw").should == post.raw - r.previous("raw").should == post.raw - r.current("cooked").should == post.cooked - r.previous("cooked").should == post.cooked - end - - it "can fallback to post for current rev only if needed" do - post = Fabricate(:post) - r = create_rev({"raw" => ["A"], "cooked" => ["AA"]}, post.id) - - r.current("raw").should == post.raw - r.previous("raw").should == "A" - r.current("cooked").should == post.cooked - r.previous("cooked").should == "AA" - end - - it "can fallback to topic if needed" do - post = Fabricate(:post) - r = create_rev({"foo" => ["A", "B"]}, post.id) - - r.current("title").should == post.topic.title - r.previous("title").should == post.topic.title - end - - it "can find title changes" do - r1 = create_rev({"title" => ["hello"]}) - r2 = create_rev({"title" => ["frog"]}) - r1.title_changes[:inline].should =~ /frog.*hello/ - r1.title_changes[:side_by_side].should =~ /hello.*frog/ - end - - it "can find category changes" do - cat1 = Fabricate(:category, name: "cat1") - cat2 = Fabricate(:category, name: "cat2") - - r1 = create_rev({"category_id" => [cat1.id, cat2.id]}) - r2 = create_rev({"category_id" => [cat2.id, cat1.id]}) - - changes = r1.category_changes - changes[:previous_category_id].should == cat1.id - changes[:current_category_id].should == cat2.id - - end - - it "can find wiki changes" do - r1 = create_rev("wiki" => [false]) - r2 = create_rev("wiki" => [true]) - - changes = r1.wiki_changes - changes[:previous_wiki].should == false - changes[:current_wiki].should == true - end - - it "can find post_type changes" do - r1 = create_rev("post_type" => [1]) - r2 = create_rev("post_type" => [2]) - - changes = r1.post_type_changes - changes[:previous_post_type].should == 1 - changes[:current_post_type].should == 2 - end - - it "hides revisions that were hidden" do - r1 = create_rev({"raw" => ["one"]}) - r2 = create_rev({"raw" => ["two"]}) - r3 = create_rev({"raw" => ["three"]}) - - r2.hide! - - r1.current("raw").should == "three" - r2.previous("raw").should == "one" - end - - it "shows revisions that were shown" do - r1 = create_rev({"raw" => ["one"]}) - r2 = create_rev({"raw" => ["two"]}) - r3 = create_rev({"raw" => ["three"]}) - - r2.hide! - r2.show! - - r2.previous("raw").should == "two" - r1.current("raw").should == "two" - end - -end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index b7d8901688c..6b21e685240 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -161,7 +161,7 @@ describe Post do post_no_images.user.trust_level = TrustLevel[0] post_no_images.save -> { - post_no_images.revise(post_no_images.user, post_two_images.raw) + post_no_images.revise(post_no_images.user, { raw: post_two_images.raw }) post_no_images.reload }.should_not change(post_no_images, :raw) end @@ -209,7 +209,7 @@ describe Post do post_no_attachments.user.trust_level = TrustLevel[0] post_no_attachments.save -> { - post_no_attachments.revise(post_no_attachments.user, post_two_attachments.raw) + post_no_attachments.revise(post_no_attachments.user, { raw: post_two_attachments.raw }) post_no_attachments.reload }.should_not change(post_no_attachments, :raw) end @@ -468,83 +468,56 @@ describe Post do it 'has no revision' do post.revisions.size.should == 0 first_version_at.should be_present - post.revise(post.user, post.raw).should == false + post.revise(post.user, { raw: post.raw }).should == false end describe 'with the same body' do it "doesn't change version" do - lambda { post.revise(post.user, post.raw); post.reload }.should_not change(post, :version) + lambda { post.revise(post.user, { raw: post.raw }); post.reload }.should_not change(post, :version) end end - describe 'ninja editing' do - before do - SiteSetting.expects(:ninja_edit_window).returns(1.minute.to_i) - post.revise(post.user, 'updated body', revised_at: post.updated_at + 10.seconds) - post.reload - end + describe 'ninja editing & edit windows' do - it 'causes no update' do + before { SiteSetting.stubs(:ninja_edit_window).returns(1.minute.to_i) } + + it 'works' do + revised_at = post.updated_at + 2.minutes + new_revised_at = revised_at + 2.minutes + + # ninja edit + post.revise(post.user, { raw: 'updated body' }, revised_at: post.updated_at + 10.seconds) + post.reload post.version.should == 1 + post.public_version.should == 1 post.revisions.size.should == 0 - post.last_version_at.should == first_version_at - end + post.last_version_at.to_i.should == first_version_at.to_i - end - - describe 'revision much later' do - - let!(:revised_at) { post.updated_at + 2.minutes } - - before do - SiteSetting.stubs(:ninja_edit_window).returns(1.minute.to_i) - post.revise(post.user, 'updated body', revised_at: revised_at) + # revision much later + post.revise(post.user, { raw: 'another updated body' }, revised_at: revised_at) post.reload - end - - it 'updates the version' do post.version.should == 2 + post.public_version.should == 2 post.revisions.size.should == 1 post.last_version_at.to_i.should == revised_at.to_i - end - - describe "new edit window" do - - before do - post.revise(post.user, 'yet another updated body', revised_at: revised_at) - post.reload - end - - it "doesn't create a new version if you do another" do - post.version.should == 2 - end - - it "doesn't change last_version_at" do - post.last_version_at.to_i.should == revised_at.to_i - end - - context "after second window" do - - let!(:new_revised_at) {revised_at + 2.minutes} - - before do - post.revise(post.user, 'yet another, another updated body', revised_at: new_revised_at) - post.reload - end - - it "does create a new version after the edit window" do - post.version.should == 3 - end - - it "does create a new version after the edit window" do - post.last_version_at.to_i.should == new_revised_at.to_i - end - - end + # new edit window + post.revise(post.user, { raw: 'yet another updated body' }, revised_at: revised_at + 10.seconds) + post.reload + post.version.should == 2 + post.public_version.should == 2 + post.revisions.size.should == 1 + post.last_version_at.to_i.should == revised_at.to_i + # after second window + post.revise(post.user, { raw: 'yet another, another updated body' }, revised_at: new_revised_at) + post.reload + post.version.should == 3 + post.public_version.should == 3 + post.revisions.size.should == 2 + post.last_version_at.to_i.should == new_revised_at.to_i end end @@ -554,38 +527,40 @@ describe Post do it "triggers a rate limiter" do EditRateLimiter.any_instance.expects(:performed!) - post.revise(changed_by, 'updated body') + post.revise(changed_by, { raw: 'updated body' }) end end describe 'with a new body' do let(:changed_by) { Fabricate(:coding_horror) } - let!(:result) { post.revise(changed_by, 'updated body') } + let!(:result) { post.revise(changed_by, { raw: 'updated body' }) } it 'acts correctly' do result.should == true post.raw.should == 'updated body' post.invalidate_oneboxes.should == true post.version.should == 2 + post.public_version.should == 2 post.revisions.size.should == 1 post.revisions.first.user.should be_present end context 'second poster posts again quickly' do - before do - SiteSetting.expects(:ninja_edit_window).returns(1.minute.to_i) - post.revise(changed_by, 'yet another updated body', revised_at: post.updated_at + 10.seconds) - post.reload - end it 'is a ninja edit, because the second poster posted again quickly' do + SiteSetting.expects(:ninja_edit_window).returns(1.minute.to_i) + post.revise(changed_by, { raw: 'yet another updated body' }, revised_at: post.updated_at + 10.seconds) + post.reload + post.version.should == 2 + post.public_version.should == 2 post.revisions.size.should == 1 end end end + end diff --git a/spec/models/topic_link_spec.rb b/spec/models/topic_link_spec.rb index e561ded7057..cbe5744aade 100644 --- a/spec/models/topic_link_spec.rb +++ b/spec/models/topic_link_spec.rb @@ -118,7 +118,7 @@ http://b.com/#{'a'*500} context 'removing a link' do before do - post.revise(post.user, "no more linkies") + post.revise(post.user, { raw: "no more linkies" }) TopicLink.extract_from(post) end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 2c2a44c00bb..f8ca3809f12 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -352,21 +352,21 @@ describe Topic do it "doesn't bump the topic on an edit to the last post that doesn't result in a new version" do lambda { SiteSetting.expects(:ninja_edit_window).returns(5.minutes) - @last_post.revise(@last_post.user, 'updated contents', revised_at: @last_post.created_at + 10.seconds) + @last_post.revise(@last_post.user, { raw: 'updated contents' }, revised_at: @last_post.created_at + 10.seconds) @topic.reload }.should_not change(@topic, :bumped_at) end it "bumps the topic when a new version is made of the last post" do lambda { - @last_post.revise(Fabricate(:moderator), 'updated contents') + @last_post.revise(Fabricate(:moderator), { raw: 'updated contents' }) @topic.reload }.should change(@topic, :bumped_at) end it "doesn't bump the topic when a post that isn't the last post receives a new version" do lambda { - @earlier_post.revise(Fabricate(:moderator), 'updated contents') + @earlier_post.revise(Fabricate(:moderator), { raw: 'updated contents' }) @topic.reload }.should_not change(@topic, :bumped_at) end @@ -689,16 +689,17 @@ describe Topic do end describe 'with category' do + before do @category = Fabricate(:category) end it "should not increase the topic_count with no category" do - lambda { Fabricate(:topic, user: @category.user); @category.reload }.should_not change(@category, :topic_count) + -> { Fabricate(:topic, user: @category.user); @category.reload }.should_not change(@category, :topic_count) end it "should increase the category's topic_count" do - lambda { Fabricate(:topic, user: @category.user, category_id: @category.id); @category.reload }.should change(@category, :topic_count).by(1) + -> { Fabricate(:topic, user: @category.user, category_id: @category.id); @category.reload }.should change(@category, :topic_count).by(1) end end @@ -777,74 +778,6 @@ describe Topic do end end - describe 'revisions' do - let(:post) { Fabricate(:post) } - let(:topic) { post.topic } - - it "has no revisions by default" do - post.revisions.size.should == 0 - end - - context 'changing title' do - - before do - topic.title = "new title for the topic" - topic.save - end - - it "creates a new revision" do - post.revisions.size.should == 1 - end - - end - - context 'changing category' do - let(:category) { Fabricate(:category) } - - it "creates a new revision" do - topic.change_category_to_id(category.id) - post.revisions.size.should == 1 - end - - it "does nothing for private messages" do - topic.archetype = "private_message" - topic.category_id = nil - - topic.change_category_to_id(category.id) - topic.category_id.should == nil - end - - context "removing a category" do - before do - topic.change_category_to_id(category.id) - topic.change_category_to_id(nil) - end - - it "creates a new revision" do - post.revisions.size.should == 2 - last_rev = post.revisions.order(:number).last - last_rev.previous("category_id").should == category.id - last_rev.current("category_id").should == SiteSetting.uncategorized_category_id - post.reload - post.version.should == 3 - end - end - - end - - context 'bumping the topic' do - before do - topic.bumped_at = 10.minutes.from_now - topic.save - end - - it "doesn't create a new version" do - post.revisions.size.should == 0 - end - end - - end - describe 'change_category' do before do diff --git a/spec/models/user_action_spec.rb b/spec/models/user_action_spec.rb index 8ca87bfc261..b38f546f734 100644 --- a/spec/models/user_action_spec.rb +++ b/spec/models/user_action_spec.rb @@ -49,7 +49,6 @@ describe UserAction do end it 'includes the events correctly' do - mystats = stats_for_user(user) expecting = [UserAction::NEW_TOPIC, UserAction::NEW_PRIVATE_MESSAGE, UserAction::GOT_PRIVATE_MESSAGE, UserAction::BOOKMARK].sort mystats.should == expecting @@ -66,7 +65,6 @@ describe UserAction do stream_count.should == 0 # groups - category = Fabricate(:category, read_restricted: true) public_topic.recover! @@ -90,18 +88,16 @@ describe UserAction do # duplicate should not exception out log_test_action - # recategorize belongs to the right user category2 = Fabricate(:category) admin = Fabricate(:admin) - public_topic.acting_user = admin - public_topic.change_category_to_id(category2.id) + public_post.revise(admin, { category_id: category2.id}) action = UserAction.stream(user_id: public_topic.user_id, guardian: Guardian.new)[0] action.acting_user_id.should == admin.id action.action_type.should == UserAction::EDIT - end + end describe 'when user likes' do @@ -121,7 +117,6 @@ describe UserAction do it "creates a new stream entry" do PostAction.act(liker, post, PostActionType.types[:like]) likee_stream.count.should == @old_count + 1 - end context "successful like" do diff --git a/spec/services/badge_granter_spec.rb b/spec/services/badge_granter_spec.rb index 1902ff14c3b..ac76c7e9bde 100644 --- a/spec/services/badge_granter_spec.rb +++ b/spec/services/badge_granter_spec.rb @@ -192,7 +192,7 @@ describe BadgeGranter do UserBadge.where(user_id: user.id, badge_id: Badge::Editor).count.should eq(0) - PostRevisor.new(post).revise!(user, "This is my new test 1235 123") + PostRevisor.new(post).revise!(user, { raw: "This is my new test 1235 123" }) BadgeGranter.process_queue! UserBadge.where(user_id: user.id, badge_id: Badge::Editor).count.should eq(1) diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 2e581eeff6f..6c789eb7c6d 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -20,7 +20,7 @@ describe PostAlerter do it "won't notify the user a second time on revision" do p1 = create_post_with_alerts(raw: '[quote="Evil Trout, post:1"]whatup[/quote]') lambda { - p1.revise(p1.user, '[quote="Evil Trout, post:1"]whatup now?[/quote]') + p1.revise(p1.user, { raw: '[quote="Evil Trout, post:1"]whatup now?[/quote]' }) }.should_not change(evil_trout.notifications, :count) end @@ -67,7 +67,7 @@ describe PostAlerter do it "won't notify the user a second time on revision" do mention_post lambda { - mention_post.revise(mention_post.user, "New raw content that still mentions @eviltrout") + mention_post.revise(mention_post.user, { raw: "New raw content that still mentions @eviltrout" }) }.should_not change(evil_trout.notifications, :count) end