Some cleanup of code to deal with past due subscriptions.

6 files changed, 30 insertions, 27 deletions

diff --git a/billing/app/controllers/billing_admin_controller.rb b/billing/app/controllers/billing_admin_controller.rb
index 419a937..cd6149f 100644
--- a/billing/app/controllers/billing_admin_controller.rb
+++ b/billing/app/controllers/billing_admin_controller.rb
@@ -2,14 +2,28 @@ class BillingAdminController < BillingBaseController
   before_filter :authorize_admin
 
   def show
-    @past_due_atleast_90_days = Braintree::Subscription.search do |search|
+
+    br_atleast_90_days = Braintree::Subscription.search do |search|
       search.days_past_due >= 90
     end
+    @past_due_atleast_90_days = braintree_resource_collection_to_array(br_atleast_90_days)
 
-    @all_past_due = Braintree::Subscription.search do |search|
+    br_all_past_due = Braintree::Subscription.search do |search|
       search.status.is Braintree::Subscription::Status::PastDue
       #cannot search by balance.
     end
+    @all_past_due = braintree_resource_collection_to_array(br_all_past_due)
+
+  end
+
+  private
+
+  def braintree_resource_collection_to_array(braintree_resource_collection)
+    array = []
+    braintree_resource_collection.each do |object|
+      array << object
+    end
+    array
   end
 
 end
diff --git a/billing/app/controllers/subscriptions_controller.rb b/billing/app/controllers/subscriptions_controller.rb
index 0a1c733..01aaab4 100644
--- a/billing/app/controllers/subscriptions_controller.rb
+++ b/billing/app/controllers/subscriptions_controller.rb
@@ -1,7 +1,7 @@
 class SubscriptionsController < BillingBaseController
   before_filter :authorize
   before_filter :fetch_subscription, :only => [:show, :destroy]
-  before_filter :only_admin_active_pending, :only => [:destroy]
+  before_filter :confirm_cancel_subscription, :only => [:destroy]
   before_filter :confirm_self_or_admin, :only => [:index]
   before_filter :confirm_no_pending_active_pastdue_subscription, :only => [:new, :create]
   # for now, admins cannot create or destroy subscriptions for others:
@@ -41,8 +41,8 @@ class SubscriptionsController < BillingBaseController
 
   end
 
-  def only_admin_active_pending
-    access_denied unless admin? or ['Pending', 'Active'].include? @subscription.status
+  def confirm_cancel_subscription
+    access_denied unless view_context.allow_cancel_subscription(@subscription)
   end
 
   def confirm_no_pending_active_pastdue_subscription
diff --git a/billing/app/helpers/billing_helper.rb b/billing/app/helpers/billing_helper.rb
index 68ed5b8..b9e5e2e 100644
--- a/billing/app/helpers/billing_helper.rb
+++ b/billing/app/helpers/billing_helper.rb
@@ -33,30 +33,19 @@ module BillingHelper
 
     if (transaction = subscription.transactions.first)
       # much quicker, but will only work if there is already a transaction associated with subscription (should generally be)
-      braintree_customer = transaction.customer_details
+      braintree_customer_id = transaction.customer_details.id
     else
-      search_results = Braintree::Customer.search do |search|
-        search.payment_method_token.is subscription.payment_method_token
-      end
-      braintree_customer = search_results.first
+      credit_card = Braintree::CreditCard.find(subscription.payment_method_token)
+      braintree_customer_id = credit_card.customer_id
     end
 
-    customer = Customer.find_by_braintree_customer_id(braintree_customer.id)
+    customer = Customer.find_by_braintree_customer_id(braintree_customer_id)
     user = User.find(customer.user_id)
 
   end
 
-  def show_set_user_subscriptions(set)
-    if set.empty?
-      return t(:none)
-    else
-      subscriptions_to_display = ''
-      set.each do |past_due_subscription|
-        subscriptions_to_display += render :partial => "subscriptions/subscription_details", :locals => {:subscription => past_due_subscription, :show_user => user_for_subscription(past_due_subscription)}
-      end
-     subscriptions_to_display.html_safe
-    end
+  def allow_cancel_subscription(subscription)
+    ['Active', 'Pending'].include? subscription.status or (admin? and subscription.status == 'Past Due')
   end
 
-
 end
diff --git a/billing/app/views/billing_admin/show.html.haml b/billing/app/views/billing_admin/show.html.haml
index 3881dc7..0382cf0 100644
--- a/billing/app/views/billing_admin/show.html.haml
+++ b/billing/app/views/billing_admin/show.html.haml
@@ -1,8 +1,7 @@
 %legend= t(:more_than_90_days_past_due)
-= show_set_user_subscriptions(@past_due_atleast_90_days)
-
+= render(:partial => "subscriptions/subscription_details", :collection => @past_due_atleast_90_days, :as => 'subscription', :locals => {:show_user => true}) || t(:none)
 %legend= t(:all_past_due)
-= show_set_user_subscriptions(@all_past_due)
+= render(:partial => "subscriptions/subscription_details", :collection => @all_past_due, :as => 'subscription', :locals => {:show_user => true}) || t(:none)
 
 %legend= t(:your_settings)
 = link_to 'view own billing settings', show_or_new_customer_link(current_user)
\ No newline at end of file
diff --git a/billing/app/views/subscriptions/_subscription_details.html.haml b/billing/app/views/subscriptions/_subscription_details.html.haml
index 3a06c20..6145c95 100644
--- a/billing/app/views/subscriptions/_subscription_details.html.haml
+++ b/billing/app/views/subscriptions/_subscription_details.html.haml
@@ -1,7 +1,8 @@
 %p
   - if local_assigns[:show_user]
     User:
-    = link_to show_user.login, user_overview_path(show_user)
+    - user_to_show = user_for_subscription(subscription)
+    = link_to user_to_show.login, user_overview_path(user_to_show)
   ID:
   = link_to subscription.id, user_subscription_path(@user, subscription.id)
   Balance:
diff --git a/billing/app/views/subscriptions/show.html.haml b/billing/app/views/subscriptions/show.html.haml
index f4d644a..2699db9 100644
--- a/billing/app/views/subscriptions/show.html.haml
+++ b/billing/app/views/subscriptions/show.html.haml
@@ -3,4 +3,4 @@
     Current
   Subscription
 = render :partial => "subscription_details",  :locals => {:subscription => @subscription}
-= link_to t(:cancel_subscription), user_subscription_path(@user, @subscription.id),  :confirm => t(:are_you_sure), :method => :delete, :class => 'btn btn-danger' if ['Active', 'Pending'].include? @subscription.status or admin? # permission check or should that just be on show? # should you be able to cancel pending subscription?
+= link_to t(:cancel_subscription), user_subscription_path(@user, @subscription.id),  :confirm => t(:are_you_sure), :method => :delete, :class => 'btn btn-danger' if allow_cancel_subscription(@subscription)