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

Admin user can merge two articles from edit page #25

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

Conversation

emgord
Copy link

@emgord emgord commented Mar 30, 2016

New feature allows admin to merge two articles from the edit page. Contributors do not have access to merge articles.

emgord added 30 commits March 28, 2016 11:06
def merge
id = params[:id]
merge_id = params[:merge_with][:merge_id]
if current_user.admin?

Choose a reason for hiding this comment

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

Creating these local variables first is nice, but if the user isn't an admin, they aren't needed to i would suggest moving them into the first conditional

Copy link
Author

Choose a reason for hiding this comment

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

Great point. I made the change.

@@ -416,6 +416,17 @@ def access_by?(user)
user.admin? || user_id == user.id
end

def merge_with(other_article_id)
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.

There is a lot of method chaining going on here. Is there a better way to get the comments associated without iterating through each one?

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