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

Easypost integrate #10

Open
wants to merge 6 commits into
base: monterail
Choose a base branch
from
Open

Easypost integrate #10

wants to merge 6 commits into from

Conversation

RubyDiver
Copy link

Description

Please, provide here a short description.
Consider including things like:

  • An overview of why the work is taking place (with any relevant links); don’t assume familiarity with the history.
  • Major changes made and a high-level idea of the chosen approach.
  • All the compromises made (in performance, readability, maintainability etc.)
    to achieve the expected result if any

Checklist

Before you move on, make sure that:

  • No unintended changes are included
  • Spelling is correct
  • There are tests covering new/changed functionality
  • Commits have meaningful names and changes. CR remarks-like commits are squashed.
  • Proper labels assigned. Use WIP label to indicate that state

.gitignore Outdated
/yarn-error.log
# Ignore application configuration
/config/application.yml
Copy link
Member

Choose a reason for hiding this comment

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

pls remove it since we don't want to use Figaro

Gemfile Outdated
@@ -10,6 +10,8 @@ gem 'pg'
gem 'sass-rails'
# Use Uglifier as compressor for JavaScript assets
gem 'uglifier'
# Used for locating stores
gem 'geocoder', '~> 1.6', '>= 1.6.7'
Copy link
Member

Choose a reason for hiding this comment

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

It's not used in this PR!

@@ -0,0 +1,2 @@
require 'easypost'
EasyPost.api_key = 'easypost_API_key'
Copy link
Member

Choose a reason for hiding this comment

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

Pls take the value of this key from rails credentials!

Copy link
Member

Choose a reason for hiding this comment

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

pls remove easy_post.rb or easypost.rb only one make sense

.gitignore Outdated

/config/master.key
/config/credentials.yml.enc
Copy link
Member

Choose a reason for hiding this comment

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

This file is encrypted and you like to store it in the repository!
https://blog.saeloun.com/2019/10/10/rails-6-adds-support-for-multi-environment-credentials.html

/config/application.yml

/config/master.key
Copy link
Member

Choose a reason for hiding this comment

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

master.key should be added to some safe place like 1password

@@ -84,6 +86,7 @@ gem 'spree_i18n', '>= 5.0'
gem 'spree_multi_vendor', " ~> 2.2"
gem 'spree_dev_tools', require: false, group: %w[test development]
gem 'spree_multi_domain', github: 'spree-contrib/spree-multi-domain'
gem 'spree_easypost', github: 'RubyDiver/spree_easypost'
Copy link
Member

Choose a reason for hiding this comment

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

Pls add the comment why we have used fork version here

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