-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
add API routes for comment likes #8439
Conversation
627c067
to
1e1130e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, only one little comment for when checking if a comment exists and belongs to the correct post.
def comment_and_post_validate(post_guid, comment_guid) | ||
if !comment_exists(comment_guid) || !comment_is_for_post(post_guid, comment_guid) | ||
render_error 404, "Comment not found for the given post" | ||
false | ||
else | ||
true | ||
end | ||
end | ||
|
||
def comment_exists(comment_guid) | ||
comment = comment_service.find!(comment_guid) | ||
comment ? true : false | ||
rescue ActiveRecord::RecordNotFound | ||
false | ||
end | ||
|
||
def comment_is_for_post(post_guid, comment_guid) | ||
comments = comment_service.find_for_post(post_guid) | ||
comment = comments.find {|comment| comment[:guid] == comment_guid } | ||
comment ? true : false | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment_exists
is kinda redundant, if the comment doesn't exist, it also doesn't exist "for the post", so only checking comment_is_for_post
is enough.
Also in comment_is_for_post
you can use .exists?
directly on an SQL level, instead of looping through all comments with find
.
Like this:
def comment_and_post_validate(post_guid, comment_guid) | |
if !comment_exists(comment_guid) || !comment_is_for_post(post_guid, comment_guid) | |
render_error 404, "Comment not found for the given post" | |
false | |
else | |
true | |
end | |
end | |
def comment_exists(comment_guid) | |
comment = comment_service.find!(comment_guid) | |
comment ? true : false | |
rescue ActiveRecord::RecordNotFound | |
false | |
end | |
def comment_is_for_post(post_guid, comment_guid) | |
comments = comment_service.find_for_post(post_guid) | |
comment = comments.find {|comment| comment[:guid] == comment_guid } | |
comment ? true : false | |
end | |
def comment_and_post_validate(post_guid, comment_guid) | |
if comment_is_for_post(post_guid, comment_guid) | |
true | |
else | |
render_error 404, "Comment not found for the given post" | |
false | |
end | |
end | |
def comment_is_for_post(post_guid, comment_guid) | |
comments = comment_service.find_for_post(post_guid) | |
comments.exists?(guid: comment_guid) | |
end |
This way the whole validation is just a single sql-query, instead of 2 queries and looping through all comments in ruby.
It's enough to check if the comment exists on the specified post, if it doesn't exist at all, that check will also fail. Also do that check directly on SQL level and just check if the comment exist instead of looping through all comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚢
No description provided.