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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Subs: Use Default Card for Subscriptions #2276

Merged
merged 16 commits into from Jul 10, 2018

Conversation

Projects
None yet
5 participants
@oeoeaio
Copy link
Contributor

oeoeaio commented May 10, 2018

What? Why?

Part 3 of 3 PRs to close #2085
This PR contains the implementation of the tools added in the previous two PRs. It replaces the existing logic for charging customers for subscriptions and for validating the payment method selection on a subscription. The last two commits contain cleanup code, removing actions, routes and attributes that are no longer required. 馃敟

What should we test?

Main things to test are:

  • As an enterprise use who wants to set up or edit a subscription that uses Stripe as the payment method, I am notified when it is not possible to charge the customer (for whatever reason, be it that they have no default card or that they have not granted permission to charge to the shop)
  • Basic testing that subscriptions which use a Stripe payment method are processed successfully when the order cycle closes
  • Need to test that something sensible happens when charging the customer fails when the order cycle closes too (for example, when the customer revokes permission to charges while the order cycle is open)

Release notes

No real release notes for this, as it is part of basic Subscriptions functionality, which hasn't been released yet (STILL!)

Dependencies

This PR depends on both #2274 and #2275, so test will fail until I have rebase on top of those. Just putting here so that others can code review if/when they get a chance.

else
$scope.cardRequired = false
$scope.subscription.credit_card_id = null
$scope.cardRequired = (paymentMethod.type == "Spree::Gateway::StripeConnect")

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

I feel this type property is way too coupled to the underlying implementation. Is this Spree imposes?

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Yes, Spree::Gateway is implemented using STI, which I don't like, but we are stuck with it unfortunately.

This comment has been minimized.

@sauloperez

sauloperez May 11, 2018

Contributor

Oh God... STI...

This comment has been minimized.

@mkllnk

mkllnk Jun 7, 2018

Member

Lol, I just googled STI: Sexually Transmissible Infections

But you probably mean Single Table Inheritance.

This comment has been minimized.

@sauloperez
params = { id: $scope.subscription.customer_id }
params.ams_prefix = 'subscription' unless $scope.subscription.id
$scope.customer = CustomerResource.get params, (response) ->
for address in ['bill_address','ship_address']

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

I believe forEach is better suited for this. It feels more natural in a functional language like JS.

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

except it's not js, its coffeescript :trollface:

This comment has been minimized.

@oeoeaio

This comment has been minimized.

@sauloperez

sauloperez May 11, 2018

Contributor

Isn't there forEach as well? Goddamn hipsters... :trollface:

@@ -87,5 +75,9 @@ def user_can_create_customer?
spree_current_user.admin? ||
spree_current_user.enterprises.include?(@customer.enterprise)
end

def ams_prefix_whitelist

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

where's this whitelist used?

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Used by a method called render_as_json to on Admin::BaseController which basically sets up a convention whereby a prefix can be submitted in a request and then it is used to find a matching serializer to render the data based on the prefix and the model of the controller. I thought it prudent to maintain a whitelist of serializer prefixes for each controller to prevent users from being able to request that data be rendered using some arbitrary serializer. Probably overkill but it's done now.

This comment has been minimized.

@mkllnk

mkllnk Jun 28, 2018

Member

Good call to implement a whitelist.

I could imagine that the ams-prefix is a symptom of a bad API design, but I haven't looked into it much.

@@ -1,5 +1,6 @@
class Api::Admin::CustomerSerializer < ActiveModel::Serializer
attributes :id, :email, :enterprise_id, :user_id, :code, :tags, :tag_list, :name
attributes :allow_charges, :default_card_present?

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

:allow_charges should go in #2275 , right?

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Oh, yes, good catch, I wonder how that got there...

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Oh, actually, no, this depends on #2275, and :allow_charges is not actually needed until now, so I think I will keep it here.

@@ -18,4 +19,9 @@ def tags
tag_rule_map || { text: tag, rules: nil }
end
end

def default_card_present?
return unless object.user

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

shouldn't always a customer be associated to a user?

EDIT now I recall having to manually update customer records to associated them to the appropriate user id. So it is not enforced.

  create_table "customers", :force => true do |t|
    t.string   "email",           :null => false
    t.integer  "enterprise_id",   :null => false
    t.string   "code"
    t.integer  "user_id"
    ...
  end

鈽濓笍 user_id is nullable 馃敶

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

See comment on #2274, we need user_id to be nullable, this is from Spree, not something that we set up

This comment has been minimized.

@sauloperez

sauloperez May 11, 2018

Contributor

馃槶

context "when the user has one credit card" do
let!(:card) { create(:credit_card, user: user) }

it "it should be assigned as the default and be returned" do

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

鈿狅笍 redundant it

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Done


context "when the user has more than one card" do
let!(:card1) { create(:credit_card, user: user) }
let!(:card2) { create(:credit_card, user: user, is_default: true) }

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

IMO naming it default_card instead of card2 will make it easier to understand for the next dev.

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Done

let(:email) { 'some@email.com' }
let(:address) { create(:address) }
let(:customer) { create(:customer, email: email) }
let(:order) { create(:completed_order_with_totals, email: email, user: nil, customer: nil, distributor: distributor) }

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

do we actually need all these models to be persisted in DB for the serializer to work? can we use FactoryBot's build_stubbed method instead?

If we avoid hitting DB we would save precious time (and our test suite is way too slow already) in this single test as the number of objects being used here is considerable.

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Yeah, you're right. I am testing stuff that is already covered by the AddressFinder spec, I've stubbed out the address finder in the spec, 2.5sec > 0.8 sec.

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Done

@@ -357,19 +356,19 @@ def stub_validations(validator, methods)
end

context "and the customer is associated with a user" do
let(:customer) { instance_double(Customer, user: user) }
before { expect(customer).to receive(:user).once { user } }

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

this is should be an allow 鈿狅笍

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Can you explain why using an expect is a problem? Is it just because we are testing the same thing for each example?

This comment has been minimized.

@sauloperez

sauloperez May 14, 2018

Contributor

It is just that you're stubbing a method here and not asserting anything, like expect is meant for.

This comment has been minimized.

@oeoeaio

oeoeaio May 17, 2018

Author Contributor

What makes you think I'm not asserting something? This expect will still cause the spec to fail if the assertion is not met.

end
end
end
end

This comment has been minimized.

@sauloperez

sauloperez May 10, 2018

Contributor

Is this related to the discussion you opened in the community? (I totally forgot about it)

This comment has been minimized.

@oeoeaio

oeoeaio May 11, 2018

Author Contributor

Yep, I'm still not really sure what to do about it, but this is the current convention, so I just stuck with it. I would quite like to decide whether we are going to stick with this approach or whether we are going to take a different path...

This comment has been minimized.

@sauloperez

sauloperez May 14, 2018

Contributor

Let's take a decision in the community thread then

@oeoeaio oeoeaio force-pushed the oeoeaio:subs-use-default-card branch from b2d7ae5 to 3df4adb May 11, 2018

This was referenced Jun 1, 2018

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Jun 7, 2018

Hm, all tests fail. I better put this back into dev. @oeoeaio Can you put it into review again once you are happy with it?

@oeoeaio

This comment has been minimized.

Copy link
Contributor Author

oeoeaio commented Jun 7, 2018

@mkllnk this is dependent on #2274 and #2275, so the test will fail until they go through and I rebase this branch.

@oeoeaio oeoeaio added the blocked label Jun 7, 2018

@oeoeaio oeoeaio force-pushed the oeoeaio:subs-use-default-card branch from 5522d4d to d0c71cc Jun 25, 2018

@oeoeaio oeoeaio removed the blocked label Jun 25, 2018

@sstead

This comment has been minimized.

Copy link

sstead commented Jun 25, 2018

Testing notes
Due to lack of time my test was only 80% as deep as I maybe would have liked, but I spotted a few issues that need work, and I can do a total test when it's next staged (or someone else can take a look)

Issues:

  • The systems isn't noticing when a customer has revoked permission of the shop to bill them after the sub is made
  • in the 2nd confirmation email to the customer where it should say 'Paid' in the shipping method section, it's saying 'not paid' - this is for customers who have been successfully billed with Stripe.

https://docs.google.com/document/d/1O6WsN2Oa3mRqzNW5XiIsAgZkHYMLjzopYBp6Ya3x_0A/edit#

@oeoeaio oeoeaio force-pushed the oeoeaio:subs-use-default-card branch from b695674 to 2bcd151 Jun 28, 2018

@oeoeaio

This comment has been minimized.

Copy link
Contributor Author

oeoeaio commented Jun 28, 2018

Ok, I've fixed those two issues, hopefully this is ready to test again.

@mkllnk

mkllnk approved these changes Jun 28, 2018

Copy link
Member

mkllnk left a comment

This all looks good to me.

@@ -87,5 +75,9 @@ def user_can_create_customer?
spree_current_user.admin? ||
spree_current_user.enterprises.include?(@customer.enterprise)
end

def ams_prefix_whitelist

This comment has been minimized.

@mkllnk

mkllnk Jun 28, 2018

Member

Good call to implement a whitelist.

I could imagine that the ams-prefix is a symptom of a bad API design, but I haven't looked into it much.

oeoeaio added some commits May 4, 2018

@mkllnk mkllnk force-pushed the oeoeaio:subs-use-default-card branch from 2bcd151 to 3dacd06 Jul 5, 2018

@mkllnk mkllnk added the pr-staged-au label Jul 5, 2018

@mkllnk

This comment has been minimized.

Copy link
Member

mkllnk commented Jul 5, 2018

@sstead sstead self-assigned this Jul 6, 2018

@sstead

This comment has been minimized.

Copy link

sstead commented Jul 6, 2018

Testing notes:

  • In the backend the shop manager can see that a customer doesn't have a billable credit card/hasn't given permission when they go to edit their subscription .. This is a good start, I'd suggest maybe an error icon so they can see that the order can't be billed without needing to go into edit the subscription
  • Subs that use stripe payment methods get processed correctly!... I made a suggestion to make the 'your credit card has been billed' messaging more clear, to make the billing seem more legitimate.
  • The messaging for scenarios where a subscription can't be charged to stripe (because customer doesn't have a default card selected or they have revoked permission to the shop) works, but there's room for improvement- such as letting the shop owner and custoomer know about the impending issue in the first email rather than only in the second email.

https://docs.google.com/document/d/1O6WsN2Oa3mRqzNW5XiIsAgZkHYMLjzopYBp6Ya3x_0A/edit#

I'd usually let Rob decide if he wan't to push, or make the improvements first. In this case i think this is probably ok to push, so people can start playing with subs, and we can make improvements later.

@daniellemoorhead

This comment has been minimized.

Copy link
Contributor

daniellemoorhead commented Jul 6, 2018

@mkllnk @sstead @kirstenalarsen in terms of getting this PR to done:

  1. The improvements Sally suggested above can be considered part of incremental improvements we need to put in beyond kicking off beta testing. Sally, can you please add these to the improvements topic on discourse - https://community.openfoodnetwork.org/t/subscriptions-improvements/1349

  2. Both Travis and CodeClimate are failing - Maikel, can you look into why this is the case and fix if possible (or if not, explain why as a comment on here). Note: This is a priority over Email Confirm final PRs.

easyoptimalhuemul-max-1mb

mkllnk added some commits Jul 9, 2018

@mkllnk mkllnk merged commit 26c2388 into openfoodfoundation:master Jul 10, 2018

3 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
semaphoreci The build passed on Semaphore.
Details

@sstead sstead removed the pr-staged-au label Jul 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment