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

add rspec messages #2

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

add rspec messages #2

wants to merge 1 commit into from

Conversation

RamonHossein
Copy link

  • Created rspec model Message.

  • Created rspec controller Message.

  • Created a helper method json which parses the JSON response to a ruby hash.

  • Created a method json_response that responds with JSON and an HTTP status code (200 by default).

  • In exception_handler we'll rescue from the exceptions and return a message.

Copy link
Member

@chimame chimame left a comment

Choose a reason for hiding this comment

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

I do not think we need to change Gemfile.

@@ -32,6 +32,9 @@ gem 'jquery-rails'
# Use ActiveModel has_secure_password
# gem 'bcrypt', '~> 3.1.7'

# a library for generating fake data.
gem 'faker'
Copy link
Member

Choose a reason for hiding this comment

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

It is desirable to be in the test group.

Copy link
Author

Choose a reason for hiding this comment

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

I would like use gem faker to generate data at random in db/seeds.rb.

Copy link
Member

Choose a reason for hiding this comment

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

If so, should you limit it with development and test? We do not need it for staging or production.

# database cleaner
gem 'database_rewinder'
# a library for setting up Ruby objects as test data.
gem 'factory_girl_rails', '~> 4.0'
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add version designation?

Copy link
Author

Choose a reason for hiding this comment

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

Habit, I wouldn't like to be problem with oldest versions.

Copy link
Member

Choose a reason for hiding this comment

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

I specified the lowest version in Gemfile.lock. I am worried about inhibiting version up by writing this.

# shoulda-matcher provides Rspec and Minitest compatible one liners that test common Rails functionality.
gem 'shoulda-matchers', '~> 3.1'
# strategies for cleaning database in Ruby.
gem 'database_cleaner'
Copy link
Member

Choose a reason for hiding this comment

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

There is no need to change from database_rewinder.

# strategies for cleaning database in Ruby.
gem 'database_cleaner'
# an IRB alternative and runtime developer console.
gem 'pry'
Copy link
Member

Choose a reason for hiding this comment

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

You should add it to development and test if necessary.

# json genarator
gem 'active_model_serializers'
# ActiveModel::Serializer implementation and Rails hooks.
gem 'active_model_serializers', '~> 0.10.0'
Copy link
Member

Choose a reason for hiding this comment

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

Why did you add version designation?

Copy link
Author

Choose a reason for hiding this comment

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

Habit, I wouldn't like to be problem with oldest versions.

Copy link
Member

Choose a reason for hiding this comment

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

I specified the lowest version in Gemfile.lock. I am worried about inhibiting version up by writing this.

end

# PATCH/PUT /messages/1
# PUT / messages / :id
Copy link
Member

Choose a reason for hiding this comment

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

It should be patch. Please check bin/rake routes.

render json: @message.errors, status: :unprocessable_entity
end
@message.update(message_params)
head :no_content
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for changing?

Copy link
Author

Choose a reason for hiding this comment

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

This is the expected behavior in case of a DELETE and UPDATE request, because if you are actually not sending any content, it is best to mention it with a :no_content.

def destroy
@message.destroy
head :no_content
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

Copy link
Author

Choose a reason for hiding this comment

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

This is the expected behavior in case of a DELETE and UPDATE request, because if you are actually not sending any content, it is best to mention it with a :no_content.

@@ -47,7 +39,7 @@ def set_message

# Only allow a trusted parameter "white list" through.
def message_params
params.require(:message).permit(:text)
params.permit(:text)
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason for changing?


included do
rescue_from ActiveRecord::RecordNotFound do |e|
json_response({ message: e.message }, :not_found)
Copy link
Member

Choose a reason for hiding this comment

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

Since modules are not loosely coupled, it is not recommended to do such a construction.

Copy link
Author

Choose a reason for hiding this comment

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

Since browsers only read 4xx & 5xx error codes, all Rails exceptions have to be inferred. Thus, ExceptionHandler simply has to manage how the 4xx / 5xx errors are passed to the browser and it provides the more graceful included method.

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