From 3d4d21693bbb4f42a9d39083f2e65ad12a5e7bf1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Wed, 22 May 2024 10:09:20 +0200 Subject: [PATCH] FIX: various revision history modal quirks (#27058) - FIX: properly scope category changes to what the current user can see - UX: previous category is now highlighted in "red", new category is highlighted in "green" - PERF: no need to serialize the categories - FIX: properly track wiki - FIX: properly track post_type (aka. Staff Color) - FIX: properly track making a topic a PM - FIX: never show the category changes when a topic is made a PM - PERF: post_revision serializer is now more leaner (never includes title changes when post_number > 1, never includes user changes if there aren't any) - UX: always sort the tags by name --- .../app/components/modal/history.hbs | 3 - .../discourse/app/components/modal/history.js | 25 +++--- .../app/components/modal/history/revision.hbs | 41 ++++++++-- .../components/modal/history/revisions.hbs | 29 +++++-- app/serializers/post_revision_serializer.rb | 76 ++++++++++++------- .../post_revision_serializer_spec.rb | 54 +++++++++++-- 6 files changed, 162 insertions(+), 66 deletions(-) diff --git a/app/assets/javascripts/discourse/app/components/modal/history.hbs b/app/assets/javascripts/discourse/app/components/modal/history.hbs index fb55fea56ee..0fa74655808 100644 --- a/app/assets/javascripts/discourse/app/components/modal/history.hbs +++ b/app/assets/javascripts/discourse/app/components/modal/history.hbs @@ -7,7 +7,6 @@ + {{d-icon + "far-edit" + class=(if @model.wiki_changes.current "diff-ins" "diff-del") + }} {{/if}} {{#if @model.post_type_changes}} - + {{d-icon + "shield-alt" + class=(if + (eq + @model.post_type_changes.current + @site.post_types.moderator_action + ) + "diff-del" + "diff-ins" + ) + }} {{/if}} - {{#if @model.category_id_changes}} - {{html-safe @previousCategory}} + {{#if @model.archetype_changes}} + {{d-icon + (if + (eq @model.archetype_changes.current "private_message") + "envelope" + "comment" + ) + }} + {{/if}} + + {{#if (and @model.category_id_changes (not @model.archetype_changes))}} + {{#if @previousCategory}} + {{html-safe @previousCategory}} + {{else}} + {{d-icon "far-eye-slash" class="diff-del"}} + {{/if}} → - {{html-safe @currentCategory}} + {{#if @currentCategory}} + {{html-safe @currentCategory}} + {{else}} + {{d-icon "far-eye-slash" class="diff-ins"}} + {{/if}} {{/if}} {{/if}} diff --git a/app/assets/javascripts/discourse/app/components/modal/history/revisions.hbs b/app/assets/javascripts/discourse/app/components/modal/history/revisions.hbs index ac356f2bdc6..d61d0fce6f8 100644 --- a/app/assets/javascripts/discourse/app/components/modal/history/revisions.hbs +++ b/app/assets/javascripts/discourse/app/components/modal/history/revisions.hbs @@ -22,19 +22,36 @@ {{/if}} {{#if @model.wiki_changes}}
- + {{d-icon + "far-edit" + class=(if @model.wiki_changes.current "diff-ins" "diff-del") + }}
{{/if}} - {{#if @model.post_type_changes}} + {{#if @model.archetype_changes}}
- + {{d-icon + (if + (eq @model.archetype_changes.current "private_message") + "envelope" + "comment" + ) + }}
{{/if}} - {{#if @model.category_id_changes}} + {{#if (and @model.category_id_changes (not @model.archetype_changes))}}
- {{html-safe @previousCategory}} + {{#if @previousCategory}} + {{html-safe @previousCategory}} + {{else}} + {{d-icon "far-eye-slash" class="diff-del"}} + {{/if}} → - {{html-safe @currentCategory}} + {{#if @currentCategory}} + {{html-safe @currentCategory}} + {{else}} + {{d-icon "far-eye-slash" class="diff-ins"}} + {{/if}}
{{/if}} {{/if}} diff --git a/app/serializers/post_revision_serializer.rb b/app/serializers/post_revision_serializer.rb index a1a6698e958..543ad073a2b 100644 --- a/app/serializers/post_revision_serializer.rb +++ b/app/serializers/post_revision_serializer.rb @@ -25,11 +25,9 @@ class PostRevisionSerializer < ApplicationSerializer :title_changes, :user_changes, :tags_changes, - :wiki, + :category_id_changes, :can_edit - has_many :categories, serializer: CategoryBadgeSerializer, embed: :objects - # Creates a field called field_name_changes with previous and # current members if a field has changed in this revision def self.add_compared_field(field) @@ -42,6 +40,7 @@ class PostRevisionSerializer < ApplicationSerializer end add_compared_field :wiki + add_compared_field :post_type def previous_hidden previous["hidden"] @@ -101,19 +100,16 @@ class PostRevisionSerializer < ApplicationSerializer user.avatar_template end - def wiki - object.post.wiki - end - def can_edit scope.can_edit?(object.post) end def edit_reason - # only show 'edit_reason' when revisions are consecutive - if scope.can_view_hidden_post_revisions? || current["revision"] == previous["revision"] + 1 - current["edit_reason"] - end + current["edit_reason"] + end + + def include_edit_reason? + scope.can_view_hidden_post_revisions? || current["revision"] == previous["revision"] + 1 end def body_changes @@ -139,10 +135,13 @@ class PostRevisionSerializer < ApplicationSerializer { inline: diff.inline_html, side_by_side: diff.side_by_side_html } end + def include_title_changes? + object.post.post_number == 1 + end + def user_changes 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 @@ -162,16 +161,30 @@ class PostRevisionSerializer < ApplicationSerializer } end + def include_user_changes? + previous["user_id"] != current["user_id"] + end + def tags_changes - changes = { - previous: filter_visible_tags(previous["tags"]), - current: filter_visible_tags(current["tags"]), - } - changes[:previous] == changes[:current] ? nil : changes + pre = filter_tags previous["tags"] + cur = filter_tags current["tags"] + + pre == cur ? nil : { previous: pre, current: cur } end def include_tags_changes? - scope.can_see_tags?(topic) && previous["tags"] != current["tags"] + previous["tags"] != current["tags"] && scope.can_see_tags?(topic) + end + + def category_id_changes + pre = filter_category_id previous["category_id"] + cur = filter_category_id current["category_id"] + + pre == cur ? nil : { previous: pre, current: cur } + end + + def include_category_id_changes? + previous["category_id"] != current["category_id"] end protected @@ -206,14 +219,15 @@ class PostRevisionSerializer < ApplicationSerializer # Retrieve any `tracked_topic_fields` PostRevisor.tracked_topic_fields.each_key do |field| - next if field == :tags # Special handling below + next if field == :tags latest_modifications[field.to_s] = [topic.public_send(field)] if topic.respond_to?(field) end latest_modifications["featured_link"] = [ - post.topic.featured_link, + topic.featured_link, ] if SiteSetting.topic_featured_link_enabled - latest_modifications["tags"] = [topic.tags.pluck(:name)] if scope.can_see_tags?(topic) + + latest_modifications["tags"] = [topic.tags.map(&:name).sort] post_revisions << PostRevision.new( number: post_revisions.last.number + 1, @@ -229,7 +243,7 @@ class PostRevisionSerializer < ApplicationSerializer revision[:revision] = pr.number revision[:hidden] = pr.hidden - pr.modifications.each_key { |field| revision[field] = pr.modifications[field][0] } + pr.modifications.each { |field, (value, _)| revision[field] = value } @all_revisions << revision end @@ -260,12 +274,16 @@ class PostRevisionSerializer < ApplicationSerializer object.user || Discourse.system_user end - def filter_visible_tags(tags) - if tags.is_a?(Array) && tags.size > 0 - @hidden_tag_names ||= DiscourseTagging.hidden_tag_names(scope) - tags - @hidden_tag_names - else - tags - end + def hidden_tags + @hidden_tags ||= DiscourseTagging.hidden_tag_names(scope) + end + + def filter_tags(tags) + tags - hidden_tags + end + + def filter_category_id(category_id) + return if category_id.blank? + Category.secured(scope).find_by(id: category_id)&.id end end diff --git a/spec/serializers/post_revision_serializer_spec.rb b/spec/serializers/post_revision_serializer_spec.rb index f1444b46ac1..0d526ad4b02 100644 --- a/spec/serializers/post_revision_serializer_spec.rb +++ b/spec/serializers/post_revision_serializer_spec.rb @@ -3,13 +3,51 @@ RSpec.describe PostRevisionSerializer do fab!(:post) { Fabricate(:post, version: 2) } + context "with secured categories" do + fab!(:group) + fab!(:private_category) { Fabricate(:private_category, group: group) } + fab!(:post_revision) do + Fabricate( + :post_revision, + post: post, + modifications: { + "category_id" => [private_category.id, post.topic.category_id], + }, + ) + end + + it "returns category changes to staff" do + json = + PostRevisionSerializer.new( + post_revision, + scope: Guardian.new(Fabricate(:admin)), + root: false, + ).as_json + + expect(json[:category_id_changes][:previous]).to eq(private_category.id) + expect(json[:category_id_changes][:current]).to eq(post.topic.category_id) + end + + it "does not return all category changes to non-staff" do + json = + PostRevisionSerializer.new( + post_revision, + scope: Guardian.new(Fabricate(:user)), + root: false, + ).as_json + + expect(json[:category_id_changes][:previous]).to eq(nil) + expect(json[:category_id_changes][:current]).to eq(post.topic.category_id) + end + end + context "with hidden tags" do fab!(:public_tag) { Fabricate(:tag, name: "public") } fab!(:public_tag2) { Fabricate(:tag, name: "visible") } fab!(:hidden_tag) { Fabricate(:tag, name: "hidden") } fab!(:hidden_tag2) { Fabricate(:tag, name: "secret") } - let(:staff_tag_group) do + fab!(:staff_tag_group) do Fabricate( :tag_group, permissions: { @@ -41,7 +79,6 @@ RSpec.describe PostRevisionSerializer do before do SiteSetting.tagging_enabled = true - staff_tag_group post.topic.tags = [public_tag2, hidden_tag] end @@ -52,10 +89,9 @@ RSpec.describe PostRevisionSerializer do scope: Guardian.new(Fabricate(:admin)), root: false, ).as_json - expect(json[:tags_changes][:previous]).to include(public_tag.name) - expect(json[:tags_changes][:previous]).to include(hidden_tag.name) - expect(json[:tags_changes][:current]).to include(public_tag2.name) - expect(json[:tags_changes][:current]).to include(hidden_tag.name) + + expect(json[:tags_changes][:previous]).to contain_exactly(public_tag.name, hidden_tag.name) + expect(json[:tags_changes][:current]).to contain_exactly(public_tag2.name, hidden_tag.name) end it "does not return hidden tags to non-staff" do @@ -65,8 +101,9 @@ RSpec.describe PostRevisionSerializer do scope: Guardian.new(Fabricate(:user)), root: false, ).as_json - expect(json[:tags_changes][:previous]).to eq([public_tag.name]) - expect(json[:tags_changes][:current]).to eq([public_tag2.name]) + + expect(json[:tags_changes][:previous]).to contain_exactly(public_tag.name) + expect(json[:tags_changes][:current]).to contain_exactly(public_tag2.name) end it "does not show tag modifications if changes are not visible to the user" do @@ -76,6 +113,7 @@ RSpec.describe PostRevisionSerializer do scope: Guardian.new(Fabricate(:user)), root: false, ).as_json + expect(json[:tags_changes]).to_not be_present end end