Skip to content
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

Merged
merged 2 commits into from
May 22, 2024
Merged

Conversation

ZogStriP
Copy link
Member

  • 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

Context - https://meta.discourse.org/t/-/304696

- 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}}
Copy link
Member Author

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))}}
Copy link
Member Author

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}}
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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,
Copy link
Member Author

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?
Copy link
Member Author

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"

@discoursebot
Copy link

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/private-categories-showing-in-topic-edit-history-if-a-user-does-not-have-access-to-it/304696/10

@discoursebot
Copy link

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

Copy link
Contributor

@pmusaraj pmusaraj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@ZogStriP ZogStriP merged commit 3d4d216 into main May 22, 2024
16 checks passed
@ZogStriP ZogStriP deleted the fix-categories-in-revision branch May 22, 2024 08:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants