-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FIX: various revision history modal quirks #27058
Conversation
- 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
@@ -7,7 +7,6 @@ | |||
<ConditionalLoadingSpinner @condition={{this.initialLoad}}> | |||
<Modal::History::Revision | |||
@model={{this.postRevision}} | |||
@wikiDisabled={{this.wikiDisabled}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those aren't needed anymore.
{{d-icon (if (eq @model.archetype_changes.current "private_message") "envelope" "comment")}} | ||
{{/if}} | ||
|
||
{{#if (and @model.category_id_changes (not @model.archetype_changes))}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only show category changes when we haven't converted the topic into a PM.
{{/if}} | ||
|
||
{{#if @model.category_id_changes}} | ||
{{html-safe @previousCategory}} | ||
{{#if @model.archetype_changes}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Show when we converted the topic to a PM or a public topic.
:can_edit | ||
|
||
has_many :categories, serializer: CategoryBadgeSerializer, embed: :objects |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those were not required. Just the category ids is enough.
@@ -42,6 +40,7 @@ def self.add_compared_field(field) | |||
end | |||
|
|||
add_compared_field :wiki | |||
add_compared_field :post_type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We needed that to track whenever a post gets a "staff color"
@@ -25,11 +25,9 @@ class PostRevisionSerializer < ApplicationSerializer | |||
:title_changes, | |||
:user_changes, | |||
:tags_changes, | |||
:wiki, | |||
:category_id_changes, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to know whether a post is currently a wiki
. We're only interested in the changes. Which is already handled by the add_compared_field :wiki
below.
But we do need to filter any category changes the current user can't see 🙈
@@ -139,10 +135,13 @@ def title_changes | |||
{ inline: diff.inline_html, side_by_side: diff.side_by_side_html } | |||
end | |||
|
|||
def include_title_changes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not serialize the title changes on "replies"
This pull request has been mentioned on Discourse Meta. There might be relevant details there: |
This pull request has been mentioned on Discourse Meta. There might be relevant details there: https://meta.discourse.org/t/daily-summary-5am-utc/291851/124 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Context - https://meta.discourse.org/t/-/304696