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
2024-05-22 10:09:20 +02:00 · 2024-05-22 10:09:20 +02:00 · 3d4d21693b
parent 52125d849f
commit 3d4d21693b
6 changed files with 162 additions and 66 deletions
--- a/app/assets/javascripts/discourse/app/components/modal/history.hbs
+++ b/app/assets/javascripts/discourse/app/components/modal/history.hbs
@ -7,7 +7,6 @@
    <ConditionalLoadingSpinner @condition={{this.initialLoad}}>
      <Modal::History::Revision
        @model={{this.postRevision}}
-        @wikiDisabled={{this.wikiDisabled}}
        @previousCategory={{this.previousCategory}}
        @currentCategory={{this.currentCategory}}
        @displayInline={{this.displayInline}}
@ -20,8 +19,6 @@
        @hiddenClasses={{this.hiddenClasses}}
        @mobileView={{this.site.mobileView}}
        @userChanges={{this.user_changes}}
-        @wikiDisabled={{this.wikiDisabled}}
-        @postTypeDisabled={{this.postTypeDisabled}}
        @previousCategory={{this.previousCategory}}
        @currentCategory={{this.currentCategory}}
        @previousTagChanges={{this.previousTagChanges}}
--- a/app/assets/javascripts/discourse/app/components/modal/history.js
+++ b/app/assets/javascripts/discourse/app/components/modal/history.js
@ -238,34 +238,29 @@ export default class History extends Component {
  }

  get previousCategory() {
-    if (this.postRevision?.category_id_changes) {
+    if (this.postRevision?.category_id_changes?.previous) {
      let category = Category.findById(
        this.postRevision.category_id_changes.previous
      );
-      return categoryBadgeHTML(category, { allowUncategorized: true });
+      return categoryBadgeHTML(category, {
+        allowUncategorized: true,
+        extraClasses: "diff-del",
+      });
    }
  }

  get currentCategory() {
-    if (this.postRevision?.category_id_changes) {
+    if (this.postRevision?.category_id_changes?.current) {
      let category = Category.findById(
        this.postRevision.category_id_changes.current
      );
-      return categoryBadgeHTML(category, { allowUncategorized: true });
+      return categoryBadgeHTML(category, {
+        allowUncategorized: true,
+        extraClasses: "diff-ins",
+      });
    }
  }

-  get wikiDisabled() {
-    return !this.postRevision.wiki_changes?.current;
-  }
-
-  get postTypeDisabled() {
-    return (
-      this.postRevision?.post_type_changes?.current !==
-      this.site.post_types.moderator_action
-    );
-  }
-
  @action
  displayInline(event) {
    event?.preventDefault();
--- a/app/assets/javascripts/discourse/app/components/modal/history/revision.hbs
+++ b/app/assets/javascripts/discourse/app/components/modal/history/revision.hbs
@ -35,17 +35,48 @@
        {{/if}}

        {{#if @model.wiki_changes}}
-          <DisabledIcon @icon="far-edit" @disabled={{@wikiDisabled}} />
+          {{d-icon
+            "far-edit"
+            class=(if @model.wiki_changes.current "diff-ins" "diff-del")
+          }}
        {{/if}}

        {{#if @model.post_type_changes}}
-          <DisabledIcon @icon="shield-alt" @disabled={{@postTypeDisabled}} />
+          {{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}}
          &rarr;
-          {{html-safe @currentCategory}}
+          {{#if @currentCategory}}
+            {{html-safe @currentCategory}}
+          {{else}}
+            {{d-icon "far-eye-slash" class="diff-ins"}}
+          {{/if}}
        {{/if}}
      </span>
    {{/if}}
--- 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}}
      <div class="row">
-        <DisabledIcon @icon="far-edit" @disabled={{@wikiDisabled}} />
+        {{d-icon
+          "far-edit"
+          class=(if @model.wiki_changes.current "diff-ins" "diff-del")
+        }}
      </div>
    {{/if}}
-    {{#if @model.post_type_changes}}
+    {{#if @model.archetype_changes}}
      <div class="row">
-        <DisabledIcon @icon="shield-alt" @disabled={{@postTypeDisabled}} />
+        {{d-icon
+          (if
+            (eq @model.archetype_changes.current "private_message")
+            "envelope"
+            "comment"
+          )
+        }}
      </div>
    {{/if}}
-    {{#if @model.category_id_changes}}
+    {{#if (and @model.category_id_changes (not @model.archetype_changes))}}
      <div class="row">
-        {{html-safe @previousCategory}}
+        {{#if @previousCategory}}
+          {{html-safe @previousCategory}}
+        {{else}}
+          {{d-icon "far-eye-slash" class="diff-del"}}
+        {{/if}}
        &rarr;
-        {{html-safe @currentCategory}}
+        {{#if @currentCategory}}
+          {{html-safe @currentCategory}}
+        {{else}}
+          {{d-icon "far-eye-slash" class="diff-ins"}}
+        {{/if}}
      </div>
    {{/if}}
  {{/if}}
--- 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
--- 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