Merge pull request #2876 from luisramos0/subs-states-fix

Fix bug in subscriptions address controller where country states were not correctly returned

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,10 @@ 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)
+      unless CountryStates.addressStateMatchesCountryStates(scope.states, scope.address.state_id)
+        scope.address.state_id = ""
 
     scope.updateAddress = ->
       scope.edit_address_form.$setPristine()
@@ -27,19 +28,9 @@ 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

app/assets/javascripts/admin/subscriptions/controllers/address_controller.js.coffee

@@ -1,10 +1,20 @@
-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
-    $filter('filter')(availableCountries, {id: country_id})[0].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', (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', (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
@@ -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?

app/assets/javascripts/admin/utils/services/country_states.js.coffee

@@ -0,0 +1,11 @@
+angular.module("admin.utils").factory "CountryStates", ($filter) ->
+  new class CountryStates
+
+    statesFor: (countries, country_id) ->
+      return [] unless country_id
+      country = $filter('filter')(countries, {id: parseInt(country_id)}, true)[0]
+      return [] unless country
+      country.states
+
+    addressStateMatchesCountryStates: (countryStates, stateId) ->
+      countryStates.some (state) -> state.id == stateId

spec/javascripts/unit/admin/utils/services/country_states_spec.js.coffee

@@ -0,0 +1,34 @@
+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"}]
+  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.utils')
+    inject (CountryStates) ->
+      countryStates = CountryStates
+
+  describe "statesFor", ->
+    it "returns empty array for nil country id", ->
+      expect(countryStates.statesFor(availableCountries, null)).toEqual []
+
+    it "returns empty array for country id not in availableCountries", ->
+      expect(countryStates.statesFor(availableCountries, 10)).toEqual []
+
+    it "returns empty array for country id in availableCountries but without states", ->
+      expect(countryStates.statesFor(availableCountries, 9)).toEqual []
+
+    it "returns states for country id in availableCountries with states", ->
+      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(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(countryStates.statesFor(availableCountries, 19)).toEqual states_in_portugal