From feaf16d8784b3686abb34dbb0431f47b52bb443b Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Tue, 16 Oct 2018 17:30:57 +0100
Subject: [PATCH 1/5] Fix bug in subscriptions address controller where the
 country id lookup was not exact and states returned were incorrect. Add unit
 tests to cover different cases

---
 .../controllers/address_controller.js.coffee  | 18 +++++---
 ...ee => providers_controller_spec.js.coffee} |  0
 .../address_controller_spec.js.coffee         | 43 +++++++++++++++++++
 3 files changed, 54 insertions(+), 7 deletions(-)
 rename spec/javascripts/unit/admin/controllers/{providers_controller_decorator.js.coffee => providers_controller_spec.js.coffee} (100%)
 create mode 100644 spec/javascripts/unit/admin/subscriptions/controllers/address_controller_spec.js.coffee

diff --git a/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee b/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
index 02c78e3615..01b5e3d9c2 100644
--- a/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
+++ b/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
@@ -1,11 +1,21 @@
 angular.module("admin.subscriptions").controller "AddressController", ($scope, $filter, StatusMessage, availableCountries) ->
   $scope.countries = availableCountries
+
   $scope.statesFor = (country_id) ->
     return [] unless country_id
-    $filter('filter')(availableCountries, {id: country_id})[0].states
+    country = $filter('filter')(availableCountries, {id: country_id}, true)[0]
+    return [] unless country
+    country.states
+
   $scope.billStates = $scope.statesFor($scope.subscription.bill_address.country_id)
   $scope.shipStates = $scope.statesFor($scope.subscription.ship_address.country_id)
 
+  $scope.$watch 'subscription.bill_address.country_id', (newValue, oldValue) ->
+    $scope.billStates = $scope.statesFor(newValue) if newValue?
+
+  $scope.$watch 'subscription.ship_address.country_id', (newValue, oldValue) ->
+    $scope.shipStates = $scope.statesFor(newValue) if newValue?
+
   $scope.registerNextCallback 'address', ->
     $scope.subscription_form.$submitted = true
     if $scope.subscription_address_form.$valid
@@ -18,9 +28,3 @@ angular.module("admin.subscriptions").controller "AddressController", ($scope, $
   $scope.registerBackCallback 'address', ->
     StatusMessage.clear()
     $scope.setView('details')
-
-  $scope.$watch 'subscription.bill_address.country_id', (newValue, oldValue) ->
-    $scope.billStates = $scope.statesFor(newValue) if newValue?
-
-  $scope.$watch 'subscription.ship_address.country_id', (newValue, oldValue) ->
-    $scope.shipStates = $scope.statesFor(newValue) if newValue?
diff --git a/spec/javascripts/unit/admin/controllers/providers_controller_decorator.js.coffee b/spec/javascripts/unit/admin/controllers/providers_controller_spec.js.coffee
similarity index 100%
rename from spec/javascripts/unit/admin/controllers/providers_controller_decorator.js.coffee
rename to spec/javascripts/unit/admin/controllers/providers_controller_spec.js.coffee
diff --git a/spec/javascripts/unit/admin/subscriptions/controllers/address_controller_spec.js.coffee b/spec/javascripts/unit/admin/subscriptions/controllers/address_controller_spec.js.coffee
new file mode 100644
index 0000000000..c622906893
--- /dev/null
+++ b/spec/javascripts/unit/admin/subscriptions/controllers/address_controller_spec.js.coffee
@@ -0,0 +1,43 @@
+describe "AddressController", ->
+  scope = null
+  subscription = { id: 1 }
+
+  states_in_spain = [{id: 55, name: "CAT", abbr: "CAT"}]
+  states_in_portugal = [{id: 55, name: "ACT", abbr: "ACT"}, {id: 5, name: "BFT", abbr: "BFT"}]
+  availableCountries = [
+    {id: 9, name: "Australia", states: []},
+    {id: 119, name: "Spain", states: states_in_spain},
+    {id: 19, name: "Portugal", states: states_in_portugal}
+  ]
+
+  beforeEach ->
+    module('admin.subscriptions')
+
+    inject ($controller, $rootScope) ->
+      scope = $rootScope
+
+      scope.registerNextCallback = () ->
+      scope.registerBackCallback = () ->
+      scope.subscription = subscription
+      subscription.bill_address = {country_id: 1}
+      subscription.ship_address = {country_id: 2}
+      $controller 'AddressController', {$scope: scope, availableCountries: availableCountries}
+
+  describe "statesFor", ->
+    it "returns empty array for nil country id", ->
+      expect(scope.statesFor(null)).toEqual []
+
+    it "returns empty array for country id not in availableCountries", ->
+      expect(scope.statesFor(10)).toEqual []
+
+    it "returns empty array for country id in availableCountries but without states", ->
+      expect(scope.statesFor(9)).toEqual []
+
+    it "returns states for country id in availableCountries with states", ->
+      expect(scope.statesFor(119)).toEqual states_in_spain
+
+    it "returns empty array for country id (11) in availableCountries but only as part of other country id (119)", ->
+      expect(scope.statesFor(11)).toEqual []
+
+    it "returns states for country id (19) in availableCountries with states even if other country ids contain the requested id (119)", ->
+      expect(scope.statesFor(19)).toEqual states_in_portugal

From 5c5a2194d6e74ff791e67764dbf220df0c37dfdc Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Tue, 16 Oct 2018 21:58:27 +0100
Subject: [PATCH 2/5] Extract country states logic out of subscriptions address
 controller into new service CountryStates

---
 .../admin/services/country_states.js.coffee   |  8 +++++
 .../controllers/address_controller.js.coffee  | 16 +++-------
 .../subscriptions/subscriptions.js.coffee     |  2 +-
 .../country_states_spec.js.coffee}            | 31 +++++++------------
 4 files changed, 25 insertions(+), 32 deletions(-)
 create mode 100644 app/assets/javascripts/admin/services/country_states.js.coffee
 rename spec/javascripts/unit/admin/{subscriptions/controllers/address_controller_spec.js.coffee => services/country_states_spec.js.coffee} (54%)

diff --git a/app/assets/javascripts/admin/services/country_states.js.coffee b/app/assets/javascripts/admin/services/country_states.js.coffee
new file mode 100644
index 0000000000..a29645d55f
--- /dev/null
+++ b/app/assets/javascripts/admin/services/country_states.js.coffee
@@ -0,0 +1,8 @@
+angular.module("ofn.admin").factory "CountryStates", ($filter) ->
+  new class CountryStates
+
+    statesFor: (countries, country_id) ->
+      return [] unless country_id
+      country = $filter('filter')(countries, {id: country_id}, true)[0]
+      return [] unless country
+      country.states
diff --git a/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee b/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
index 01b5e3d9c2..30dff9975a 100644
--- a/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
+++ b/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
@@ -1,20 +1,14 @@
-angular.module("admin.subscriptions").controller "AddressController", ($scope, $filter, StatusMessage, availableCountries) ->
+angular.module("admin.subscriptions").controller "AddressController", ($scope, StatusMessage, availableCountries, CountryStates) ->
   $scope.countries = availableCountries
 
-  $scope.statesFor = (country_id) ->
-    return [] unless country_id
-    country = $filter('filter')(availableCountries, {id: country_id}, true)[0]
-    return [] unless country
-    country.states
-
-  $scope.billStates = $scope.statesFor($scope.subscription.bill_address.country_id)
-  $scope.shipStates = $scope.statesFor($scope.subscription.ship_address.country_id)
+  $scope.billStates = CountryStates.statesFor(availableCountries, $scope.subscription.bill_address.country_id)
+  $scope.shipStates = CountryStates.statesFor(availableCountries, $scope.subscription.ship_address.country_id)
 
   $scope.$watch 'subscription.bill_address.country_id', (newValue, oldValue) ->
-    $scope.billStates = $scope.statesFor(newValue) if newValue?
+    $scope.billStates = CountryStates.statesFor(availableCountries, newValue) if newValue?
 
   $scope.$watch 'subscription.ship_address.country_id', (newValue, oldValue) ->
-    $scope.shipStates = $scope.statesFor(newValue) if newValue?
+    $scope.shipStates = CountryStates.statesFor(availableCountries, newValue) if newValue?
 
   $scope.registerNextCallback 'address', ->
     $scope.subscription_form.$submitted = true
diff --git a/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee b/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee
index 9e2c3d5696..066552b598 100644
--- a/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee
+++ b/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee
@@ -1 +1 @@
-angular.module("admin.subscriptions", ['ngResource','admin.indexUtils','admin.dropdown'])
+angular.module("admin.subscriptions", ['ngResource','admin.indexUtils','admin.dropdown', 'ofn.admin'])
diff --git a/spec/javascripts/unit/admin/subscriptions/controllers/address_controller_spec.js.coffee b/spec/javascripts/unit/admin/services/country_states_spec.js.coffee
similarity index 54%
rename from spec/javascripts/unit/admin/subscriptions/controllers/address_controller_spec.js.coffee
rename to spec/javascripts/unit/admin/services/country_states_spec.js.coffee
index c622906893..5b3683b9d7 100644
--- a/spec/javascripts/unit/admin/subscriptions/controllers/address_controller_spec.js.coffee
+++ b/spec/javascripts/unit/admin/services/country_states_spec.js.coffee
@@ -1,6 +1,5 @@
-describe "AddressController", ->
-  scope = null
-  subscription = { id: 1 }
+describe "CountryStates service", ->
+  countryStates = null
 
   states_in_spain = [{id: 55, name: "CAT", abbr: "CAT"}]
   states_in_portugal = [{id: 55, name: "ACT", abbr: "ACT"}, {id: 5, name: "BFT", abbr: "BFT"}]
@@ -11,33 +10,25 @@ describe "AddressController", ->
   ]
 
   beforeEach ->
-    module('admin.subscriptions')
-
-    inject ($controller, $rootScope) ->
-      scope = $rootScope
-
-      scope.registerNextCallback = () ->
-      scope.registerBackCallback = () ->
-      scope.subscription = subscription
-      subscription.bill_address = {country_id: 1}
-      subscription.ship_address = {country_id: 2}
-      $controller 'AddressController', {$scope: scope, availableCountries: availableCountries}
+    module('ofn.admin')
+    inject (CountryStates) ->
+      countryStates = CountryStates
 
   describe "statesFor", ->
     it "returns empty array for nil country id", ->
-      expect(scope.statesFor(null)).toEqual []
+      expect(countryStates.statesFor(availableCountries, null)).toEqual []
 
     it "returns empty array for country id not in availableCountries", ->
-      expect(scope.statesFor(10)).toEqual []
+      expect(countryStates.statesFor(availableCountries, 10)).toEqual []
 
     it "returns empty array for country id in availableCountries but without states", ->
-      expect(scope.statesFor(9)).toEqual []
+      expect(countryStates.statesFor(availableCountries, 9)).toEqual []
 
     it "returns states for country id in availableCountries with states", ->
-      expect(scope.statesFor(119)).toEqual states_in_spain
+      expect(countryStates.statesFor(availableCountries, 119)).toEqual states_in_spain
 
     it "returns empty array for country id (11) in availableCountries but only as part of other country id (119)", ->
-      expect(scope.statesFor(11)).toEqual []
+      expect(countryStates.statesFor(availableCountries, 11)).toEqual []
 
     it "returns states for country id (19) in availableCountries with states even if other country ids contain the requested id (119)", ->
-      expect(scope.statesFor(19)).toEqual states_in_portugal
+      expect(countryStates.statesFor(availableCountries, 19)).toEqual states_in_portugal

From 1804bf5a2b1bf1124f03a236edc75d2f52c87a27 Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Tue, 16 Oct 2018 22:45:38 +0100
Subject: [PATCH 3/5] Extract countryStates logic from customer_address modal
 and re-use new CountryStates service

---
 .../admin/customers/customers.js.coffee         |  2 +-
 .../directives/edit_address_dialog.js.coffee    | 17 +++++------------
 .../admin/services/country_states.js.coffee     |  5 ++++-
 3 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/app/assets/javascripts/admin/customers/customers.js.coffee b/app/assets/javascripts/admin/customers/customers.js.coffee
index fe8ae1de5b..aa8a1cfc19 100644
--- a/app/assets/javascripts/admin/customers/customers.js.coffee
+++ b/app/assets/javascripts/admin/customers/customers.js.coffee
@@ -1 +1 @@
-angular.module("admin.customers", ['ngResource', 'admin.tagRules', 'admin.indexUtils', 'admin.utils', 'admin.dropdown'])
+angular.module("admin.customers", ['ngResource', 'admin.tagRules', 'admin.indexUtils', 'admin.utils', 'admin.dropdown', 'ofn.admin'])
diff --git a/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee b/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee
index eab83167a7..ca2d2ca817 100644
--- a/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee
+++ b/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee
@@ -1,4 +1,4 @@
-angular.module("admin.customers").directive 'editAddressDialog', ($compile, $templateCache, $filter, DialogDefaults, Customers, StatusMessage) ->
+angular.module("admin.customers").directive 'editAddressDialog', ($compile, $templateCache, DialogDefaults, Customers, StatusMessage, CountryStates) ->
   restrict: 'A'
   scope: true
   link: (scope, element, attr) ->
@@ -6,9 +6,9 @@ angular.module("admin.customers").directive 'editAddressDialog', ($compile, $tem
     scope.errors = []
 
     scope.$watch 'address.country_id', (newCountryID) ->
-      if newCountryID
-        scope.states = scope.filterStates(newCountryID)
-        scope.clearState() unless scope.addressStateMatchesCountry()
+      return unless newCountryID
+      scope.states = CountryStates.statesFor(scope.availableCountries, newCountryID)
+      scope.clearState() unless CountryStates.addressStateMatchesCountryStates(scope.states, scope.address.state_id)
 
     scope.updateAddress = ->
       scope.edit_address_form.$setPristine()
@@ -27,19 +27,12 @@ angular.module("admin.customers").directive 'editAddressDialog', ($compile, $tem
       else
         scope.addressType = 'ship_address'
       scope.address = scope.customer[scope.addressType]
-      scope.states = scope.filterStates(scope.address?.country_id)
+      scope.states = CountryStates.statesFor(scope.availableCountries, scope.address?.country_id)
 
       template = $compile($templateCache.get('admin/edit_address_dialog.html'))(scope)
       template.dialog(DialogDefaults)
       template.dialog('open')
       scope.$apply()
 
-    scope.filterStates = (countryID) ->
-      return [] unless countryID
-      $filter('filter')(scope.availableCountries, {id: parseInt(countryID)}, true)[0].states
-
     scope.clearState = ->
       scope.address.state_id = ""
-
-    scope.addressStateMatchesCountry = ->
-      scope.states.some (state) -> state.id == scope.address.state_id
\ No newline at end of file
diff --git a/app/assets/javascripts/admin/services/country_states.js.coffee b/app/assets/javascripts/admin/services/country_states.js.coffee
index a29645d55f..7977bc3535 100644
--- a/app/assets/javascripts/admin/services/country_states.js.coffee
+++ b/app/assets/javascripts/admin/services/country_states.js.coffee
@@ -3,6 +3,9 @@ angular.module("ofn.admin").factory "CountryStates", ($filter) ->
 
     statesFor: (countries, country_id) ->
       return [] unless country_id
-      country = $filter('filter')(countries, {id: country_id}, true)[0]
+      country = $filter('filter')(countries, {id: parseInt(country_id)}, true)[0]
       return [] unless country
       country.states
+
+    addressStateMatchesCountryStates: (countryStates, stateId) ->
+      countryStates.some (state) -> state.id == stateId

From c281927372f96ac208c8497ecb0a5daec7cccd6b Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Tue, 16 Oct 2018 23:02:26 +0100
Subject: [PATCH 4/5] Fix subscription address form. It now clears the state
 selection when a different country is selected

---
 .../directives/edit_address_dialog.js.coffee       |  6 ++----
 .../controllers/address_controller.js.coffee       | 14 ++++++++++----
 2 files changed, 12 insertions(+), 8 deletions(-)

diff --git a/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee b/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee
index ca2d2ca817..f52f4379da 100644
--- a/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee
+++ b/app/assets/javascripts/admin/customers/directives/edit_address_dialog.js.coffee
@@ -8,7 +8,8 @@ angular.module("admin.customers").directive 'editAddressDialog', ($compile, $tem
     scope.$watch 'address.country_id', (newCountryID) ->
       return unless newCountryID
       scope.states = CountryStates.statesFor(scope.availableCountries, newCountryID)
-      scope.clearState() unless CountryStates.addressStateMatchesCountryStates(scope.states, scope.address.state_id)
+      unless CountryStates.addressStateMatchesCountryStates(scope.states, scope.address.state_id)
+        scope.address.state_id = ""
 
     scope.updateAddress = ->
       scope.edit_address_form.$setPristine()
@@ -33,6 +34,3 @@ angular.module("admin.customers").directive 'editAddressDialog', ($compile, $tem
       template.dialog(DialogDefaults)
       template.dialog('open')
       scope.$apply()
-
-    scope.clearState = ->
-      scope.address.state_id = ""
diff --git a/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee b/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
index 30dff9975a..a9ba992b5c 100644
--- a/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
+++ b/app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee
@@ -4,11 +4,17 @@ angular.module("admin.subscriptions").controller "AddressController", ($scope, S
   $scope.billStates = CountryStates.statesFor(availableCountries, $scope.subscription.bill_address.country_id)
   $scope.shipStates = CountryStates.statesFor(availableCountries, $scope.subscription.ship_address.country_id)
 
-  $scope.$watch 'subscription.bill_address.country_id', (newValue, oldValue) ->
-    $scope.billStates = CountryStates.statesFor(availableCountries, newValue) if newValue?
+  $scope.$watch 'subscription.bill_address.country_id', (newCountryID) ->
+    return unless newCountryID
+    $scope.billStates = CountryStates.statesFor(availableCountries, newCountryID)
+    unless CountryStates.addressStateMatchesCountryStates($scope.billStates, $scope.subscription.bill_address.state_id)
+      $scope.subscription.bill_address.state_id = ""
 
-  $scope.$watch 'subscription.ship_address.country_id', (newValue, oldValue) ->
-    $scope.shipStates = CountryStates.statesFor(availableCountries, newValue) if newValue?
+  $scope.$watch 'subscription.ship_address.country_id', (newCountryID) ->
+    return unless newCountryID
+    $scope.shipStates = CountryStates.statesFor(availableCountries, newCountryID)
+    unless CountryStates.addressStateMatchesCountryStates($scope.shipStates, $scope.subscription.ship_address.state_id)
+      $scope.subscription.ship_address.state_id = ""
 
   $scope.registerNextCallback 'address', ->
     $scope.subscription_form.$submitted = true

From fe9f4a1c95d6201bc9b8a22c6f6d0d851f08b8da Mon Sep 17 00:00:00 2001
From: luisramos0 <luisramos0@gmail.com>
Date: Wed, 17 Oct 2018 12:16:33 +0100
Subject: [PATCH 5/5] Move countryStates service from ofn.admin to admin.utils
 to remove bad dependency from admin modules (customers and subscriptions) to
 main ofn.admin module. Now the dependency admin.utils is used instead

---
 app/assets/javascripts/admin/customers/customers.js.coffee      | 2 +-
 .../javascripts/admin/subscriptions/subscriptions.js.coffee     | 2 +-
 .../admin/{ => utils}/services/country_states.js.coffee         | 2 +-
 .../admin/{ => utils}/services/country_states_spec.js.coffee    | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)
 rename app/assets/javascripts/admin/{ => utils}/services/country_states.js.coffee (84%)
 rename spec/javascripts/unit/admin/{ => utils}/services/country_states_spec.js.coffee (98%)

diff --git a/app/assets/javascripts/admin/customers/customers.js.coffee b/app/assets/javascripts/admin/customers/customers.js.coffee
index aa8a1cfc19..fe8ae1de5b 100644
--- a/app/assets/javascripts/admin/customers/customers.js.coffee
+++ b/app/assets/javascripts/admin/customers/customers.js.coffee
@@ -1 +1 @@
-angular.module("admin.customers", ['ngResource', 'admin.tagRules', 'admin.indexUtils', 'admin.utils', 'admin.dropdown', 'ofn.admin'])
+angular.module("admin.customers", ['ngResource', 'admin.tagRules', 'admin.indexUtils', 'admin.utils', 'admin.dropdown'])
diff --git a/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee b/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee
index 066552b598..9e2c3d5696 100644
--- a/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee
+++ b/app/assets/javascripts/admin/subscriptions/subscriptions.js.coffee
@@ -1 +1 @@
-angular.module("admin.subscriptions", ['ngResource','admin.indexUtils','admin.dropdown', 'ofn.admin'])
+angular.module("admin.subscriptions", ['ngResource','admin.indexUtils','admin.dropdown'])
diff --git a/app/assets/javascripts/admin/services/country_states.js.coffee b/app/assets/javascripts/admin/utils/services/country_states.js.coffee
similarity index 84%
rename from app/assets/javascripts/admin/services/country_states.js.coffee
rename to app/assets/javascripts/admin/utils/services/country_states.js.coffee
index 7977bc3535..30161b5bb0 100644
--- a/app/assets/javascripts/admin/services/country_states.js.coffee
+++ b/app/assets/javascripts/admin/utils/services/country_states.js.coffee
@@ -1,4 +1,4 @@
-angular.module("ofn.admin").factory "CountryStates", ($filter) ->
+angular.module("admin.utils").factory "CountryStates", ($filter) ->
   new class CountryStates
 
     statesFor: (countries, country_id) ->
diff --git a/spec/javascripts/unit/admin/services/country_states_spec.js.coffee b/spec/javascripts/unit/admin/utils/services/country_states_spec.js.coffee
similarity index 98%
rename from spec/javascripts/unit/admin/services/country_states_spec.js.coffee
rename to spec/javascripts/unit/admin/utils/services/country_states_spec.js.coffee
index 5b3683b9d7..6e5247cdb7 100644
--- a/spec/javascripts/unit/admin/services/country_states_spec.js.coffee
+++ b/spec/javascripts/unit/admin/utils/services/country_states_spec.js.coffee
@@ -10,7 +10,7 @@ describe "CountryStates service", ->
   ]
 
   beforeEach ->
-    module('ofn.admin')
+    module('admin.utils')
     inject (CountryStates) ->
       countryStates = CountryStates