Skip to content
This repository was archived by the owner on Jul 22, 2025. It is now read-only.

DEV: Use the PostQuotedContent component to render the accepted answer #387

Closed
wants to merge 12 commits into from

Conversation

megothss
Copy link
Contributor

@megothss megothss commented Jul 22, 2025

This PR modernizes the accepted answer functionality in the discourse-solved plugin with several key improvements:

Core Changes

  • Component Refactoring: Replaced the solved-accepted-answer component with the PostQuotedContent core component for better consistency and maintainability
  • Real-time Updates: Introduced MessageBus integration to provide live synchronization of solution acceptance/unacceptance across all users
  • Modern Property System: Migrated from legacy Ember get/set patterns to tracked properties for better reactivity

Technical Improvements

  • Added loading states for Accept/Unaccept buttons to enhance user experience during async operations
  • Refactored accepted_answer_post_info logic for cleaner data handling
  • Updated system tests to verify expandable accepted answer quote behavior
  • Improved test reliability by using topic_with_op fabrication

Compatibility

  • Added compatibility entry for versions < 3.5.0.beta8-dev
  • Maintained backward compatibility while modernizing the codebase

The changes ensure that solution state updates are reflected immediately across all connected users while providing a more robust and maintainable foundation for the plugin.

megothss added 12 commits June 20, 2025 18:45
Refactored the `solved-accepted-answer` component to utilize the `PostQuotedContent` component, simplifying the structure and removing redundant code. Updated system tests to verify behavior of expandable accepted answer quotes. Added new compatibility entry for version `< 3.5.0.beta7-dev` and included TODO comments regarding event handling for Glimmer Post Stream.
…er logic

Refactored the `topic` model to replace legacy Ember property access (`get/set`) with modern tracked properties. Updated related logic in `add-topic-list-class` and `solved-unaccept-answer-button` to align with these changes.
Refactored `solved_spec.rb` to make `accepted answer` assertions more explicit and structured. Consolidated selector handling and clarified expectation of the expanded quote behavior.
Introduce real-time message bus updates for accepted and unaccepted solutions, ensuring live synchronization across users.

Key changes:
- Publish solution acceptance/unacceptance updates via MessageBus.
- Refactor `accepted_answer_post_info` and related logic for cleaner handling of accepted answer data.
- Update both backend and frontend to support reactive updates when solutions are toggled.
- Add loading states for Accept/Unaccept buttons to enhance UX during async operations.
…tency

This ensures tests work with topics that include an original post by default, improving test reliability and coverage. Adjustments were made to guardian_extensions_spec and solved_spec to reflect this.
…eta8-dev`

Updated `.discourse-compatibility` to reflect compatibility with core version `< 3.5.0.beta8-dev`.
- Hide blockquote for accepted answers without excerpts to avoid unnecessary visual elements.
- Adjust title padding for better alignment when excerpts are absent.
- Ensure no content fallback (`""`) when excerpts are not provided.
- Add `accepted-answer--has-excerpt` class for proper styling differentiation.
- Add a newline for better code readability and spacing consistency within the `solutions.scss` file.
- No functional changes made.
DiscourseEvent.trigger(:accepted_solution, post)
MessageBus.publish("/topic/#{topic.id}", message)
Copy link
Member

Choose a reason for hiding this comment

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

We need to add some security to this message, so that it only goes out to users/groups that have access to the topic

Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines +59 to +61
expect(topic_page).not_to have_css(".accepted-answer.quote")
expect(topic_page).not_to have_css(".title .accepted-answer--solver")
expect(topic_page).not_to have_css(".title .accepted-answer--accepter")
Copy link
Contributor

Choose a reason for hiding this comment

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

Most likely you wanted this here:

Suggested change
expect(topic_page).not_to have_css(".accepted-answer.quote")
expect(topic_page).not_to have_css(".title .accepted-answer--solver")
expect(topic_page).not_to have_css(".title .accepted-answer--accepter")
expect(topic_page).to have_no_css(".accepted-answer.quote")
expect(topic_page).to have_no_css(".title .accepted-answer--solver")
expect(topic_page).to have_no_css(".title .accepted-answer--accepter")

find(".post-action-menu__solved-unaccepted").click

expect(topic_page).to have_css(".post-action-menu__solved-accepted")

accepted_answer_quote = topic_page.find("aside.accepted-answer.quote")
expect(accepted_answer_quote["data-expanded"]).to eq("false")
Copy link
Contributor

Choose a reason for hiding this comment

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

that's not a correct way to do, it's not using auto-wait, should be:

Suggested change
expect(accepted_answer_quote["data-expanded"]).to eq("false")
expect(topic_page).to have_css("aside.accepted-answer.quote[data-expanded='false']")

Haven't tested locally, but you get the idea...

Comment on lines +41 to +42
accepted_answer_quote.find("button.quote-toggle").click
expect(accepted_answer_quote["data-expanded"]).to eq("true")
Copy link
Contributor

@jjaffeux jjaffeux Jul 22, 2025

Choose a reason for hiding this comment

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

same thing, it's not auto-waiting, see above for solution

Comment on lines +45 to +49
try {
await unacceptPost(post);
} finally {
this.saving = false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

should we surface an error when it fails?

Copy link
Contributor

@jjaffeux jjaffeux left a comment

Choose a reason for hiding this comment

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

Approved, but would be nice to fix the spec comments

Copy link
Contributor

@jjaffeux jjaffeux left a comment

Choose a reason for hiding this comment

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

unapproved, indeed the comment from david is important

@davidtaylorhq
Copy link
Member

Moving plugin to core, so we'll need to re-open this in discourse/discourse. Sorry @megothss

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants