Skip to content

Conversation

@ArthurZaharov
Copy link

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
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
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
MAILER_SENDER_ADDRESS=noreply@example.com
Copy link
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?

skip_before_action :authenticate_user!

def create
result = CreateJwt.call(authentication_params)
Copy link
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
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
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
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?


# 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
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
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.

# is enabled by default.

# Enable parameter wrapping for JSON. You can disable this by setting :format to an empty array.
ActiveSupport.on_load(:action_controller) do
Copy link
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.

3 participants