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

New feature added to allow admin users to merge articles #29

Open
wants to merge 29 commits into
base: master
Choose a base branch
from

Conversation

trowbrsa
Copy link

This pull request includes a feature that allows admin users to merge together two articles. This is only visible to logged-in admins, and the feature does not allow you to attempt to merge the same two articles or merge an article that doesn't exist.

Please note that this is still a draft feature. I still have more cucumber tests to write to ensure that everything is working correctly. I also want to improve the user experience in v2. Specifically, I want to make sure that a green 'success' notification that pops up for users when two articles are successfully merged (currently it is a red 'error' notification. Additionally, I think a more improved user experience would redirect users back to the 'edit' page after they merge (they are currently redirected to the index). Lastly, I want to give admins the option of selecting which author and title they want to assign to the merged post (currently it defaults to the post for which they select the merge(.

Thanks for any feedback!

…ctly for new categories, updated indentation
…tent and admin can see 'Merge Articles' option
@@ -37,6 +37,24 @@ def edit
new_or_edit
end

def merge
if current_user.admin?

Choose a reason for hiding this comment

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

Should something happen if the user is not an admin? I know it might not show up on the view, but someone could still potentially hit the URL anyway.

Copy link
Author

Choose a reason for hiding this comment

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

Great suggestion - updated it so that a non-admin gets a flash notice and then is redirected to the index page.


def merge_with(other_article_id)
# find each of the comments associated w/ the article
Article.find(other_article_id).comments.each do |comment|

Choose a reason for hiding this comment

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

Is there a way to associate the comments without iterating through each one?

Copy link
Author

Choose a reason for hiding this comment

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

I tried this: self.comments += other_article.comments and then saving self.comments, but found that the other_article comments were not actually being saved. Is there another way to reassign the comments without iterating through each one?

@trowbrsa
Copy link
Author

trowbrsa commented Apr 1, 2016

I've implemented your suggestions, thank you!

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

Successfully merging this pull request may close these issues.

2 participants