[ongoing] refactor user.subscriptions to save history

TODO: - refactor subscription.save_with_payment (stripe)
- offer free days
- extend the subscription
- renew a subscription
- buy subscription + reservation

app/controllers/api/subscriptions_controller.rb

@@ -1,7 +1,7 @@
 class API::SubscriptionsController < API::ApiController
   include FablabConfiguration
 
-  before_action :set_subscription, only: [:show, :edit, :update, :destroy]
+  before_action :set_subscription, only: %i[show edit update destroy]
   before_action :authenticate_user!
 
   def show
@@ -12,15 +12,12 @@ class API::SubscriptionsController < API::ApiController
     if fablab_plans_deactivated?
       head 403
     else
-      if current_user.is_admin?
+      method = current_user.is_admin? ? :local : :stripe
-        @subscription = Subscription.find_or_initialize_by(user_id: subscription_params[:user_id])
+      user_id = current_user.is_admin? ? subscription_params[:user_id] : current_user.id
-        @subscription.attributes = subscription_params
+
-        is_subscribe = @subscription.save_with_local_payment(true, coupon_params[:coupon_code])
+      @subscription = Subscription.new(subscription_params)
-      else
+      is_subscribe = Subscribe.new(method, user_id).pay_and_save(@subscription, coupon_params[:coupon_code], true)
-        @subscription = Subscription.find_or_initialize_by(user_id: current_user.id)
+
-        @subscription.attributes = subscription_params
-        is_subscribe = @subscription.save_with_payment(true, coupon_params[:coupon_code])
-      end
       if is_subscribe
         render :show, status: :created, location: @subscription
       else
@@ -44,6 +41,7 @@ class API::SubscriptionsController < API::ApiController
   end
 
   private
+
   # Use callbacks to share common setup or constraints between actions.
   def set_subscription
     @subscription = Subscription.find(params[:id])
@@ -64,11 +62,9 @@ class API::SubscriptionsController < API::ApiController
 
   # TODO refactor subscriptions logic and move this in model/validator
   def valid_card_token?(token)
-      begin
     Stripe::Token.retrieve(token)
   rescue Stripe::InvalidRequestError => e
     @subscription.errors[:card_token] << e.message
     false
   end
 end
-end

app/models/invoice.rb

@@ -169,7 +169,7 @@ class Invoice < ActiveRecord::Base
 
   def is_subscription_invoice?
     invoice_items.each do |ii|
-      return true if ii.subscription and !ii.subscription.is_expired?
+      return true if ii.subscription and !ii.subscription.expired?
     end
     false
   end

app/models/notification_type.rb

@@ -2,7 +2,7 @@ class NotificationType
   include NotifyWith::NotificationType
 
   # DANGER: dont remove a notification type!!!
-  notification_type_names %w(
+  notification_type_names %w[
     notify_admin_when_project_published
     notify_project_collaborator_to_valid
     notify_project_author_when_collaborator_valid
@@ -42,5 +42,5 @@ class NotificationType
     notify_admin_export_complete
     notify_member_about_coupon
     notify_member_reservation_reminder
-  )
+  ]
 end

app/models/subscription.rb

@@ -13,22 +13,24 @@ class Subscription < ActiveRecord::Base
   attr_accessor :card_token
 
   # creation
-  after_save :notify_member_subscribed_plan, if: :is_new?
+  after_save :notify_member_subscribed_plan, if: :new?
-  after_save :notify_admin_subscribed_plan, if: :is_new?
+  after_save :notify_admin_subscribed_plan, if: :new?
   after_save :notify_partner_subscribed_plan, if: :of_partner_plan?
 
   # Stripe subscription payment
   # @params [invoice] if true then subscription pay itself, dont pay with reservation
   #                   if false then subscription pay with reservation
   def save_with_payment(invoice = true, coupon_code = nil)
-    if valid?
+    return unless valid?
+
     begin
       customer = Stripe::Customer.retrieve(user.stp_customer_id)
       invoice_items = []
 
       unless coupon_code.nil?
         @coupon = Coupon.find_by(code: coupon_code)
-          if not @coupon.nil? and @coupon.status(user.id) == 'active'
+        raise InvalidCouponError if @coupon.nil? || @coupon.status(user.id) != 'active'
+
         total = plan.amount
 
         discount = 0
@@ -46,9 +48,6 @@ class Subscription < ActiveRecord::Base
           currency: Rails.application.secrets.stripe_currency,
           description: "coupon #{@coupon.code} - subscription"
         )
-          else
-            raise InvalidCouponError
-          end
       end
 
       # only add a wallet invoice item if pay subscription
@@ -131,28 +130,22 @@ class Subscription < ActiveRecord::Base
       return false
     end
   end
-  end
 
-  # @params [invoice] if true then subscription pay itself, dont pay with reservation
+  # @params [invoice] if true then only the subscription is payed, without reservation
-  #                   if false then subscription pay with reservation
+  #                   if false then the subscription is payed with reservation
   def save_with_local_payment(invoice = true, coupon_code = nil)
-    if valid?
+    return false unless valid?
-      # very important to set expired_at to nil that can allow method is_new? to return true
+
-      # for send the notification
+    return false unless save
-      # TODO: Refactoring
+
-      update_column(:expired_at, nil) unless new_record?
+    UsersCredits::Manager.new(user: user).reset_credits if expired_date_changed
-      self.stp_subscription_id = nil
-      self.canceled_at = nil
-      set_expired_at
-      if save
-        UsersCredits::Manager.new(user: self.user).reset_credits if expired_date_changed
     if invoice
       @wallet_amount_debit = get_wallet_amount_debit
 
       # debit wallet
       wallet_transaction = debit_user_wallet
 
-          if !self.user.invoicing_disabled?
+      unless user.invoicing_disabled?
         invoc = generate_invoice(nil, coupon_code)
         if wallet_transaction
           invoc.wallet_amount = @wallet_amount_debit
@@ -161,13 +154,7 @@ class Subscription < ActiveRecord::Base
         invoc.save
       end
     end
-        return true
+    true
-      else
-        return false
-      end
-    else
-      return false
-    end
   end
 
   def generate_invoice(stp_invoice_id = nil, coupon_code = nil)
@@ -200,19 +187,19 @@ class Subscription < ActiveRecord::Base
   end
 
   def cancel
-    if stp_subscription_id.present?
+    return unless stp_subscription_id.present?
+
     stp_subscription = stripe_subscription
     stp_subscription.delete(at_period_end: true)
     update_columns(canceled_at: Time.now)
   end
-  end
 
   def stripe_subscription
     user.stripe_customer.subscriptions.retrieve(stp_subscription_id) if stp_subscription_id.present?
   end
 
   def expire(time)
-    if !is_expired?
+    if !expired?
       update_columns(expired_at: time, canceled_at: time)
       notify_admin_subscription_canceled
       notify_member_subscription_canceled
@@ -222,7 +209,7 @@ class Subscription < ActiveRecord::Base
     end
   end
 
-  def is_expired?
+  def expired?
     expired_at <= Time.now
   end
 
@@ -231,14 +218,15 @@ class Subscription < ActiveRecord::Base
 
     self.expired_at = expired_at
     if save
-      UsersCredits::Manager.new(user: self.user).reset_credits if !free_days
+      UsersCredits::Manager.new(user: user).reset_credits unless free_days
       notify_subscription_extended(free_days)
       return true
     end
-    return false
+    false
   end
 
   private
+
   def notify_member_subscribed_plan
     NotificationCenter.call type: 'notify_member_subscribed_plan',
                             receiver: user,
@@ -289,8 +277,9 @@ class Subscription < ActiveRecord::Base
   end
 
   def expired_date_changed
-    p_value = self.previous_changes[:expired_at][0]
+    p_value = previous_changes[:expired_at][0]
     return true if p_value.nil?
+
     p_value.to_date != expired_at.to_date and expired_at > p_value
   end
 
@@ -298,7 +287,7 @@ class Subscription < ActiveRecord::Base
   #   !expired_at_was.nil? and expired_at_changed?
   # end
 
-  def is_new?
+  def new?
     expired_at_was.nil?
   end
 
@@ -308,18 +297,16 @@ class Subscription < ActiveRecord::Base
 
   def get_wallet_amount_debit
     total = plan.amount
-    if @coupon
+    total = CouponService.new.apply(total, @coupon, user.id) if @coupon
-      total = CouponService.new.apply(total, @coupon, user.id)
-    end
     wallet_amount = (user.wallet.amount * 100).to_i
-    return wallet_amount >= total ? total : wallet_amount
+    wallet_amount >= total ? total : wallet_amount
   end
 
   def debit_user_wallet
-    if @wallet_amount_debit.present? and @wallet_amount_debit != 0
+    return unless @wallet_amount_debit.present? || @wallet_amount_debit.zero?
+
     amount = @wallet_amount_debit / 100.0
-      return WalletService.new(user: user, wallet: user.wallet).debit(amount, self)
+    WalletService.new(user: user, wallet: user.wallet).debit(amount, self)
-    end
   end
 
   def clear_wallet_and_goupon_invoice_items(invoice_items)

app/models/user.rb

@@ -220,7 +220,7 @@ class User < ActiveRecord::Base
 
   ## used to allow the migration of existing users between authentication providers
   def generate_auth_migration_token
-    update_attributes(:auth_token => Devise.friendly_token)
+    update_attributes(auth_token: Devise.friendly_token)
   end
 
   ## link the current user to the given provider (omniauth attributes hash)
@@ -241,7 +241,7 @@ class User < ActiveRecord::Base
   ## Merge the provided User's SSO details into the current user and drop the provided user to ensure the unity
   ## @param sso_user {User} the provided user will be DELETED after the merge was successful
   def merge_from_sso(sso_user)
-    # update the attibutes to link the account to the sso account
+    # update the attributes to link the account to the sso account
     self.provider = sso_user.provider
     self.uid = sso_user.uid
 
@@ -264,7 +264,7 @@ class User < ActiveRecord::Base
       set_data_from_sso_mapping(field, value) unless field == 'user.email' && value.end_with?('-duplicate')
     end
 
-    # run the account transfert in an SQL transaction to ensure data integrity
+    # run the account transfer in an SQL transaction to ensure data integrity
     User.transaction do
       # remove the temporary account
       sso_user.destroy
@@ -319,7 +319,7 @@ class User < ActiveRecord::Base
   end
 
   def notify_admin_when_user_is_created
-    if need_completion? and not provider.nil?
+    if need_completion? && !provider.nil?
       NotificationCenter.call type: 'notify_admin_when_user_is_imported',
                               receiver: User.admins,
                               attached_object: self

app/services/subscription_extension_after_reservation.rb

@@ -2,7 +2,8 @@ class SubscriptionExtensionAfterReservation
   attr_accessor :user, :reservation
 
   def initialize(reservation)
-    @user, @reservation = reservation.user, reservation
+    @user = reservation.user
+    @reservation = reservation
   end
 
   def extend_subscription_if_eligible
@@ -13,9 +14,10 @@ class SubscriptionExtensionAfterReservation
     return false unless reservation.reservable_type == 'Training'
     return false if user.reservations.where(reservable_type: 'Training').count != 1
     return false unless user.subscription
-    return false if user.subscription.is_expired?
+    return false if user.subscription.expired?
     return false unless user.subscribed_plan.is_rolling
-    return true
+
+    true
   end
 
   def extend_subscription

app/services/subscriptions/subscribe.rb

@@ -0,0 +1,17 @@
+class Subscribe
+  attr_accessor :payment_method, :user_id
+
+  def initialize(payment_method, user_id)
+    @payment_method = payment_method
+    @user_id = user_id
+  end
+
+  def pay_and_save(subscription, coupon, invoice)
+    subscription.user_id = user_id
+    if payment_method == :local
+      subscription.save_with_local_payment(invoice, coupon)
+    elsif payment_method == :stripe
+      subscription.save_with_payment(invoice, coupon)
+    end
+  end
+end

app/services/users_credits/manager.rb

@@ -1,7 +1,7 @@
 require 'forwardable'
 
 module UsersCredits
-  class AlreadyUpdatedError < StandardError;end;
+  class AlreadyUpdatedError < StandardError; end
 
   class Manager
     extend Forwardable
@@ -31,7 +31,8 @@ module UsersCredits
   end
 
   module Managers
-    class User # that class is responsible for reseting users_credits of a user
+    # that class is responsible for resetting users_credits of a user
+    class User
       attr_reader :user
 
       def initialize(user)
@@ -46,7 +47,8 @@ module UsersCredits
     class Reservation
       attr_reader :reservation
 
-      def initialize(reservation, plan) # a plan can be passed to do a simulation (if user didn't have a subscription YET)
+      # a plan can be passed to do a simulation (if user didn't have a subscription YET)
+      def initialize(reservation, plan)
         @reservation = reservation
         @already_updated = false
         @plan = plan
@@ -71,8 +73,10 @@ module UsersCredits
     private_constant :Reservation
 
 
-    class Machine < Reservation # that class is responsible for knowing how to update users_credit of a given user for a given reservation
+    # that class is responsible for knowing how to update users_credit of a given user for a given reservation
-      def will_use_credits?  # to known if a credit will be used in the context of the given reservation
+    class Machine < Reservation
+      # to known if a credit will be used in the context of the given reservation
+      def will_use_credits?
         _will_use_credits?[0]
       end
 
@@ -97,6 +101,7 @@ module UsersCredits
       end
 
       private
+
       def _will_use_credits?
         return false, 0 unless plan
 
@@ -108,7 +113,7 @@ module UsersCredits
 
           free_hours_count = [remaining_hours, reservation.slots.size].min
 
-            if free_hours_count > 0
+          if free_hours_count.positive?
             return true, free_hours_count, machine_credit
           else
             return false, free_hours_count, machine_credit
@@ -118,7 +123,8 @@ module UsersCredits
       end
     end
 
-    class Training < Reservation # same as class Machine but for Training reservation
+    # same as class Machine but for Training reservation
+    class Training < Reservation
       def will_use_credits?
         _will_use_credits?[0]
       end
@@ -132,6 +138,7 @@ module UsersCredits
       end
 
       private
+
       def _will_use_credits?
         return false, nil unless plan
 
@@ -151,12 +158,12 @@ module UsersCredits
         false
       end
 
-      def update_credits
+      def update_credits; end
-      end
     end
 
     class Space < Reservation
-      def will_use_credits?  # to known if a credit will be used in the context of the given reservation
+      # to known if a credit will be used in the context of the given reservation
+      def will_use_credits?
         _will_use_credits?[0]
       end
 
@@ -168,7 +175,8 @@ module UsersCredits
         super
 
         will_use_credits, free_hours_count, space_credit = _will_use_credits?
-        if will_use_credits
+        return unless will_use_credits
+
         users_credit = user.users_credits.find_or_initialize_by(credit_id: space_credit.id)
 
         if users_credit.new_record?
@@ -178,9 +186,9 @@ module UsersCredits
         end
         users_credit.save!
       end
-      end
 
       private
+
       def _will_use_credits?
         return false, 0 unless plan
 
@@ -192,7 +200,7 @@ module UsersCredits
 
           free_hours_count = [remaining_hours, reservation.slots.size].min
 
-          if free_hours_count > 0
+          if free_hours_count.positive?
             return true, free_hours_count, space_credit
           else
             return false, free_hours_count, space_credit

test/integration/subscriptions/renew_as_admin_test.rb

@@ -10,7 +10,7 @@ class Subscriptions::RenewAsAdminTest < ActionDispatch::IntegrationTest
     user = User.find_by(username: 'kdumas')
     plan = Plan.find_by(base_name: 'Mensuel tarif réduit')
 
-    VCR.use_cassette("subscriptions_admin_renew_success") do
+    VCR.use_cassette('subscriptions_admin_renew_success') do
       post '/api/subscriptions',
            {
              subscription: {
@@ -39,10 +39,16 @@ class Subscriptions::RenewAsAdminTest < ActionDispatch::IntegrationTest
 
     # Check that the user benefit from prices of his plan
     printer = Machine.find_by(slug: 'imprimante-3d')
-    assert_equal 10, (printer.prices.find_by(group_id: user.group_id, plan_id: user.subscription.plan_id).amount / 100), 'machine hourly price does not match'
+    assert_equal 10,
+                 (printer.prices.find_by(group_id: user.group_id, plan_id: user.subscription.plan_id).amount / 100),
+                 'machine hourly price does not match'
 
     # Check notification was sent to the user
-    notification = Notification.find_by(notification_type_id: NotificationType.find_by_name('notify_member_subscribed_plan'), attached_object_type: 'Subscription', attached_object_id: subscription[:id])
+    notification = Notification.find_by(
+      notification_type_id: NotificationType.find_by_name('notify_member_subscribed_plan'),
+      attached_object_type: 'Subscription',
+      attached_object_id: subscription[:id]
+    )
     assert_not_nil notification, 'user notification was not created'
     assert_equal user.id, notification.receiver_id, 'wrong user notified'
 
@@ -50,7 +56,41 @@ class Subscriptions::RenewAsAdminTest < ActionDispatch::IntegrationTest
     invoice = Invoice.find_by(invoiced_type: 'Subscription', invoiced_id: subscription[:id])
     assert_invoice_pdf invoice
     assert_equal plan.amount, invoice.total, 'Invoice total price does not match the bought subscription'
+  end
 
+  test 'admin successfully offer free days' do
+    user = User.find_by(username: 'pdurand')
+    subscription = user.subscription
+    new_date = (1.month.from_now - 4.days).utc
+
+    VCR.use_cassette('subscriptions_admin_offer_free_days') do
+      put "/api/subscriptions/#{subscription.id}",
+          {
+            subscription: {
+              expired_at: new_date.strftime('%Y-%m-%d %H:%M:%S.%9N Z'),
+              free: true
+            }
+          }.to_json, default_headers
+    end
+
+    # Check response format & status
+    assert_equal 200, response.status, response.body
+    assert_equal Mime::JSON, response.content_type
+
+    # Check that the subscribed plan was not altered
+    res_subscription = json_response(response.body)
+    assert_equal subscription.id, res_subscription[:id], 'subscription id has changed'
+    assert_equal subscription.plan_id, res_subscription[:plan_id], 'subscribed plan does not match'
+    assert_dates_equal new_date, res_subscription[:expired_at], 'subscription end date was not updated'
+
+    # Check notification was sent to the user
+    notification = Notification.find_by(
+      notification_type_id: NotificationType.find_by_name('notify_member_subscription_extended'),
+      attached_object_type: 'Subscription',
+      attached_object_id: subscription[:id]
+    )
+    assert_not_nil notification, 'user notification was not created'
+    assert_equal user.id, notification.receiver_id, 'wrong user notified'
   end
 
 end

test/integration/subscriptions/renew_as_user_test.rb

@@ -9,7 +9,7 @@ class Subscriptions::RenewAsUserTest < ActionDispatch::IntegrationTest
   test 'user successfully renew a subscription after it has ended' do
     plan = Plan.find_by(group_id: @user.group.id, type: 'Plan', base_name: 'Mensuel')
 
-    VCR.use_cassette("subscriptions_user_renew_success", :erb => true) do
+    VCR.use_cassette('subscriptions_user_renew_success', erb: true) do
       post '/api/subscriptions',
            {
              subscription: {
@@ -21,7 +21,7 @@ class Subscriptions::RenewAsUserTest < ActionDispatch::IntegrationTest
     end
 
     # Check response format & status
-    assert_equal 201, response.status, "API does not return the expected status."+response.body
+    assert_equal 201, response.status, "API does not return the expected status. #{response.body}"
     assert_equal Mime::JSON, response.content_type
 
     # Check the correct plan was subscribed
@@ -30,16 +30,24 @@ class Subscriptions::RenewAsUserTest < ActionDispatch::IntegrationTest
 
     # Check that the user has the correct subscription
     assert_not_nil @user.subscription, "user's subscription was not found"
-    assert (@user.subscription.expired_at > DateTime.now), "user's subscription expiration was not updated ... VCR cassettes may be outdated, please check the gitlab wiki"
+    assert (@user.subscription.expired_at > DateTime.now),
-    assert_in_delta 5, (DateTime.now.to_i - @user.subscription.updated_at.to_i), 10, "user's subscription was not updated recently"
+           "user's subscription expiration was not updated ... VCR cassettes may be outdated, please check the gitlab wiki"
+    assert_in_delta 5,
+                    (DateTime.now.to_i - @user.subscription.updated_at.to_i),
+                    10,
+                    "user's subscription was not updated recently"
 
     # Check that the credits were reset correctly
     assert_empty @user.users_credits, 'credits were not reset'
 
     # Check notifications were sent for every admins
-    notifications = Notification.where(notification_type_id: NotificationType.find_by_name('notify_admin_subscribed_plan'), attached_object_type: 'Subscription', attached_object_id: subscription[:id])
+    notifications = Notification.where(
+      notification_type_id: NotificationType.find_by_name('notify_admin_subscribed_plan'),
+      attached_object_type: 'Subscription',
+      attached_object_id: subscription[:id]
+    )
     assert_not_empty notifications, 'no notifications were created'
-    notified_users_ids = notifications.map {|n| n.receiver_id }
+    notified_users_ids = notifications.map(&:receiver_id)
     User.admins.each do |adm|
       assert_includes notified_users_ids, adm.id, "Admin #{adm.id} was not notified"
     end
@@ -57,7 +65,7 @@ class Subscriptions::RenewAsUserTest < ActionDispatch::IntegrationTest
 
     previous_expiration = @user.subscription.expired_at.to_i
 
-    VCR.use_cassette("subscriptions_user_renew_failed") do
+    VCR.use_cassette('subscriptions_user_renew_failed') do
       post '/api/subscriptions',
            {
              subscription: {
@@ -69,7 +77,7 @@ class Subscriptions::RenewAsUserTest < ActionDispatch::IntegrationTest
     end
 
     # Check response format & status
-    assert_equal 422, response.status, "API does not return the expected status."+response.body
+    assert_equal 422, response.status, "API does not return the expected status. #{response.body}"
     assert_equal Mime::JSON, response.content_type
 
     # Check the error was handled

test/test_helper.rb

@@ -120,6 +120,11 @@ class ActiveSupport::TestCase
     end
   end
 
+  def assert_dates_equal(expected, actual, msg = nil)
+    assert_not_nil actual, msg
+    assert_equal expected.to_date, actual.to_date, msg
+  end
+
   private
 
   # Parse a line of text read from a PDF file and return the price included inside