From 1f6a2c30546d3de73651c47e32f6cfa9dcde2209 Mon Sep 17 00:00:00 2001 From: Greg Date: Wed, 30 Jun 2021 13:37:10 -0700 Subject: [PATCH 1/7] Re-factor subscription logic for new pricing structure --- app/controllers/plans_controller.rb | 7 +- app/controllers/subscriptions_controller.rb | 7 +- .../ReposContainer/components/App.jsx | 13 +- app/models/metered_stripe_plan.rb | 46 ----- app/models/owner.rb | 14 +- app/models/plan_selector.rb | 81 ++++---- app/models/repo.rb | 4 - app/models/stripe_plan.rb | 46 ++++- app/models/user.rb | 17 +- ...s_with_membership_or_subscription_query.rb | 6 +- ...143936_add_stripe_customer_id_to_owners.rb | 5 + db/schema.rb | 3 +- lib/tasks/plan.rake | 11 ++ spec/features/plans_spec.rb | 2 +- spec/models/plan_selector_spec.rb | 178 ++++++++---------- spec/presenters/plan_presenter_spec.rb | 8 +- spec/services/repo_subscriber_spec.rb | 16 +- 17 files changed, 226 insertions(+), 238 deletions(-) delete mode 100644 app/models/metered_stripe_plan.rb create mode 100644 db/migrate/20210627143936_add_stripe_customer_id_to_owners.rb diff --git a/app/controllers/plans_controller.rb b/app/controllers/plans_controller.rb index 1a96cf6384..e4065dc934 100644 --- a/app/controllers/plans_controller.rb +++ b/app/controllers/plans_controller.rb @@ -1,9 +1,4 @@ class PlansController < ApplicationController - MARKETPLACE_URL = ENV.fetch( - "MARKETPLACE_URL", - "https://www.github.com/marketplace/hound" - ) - helper_method :plan_selector def index @@ -28,7 +23,7 @@ def plan_selector def marketplace_upgrade_url if plan_selector.marketplace_plan? - "#{MARKETPLACE_URL}/order/#{plan_selector.next_plan.slug}?account=#{repo.owner.name}" + plan_selector.marketplace_upgrade_url end end diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index a028aa1175..61000f8cce 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -7,18 +7,19 @@ class FailedToActivate < StandardError; end def create plan_selector = PlanSelector.new(user: current_user, repo: repo) + if plan_selector.paywall? + render json: {}, status: :payment_required if plan_selector.upgrade? render json: {}, status: :payment_required - elsif plan_selector.marketplace_plan? + else repo.activate render json: repo, status: :created - else - activate_and_create_subscription end end def update + # candidate for removal? activate_and_create_subscription end diff --git a/app/javascript/components/ReposContainer/components/App.jsx b/app/javascript/components/ReposContainer/components/App.jsx index ada18ba7fd..a89330e0a1 100644 --- a/app/javascript/components/ReposContainer/components/App.jsx +++ b/app/javascript/components/ReposContainer/components/App.jsx @@ -141,13 +141,12 @@ export default class App extends React.Component { } createSubscriptionWithExistingCard(repo) { - Ajax.createSubscription({ repo_id: repo.id }) - .then(resp => { - this.activateAndTrackRepoSubscription( - repo, resp.stripe_subscription_id - ); - }) - .catch(error => this.onSubscriptionError(repo, error)); + Ajax.createSubscription({ repo_id: repo.id }).then( resp => { + this.activateAndTrackRepoSubscription( + repo, + resp.stripe_subscription_id + ); + }).catch(error => this.onSubscriptionError(repo, error)); } activateFreeRepo(repo) { diff --git a/app/models/metered_stripe_plan.rb b/app/models/metered_stripe_plan.rb deleted file mode 100644 index b93e6dabbd..0000000000 --- a/app/models/metered_stripe_plan.rb +++ /dev/null @@ -1,46 +0,0 @@ -class MeteredStripePlan < Plan - PLANS = [ - { - id: nil, - price: 0, - title: "Open Source", - range: (0..0), - }, - { - id: "plan_FXpsAlar939qfx", - price: 29, - title: "Chihuahua", - range: (0..50), - }, - { - id: "plan_FXpsHlYOH8tAfo", - price: 49, - title: "Terrier", - range: (51..300), - }, - { - id: "plan_FXptlXmCZwt7Rf", - price: 99, - title: "Labrador", - range: (301..1_000), - }, - { - id: "plan_FXptCDypXrtK0c", - price: 199, - title: "Husky", - range: (1_001..3_000), - }, - { - id: "plan_FXpu6Y3Dhrllj6", - price: 299, - title: "Great Dane", - range: (3_001..10_000), - }, - { - id: "price_1IRPYr2QAwplLIsJv7ilvH7K", - price: 299 * 12, - title: "Great Dane (Yearly)", - range: (3_001..10_000), - }, - ].freeze -end diff --git a/app/models/owner.rb b/app/models/owner.rb index 7966123028..960a263269 100644 --- a/app/models/owner.rb +++ b/app/models/owner.rb @@ -51,13 +51,13 @@ def recent_invoice_url end def recent_builds - Build.where( - "DATE(created_at) > DATE(?) AND DATE(created_at) < DATE(?)", - 1.month.ago, - Time.current, - ).where( - repo_id: repo_ids, - ).count + Build. + where(repo_id: repo_ids) + where( + "DATE(created_at) > DATE(?) AND DATE(created_at) < DATE(?)", + 1.month.ago, + Time.current, + ).count end def metered_plan? diff --git a/app/models/plan_selector.rb b/app/models/plan_selector.rb index 089bc8706e..743b9fb571 100644 --- a/app/models/plan_selector.rb +++ b/app/models/plan_selector.rb @@ -1,76 +1,81 @@ class PlanSelector BULK_ID = "bulk".freeze + MARKETPLACE_URL = ENV.fetch( + "MARKETPLACE_URL", + "https://www.github.com/marketplace/hound" + ) + + delegate :owner, to: :repo, allow_nil: true def initialize(user:, repo: nil) @user = user - @repo = repo || user.first_available_repo + @repo = repo || user.first_active_private_repo end - def current_plan + def paywall? + owner.stripe_customer_id.blank? && marketplace_plan_id.blank? + end + + def upgrade? if marketplace_plan? - plans.detect { |plan| plan.id == marketplace_plan_id } - elsif metered_plan? - plans.detect { |plan| plan.id == user.payment_gateway_subscription.plan } + owner.active_private_repos_count + 1 > current_plan.allowance else - find_plan_by_active_repo_count(active_repo_count) + owner.recent_builds >= current_plan.allowance end end - def upgrade? - if repo&.owner&.whitelisted? - false - elsif metered_plan? - current_plan.open_source? + def marketplace_plan? + marketplace_plan_id.present? + end + + def current_plan + if marketplace_plan? + plans.detect { |plan| plan.id == marketplace_plan_id } else - !!(next_plan && next_plan.allowance > current_plan.allowance) + if owner + plans.detect do |plan| + plan.id == owner.payment_gateway_subscription.plan + end || free_plan + else + free_plan + end end end def next_plan - find_plan_by_active_repo_count(active_repo_count.succ) - end - - def previous_plan - find_plan_by_active_repo_count(active_repo_count.pred) + current_plan_index = plans.index(current_plan) + if current_plan_index + plans[current_plan_index + 1] || plans.last + end end - def marketplace_plan? - marketplace_plan_id.present? + def current_marketplace_plan + plans.detect do |plan| + plan.range.include?(owner.active_private_repos_count) + end end def plans - plan_class::PLANS.map { |plan| plan_class.new(**plan) } + @_plans ||= plan_class::PLANS.map { |plan| plan_class.new(**plan) } end private attr_reader :user, :repo - def find_plan_by_active_repo_count(active_repo_count) - plans.detect do |plan| - plan.range.include? active_repo_count - end + def free_plan + plan_class.new(**plan_class::PLANS[0]) end def plan_class - if marketplace_plan? - GitHubPlan - elsif metered_plan? - MeteredStripePlan - else - StripePlan - end + marketplace_plan? ? GitHubPlan : StripePlan end def marketplace_plan_id - repo&.owner&.marketplace_plan_id - end - - def active_repo_count - user.subscribed_repos.size + owner&.marketplace_plan_id end - def metered_plan? - repo&.metered_plan? + def marketplace_upgrade_url + "#{MARKETPLACE_URL}/order/#{next_plan.slug}?account=#{repo&.owner&.name}" end end diff --git a/app/models/repo.rb b/app/models/repo.rb index 365d6efea1..66b351a7ad 100644 --- a/app/models/repo.rb +++ b/app/models/repo.rb @@ -39,10 +39,6 @@ def stripe_subscription_id end end - def metered_plan? - owner.metered_plan? - end - def total_violations builds.sum(:violations_count) end diff --git a/app/models/stripe_plan.rb b/app/models/stripe_plan.rb index 72ee75ae63..9243ff1822 100644 --- a/app/models/stripe_plan.rb +++ b/app/models/stripe_plan.rb @@ -1,8 +1,46 @@ class StripePlan < Plan PLANS = [ - { id: "basic", price: 0, range: 0..0, title: "Hound" }, - { id: "tier1", price: 49, range: 1..4, title: "Chihuahua" }, - { id: "tier2", price: 99, range: 5..10, title: "Labrador" }, - { id: "tier3", price: 249, range: 11..30, title: "Great Dane" }, + { + id: "free", + price: 0, + title: "Open Source", + range: (0..0), + }, + { + id: "plan_FXpsAlar939qfx", + price: 29, + title: "Chihuahua", + range: (0..50), + }, + { + id: "plan_FXpsHlYOH8tAfo", + price: 49, + title: "Terrier", + range: (51..300), + }, + { + id: "plan_FXptlXmCZwt7Rf", + price: 99, + title: "Labrador", + range: (301..1_000), + }, + { + id: "plan_FXptCDypXrtK0c", + price: 199, + title: "Husky", + range: (1_001..3_000), + }, + { + id: "plan_FXpu6Y3Dhrllj6", + price: 299, + title: "Great Dane", + range: (3_001..10_000), + }, + { + id: "price_1IRPYr2QAwplLIsJv7ilvH7K", + price: 299 * 12, + title: "Great Dane (Yearly)", + range: (3_001..10_000), + }, ].freeze end diff --git a/app/models/user.rb b/app/models/user.rb index 9805ed3abe..2b2319552d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -11,7 +11,7 @@ class User < ApplicationRecord before_create :generate_remember_token - delegate :current_plan, :next_plan, :previous_plan, to: :plan_selector + delegate :current_plan, :next_plan, to: :plan_selector def next_plan_price next_plan.price @@ -57,20 +57,15 @@ def repos_by_activation_ability repos. order("memberships.admin DESC"). order(active: :desc). - order(Arel.sql("LOWER(name) ASC")) + order("LOWER(name) ASC") end def card_exists? stripe_customer_id.present? end - def subscribed_repos - if plan_selector.marketplace_plan? - # This assumes a user manages one Marketplace purchase. - first_available_repo.owner.repos.active.where(private: true) - else - super - end + def owner_ids + repos.distinct.pluck(:owner_id) end def first_available_repo @@ -81,6 +76,10 @@ def first_available_repo end end + def first_active_private_repo + repos.active.where(private: true).first + end + def marketplace_user? plan_selector.marketplace_plan? end diff --git a/app/queries/repos_with_membership_or_subscription_query.rb b/app/queries/repos_with_membership_or_subscription_query.rb index 3a53c3e794..b8b3e159c2 100644 --- a/app/queries/repos_with_membership_or_subscription_query.rb +++ b/app/queries/repos_with_membership_or_subscription_query.rb @@ -6,11 +6,15 @@ def initialize(user) end def call - subscribed_repos | activatable_repos + subscribed_repos | activatable_repos | owner_active_repos end private + def owner_active_repos + Repo.active.where(owner_id: @user.owner_ids) + end + def subscribed_repos @user.subscribed_repos.includes(*repo_includes) end diff --git a/db/migrate/20210627143936_add_stripe_customer_id_to_owners.rb b/db/migrate/20210627143936_add_stripe_customer_id_to_owners.rb new file mode 100644 index 0000000000..bca36e2ab8 --- /dev/null +++ b/db/migrate/20210627143936_add_stripe_customer_id_to_owners.rb @@ -0,0 +1,5 @@ +class AddStripeCustomerIdToOwners < ActiveRecord::Migration[6.0] + def change + add_column :owners, :stripe_customer_id, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index e9e742687e..6bca9a150a 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2019_12_12_043041) do +ActiveRecord::Schema.define(version: 2021_06_27_143936) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -83,6 +83,7 @@ t.boolean "whitelisted", default: false, null: false t.integer "marketplace_plan_id" t.string "stripe_subscription_id" + t.string "stripe_customer_id" t.index ["github_id"], name: "index_owners_on_github_id", unique: true t.index ["name"], name: "index_owners_on_name", unique: true end diff --git a/lib/tasks/plan.rake b/lib/tasks/plan.rake index 3e9c46ce0a..4a2dbdad4d 100644 --- a/lib/tasks/plan.rake +++ b/lib/tasks/plan.rake @@ -3,4 +3,15 @@ namespace :plan do task update_all: :environment do UpdateGitHubPlans.call end + + desc "Update marketplace plans for all owners" + task migrate_owners: :environment do + owners = Repo.joins(:subscription).map(&:owner).uniq + owners.each do |owner| + stripe_user = owner.repos.active.find(&:subscription).subscription.user + puts "Onwer #{owner.name} mapped to user #{stripe_user.username}" + + owner.update!(stripe_customer_id: stripe_user.stripe_customer_id) + end + end end diff --git a/spec/features/plans_spec.rb b/spec/features/plans_spec.rb index c3ab92e1dc..d83226e4e0 100644 --- a/spec/features/plans_spec.rb +++ b/spec/features/plans_spec.rb @@ -57,7 +57,7 @@ stub_repository_invitations(repo.name) stub_customer_find_request stub_subscription_create_request( - plan: MeteredStripePlan::PLANS.second.fetch(:id), + plan: StripePlan::PLANS.second.fetch(:id), repo_ids: repo.id, ) stub_subscription_update_request( diff --git a/spec/models/plan_selector_spec.rb b/spec/models/plan_selector_spec.rb index 99984a79be..ec8d768fe9 100644 --- a/spec/models/plan_selector_spec.rb +++ b/spec/models/plan_selector_spec.rb @@ -3,163 +3,143 @@ require "app/models/plan" require "app/models/stripe_plan" require "app/models/github_plan" -require "app/models/metered_stripe_plan" require "app/models/plan_selector" RSpec.describe PlanSelector do describe "#current_plan" do - it "returns user's current plan" do - user = instance_double( - "User", - subscribed_repos: [double], - metered_plan?: false, - ) - repo = instance_double( - "Repo", - metered_plan?: false, - owner: double.as_null_object, + it "returns owner's current plan" do + plan = StripePlan::PLANS[1] + payment_gateway = instance_double( + "PaymentGatewaySubscription", + plan: plan[:id], ) + owner = build_owner(payment_gateway_subscription: payment_gateway) + user = instance_double("User") + repo = instance_double("Repo", owner: owner) plan_selector = PlanSelector.new(user: user, repo: repo) - expect(plan_selector.current_plan). - to eq StripePlan.new(**StripePlan::PLANS[1]) + actual = plan_selector.current_plan + + expect(actual).to eq StripePlan.new(plan) end end describe "#upgrade?" do - context "user is a Stripe subscriber" do - context "when the next plan is different to the current plan" do + context "when owner is subscribed via Stripe" do + context "when the owner isn't subsribed to any plan" do it "returns true" do - user = instance_double( - "User", - subscribed_repos: Array.new(4) { double }, - ) - repo = instance_double( - "Repo", - metered_plan?: false, - owner: double(whitelisted?: false, marketplace_plan_id: nil), + payment_gateway = instance_double( + "PaymentGatewaySubscription", + plan: nil ) + owner = build_owner(payment_gateway_subscription: payment_gateway) + user = instance_double("User") + repo = instance_double("Repo", owner: owner) plan_selector = PlanSelector.new(user: user, repo: repo) - expect(plan_selector).to be_upgrade + actual = plan_selector.upgrade? + + expect(actual).to eq true end end - context "when the user has no repos" do + context "when recent builds is greater or equal than plan allowance" do it "returns true" do - user = instance_double( - "User", - subscribed_repos: [], - first_available_repo: nil, - metered_plan?: false, - ) - plan_selector = PlanSelector.new(user: user, repo: nil) + owner = build_owner(recent_builds: 50) + repo = instance_double("Repo", owner: owner) + user = instance_double("User") + plan_selector = PlanSelector.new(user: user, repo: repo) - expect(plan_selector).to be_upgrade + actual = plan_selector.upgrade? + + expect(actual).to eq true end end - context "when the next plan is not the same as the current plan" do + context "recent builds is less than plan allowance" do it "returns false" do - user = instance_double( - "User", - subscribed_repos: Array.new(3) { double }, - metered_plan?: false, - ) - repo = instance_double( - "Repo", - owner: double.as_null_object, - ) + owner = build_owner(recent_builds: 49) + repo = instance_double("Repo", owner: owner) + user = instance_double("User") plan_selector = PlanSelector.new(user: user, repo: repo) - expect(plan_selector).not_to be_upgrade + actual = plan_selector.upgrade? + + expect(actual).to eq false end end end - context "user is a GitHub Marketplace subscriber" do + context "when owner is subscribed via GitHub Marketplace" do [ { current_plan: 0, repos: 0, expected: true }, { current_plan: 1, repos: 3, expected: false }, { current_plan: 1, repos: 4, expected: true }, { current_plan: 2, repos: 5, expected: false }, { current_plan: 2, repos: 19, expected: false }, - { current_plan: 2, repos: 20, expected: false }, + { current_plan: 2, repos: 20, expected: true }, ].each do |test_data| context "when account has #{test_data[:repos]} private repos" do it "returns #{test_data[:expected]}" do current_plan = GitHubPlan::PLANS[test_data[:current_plan]] owner = instance_double( "Owner", - whitelisted?: false, marketplace_plan_id: current_plan[:id], + active_private_repos_count: test_data[:repos], ) - repo = instance_double("Repo", owner: owner, metered_plan?: false) - user = instance_double( - "User", - subscribed_repos: double(size: test_data[:repos]), - ) + repo = instance_double("Repo", owner: owner) + user = instance_double("User") plan_selector = described_class.new(user: user, repo: repo) - expect(plan_selector.upgrade?).to eq(test_data[:expected]) - end - end - end - end - - context "user is a MeteredStripe subscriber" do - context "when the current plan is open source (free)" do - it "returns true" do - user = instance_double( - "User", - subscribed_repos: Array.new(1) { double }, - payment_gateway_subscription: double( - plan: MeteredStripePlan::PLANS[0]["id"], - ), - ) - repo = instance_double( - "Repo", - metered_plan?: true, - owner: double(whitelisted?: false, marketplace_plan_id: nil), - ) - plan_selector = PlanSelector.new(user: user, repo: repo) + actual = plan_selector.upgrade? - expect(plan_selector).to be_upgrade + expect(actual).to eq(test_data[:expected]) + end end end end end describe "#next_plan" do - context "when the user has no subscribed repos" do + context "when the owner has no subscribed repos" do it "returns the first paid plan" do - owner = instance_double("Owner", marketplace_plan_id: nil) - repo = instance_double("Repo", owner: owner, metered_plan?: false) - user = instance_double( - "User", - subscribed_repos: [], - first_available_repo: repo, - ) - plan_selector = PlanSelector.new(user: user, repo: nil) - - expect(plan_selector.next_plan). - to eq StripePlan.new(**StripePlan::PLANS[1]) + owner = instance_double("Owner") + user = instance_double("User", first_active_private_repo: nil) + repo = instance_double("Repo", owner: owner) + plan_selector = PlanSelector.new(user: user) + + actual = plan_selector.next_plan + + expect(actual).to eq StripePlan.new(**StripePlan::PLANS[1]) end end - end - describe "#previous_plan" do - it "returns the second paid plan" do - owner = instance_double("Owner", marketplace_plan_id: nil) - repo = instance_double("Repo", owner: owner, metered_plan?: false) - user = instance_double( - "User", - subscribed_repos: Array.new(10) { double }, - ) - plan_selector = PlanSelector.new(user: user, repo: repo) + context "when the user has maxed out recent builds" do + it "returns the next paid plan" do + owner = build_owner(recent_builds: 55) + repo = instance_double("Repo", owner: owner) + user = instance_double("User", first_active_private_repo: repo) + plan_selector = PlanSelector.new(user: user) + + actual = plan_selector.next_plan - expect(plan_selector.previous_plan). - to eq StripePlan.new(StripePlan::PLANS[2]) + expect(actual).to eq StripePlan.new(**StripePlan::PLANS[2]) + end end end + + def build_owner(options = {}) + plan = StripePlan::PLANS[1] + payment_gateway_subscription = instance_double( + "PaymentGatewaySubscription", + plan: plan[:id], + ) + default_options = { + recent_builds: 0, + marketplace_plan_id: nil, + payment_gateway_subscription: payment_gateway_subscription, + } + + instance_double("Owner", default_options.merge(options)) + end end diff --git a/spec/presenters/plan_presenter_spec.rb b/spec/presenters/plan_presenter_spec.rb index 60c96b5db2..af82d74ca9 100644 --- a/spec/presenters/plan_presenter_spec.rb +++ b/spec/presenters/plan_presenter_spec.rb @@ -17,7 +17,7 @@ membership = create(:membership) user = membership.user create(:subscription, repo: membership.repo, user: user) - plan = MeteredStripePlan.new(MeteredStripePlan::PLANS.first) + plan = StripePlan::PLANS.first presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to be_current @@ -29,7 +29,7 @@ membership = create(:membership) user = membership.user create(:subscription, repo: membership.repo, user: user) - plan = MeteredStripePlan.new(MeteredStripePlan::PLANS.second) + plan = StripePlan::PLANS.second presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to_not be_current @@ -43,7 +43,7 @@ membership = create(:membership) user = membership.user create(:subscription, repo: membership.repo, user: user) - plan = MeteredStripePlan.new(MeteredStripePlan::PLANS.second) + plan = StripePlan::PLANS.second presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to be_next @@ -55,7 +55,7 @@ membership = create(:membership) user = membership.user create(:subscription, repo: membership.repo, user: user) - plan = MeteredStripePlan.new(MeteredStripePlan::PLANS.first) + plan = StripePlan::PLANS.first presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to_not be_next diff --git a/spec/services/repo_subscriber_spec.rb b/spec/services/repo_subscriber_spec.rb index 4244b44539..8e919a3890 100644 --- a/spec/services/repo_subscriber_spec.rb +++ b/spec/services/repo_subscriber_spec.rb @@ -14,7 +14,7 @@ stub_customer_find_request update_request = stub_customer_update_request subscription_request = stub_subscription_create_request( - plan: user.next_plan.id, + plan: StripePlan::PLANS[1][:id], repo_ids: repo.id, ) subscription_update_request = stub_subscription_update_request( @@ -39,7 +39,7 @@ ) stub_customer_find_request subscription_request = stub_subscription_create_request( - plan: user.next_plan.id, + plan: StripePlan::PLANS[1][:id], repo_ids: repo.id, ) subscription_update_request = stub_subscription_update_request( @@ -66,7 +66,7 @@ user = create(:user, repos: [repo], stripe_customer_id: "",) customer_request = stub_customer_create_request(user) subscription_request = stub_subscription_create_request( - plan: user.next_plan.id, + plan: StripePlan::PLANS[1][:id], repo_ids: repo.id, ) update_request = stub_subscription_update_request(repo_ids: repo.id) @@ -87,7 +87,9 @@ repo = create(:repo) user = create(:user, repos: [repo]) stub_customer_create_request(user) - stub_failed_subscription_create_request(user.next_plan.id) + stub_failed_subscription_create_request( + StripePlan::PLANS[1][:id], + ) result = RepoSubscriber.subscribe(repo, user, "cardtoken") @@ -113,12 +115,10 @@ user = create(:user, repos: [repo]) stub_customer_create_request(user) stub_subscription_create_request( - plan: user.next_plan.id, - repo_ids: repo.id, - ) - stub_subscription_update_request( + plan: StripePlan::PLANS[1][:id], repo_ids: repo.id, ) + stub_subscription_update_request(repo_ids: repo.id) allow(repo).to receive(:create_subscription!).and_raise(StandardError) result = RepoSubscriber.subscribe(repo, user, "cardtoken") From 5e27cd3246e390b45bab3ead73496f5acddfa3e9 Mon Sep 17 00:00:00 2001 From: Greg Date: Wed, 30 Jun 2021 15:57:31 -0700 Subject: [PATCH 2/7] More fixes --- app/controllers/subscriptions_controller.rb | 4 +- app/models/home.rb | 2 +- app/models/owner.rb | 6 +- app/models/plan_selector.rb | 39 ++--- app/models/user.rb | 24 ++-- app/presenters/account_page.rb | 5 +- .../subscriptions_controller_spec.rb | 133 ++++++------------ spec/factories.rb | 2 + spec/models/home_spec.rb | 12 +- spec/models/plan_selector_spec.rb | 53 ++----- spec/models/user_spec.rb | 21 +-- spec/presenters/plan_presenter_spec.rb | 39 +++-- ...customer_find_with_tier2_subscription.json | 84 ----------- ...customer_find_with_tier3_subscription.json | 84 ----------- 14 files changed, 134 insertions(+), 374 deletions(-) delete mode 100644 spec/support/fixtures/stripe_customer_find_with_tier2_subscription.json delete mode 100644 spec/support/fixtures/stripe_customer_find_with_tier3_subscription.json diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index 61000f8cce..154f961eeb 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -5,11 +5,11 @@ class FailedToActivate < StandardError; end before_action :update_email def create - plan_selector = PlanSelector.new(user: current_user, repo: repo) + plan_selector = PlanSelector.new(repo.owner) if plan_selector.paywall? render json: {}, status: :payment_required - if plan_selector.upgrade? + elsif plan_selector.upgrade? render json: {}, status: :payment_required else repo.activate diff --git a/app/models/home.rb b/app/models/home.rb index dbb9395869..6cd455d0ff 100644 --- a/app/models/home.rb +++ b/app/models/home.rb @@ -32,6 +32,6 @@ def open_source_repos end def plan_selector - PlanSelector.new(user: user) + PlanSelector.new(user.owner) end end diff --git a/app/models/owner.rb b/app/models/owner.rb index 960a263269..b2b880a4f3 100644 --- a/app/models/owner.rb +++ b/app/models/owner.rb @@ -52,7 +52,7 @@ def recent_invoice_url def recent_builds Build. - where(repo_id: repo_ids) + where(repo_id: repo_ids). where( "DATE(created_at) > DATE(?) AND DATE(created_at) < DATE(?)", 1.month.ago, @@ -64,6 +64,10 @@ def metered_plan? marketplace_plan_id.blank? end + def stripe_plan_id + @_stripe_plan_id ||= PaymentGatewayCustomer.new(self).subscription.plan + end + private def hound_config diff --git a/app/models/plan_selector.rb b/app/models/plan_selector.rb index 743b9fb571..9dbbdd3867 100644 --- a/app/models/plan_selector.rb +++ b/app/models/plan_selector.rb @@ -5,11 +5,8 @@ class PlanSelector "https://www.github.com/marketplace/hound" ) - delegate :owner, to: :repo, allow_nil: true - - def initialize(user:, repo: nil) - @user = user - @repo = repo || user.first_active_private_repo + def initialize(owner) + @owner = owner end def paywall? @@ -24,21 +21,11 @@ def upgrade? end end - def marketplace_plan? - marketplace_plan_id.present? - end - def current_plan if marketplace_plan? plans.detect { |plan| plan.id == marketplace_plan_id } else - if owner - plans.detect do |plan| - plan.id == owner.payment_gateway_subscription.plan - end || free_plan - else - free_plan - end + plans.detect { |plan| plan.id == owner.stripe_plan_id } || free_plan end end @@ -49,19 +36,13 @@ def next_plan end end - def current_marketplace_plan - plans.detect do |plan| - plan.range.include?(owner.active_private_repos_count) - end - end - def plans @_plans ||= plan_class::PLANS.map { |plan| plan_class.new(**plan) } end private - attr_reader :user, :repo + attr_reader :owner def free_plan plan_class.new(**plan_class::PLANS[0]) @@ -71,11 +52,15 @@ def plan_class marketplace_plan? ? GitHubPlan : StripePlan end - def marketplace_plan_id - owner&.marketplace_plan_id + def marketplace_plan? + marketplace_plan_id.present? end - def marketplace_upgrade_url - "#{MARKETPLACE_URL}/order/#{next_plan.slug}?account=#{repo&.owner&.name}" + def marketplace_plan_id + owner.marketplace_plan_id end + + # def marketplace_upgrade_url + # "#{MARKETPLACE_URL}/order/#{next_plan.slug}?account=#{owner.name}" + # end end diff --git a/app/models/user.rb b/app/models/user.rb index 2b2319552d..cefdd7f3d3 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -72,24 +72,32 @@ def first_available_repo if subscriptions.any? subscriptions.first.repo else - repos.order(:active).first + repos.order([:private, :active]).first end end - def first_active_private_repo - repos.active.where(private: true).first - end - def marketplace_user? plan_selector.marketplace_plan? end def metered_plan? - first_available_repo&.owner&.metered_plan? + owner.metered_plan? end def recent_builds - first_available_repo.owner.recent_builds + owner.recent_builds + end + + def owner + @_owner ||= begin + user_as_owner = Owner.find_by(name: username) + + if user_as_owner.stripe_plan_id.present? + user_as_owner + else + first_available_repo&.owner || user_as_owner + end + end end private @@ -104,7 +112,7 @@ def payment_gateway_customer end def plan_selector - @_plan_selector ||= PlanSelector.new(user: self) + @_plan_selector ||= PlanSelector.new(owner) end def generate_remember_token diff --git a/app/presenters/account_page.rb b/app/presenters/account_page.rb index 26248c50a8..2407757d8a 100644 --- a/app/presenters/account_page.rb +++ b/app/presenters/account_page.rb @@ -20,7 +20,10 @@ def plan end def plans - plan_selector = PlanSelector.new(user: user) + plan_selector = PlanSelector.new( + user: user, + repo: user.first_available_repo, + ) plan_selector.plans.map do |plan| PlanPresenter.new(plan: plan, user: user) diff --git a/spec/controllers/subscriptions_controller_spec.rb b/spec/controllers/subscriptions_controller_spec.rb index c851366064..12b33b3cc7 100644 --- a/spec/controllers/subscriptions_controller_spec.rb +++ b/spec/controllers/subscriptions_controller_spec.rb @@ -3,111 +3,87 @@ describe SubscriptionsController do describe "#create" do context "when subscription succeeds" do - context "with Metered subscription" do - it "subscribes the user to the repo" do - repo = create(:repo, private: true) - user = create(:user, :stripe) - membership = create(:membership, repo: repo, user: user) - create(:subscription, repo: repo, user: user) - allow(RepoSubscriber).to receive(:subscribe).and_return(true) - stub_sign_in(user) + context "with existing Stripe subscriber" do + it "responds with activated repo" do + owner = create(:owner, :stripe) + repo = create(:repo, owner: owner, private: true) + user = create(:user, repos: [repo]) stub_customer_find_request_with_subscriptions + stub_sign_in(user) - post( - :create, - params: { - repo_id: repo.id, - card_token: "cardtoken", - email: "jimtom@example.com", - }, - format: :json, - ) + post :create, params: { repo_id: repo.id, card_token: "cardtoken" } - expect(RepoSubscriber).to have_received(:subscribe). - with(repo, membership.user, "cardtoken") + expect(response.status).to eq 201 + expect(JSON.parse(response.body)).to match hash_including( + "name" => repo.name, + "active" => true, + "owner" => hash_including("name" => owner.name), + ) end end - context "with GitHub subscription" do + context "with GitHub Marketplace subscriber" do it "subscribes the user to the repo" do - repo = create(:repo, private: true) - repo.owner.update!(marketplace_plan_id: GitHubPlan::PLANS.last[:id]) - membership = create(:membership, repo: repo) - allow(RepoSubscriber).to receive(:subscribe) - stub_sign_in(membership.user) - - post( - :create, - params: { - repo_id: repo.id, - card_token: "cardtoken", - email: "jimtom@example.com", - }, - format: :json, - ) + owner = create(:owner, marketplace_plan_id: GitHubPlan::PLANS[1][:id]) + repo = create(:repo, owner: owner, private: true) + user = create(:user, repos: [repo]) + stub_sign_in(user) + + post :create, params: { repo_id: repo.id, card_token: "cardtoken" } - expect(RepoSubscriber).not_to have_received(:subscribe) + expect(response.status).to eq 201 + expect(JSON.parse(response.body)).to match hash_including( + "name" => repo.name, + "active" => true, + "owner" => hash_including("name" => owner.name), + ) end end it "updates the current user's email address" do - user = create(:user, email: nil) repo = create(:repo) - user.repos << repo - allow(RepoSubscriber).to receive(:subscribe).and_return(true) + user = create(:user, email: nil, repos: [repo]) stub_sign_in(user) - post( - :create, - params: { - repo_id: repo.id, - card_token: "cardtoken", - email: "jimtom@example.com", - }, - format: :json, - ) + post :create, params: { repo_id: repo.id, email: "jimtom@example.com" } expect(user.reload.email).to eq "jimtom@example.com" end end - context "when subscription fails" do - it "deactivates repo" do + context "when repo activation requires payment" do + it "responds with payment_required and does not activate the repo" do repo = create(:repo, private: true) - user = create(:user, :stripe) - create(:membership, repo: repo, user: user) - create(:subscription, repo: repo, user: user) - deactivate_repo = instance_double("DeactivateRepo", call: nil) - allow(DeactivateRepo).to receive(:new).and_return(deactivate_repo) - allow(RepoSubscriber).to receive(:subscribe).and_return(false) + user = create(:user, repos: [repo]) stub_sign_in(user) - stub_customer_find_request_with_subscriptions - post :create, params: { repo_id: repo.id }, format: :json + post :create, params: { repo_id: repo.id } - expect(response.code).to eq "502" - expect(deactivate_repo).to have_received(:call) + expect(response).to have_http_status(:payment_required) + expect(repo.reload.active).to eq false end end - context "when the current plan is open source (free)" do + context "when the current plan requires an upgrade" do it "notifies that payment is required" do - membership = create(:membership) - repo = membership.repo - user = membership.user + owner = create(:owner, marketplace_plan_id: GitHubPlan::PLANS[0][:id]) + repo = create(:repo, owner: owner, private: true) + user = create(:user, repos: [repo]) stub_sign_in(user) post :create, params: { repo_id: repo.id } expect(response).to have_http_status(:payment_required) + expect(repo.reload.active).to eq false end end end + # need to figure out what this action is for and how to simplify it describe "#update" do context "when the subscription can be created" do it "returns 'Created'" do - repo = create(:repo, name: "foo/bar", private: true) + repo = create(:repo, private: true) user = create(:user, repos: [repo]) create(:subscription, repo: repo, user: user) stub_sign_in(user) @@ -120,7 +96,7 @@ context "when the subscription cannot be created" do it "returns an error and deactivates the repo" do - repo = create(:repo, name: "foo/bar", private: true) + repo = create(:repo, private: true) user = create(:user, repos: [repo]) deactivate_repo = instance_double("DeactivateRepo", call: nil) allow(RepoSubscriber).to receive(:subscribe).and_return(false) @@ -148,14 +124,7 @@ allow(DeactivateRepo).to receive(:new).and_return(deactivate_repo) stub_sign_in(current_user) - delete( - :destroy, - params: { - repo_id: repo.id, - card_token: "cardtoken", - }, - format: :json, - ) + delete :destroy, params: { repo_id: repo.id, card_token: "cardtoken" } expect(response.status).to eq(409) response_body = JSON.parse(response.body) @@ -176,14 +145,7 @@ allow(RepoSubscriber).to receive(:unsubscribe).and_return(true) stub_sign_in(current_user) - delete( - :destroy, - params: { - repo_id: repo.id, - card_token: "cardtoken", - }, - format: :json, - ) + delete :destroy, params: { repo_id: repo.id, card_token: "cardtoken" } expect(deactivate_repo).to have_received(:call) expect(DeactivateRepo).to have_received(:new). @@ -192,12 +154,7 @@ with(repo, subscribed_user) expect(analytics).to have_tracked("Repo Deactivated"). for_user(current_user). - with( - properties: { - name: repo.name, - private: true, - }, - ) + with(properties: { name: repo.name, private: true }) end end end diff --git a/spec/factories.rb b/spec/factories.rb index 7f67d0d106..f9f5ea57c5 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -88,6 +88,8 @@ end factory :owner do + trait(:stripe) { stripe_customer_id { "cus_2e3fqARc1uHtCv" } } + github_id name { generate(:github_name) } end diff --git a/spec/models/home_spec.rb b/spec/models/home_spec.rb index 802ddd31de..44fa144c1d 100644 --- a/spec/models/home_spec.rb +++ b/spec/models/home_spec.rb @@ -14,7 +14,7 @@ presenter = instance_double("PlanPresenter") private_plan = instance_double("StripePlan", open_source?: false) plans = [open_source_plan, private_plan] - user = instance_double("User") + user = instance_double("User", owner: double) home = Home.new(user) plan_selector = instance_double("PlanSelector", plans: plans) allow(PlanSelector).to receive(:new).and_return(plan_selector) @@ -25,9 +25,7 @@ plan: open_source_plan, user: user, ) - expect(PlanSelector).to have_received(:new).once.with( - user: user, - ) + expect(PlanSelector).to have_received(:new).once.with(user.owner) end end @@ -37,7 +35,7 @@ presenter = instance_double("PlanPresenter") private_plan = instance_double("StripePlan", open_source?: false) plans = [open_source_plan, private_plan] - user = instance_double("User") + user = instance_double("User", owner: double) home = Home.new(user) plan_selector = instance_double("PlanSelector", plans: plans) allow(PlanSelector).to receive(:new).and_return(plan_selector) @@ -48,9 +46,7 @@ plan: private_plan, user: user, ) - expect(PlanSelector).to have_received(:new).once.with( - user: user, - ) + expect(PlanSelector).to have_received(:new).once.with(user.owner) end end end diff --git a/spec/models/plan_selector_spec.rb b/spec/models/plan_selector_spec.rb index ec8d768fe9..e27e786e1e 100644 --- a/spec/models/plan_selector_spec.rb +++ b/spec/models/plan_selector_spec.rb @@ -9,33 +9,21 @@ describe "#current_plan" do it "returns owner's current plan" do plan = StripePlan::PLANS[1] - payment_gateway = instance_double( - "PaymentGatewaySubscription", - plan: plan[:id], - ) - owner = build_owner(payment_gateway_subscription: payment_gateway) - user = instance_double("User") - repo = instance_double("Repo", owner: owner) - plan_selector = PlanSelector.new(user: user, repo: repo) + owner = build_owner(stripe_plan_id: plan[:id]) + plan_selector = PlanSelector.new(owner) actual = plan_selector.current_plan - expect(actual).to eq StripePlan.new(plan) + expect(actual).to eq StripePlan.new(**plan) end end describe "#upgrade?" do context "when owner is subscribed via Stripe" do - context "when the owner isn't subsribed to any plan" do + context "when the owner isn't subscribed to any plan" do it "returns true" do - payment_gateway = instance_double( - "PaymentGatewaySubscription", - plan: nil - ) - owner = build_owner(payment_gateway_subscription: payment_gateway) - user = instance_double("User") - repo = instance_double("Repo", owner: owner) - plan_selector = PlanSelector.new(user: user, repo: repo) + owner = build_owner(stripe_plan_id: nil) + plan_selector = PlanSelector.new(owner) actual = plan_selector.upgrade? @@ -46,9 +34,7 @@ context "when recent builds is greater or equal than plan allowance" do it "returns true" do owner = build_owner(recent_builds: 50) - repo = instance_double("Repo", owner: owner) - user = instance_double("User") - plan_selector = PlanSelector.new(user: user, repo: repo) + plan_selector = PlanSelector.new(owner) actual = plan_selector.upgrade? @@ -59,9 +45,7 @@ context "recent builds is less than plan allowance" do it "returns false" do owner = build_owner(recent_builds: 49) - repo = instance_double("Repo", owner: owner) - user = instance_double("User") - plan_selector = PlanSelector.new(user: user, repo: repo) + plan_selector = PlanSelector.new(owner) actual = plan_selector.upgrade? @@ -87,9 +71,7 @@ marketplace_plan_id: current_plan[:id], active_private_repos_count: test_data[:repos], ) - repo = instance_double("Repo", owner: owner) - user = instance_double("User") - plan_selector = described_class.new(user: user, repo: repo) + plan_selector = PlanSelector.new(owner) actual = plan_selector.upgrade? @@ -103,10 +85,8 @@ describe "#next_plan" do context "when the owner has no subscribed repos" do it "returns the first paid plan" do - owner = instance_double("Owner") - user = instance_double("User", first_active_private_repo: nil) - repo = instance_double("Repo", owner: owner) - plan_selector = PlanSelector.new(user: user) + owner = build_owner(stripe_plan_id: nil) + plan_selector = PlanSelector.new(owner) actual = plan_selector.next_plan @@ -117,9 +97,7 @@ context "when the user has maxed out recent builds" do it "returns the next paid plan" do owner = build_owner(recent_builds: 55) - repo = instance_double("Repo", owner: owner) - user = instance_double("User", first_active_private_repo: repo) - plan_selector = PlanSelector.new(user: user) + plan_selector = PlanSelector.new(owner) actual = plan_selector.next_plan @@ -129,15 +107,10 @@ end def build_owner(options = {}) - plan = StripePlan::PLANS[1] - payment_gateway_subscription = instance_double( - "PaymentGatewaySubscription", - plan: plan[:id], - ) default_options = { + stripe_plan_id: StripePlan::PLANS[1][:id], recent_builds: 0, marketplace_plan_id: nil, - payment_gateway_subscription: payment_gateway_subscription, } instance_double("Owner", default_options.merge(options)) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 950e21e781..59af8bca33 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -8,7 +8,8 @@ describe "#current_plan" do it "returns the current plan" do - user = User.new + user = create(:user) + create(:owner, name: user.username) expect(user.current_plan).to eq StripePlan.new(**StripePlan::PLANS[0]) end @@ -16,7 +17,8 @@ describe "#next_plan" do it "returns the next plan" do - user = User.new + user = create(:user) + create(:owner, name: user.username) expect(user.next_plan).to eq StripePlan.new(**StripePlan::PLANS[1]) end @@ -24,20 +26,21 @@ describe "#plan_max" do it "returns the current plan's allowance" do - owner = Owner.new(marketplace_plan_id: nil) - repos = Array.new(5) { Repo.new(owner: owner) } - user = User.new(subscribed_repos: repos) + user = create(:user) + create(:owner, :stripe, name: user.username) + stub_customer_find_request_with_subscriptions - expect(user.plan_max).to eq 0 + expect(user.plan_max).to eq 50 end end describe "#next_plan_price" do it "returns the price of the next plan" do - subscription = create(:subscription) - user = subscription.user + user = create(:user) + create(:owner, :stripe, name: user.username) + stub_customer_find_request_with_subscriptions - expect(user.next_plan_price).to eq 29 + expect(user.next_plan_price).to eq 49 end end diff --git a/spec/presenters/plan_presenter_spec.rb b/spec/presenters/plan_presenter_spec.rb index af82d74ca9..77cd69b799 100644 --- a/spec/presenters/plan_presenter_spec.rb +++ b/spec/presenters/plan_presenter_spec.rb @@ -14,10 +14,9 @@ describe "#current?" do context "when the plan matches the user's current plan" do it "returns true" do - membership = create(:membership) - user = membership.user - create(:subscription, repo: membership.repo, user: user) - plan = StripePlan::PLANS.first + user = create(:user) + owner = create(:owner, name: user.username) + plan = StripePlan.new(**StripePlan::PLANS.first) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to be_current @@ -26,10 +25,9 @@ context "when the plan does not match the user's current plan" do it "returns false" do - membership = create(:membership) - user = membership.user - create(:subscription, repo: membership.repo, user: user) - plan = StripePlan::PLANS.second + user = create(:user) + owner = create(:owner, name: user.username) + plan = StripePlan.new(**StripePlan::PLANS.second) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to_not be_current @@ -40,10 +38,9 @@ describe "#next?" do context "when the plan matches the user's next plan" do it "returns true" do - membership = create(:membership) - user = membership.user - create(:subscription, repo: membership.repo, user: user) - plan = StripePlan::PLANS.second + user = create(:user) + owner = create(:owner, name: user.username) + plan = StripePlan.new(**StripePlan::PLANS.second) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to be_next @@ -54,8 +51,8 @@ it "returns false" do membership = create(:membership) user = membership.user - create(:subscription, repo: membership.repo, user: user) - plan = StripePlan::PLANS.first + create(:owner, name: user.username) + plan = StripePlan.new(**StripePlan::PLANS[2]) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to_not be_next @@ -65,8 +62,8 @@ describe "#open_source?" do it "returns the plan's open source state" do - plan = instance_double("StripePlan", open_source?: true) user = instance_double("User") + plan = instance_double("StripePlan", open_source?: true) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter).to be_open_source @@ -75,8 +72,8 @@ describe "#price" do it "returns the plan's price" do - user = create(:user) - plan = build_stubbed(:plan) + user = instance_double("User") + plan = instance_double("StripePlan", price: 123) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter.price).to eq plan.price @@ -86,8 +83,8 @@ describe "#to_partial_path" do context "when the plan is for open source repos" do it "returns 'plans/open_source'" do - plan = instance_double("StripePlan", open_source?: true) user = instance_double("User") + plan = instance_double("StripePlan", open_source?: true) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter.to_partial_path).to eq("plans/open_source") @@ -96,8 +93,8 @@ context "when the plan is for private repos" do it "returns 'plans/private'" do - plan = instance_double("StripePlan", open_source?: false) user = instance_double("User") + plan = instance_double("StripePlan", open_source?: false) presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter.to_partial_path).to eq("plans/private") @@ -107,8 +104,8 @@ describe "#title" do it "returns the plan's title" do - user = create(:user) - plan = build_stubbed(:plan) + user = instance_double("User") + plan = instance_double("StripePlan", title: "Foo Plan") presenter = PlanPresenter.new(plan: plan, user: user) expect(presenter.title).to eq plan.title diff --git a/spec/support/fixtures/stripe_customer_find_with_tier2_subscription.json b/spec/support/fixtures/stripe_customer_find_with_tier2_subscription.json deleted file mode 100644 index 9a64fa878a..0000000000 --- a/spec/support/fixtures/stripe_customer_find_with_tier2_subscription.json +++ /dev/null @@ -1,84 +0,0 @@ -{ - "object": "customer", - "created": 1380227987, - "id": "cus_2e3fqARc1uHtCv", - "livemode": false, - "description": null, - "email": "scott@thoughtbot.com", - "delinquent": false, - "metadata": { - }, - "subscriptions": { - "object": "list", - "total_count": 0, - "has_more": false, - "url": "/v1/customers/cus_2e3fqARc1uHtCv/subscriptions", - "data": [{ - "id": "sub_488ZZngNkyRMiR", - "plan": { - "interval": "month", - "name": "Personal", - "created": 1401465831, - "amount": 900, - "currency": "usd", - "id": "tier2", - "object": "plan", - "livemode": false, - "interval_count": 1, - "trial_period_days": null, - "metadata": { - }, - "statement_description": null - }, - "object": "subscription", - "start": 1407216749, - "status": "active", - "customer": "cus_2e3fqARc1uHtCv", - "cancel_at_period_end": false, - "current_period_start": 1407216749, - "current_period_end": 1409895149, - "ended_at": null, - "trial_start": null, - "trial_end": null, - "canceled_at": null, - "quantity": 1, - "application_fee_percent": null, - "discount": null, - "metadata": { - } - }] - }, - "discount": null, - "account_balance": 0, - "currency": "usd", - "cards": { - "object": "list", - "total_count": 1, - "has_more": false, - "url": "/v1/customers/cus_2e3fqARc1uHtCv/cards", - "data": [ - { - "id": "card_102e3c2QAwplLIsJn2oXQKvB", - "object": "card", - "last4": "4242", - "type": "Visa", - "exp_month": 12, - "exp_year": 2013, - "fingerprint": "EEsAljuM0b4ras7N", - "country": "US", - "name": null, - "address_line1": null, - "address_line2": null, - "address_city": null, - "address_state": null, - "address_zip": null, - "address_country": null, - "cvc_check": "pass", - "address_line1_check": null, - "address_zip_check": null, - "customer": "cus_2e3fqARc1uHtCv" - } - ] - }, - "default_card": "card_102e3c2QAwplLIsJn2oXQKvB" -} diff --git a/spec/support/fixtures/stripe_customer_find_with_tier3_subscription.json b/spec/support/fixtures/stripe_customer_find_with_tier3_subscription.json deleted file mode 100644 index 9e2e855756..0000000000 --- a/spec/support/fixtures/stripe_customer_find_with_tier3_subscription.json +++ /dev/null @@ -1,84 +0,0 @@ -{ - "object": "customer", - "created": 1380227987, - "id": "cus_2e3fqARc1uHtCv", - "livemode": false, - "description": null, - "email": "scott@thoughtbot.com", - "delinquent": false, - "metadata": { - }, - "subscriptions": { - "object": "list", - "total_count": 0, - "has_more": false, - "url": "/v1/customers/cus_2e3fqARc1uHtCv/subscriptions", - "data": [{ - "id": "sub_488ZZngNkyRMiR", - "plan": { - "interval": "month", - "name": "Personal", - "created": 1401465831, - "amount": 900, - "currency": "usd", - "id": "tier3", - "object": "plan", - "livemode": false, - "interval_count": 1, - "trial_period_days": null, - "metadata": { - }, - "statement_description": null - }, - "object": "subscription", - "start": 1407216749, - "status": "active", - "customer": "cus_2e3fqARc1uHtCv", - "cancel_at_period_end": false, - "current_period_start": 1407216749, - "current_period_end": 1409895149, - "ended_at": null, - "trial_start": null, - "trial_end": null, - "canceled_at": null, - "quantity": 1, - "application_fee_percent": null, - "discount": null, - "metadata": { - } - }] - }, - "discount": null, - "account_balance": 0, - "currency": "usd", - "cards": { - "object": "list", - "total_count": 1, - "has_more": false, - "url": "/v1/customers/cus_2e3fqARc1uHtCv/cards", - "data": [ - { - "id": "card_102e3c2QAwplLIsJn2oXQKvB", - "object": "card", - "last4": "4242", - "type": "Visa", - "exp_month": 12, - "exp_year": 2013, - "fingerprint": "EEsAljuM0b4ras7N", - "country": "US", - "name": null, - "address_line1": null, - "address_line2": null, - "address_city": null, - "address_state": null, - "address_zip": null, - "address_country": null, - "cvc_check": "pass", - "address_line1_check": null, - "address_zip_check": null, - "customer": "cus_2e3fqARc1uHtCv" - } - ] - }, - "default_card": "card_102e3c2QAwplLIsJn2oXQKvB" -} From d38afd27d94f168b3affe98dd76669975c9a60a2 Mon Sep 17 00:00:00 2001 From: Greg Date: Wed, 30 Jun 2021 16:01:27 -0700 Subject: [PATCH 3/7] Fix more tests --- spec/controllers/activations_controller_spec.rb | 4 ++-- spec/controllers/users_controller_spec.rb | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/controllers/activations_controller_spec.rb b/spec/controllers/activations_controller_spec.rb index 6773f13cb0..0efb89696f 100644 --- a/spec/controllers/activations_controller_spec.rb +++ b/spec/controllers/activations_controller_spec.rb @@ -29,6 +29,7 @@ "config_repo" => nil, "whitelisted" => false, "marketplace_plan_id" => nil, + "stripe_customer_id" => nil, "stripe_subscription_id" => nil, }, ) @@ -38,8 +39,7 @@ context "when repo is not public" do it "does not activate" do repo = create(:repo, private: true) - user = create(:user) - user.repos << repo + user = create(:user, repos: [repo]) stub_sign_in(user) expect { post :create, params: { repo_id: repo.id }, format: :json }. diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index a54f70e185..870d1e7c05 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -4,6 +4,7 @@ describe "#show" do it "returns current user in json" do user = create(:user) + create(:owner, name: user.username) stub_sign_in(user) get :show, format: :json From 09675b9a2f3d18206d11832c775039a878a3d060 Mon Sep 17 00:00:00 2001 From: Greg Date: Fri, 2 Jul 2021 16:41:56 -0700 Subject: [PATCH 4/7] More test fixes --- app/presenters/account_page.rb | 5 +---- spec/presenters/account_page_spec.rb | 14 ++++++-------- spec/serializers/user_serializer_spec.rb | 8 ++++++-- 3 files changed, 13 insertions(+), 14 deletions(-) diff --git a/app/presenters/account_page.rb b/app/presenters/account_page.rb index 2407757d8a..8ae2dd9001 100644 --- a/app/presenters/account_page.rb +++ b/app/presenters/account_page.rb @@ -20,10 +20,7 @@ def plan end def plans - plan_selector = PlanSelector.new( - user: user, - repo: user.first_available_repo, - ) + plan_selector = PlanSelector.new(user.owner) plan_selector.plans.map do |plan| PlanPresenter.new(plan: plan, user: user) diff --git a/spec/presenters/account_page_spec.rb b/spec/presenters/account_page_spec.rb index 342c9438c9..9ab505c4f0 100644 --- a/spec/presenters/account_page_spec.rb +++ b/spec/presenters/account_page_spec.rb @@ -47,16 +47,14 @@ it "returns all of the presentable, available plans" do presenter = instance_double("PlanPresenter") plan = instance_double("StripePlan") - user = instance_double("User") + owner = instance_double("Owner") + user = instance_double("User", owner: owner) page = AccountPage.new(user) plan_selector = instance_double("PlanSelector", plans: [plan]) - allow(PlanSelector).to( - receive(:new).once.with(user: user).and_return(plan_selector) - ) - allow(PlanPresenter).to receive(:new).once.with( - plan: plan, - user: user, - ).and_return(presenter) + allow(PlanSelector).to receive(:new).once.with(owner). + and_return(plan_selector) + allow(PlanPresenter).to receive(:new).once.with(plan: plan, user: user). + and_return(presenter) expect(page.plans).to eq [presenter] end diff --git a/spec/serializers/user_serializer_spec.rb b/spec/serializers/user_serializer_spec.rb index 9b3c7d44c9..8ca04a92fc 100644 --- a/spec/serializers/user_serializer_spec.rb +++ b/spec/serializers/user_serializer_spec.rb @@ -5,6 +5,7 @@ it "is the number of repos the user is subscribed to" do subscribed_repos = class_double(Repo, count: 1) user = instance_double(User, subscribed_repos: subscribed_repos) + serializer = UserSerializer.new(user) expect(serializer.subscribed_repo_count).to eq 1 @@ -12,11 +13,14 @@ end describe "#plan_max" do - it "returns the number of subscriptions allowed within the current plan" do + it "returns the number of builds allowed within the current plan" do user = create(:user) + create(:owner, :stripe, name: user.username) + stub_customer_find_request_with_subscriptions + serializer = UserSerializer.new(user) - expect(serializer.object.plan_max).to eq 0 + expect(serializer.object.plan_max).to eq 50 end end end From 7404727a0be888818c61cccb0708b91476076c85 Mon Sep 17 00:00:00 2001 From: Greg Date: Sun, 11 Jul 2021 17:49:08 -0700 Subject: [PATCH 5/7] Fix remaining tests --- app/controllers/plans_controller.rb | 2 +- app/controllers/subscriptions_controller.rb | 1 - app/models/plan_selector.rb | 14 +-- app/models/user.rb | 2 +- app/services/repo_subscriber.rb | 8 +- spec/factories.rb | 18 ++-- spec/features/account_spec.rb | 98 +++++++++---------- spec/features/plans_spec.rb | 2 +- spec/features/repo_list_spec.rb | 2 +- spec/features/user_activates_a_repo_spec.rb | 2 +- spec/features/user_deactivates_a_repo_spec.rb | 4 +- spec/models/payment_gateway_customer_spec.rb | 25 ++--- spec/services/repo_subscriber_spec.rb | 33 +++---- spec/support/fake_github.rb | 4 +- spec/support/helpers/stripe_api_helper.rb | 40 +++----- 15 files changed, 114 insertions(+), 141 deletions(-) diff --git a/app/controllers/plans_controller.rb b/app/controllers/plans_controller.rb index e4065dc934..a55ac2ffba 100644 --- a/app/controllers/plans_controller.rb +++ b/app/controllers/plans_controller.rb @@ -18,7 +18,7 @@ def plan_params end def plan_selector - @_plan_selector ||= PlanSelector.new(user: current_user, repo: repo) + @_plan_selector ||= PlanSelector.new(repo.owner) end def marketplace_upgrade_url diff --git a/app/controllers/subscriptions_controller.rb b/app/controllers/subscriptions_controller.rb index 154f961eeb..de161ff5eb 100644 --- a/app/controllers/subscriptions_controller.rb +++ b/app/controllers/subscriptions_controller.rb @@ -19,7 +19,6 @@ def create end def update - # candidate for removal? activate_and_create_subscription end diff --git a/app/models/plan_selector.rb b/app/models/plan_selector.rb index 9dbbdd3867..3df458dfc6 100644 --- a/app/models/plan_selector.rb +++ b/app/models/plan_selector.rb @@ -40,6 +40,10 @@ def plans @_plans ||= plan_class::PLANS.map { |plan| plan_class.new(**plan) } end + def marketplace_plan? + marketplace_plan_id.present? + end + private attr_reader :owner @@ -52,15 +56,11 @@ def plan_class marketplace_plan? ? GitHubPlan : StripePlan end - def marketplace_plan? - marketplace_plan_id.present? - end - def marketplace_plan_id owner.marketplace_plan_id end - # def marketplace_upgrade_url - # "#{MARKETPLACE_URL}/order/#{next_plan.slug}?account=#{owner.name}" - # end + def marketplace_upgrade_url + "#{MARKETPLACE_URL}/order/#{next_plan.slug}?account=#{owner.name}" + end end diff --git a/app/models/user.rb b/app/models/user.rb index cefdd7f3d3..bdece3da69 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -92,7 +92,7 @@ def owner @_owner ||= begin user_as_owner = Owner.find_by(name: username) - if user_as_owner.stripe_plan_id.present? + if user_as_owner&.stripe_plan_id.present? user_as_owner else first_available_repo&.owner || user_as_owner diff --git a/app/services/repo_subscriber.rb b/app/services/repo_subscriber.rb index 38af3773b1..1a9702a2f9 100644 --- a/app/services/repo_subscriber.rb +++ b/app/services/repo_subscriber.rb @@ -28,13 +28,7 @@ def unsubscribe private def create_subscription - plan_selector = PlanSelector.new(user: stripe_user || user, repo: repo) - - plan = if plan_selector.current_plan.open_source? - plan_selector.next_plan - else - plan_selector.current_plan - end + plan = user.current_plan.open_source? ? user.next_plan : user.current_plan payment_gateway_subscription = stripe_customer.find_or_create_subscription( plan: plan.id, diff --git a/spec/factories.rb b/spec/factories.rb index f9f5ea57c5..d6b8e218ba 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -40,6 +40,13 @@ initialize_with { new(id: id, price: price, range: range, title: title) } end + factory :owner do + trait(:stripe) { stripe_customer_id { "cus_2e3fqARc1uHtCv" } } + + github_id + name { generate(:github_name) } + end + factory :repo do trait(:active) { active { true } } trait(:inactive) { active { false } } @@ -57,9 +64,9 @@ end factory :user do - username { generate(:github_name) } - trait(:stripe) { stripe_customer_id { "cus_2e3fqARc1uHtCv" } } + + username { generate(:github_name) } end factory :membership do @@ -86,11 +93,4 @@ line_number { 42 } messages { ["Trailing whitespace detected."] } end - - factory :owner do - trait(:stripe) { stripe_customer_id { "cus_2e3fqARc1uHtCv" } } - - github_id - name { generate(:github_name) } - end end diff --git a/spec/features/account_spec.rb b/spec/features/account_spec.rb index ab315639bf..1458f2f866 100644 --- a/spec/features/account_spec.rb +++ b/spec/features/account_spec.rb @@ -1,8 +1,9 @@ require "rails_helper" feature "Account" do - scenario "user without Stripe Customer ID" do - user = create(:user, stripe_customer_id: nil) + scenario "user without any repos loaded" do + user = create(:user) + create(:owner, name: user.username) sign_in_as(user) visit account_path @@ -10,20 +11,11 @@ expect(page).not_to have_text("Update Credit Card") end - scenario "user with Stripe Customer ID", js: true do - user = create(:user, stripe_customer_id: "123") - stub_customer_find_request(user.stripe_customer_id) - - sign_in_as(user) - visit account_path - - expect(page).to have_text("Update Credit Card") - end - scenario "returns a list of all plans", :js do user = create(:user) - sign_in_as(user, "letmein") + create(:owner, name: user.username) + sign_in_as(user, "letmein") visit account_path plans = page.all(".plan") @@ -68,14 +60,29 @@ end end - scenario "user with a subscription views account page" do - user = create(:user, stripe_customer_id: stripe_customer_id) - create(:subscription, user: user) + scenario "org admin updates billing email", :js do + user_on_page = UserOnPage.new + email_address = "somebody.else@example.com" + user = create(:user, :stripe) + owner = create(:owner, :stripe, name: user.username) + stub_customer_find_request(owner.stripe_customer_id) + stub_customer_update_request(email: email_address) + + sign_in_as(user) + visit account_path + user_on_page.update(email_address) - responses = [individual_subscription_response] + expect(user_on_page).to be_updated + end + + scenario "org admin with Stripe subscription views account page", :js do + user = create(:user, :stripe) # remove stripe dependency on user + owner = create(:owner, :stripe) + repo = create(:repo, owner: owner) + create(:subscription, user: user, repo: repo) stub_customer_find_request_with_subscriptions( - stripe_customer_id, - generate_subscriptions_response(responses), + owner.stripe_customer_id, + generate_subscriptions_response(individual_subscription_response), ) sign_in_as(user) @@ -85,16 +92,17 @@ expect(page).to have_text("Great Dane") expect(page).to have_text("$49") end + expect(page).to have_text("Update Credit Card") end - scenario "user with discounted-amount subscription views account page" do - user = create(:user, stripe_customer_id: stripe_customer_id) - create(:subscription, user: user) - - responses = [discounted_amount_subscription_response] + scenario "org admin with discounted subscription views account page" do + user = create(:user, :stripe) # remove stripe dependency on user + owner = create(:owner, :stripe) + repo = create(:repo, owner: owner) + create(:subscription, user: user, repo: repo) stub_customer_find_request_with_subscriptions( - stripe_customer_id, - generate_subscriptions_response(responses), + owner.stripe_customer_id, + generate_subscriptions_response(discounted_amount_subscription_response), ) sign_in_as(user) @@ -107,13 +115,13 @@ end scenario "user with discounted-percentage subscription views account page" do - user = create(:user, stripe_customer_id: stripe_customer_id) - create(:subscription, user: user) - - responses = [discounted_percent_subscription_response] + user = create(:user, :stripe) # remove stripe dependency on user + owner = create(:owner, :stripe) + repo = create(:repo, owner: owner) + create(:subscription, user: user, repo: repo) stub_customer_find_request_with_subscriptions( - stripe_customer_id, - generate_subscriptions_response(responses), + owner.stripe_customer_id, + generate_subscriptions_response(discounted_percent_subscription_response), ) sign_in_as(user) @@ -125,38 +133,22 @@ end end - scenario "user updates their email address", :js do - email_address = "somebody.else@example.com" - stub_customer_find_request - stub_customer_update_request(email: email_address) - - sign_in_as(create(:user, :stripe)) - visit account_path - user = user_on_page - user.update(email_address) - - expect(user).to be_updated - end - private def stub_customer_find_request_with_subscriptions(customer_id, subscriptions) - stub_request(:get, "#{stripe_base_url}/customers/#{customer_id}"). + url = "#{StripeApiHelper::STRIPE_BASE_URL}/customers/#{customer_id}" + stub_request(:get, url). with(headers: { "Authorization" => "Bearer #{ENV['STRIPE_API_KEY']}" }). to_return(status: 200, body: merge_customer_subscriptions(subscriptions)) end - def user_on_page - UserOnPage.new - end - - def generate_subscriptions_response(subscriptions) + def generate_subscriptions_response(subscription) { "object" => "list", - "total_count" => subscriptions.length, + "total_count" => 1, "has_more" => false, "url" => "/v1/customers/cus_2e3fqARc1uHtCv/subscriptions", - "data" => subscriptions, + "data" => [subscription], } end diff --git a/spec/features/plans_spec.rb b/spec/features/plans_spec.rb index d83226e4e0..99a3928cd0 100644 --- a/spec/features/plans_spec.rb +++ b/spec/features/plans_spec.rb @@ -55,7 +55,7 @@ repo = create(:repo, :private) create(:membership, :admin, repo: repo, user: user) stub_repository_invitations(repo.name) - stub_customer_find_request + stub_customer_find_request(user.stripe_customer_id) stub_subscription_create_request( plan: StripePlan::PLANS.second.fetch(:id), repo_ids: repo.id, diff --git a/spec/features/repo_list_spec.rb b/spec/features/repo_list_spec.rb index 0aaa470a91..76a0263db7 100644 --- a/spec/features/repo_list_spec.rb +++ b/spec/features/repo_list_spec.rb @@ -68,7 +68,7 @@ click_button I18n.t("sync_repos") - expect(page).to have_text("TEST_GITHUB_LOGIN/TEST_GITHUB_REPO_NAME") + expect(page).to have_text("test_login/test_repo") expect(page).not_to have_text(repo.name) end diff --git a/spec/features/user_activates_a_repo_spec.rb b/spec/features/user_activates_a_repo_spec.rb index 1a4167684e..bb0a2ad7dc 100644 --- a/spec/features/user_activates_a_repo_spec.rb +++ b/spec/features/user_activates_a_repo_spec.rb @@ -5,7 +5,7 @@ user = create(:user, :stripe) repo = create(:repo, :private) create(:membership, :admin, repo: repo, user: user) - stub_customer_find_request + stub_customer_find_request(user.stripe_customer_id) current_plan = user.current_plan.id stub_subscription_create_request(plan: current_plan, repo_ids: repo.id) diff --git a/spec/features/user_deactivates_a_repo_spec.rb b/spec/features/user_deactivates_a_repo_spec.rb index 8e16d83a26..426cc9eb4e 100644 --- a/spec/features/user_deactivates_a_repo_spec.rb +++ b/spec/features/user_deactivates_a_repo_spec.rb @@ -2,10 +2,11 @@ feature "user deactivates a repo", js: true do context "when the user has a subscription" do - context "when the user does not have a membership" do + context "and the user does not have a membership to that repo" do scenario "successfully deactivates the repo" do token = "letmein" user = create(:user) + owner = create(:owner, name: user.username) repo = create(:repo, :active, private: true) create(:subscription, user: user, repo: repo) gateway_subscription = instance_double( @@ -32,6 +33,7 @@ expect(page).not_to have_css(".repos-syncing") expect(page).not_to have_text(repo.name) + expect(page).not_to have_css(".active") end end end diff --git a/spec/models/payment_gateway_customer_spec.rb b/spec/models/payment_gateway_customer_spec.rb index 66656750ca..0ac64861f4 100644 --- a/spec/models/payment_gateway_customer_spec.rb +++ b/spec/models/payment_gateway_customer_spec.rb @@ -4,9 +4,9 @@ describe "#update_card" do it "updates card" do new_card_token = "newcardtoken" - user = build(:user, stripe_customer_id: stripe_customer_id) + user = build_stubbed(:user, :stripe) customer = PaymentGatewayCustomer.new(user) - stub_customer_find_request + stub_customer_find_request(user.stripe_customer_id) update_request = stub_customer_update_request(card: new_card_token) customer.update_card(new_card_token) @@ -18,9 +18,9 @@ describe "#update_email" do it "updates customer email" do new_email = "new-email@example.com" - user = build(:user, stripe_customer_id: stripe_customer_id) + user = build(:user, :stripe) customer = PaymentGatewayCustomer.new(user) - stub_customer_find_request + stub_customer_find_request(user.stripe_customer_id) update_request = stub_customer_update_request(email: new_email) customer.update_email(new_email) @@ -31,9 +31,9 @@ context "when update request fails" do it "returns false" do new_email = "new-email@example.com" - user = build(:user, stripe_customer_id: stripe_customer_id) + user = build_stubbed(:user, :stripe) customer = PaymentGatewayCustomer.new(user) - stub_customer_find_request + stub_customer_find_request(user.stripe_customer_id) stub_failed_customer_update_request(email: new_email) customer = customer.update_email(new_email) @@ -45,8 +45,8 @@ describe "#email" do it "returns customer email" do - user = build_stubbed(:user, stripe_customer_id: stripe_customer_id) - stub_customer_find_request + user = build_stubbed(:user, :stripe) + stub_customer_find_request(user.stripe_customer_id) payment_gateway_customer = PaymentGatewayCustomer.new(user) customer = payment_gateway_customer.customer @@ -57,11 +57,12 @@ describe "#customer" do context "when stripe_customer_id is present" do it "retrieve customer data" do - user = build_stubbed(:user, stripe_customer_id: stripe_customer_id) - stub_customer_find_request + user = build_stubbed(:user, :stripe) + stub_customer_find_request(user.stripe_customer_id) payment_gateway_customer = PaymentGatewayCustomer.new(user) - expect(payment_gateway_customer.customer.id).to eq stripe_customer_id + expect(payment_gateway_customer.customer.id). + to eq user.stripe_customer_id end end @@ -103,7 +104,7 @@ describe "#subscription" do context "when stripe_customer_id is present" do it "retrieves subscription data" do - user = build_stubbed(:user, stripe_customer_id: stripe_customer_id) + user = build_stubbed(:user, :stripe) stub_customer_find_request_with_subscriptions payment_gateway_customer = PaymentGatewayCustomer.new(user) diff --git a/spec/services/repo_subscriber_spec.rb b/spec/services/repo_subscriber_spec.rb index 8e919a3890..878f7dd635 100644 --- a/spec/services/repo_subscriber_spec.rb +++ b/spec/services/repo_subscriber_spec.rb @@ -4,14 +4,10 @@ describe ".subscribe" do context "when Stripe customer exists" do context "when a subscription doesn't exist" do - it "creates a new Stripe and repo subscriptions" do + it "creates a new Stripe subscription" do repo = create(:repo, private: true) - user = create( - :user, - stripe_customer_id: stripe_customer_id, - repos: [repo], - ) - stub_customer_find_request + user = create(:user, :stripe, repos: [repo]) + stub_customer_find_request(user.stripe_customer_id) update_request = stub_customer_update_request subscription_request = stub_subscription_create_request( plan: StripePlan::PLANS[1][:id], @@ -27,17 +23,13 @@ expect(subscription_update_request).to have_been_requested expect(update_request).not_to have_been_requested expect(repo.subscription.stripe_subscription_id). - to eq(stripe_subscription_id) + to eq(StripeApiHelper::STRIPE_SUBSCRIPTION_ID) end it "creates a Stripe subscription using new card" do repo = create(:repo, private: true) - user = create( - :user, - stripe_customer_id: stripe_customer_id, - repos: [repo], - ) - stub_customer_find_request + user = create(:user, :stripe, repos: [repo]) + stub_customer_find_request(user.stripe_customer_id) subscription_request = stub_subscription_create_request( plan: StripePlan::PLANS[1][:id], repo_ids: repo.id, @@ -55,7 +47,7 @@ expect(subscription_update_request).to have_been_requested expect(customer_update_request).to have_been_requested expect(repo.subscription.stripe_subscription_id). - to eq(stripe_subscription_id) + to eq(StripeApiHelper::STRIPE_SUBSCRIPTION_ID) end end end @@ -77,8 +69,9 @@ expect(subscription_request).to have_been_requested expect(update_request).to have_been_requested expect(repo.subscription.stripe_subscription_id). - to eq(stripe_subscription_id) - expect(user.stripe_customer_id).to eq stripe_customer_id + to eq(StripeApiHelper::STRIPE_SUBSCRIPTION_ID) + expect(user.stripe_customer_id). + to eq StripeApiHelper::STRIPE_CUSTOMER_ID end end @@ -171,7 +164,7 @@ describe ".unsubscribe" do it "downgrades the Stripe plan" do subscription = subscription_with_user - stub_customer_find_request + stub_customer_find_request(subscription.user.stripe_customer_id) stub_subscription_find_request(subscription, quantity: 2) stripe_delete_request = stub_subscription_delete_request subscription_update_request = stub_subscription_update_request( @@ -211,11 +204,11 @@ end def subscription_with_user - user = create(:user, stripe_customer_id: stripe_customer_id) + user = create(:user, :stripe) repo = create(:repo, :private, users: [user]) create( :subscription, - stripe_subscription_id: stripe_subscription_id, + stripe_subscription_id: StripeApiHelper::STRIPE_SUBSCRIPTION_ID, user: user, repo: repo, ) diff --git a/spec/support/fake_github.rb b/spec/support/fake_github.rb index 720663eb45..19d9a01a41 100644 --- a/spec/support/fake_github.rb +++ b/spec/support/fake_github.rb @@ -31,9 +31,9 @@ class FakeGitHub < Sinatra::Base { repositories: [ { - full_name: "TEST_GITHUB_LOGIN/TEST_GITHUB_REPO_NAME", + full_name: "test_login/test_repo", id: 1, - owner: { id: 1, login: "TEST_GITHUB_LOGIN" }, + owner: { id: 1, login: "test_login" }, permissions: { admin: true }, private: false, }, diff --git a/spec/support/helpers/stripe_api_helper.rb b/spec/support/helpers/stripe_api_helper.rb index 1fbaae54ef..be9d31c975 100644 --- a/spec/support/helpers/stripe_api_helper.rb +++ b/spec/support/helpers/stripe_api_helper.rb @@ -1,14 +1,10 @@ module StripeApiHelper - def stripe_customer_id - "cus_2e3fqARc1uHtCv" - end - - def stripe_subscription_id - "sub_488ZZngNkyRMiR" - end + STRIPE_BASE_URL = "https://api.stripe.com/v1".freeze + STRIPE_CUSTOMER_ID = "cus_2e3fqARc1uHtCv" + STRIPE_SUBSCRIPTION_ID = "sub_488ZZngNkyRMiR" def stub_customer_create_request(user) - stub_request(:post, "#{stripe_base_url}/customers").with( + stub_request(:post, "#{STRIPE_BASE_URL}/customers").with( body: { "card" => "cardtoken", "metadata" => { "user_id" => "#{user.id}" } @@ -20,8 +16,8 @@ def stub_customer_create_request(user) ) end - def stub_customer_find_request(customer_id = stripe_customer_id) - stub_request(:get, "#{stripe_base_url}/customers/#{customer_id}").with( + def stub_customer_find_request(customer_id) + stub_request(:get, "#{STRIPE_BASE_URL}/customers/#{customer_id}").with( headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } ).to_return( status: 200, @@ -32,7 +28,7 @@ def stub_customer_find_request(customer_id = stripe_customer_id) def stub_customer_find_request_with_subscriptions stub_request( :get, - "#{stripe_base_url}/customers/#{stripe_customer_id}", + "#{STRIPE_BASE_URL}/customers/#{STRIPE_CUSTOMER_ID}", ).with( headers: { "Authorization" => "Bearer #{ENV['STRIPE_API_KEY']}" } ).to_return( @@ -46,7 +42,7 @@ def stub_customer_find_request_with_subscriptions def stub_customer_update_request(attrs = { card: "card-token" }) stub_request( :post, - "#{stripe_base_url}/customers/#{stripe_customer_id}", + "#{STRIPE_BASE_URL}/customers/#{STRIPE_CUSTOMER_ID}", ).with( body: attrs, headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } @@ -59,7 +55,7 @@ def stub_customer_update_request(attrs = { card: "card-token" }) def stub_failed_customer_update_request(attrs = { email: "email@foo.com" }) stub_request( :post, - "#{stripe_base_url}/customers/#{stripe_customer_id}", + "#{STRIPE_BASE_URL}/customers/#{STRIPE_CUSTOMER_ID}", ).with( body: attrs, headers: { "Authorization" => "Bearer #{ENV['STRIPE_API_KEY']}" } @@ -81,7 +77,7 @@ def stub_subscription_create_request(plan: "free", repo_ids: "") } stub_request( :post, - "#{stripe_base_url}/customers/#{stripe_customer_id}/subscriptions", + "#{STRIPE_BASE_URL}/customers/#{STRIPE_CUSTOMER_ID}/subscriptions", ).with( body: hash_including(body), headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } @@ -96,7 +92,7 @@ def stub_subscription_update_request(repo_ids:) stub_request( :post, - "#{stripe_base_url}/subscriptions/#{stripe_subscription_id}", + "#{STRIPE_BASE_URL}/subscriptions/#{STRIPE_SUBSCRIPTION_ID}", ).with( body: body, headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } @@ -111,7 +107,7 @@ def stub_subscription_find_request(subscription, quantity: 1) File.read("spec/support/fixtures/stripe_subscription_find.json") ) body["quantity"] = quantity - request_url = "#{stripe_base_url}/customers/#{stripe_customer_id}/"\ + request_url = "#{STRIPE_BASE_URL}/customers/#{STRIPE_CUSTOMER_ID}/"\ "subscriptions/#{subscription.stripe_subscription_id}" stub_request(:get, request_url).with( @@ -122,7 +118,7 @@ def stub_subscription_find_request(subscription, quantity: 1) def stub_subscription_delete_request stub_request( :delete, - "#{stripe_base_url}/subscriptions/#{stripe_subscription_id}", + "#{STRIPE_BASE_URL}/subscriptions/#{STRIPE_SUBSCRIPTION_ID}", ).with( headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } ).to_return( @@ -134,7 +130,7 @@ def stub_subscription_delete_request def stub_subscription_meta_data_update_request(subscription) stub_request( :post, - "#{stripe_base_url}/subscriptions/#{stripe_subscription_id}", + "#{STRIPE_BASE_URL}/subscriptions/#{STRIPE_SUBSCRIPTION_ID}", ).with( body: { metadata: { repo_ids: subscription.repo_id.to_s } }, headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } @@ -147,7 +143,7 @@ def stub_subscription_meta_data_update_request(subscription) def stub_failed_subscription_create_request(plan_type) stub_request( :post, - "#{stripe_base_url}/customers/#{stripe_customer_id}/subscriptions", + "#{STRIPE_BASE_URL}/customers/#{STRIPE_CUSTOMER_ID}/subscriptions", ).with( body: hash_including("plan" => plan_type), headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } @@ -167,7 +163,7 @@ def stub_failed_subscription_create_request(plan_type) def stub_failed_subscription_destroy_request stub_request( :destroy, - "#{stripe_base_url}/customers/#{stripe_customer_id}/subscriptions", + "#{STRIPE_BASE_URL}/customers/#{STRIPE_CUSTOMER_ID}/subscriptions", ).with( headers: { "Authorization" => "Bearer #{ENV["STRIPE_API_KEY"]}" } ).to_return( @@ -180,8 +176,4 @@ def stub_failed_subscription_destroy_request }.to_json ) end - - def stripe_base_url - "https://api.stripe.com/v1" - end end From 4cbfcfa5c36fd21edd8cae4c337acc160380dd9f Mon Sep 17 00:00:00 2001 From: Greg Date: Sun, 11 Jul 2021 17:53:48 -0700 Subject: [PATCH 6/7] Fix lints --- app/models/plan_selector.rb | 2 +- lib/tasks/plan.rake | 2 +- spec/features/account_spec.rb | 2 +- spec/features/user_deactivates_a_repo_spec.rb | 2 +- spec/presenters/plan_presenter_spec.rb | 6 +++--- spec/support/helpers/stripe_api_helper.rb | 4 ++-- 6 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/plan_selector.rb b/app/models/plan_selector.rb index 3df458dfc6..24a21c5bb4 100644 --- a/app/models/plan_selector.rb +++ b/app/models/plan_selector.rb @@ -2,7 +2,7 @@ class PlanSelector BULK_ID = "bulk".freeze MARKETPLACE_URL = ENV.fetch( "MARKETPLACE_URL", - "https://www.github.com/marketplace/hound" + "https://www.github.com/marketplace/hound", ) def initialize(owner) diff --git a/lib/tasks/plan.rake b/lib/tasks/plan.rake index 4a2dbdad4d..dcf94f20b6 100644 --- a/lib/tasks/plan.rake +++ b/lib/tasks/plan.rake @@ -8,7 +8,7 @@ namespace :plan do task migrate_owners: :environment do owners = Repo.joins(:subscription).map(&:owner).uniq owners.each do |owner| - stripe_user = owner.repos.active.find(&:subscription).subscription.user + stripe_user = owner.repos.active.detect(&:subscription).subscription.user puts "Onwer #{owner.name} mapped to user #{stripe_user.username}" owner.update!(stripe_customer_id: stripe_user.stripe_customer_id) diff --git a/spec/features/account_spec.rb b/spec/features/account_spec.rb index 1458f2f866..2883e5c41b 100644 --- a/spec/features/account_spec.rb +++ b/spec/features/account_spec.rb @@ -136,7 +136,7 @@ private def stub_customer_find_request_with_subscriptions(customer_id, subscriptions) - url = "#{StripeApiHelper::STRIPE_BASE_URL}/customers/#{customer_id}" + url = "#{StripeApiHelper::STRIPE_BASE_URL}/customers/#{customer_id}" stub_request(:get, url). with(headers: { "Authorization" => "Bearer #{ENV['STRIPE_API_KEY']}" }). to_return(status: 200, body: merge_customer_subscriptions(subscriptions)) diff --git a/spec/features/user_deactivates_a_repo_spec.rb b/spec/features/user_deactivates_a_repo_spec.rb index 426cc9eb4e..688bf2680c 100644 --- a/spec/features/user_deactivates_a_repo_spec.rb +++ b/spec/features/user_deactivates_a_repo_spec.rb @@ -6,9 +6,9 @@ scenario "successfully deactivates the repo" do token = "letmein" user = create(:user) - owner = create(:owner, name: user.username) repo = create(:repo, :active, private: true) create(:subscription, user: user, repo: repo) + create(:owner, name: user.username) gateway_subscription = instance_double( "PaymentGatewaySubscription", unsubscribe: true, diff --git a/spec/presenters/plan_presenter_spec.rb b/spec/presenters/plan_presenter_spec.rb index 77cd69b799..0d43c9923e 100644 --- a/spec/presenters/plan_presenter_spec.rb +++ b/spec/presenters/plan_presenter_spec.rb @@ -15,7 +15,7 @@ context "when the plan matches the user's current plan" do it "returns true" do user = create(:user) - owner = create(:owner, name: user.username) + create(:owner, name: user.username) plan = StripePlan.new(**StripePlan::PLANS.first) presenter = PlanPresenter.new(plan: plan, user: user) @@ -26,7 +26,7 @@ context "when the plan does not match the user's current plan" do it "returns false" do user = create(:user) - owner = create(:owner, name: user.username) + create(:owner, name: user.username) plan = StripePlan.new(**StripePlan::PLANS.second) presenter = PlanPresenter.new(plan: plan, user: user) @@ -39,7 +39,7 @@ context "when the plan matches the user's next plan" do it "returns true" do user = create(:user) - owner = create(:owner, name: user.username) + create(:owner, name: user.username) plan = StripePlan.new(**StripePlan::PLANS.second) presenter = PlanPresenter.new(plan: plan, user: user) diff --git a/spec/support/helpers/stripe_api_helper.rb b/spec/support/helpers/stripe_api_helper.rb index be9d31c975..d84bd39cae 100644 --- a/spec/support/helpers/stripe_api_helper.rb +++ b/spec/support/helpers/stripe_api_helper.rb @@ -1,7 +1,7 @@ module StripeApiHelper STRIPE_BASE_URL = "https://api.stripe.com/v1".freeze - STRIPE_CUSTOMER_ID = "cus_2e3fqARc1uHtCv" - STRIPE_SUBSCRIPTION_ID = "sub_488ZZngNkyRMiR" + STRIPE_CUSTOMER_ID = "cus_2e3fqARc1uHtCv".freeze + STRIPE_SUBSCRIPTION_ID = "sub_488ZZngNkyRMiR".freeze def stub_customer_create_request(user) stub_request(:post, "#{STRIPE_BASE_URL}/customers").with( From 6c2f191f0651e6dc93a3bfda463e6ec36cedf835 Mon Sep 17 00:00:00 2001 From: Greg Date: Fri, 16 Jul 2021 22:51:34 -0700 Subject: [PATCH 7/7] Owner subscription --- app/models/payment_gateway_customer.rb | 3 ++- app/services/create_subscription.rb | 16 ++++++++++++++++ lib/tasks/plan.rake | 8 ++++++-- 3 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 app/services/create_subscription.rb diff --git a/app/models/payment_gateway_customer.rb b/app/models/payment_gateway_customer.rb index 99087cdc02..e0fe73df94 100644 --- a/app/models/payment_gateway_customer.rb +++ b/app/models/payment_gateway_customer.rb @@ -52,8 +52,9 @@ def update_email(email) private def create_subscription(options) + subscription = customer.subscriptions.create(options) PaymentGatewaySubscription.new( - stripe_subscription: customer.subscriptions.create(options), + stripe_subscription: subscription, user: user, ).tap do |subscription| Analytics.new(user).track_purchase(subscription.stripe_subscription) diff --git a/app/services/create_subscription.rb b/app/services/create_subscription.rb new file mode 100644 index 0000000000..58fa3a87de --- /dev/null +++ b/app/services/create_subscription.rb @@ -0,0 +1,16 @@ +class CreateSubscription + pattr_initialize :owner + + def call + if owner.stripe_subscription_id.blank? + payment_gateway_subscription = payment_gateway_customer. + retrieve_subscription(repo.subscription.stripe_subscription_id) + end + end + + private + + def payment_gateway_customer + @_payment_gateway_customer ||= PaymentGatewayCustomer.new(owner) + end +end diff --git a/lib/tasks/plan.rake b/lib/tasks/plan.rake index dcf94f20b6..6bf993eb58 100644 --- a/lib/tasks/plan.rake +++ b/lib/tasks/plan.rake @@ -8,10 +8,14 @@ namespace :plan do task migrate_owners: :environment do owners = Repo.joins(:subscription).map(&:owner).uniq owners.each do |owner| - stripe_user = owner.repos.active.detect(&:subscription).subscription.user + subscription = owner.repos.active.detect(&:subscription).subscription + stripe_user = subscription.user puts "Onwer #{owner.name} mapped to user #{stripe_user.username}" - owner.update!(stripe_customer_id: stripe_user.stripe_customer_id) + owner.update!( + stripe_customer_id: stripe_user.stripe_customer_id, + stripe_subscription_id: subscription.stripe_subscription_id, + ) end end end