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

[WIP] Update ruby/rails #228

Closed
wants to merge 27 commits into from
Closed

[WIP] Update ruby/rails #228

wants to merge 27 commits into from

Conversation

ArthurZaharov
Copy link
Contributor

No description provided.

@timurvafin timurvafin had a problem deploying to fs-rails-base-api-pr-228 September 7, 2018 20:17 Failure
@timurvafin timurvafin had a problem deploying to fs-rails-base-api-pr-228 September 9, 2018 10:16 Failure
@timurvafin timurvafin had a problem deploying to fs-rails-base-api-pr-228 September 10, 2018 08:27 Failure
@timurvafin timurvafin had a problem deploying to fs-rails-base-api-pr-228 September 10, 2018 08:34 Failure
@timurvafin timurvafin had a problem deploying to fs-rails-base-api-pr-228 September 10, 2018 08:39 Failure
ArthurZaharov and others added 3 commits September 10, 2018 11:40
… update-ruby-rails

* 'update-ruby-rails' of github.com:fs/rails-base-api:
  Remove byebug from git history
@timurvafin timurvafin had a problem deploying to fs-rails-base-api-pr-228 September 10, 2018 08:46 Failure
@timurvafin timurvafin temporarily deployed to fs-rails-base-api-pr-228 September 10, 2018 09:29 Inactive
@timurvafin timurvafin temporarily deployed to fs-rails-base-api-pr-228 September 10, 2018 12:10 Inactive
@timurvafin timurvafin temporarily deployed to fs-rails-base-api-pr-228 September 10, 2018 12:31 Inactive
@timurvafin timurvafin temporarily deployed to fs-rails-base-api-pr-228 September 11, 2018 13:40 Inactive
@timurvafin timurvafin temporarily deployed to fs-rails-base-api-pr-228 September 11, 2018 14:04 Inactive
.env.example Outdated

# Specify assets server host name, eg.: d2oek0c5zwe48d.cloudfront.net
ASSET_HOST=lvh.me:5000
S3_ASSET_HOST=https://d1ltghz9ehkb4n.cloudfront.net
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should it be commented?

# CANONICAL_HOST=paste_single_hostname_here

# Comma separated list of IPs to access admin staff like RackMiniProfiler, Sidekiq Web, Flipper UI
IP_WHITELIST=127.0.0.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I can see we have not this gems for now.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think we need to add RackMiniProfiler now?

# ROLLBAR_ACCESS_TOKEN=your_key_here

# We send email using this "from" address
[email protected]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we comment it for now? We have removed devise and we do not send emails.

Copy link
Member

Choose a reason for hiding this comment

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

But we have mailer settings, and most of the applications send emails. Do you think our base application should be ready to send emails out of the box?

README.md Show resolved Hide resolved
skip_before_action :authenticate_user!

def create
result = CreateJwt.call(authentication_params)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Extract to method?

def create
  if create_jwt.success?
    ...
  else
    ...
  end
end

def create_jwt
  @create_jwt ||= CreateJwt.call(...)
end

if result.success?
respond_with_resource(result.jwt_token, status: :created, location: nil)
else
respond_with_error(:invalid_credentials)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we expose error (error_code) in interactor and use it here?

respond_with_error(result.error_code)

in interactor

context.fail!(error_code: :invalid_credentials)

It will allow to use similar format in controllers.

@@ -0,0 +1,35 @@
require "securerandom"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need it?

@@ -0,0 +1,11 @@
#!/usr/bin/env ruby
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do we need this file?

@@ -1,4 +1,4 @@
# Be sure to restart your server when you modify this file.

# Configure sensitive parameters which will be filtered from the log file.
Rails.application.config.filter_parameters += %i(password auth_token)
Rails.application.config.filter_parameters += [:password]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to knock gem (https://github.com/nsarno/knock/blob/master/lib/knock/authenticable.rb#L11) token can be passed through parameter. Should we filter it to here?

@@ -1,4 +1,4 @@
app_config.app_generators do |g|
Rails.application.config.app_generators do |g|
g.fixture_replacement :factory_girl, dir: "spec/factories"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be factroy_bot as I think.

@@ -5,8 +5,10 @@

# Enable parameter wrapping for JSON. You can disable this by setting :format to an empty array.
ActiveSupport.on_load(:action_controller) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do not need it for jsonapi requests as I think.

@timurvafin timurvafin temporarily deployed to fs-rails-base-api-pr-228 September 12, 2018 12:11 Inactive
@timurvafin
Copy link
Member

Closed in favor of #230

@timurvafin timurvafin closed this Sep 12, 2018
@timurvafin timurvafin deleted the update-ruby-rails branch September 12, 2018 12:17
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