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

Feature: Merge multiple responses #4112

Closed

Conversation

adrapereira
Copy link
Contributor

This feature was requested in meta here.

It adds a button to a topic's administration menu enabling the admin to merge 2 or more posts by the same user.

Necessary conditions to merge posts:

  • All posts must be deletable (so the main post of a topic cannot be among the selected posts)
  • 2 or more posts must be selected
  • All posts must be by the same user

There's no additional logic to account for posts that are replies to other responses, all posts will be merged in the same way, chronologically, with two new lines between each one.

The merged content will be edited into the last selected posts while the remaining responses will be marked as deleted.

@discoursebot
Copy link

You've signed the CLA, adrapereira. Thank you! This pull request is ready for review.

const selectedPosts = this.get('selectedPosts');
let canDelete = true;
selectedPosts.forEach(function(p) {
if (!canDelete || !p.get('can_delete')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can leave out the first part of this condition here.

@tgxworld
Copy link
Contributor

Hi @adrapereira !

Thank you for the PR :)

Are you able to add some tests to make sure PostMerger is doing what you expect it to do? Feel free to ping me if you need any help with this? Thanks

post_ids: selectedPosts.map(function(p) { return p.get('id'); }),
reply_post_ids: selectedReplies.map(function(p) { return p.get('id'); })
}
});
Copy link
Contributor

@tgxworld tgxworld May 17, 2016

Choose a reason for hiding this comment

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

It seems like we're not updating the client and handling errors on the ajax call here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tgxworld The client is updated. 😃 There's no error handling in the AJAX call since the other AJAX calls didn't have it either. Can you point me to an example of how it should be handled in Discourse?

@coding-horror
Copy link
Contributor

@tgxworld if this still looks good, can we merge it in?

@adrapereira
Copy link
Contributor Author

@coding-horror Not yet, I still need to add some tests.

@adrapereira
Copy link
Contributor Author

@tgxworld I've added some tests to PostMerger. Please let me know if you need anything more.

if (result) {
const selectedPosts = this.get('selectedPosts');

Discourse.Post.mergePosts(selectedPosts);
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's import the Post model instead.
import Post from 'discourse/models/post';

@tgxworld
Copy link
Contributor

tgxworld commented Jun 22, 2016

@adrapereira Sorry for the late reply. I've left some comments after having a second look :)

@SamSaffron
Copy link
Member

@adrapereira branch is in conflict state now, can you rebase?

@coding-horror
Copy link
Contributor

This seems in good shape if you can rebate?

@adrapereira
Copy link
Contributor Author

I think this is ready for merge.

@tgxworld
Copy link
Contributor

Closing in favor of #4346

@tgxworld tgxworld closed this Jul 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants