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

Using Address and Credit Card struct #74

Merged
merged 3 commits into from
Jan 4, 2018
Merged

Using Address and Credit Card struct #74

merged 3 commits into from
Jan 4, 2018

Conversation

jyotigautam
Copy link
Member

Following a proper format for Address and Credit Card using structs to maintain the consistency.

@oyeb
Copy link
Contributor

oyeb commented Jan 3, 2018

Notice that you have 2 commits (8bc5b77, 830da82) that introduce exactly the same changes. This happened because you pulled this branch back into your local, instead of doing a force push. Could you please clean up your local and do a force push?
You've also made seemingly unintentional edits to stripe.ex.

@@ -1,9 +1,9 @@
defmodule Gringotts.Gateways.Stripe do

Copy link
Member

Choose a reason for hiding this comment

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

@jyotigautam you have changed stripe.ex unnecessarily, there will be no breaking changes this time, but before committing code you should check that you are not adding any other unnecessary files.

def store(payment, opts \\ []) do
params = [email: opts[:email]]++card_params(payment)
params = [
email: opts[:email]
Copy link
Member

@chandradot99 chandradot99 Jan 3, 2018

Choose a reason for hiding this comment

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

no need to separate email in three different lines, you can do something like this

  [email: opts[:email]]
   ++ card_params(payment)
   ++ address_params(opts[:billing_address])

first_name: "John",
last_name: "Doe",
verification_code: "123",
brand: "visa"
}

Copy link
Member

Choose a reason for hiding this comment

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

@jyotigautam you are assigning a value for card whereas you are declaring module attribute for address, since these examples are going to be executed in iex, module attribute will not work. change it to below throughout the examples.

address = %Address{
 +    street1: "123 Main",
 +    street2: "Suite 100",
 +    city: "New York",
 +    region: "NY",
 +    country: "US",
 +    postal_code: "11111",
 +    phone: "(555)555-5555"
 +  }

Copy link
Member Author

Choose a reason for hiding this comment

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

Have assigned this address struct to a variable instead of using it as a module attribute as you said.

Removed changes made to stripe. Modified accessors according
to a struct. Indentation fixes.
@pkrawat1
Copy link
Member

pkrawat1 commented Jan 4, 2018

@jyotigautam coverage decreased please add necessary test cases.

@jyotigautam
Copy link
Member Author

@pkrawat1 same functions were added twice, that's why the coverage decreased.I have amended the code.

@pkrawat1 pkrawat1 merged commit ea11367 into dev Jan 4, 2018
@pkrawat1 pkrawat1 deleted the trexle_fixes branch January 4, 2018 11:11
pkrawat1 pushed a commit that referenced this pull request Jan 25, 2018
* Address and Credit Card struct usage
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants