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.

Sign up for GitHub

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

[API] Commenting through a token #1513

Merged
merged 14 commits into from
Jul 11, 2017
Merged

[API] Commenting through a token #1513

Changes from 7 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
7579f8d
Start working on a function to add commenting feature through a token
ryzokuken Jul 8, 2017
829fae9
Add authorization
ryzokuken Jul 8, 2017
a095e8d
Avoid ruby hash rocket syntax
ryzokuken Jul 8, 2017
0ff8697
Facepalm
ryzokuken Jul 8, 2017
195bc07
Start working on a private function for adding comments
ryzokuken Jul 9, 2017
787134c
Finalize the original function using the private helper
ryzokuken Jul 9, 2017
a5834f1
Make comment_by_token method using the private function
ryzokuken Jul 9, 2017
d2846ac
Move CommentError and create_comment to CommentHelper
ryzokuken Jul 9, 2017
917c90e
Write first functional test; Introduce wierd 406 error
ryzokuken Jul 9, 2017
48ecaae
Fix 406 issue
ryzokuken Jul 10, 2017
f80878b
Add second test and improve responses
ryzokuken Jul 10, 2017
61bca00
Fix test names
ryzokuken Jul 10, 2017
e513cdd
Add test for invalid comment
ryzokuken Jul 10, 2017
1a9c61e
Add recommendations
ryzokuken Jul 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 58 additions & 4 deletions app/controllers/comment_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,16 @@
class CommentError < ArgumentError
end

def create_comment(node, user, body)
Copy link
Member

Choose a reason for hiding this comment

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

This type of logic should go in helpers. Keep the controller code as thin as possible. Only keep action related methods

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, If @jywarren approves the function, I would move it to the corresponding helper class.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe look at how we've done error responses in other parts of the code, and compare also with best practices from modern Rails guides?

@comment = node.add_comment(uid: user.uid, body: body)
if user && @comment.save
@comment.notify user
return @comment
else
raise CommentError.new()
end
end

class CommentController < ApplicationController
respond_to :html, :xml, :json
before_filter :require_user, only: %i[create update delete]
Expand All @@ -12,9 +25,11 @@ def index
# create node comments
def create
@node = Node.find params[:id]
@comment = @node.add_comment(uid: current_user.uid, body: params[:body])
if current_user && @comment.save
@comment.notify(current_user)
@body = params[:body]
@user = current_user

begin
@comment = create_comment(@node, @user, @body)
respond_with do |format|
if params[:type] && params[:type] == 'question'
@answer_id = 0
Expand All @@ -30,12 +45,51 @@ def create
end
end
end
else
rescue CommentError
flash[:error] = 'The comment could not be saved.'
render text: 'failure'
end
end

def create_by_token
@node = Node.find params[:id]
@user = Users.find_by_username params[:username]
@body = pararms[:body]
@token = request.headers["token"]

if @user && @user.token == @token
begin
@comment = create_comment(@node, @user, @body)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a comment here telling where to find this method? I think for newcomers the Rails organization can be a bit arcane and although it adds a line, this could be really helpful for some.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do.

msg = {
Copy link
Member

Choose a reason for hiding this comment

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

I guess this just has a /lot/ of repeated code here -- the only unique line in 52-59 vs 61-68 vs 71-78 is :created and "Created", you know? If you're going through the trouble to a whole new class for CommentError, i just feel that it's not justified given how little this code is reducing repetition. Also, isn't it a bit odd to need to begin/rescue in here?

I'm trying to find a guide to best-practices in Rails controller error handling, to help DRY out this code. But I'm not finding much that's not based on model validation error handling...

Copy link
Member Author

Choose a reason for hiding this comment

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

I get your point. I used the raise -> rescue workflow here because I call the @comment.save inside the function but wanted the calling function to know that something went wrong and do something sensible, and decided that throwing an error would be the best possible way to achieve this. If you could suggest a better way, I would be more than happy to use it instead, though.

status: :created,
message: "Created"
}
respond_to do |format|
format.xml { render xml: msg.to_xml }
format.json { render json: msg.to_json }
end
rescue CommentError
msg = {
status: :bad_request,
message: "Bad Request"
}
respond_to do |format|
format.xml { render xml: msg.to_xml }
format.json { render json: msg.to_json }
end
end
else
msg = {
status: :unauthorized,
message: "Unauthorized"
}
respond_to do |format|
format.xml { render xml: msg.to_xml }
format.json { render json: msg.to_json }
end
end
end

# create answer comments
def answer_create
@answer_id = params[:aid]
Expand Down