Fix migration-start id-dependent form

Share migration migration-start form was requiring the IDs
to be inputted for the new_share_type and new_share_network
fields. The main problem was finding the ID for the share type.

This fix redesigns the form to include dropdown lists instead of
text fields, allowing the share type and share network to be
selected from a list instead of having to input their IDs.

Closes-bug: #1626780
Change-Id: I60d00d3e3beb994f46db3543b59fed767b1823cb

manila_ui/dashboards/admin/shares/forms.py

@@ -14,6 +14,7 @@
 #    under the License.
 
 from django.conf import settings
+from django.core.urlresolvers import reverse
 from django.forms import ValidationError  # noqa
 from django.utils.translation import ugettext_lazy as _
 from oslo_utils import strutils
@@ -71,16 +72,26 @@ class MigrationStart(forms.SelfHandlingForm):
         help_text=("Defines whether this share should have all its file "
                    "metadata preserved during migration. If set so, this will "
                    "prevent the use of the Data Service for migration."))
-    new_share_network_id = forms.CharField(
+    new_share_network = forms.ChoiceField(
-        max_length=255, label=_("ID of share network to be set in migrated "
+        label=_("New share network to be set in migrated share"),
-                                "share"), required=False,
+        required=False,
-        help_text=_("Input the ID of the share network where the share should"
+        help_text=_("Input the new share network where the share should"
-                    " be migrated to."))
+                    " be migrated to if you would like to change it."))
-    new_share_type_id = forms.CharField(
+    new_share_type = forms.ChoiceField(
-        max_length=255, label=_("ID of share type to be set in migrating "
+        label=_("New share type to be set in migrating share"), required=False,
-                                "share"), required=False,
+        help_text=_("Input the new share type which the migrating share will "
-        help_text=_("Input the ID of the share type which the migrating share"
+                    "be set to if you would like to change the type."))
-                    " will be set to."))
+
+    def __init__(self, request, *args, **kwargs):
+        super(MigrationStart, self).__init__(request, *args, **kwargs)
+        share_networks = manila.share_network_list(request)
+        share_types = manila.share_type_list(request)
+        st_choices = [('', '')] + [(st.id, st.name) for st in share_types]
+        sn_choices = (
+            [('', '')] +
+            [(sn.id, sn.name or sn.id) for sn in share_networks])
+        self.fields['new_share_type'].choices = st_choices
+        self.fields['new_share_network'].choices = sn_choices
 
     def handle(self, request, data):
         share_name = _get_id_if_name_empty(data)
@@ -93,8 +104,8 @@ class MigrationStart(forms.SelfHandlingForm):
                 preserve_metadata=data['preserve_metadata'],
                 nondisruptive=data['nondisruptive'],
                 dest_host=data['host'],
-                new_share_network_id=data['new_share_network_id'],
+                new_share_network_id=data['new_share_network'],
-                new_share_type_id=data['new_share_type_id'])
+                new_share_type_id=data['new_share_type'])
 
             messages.success(
                 request,
@@ -102,8 +113,10 @@ class MigrationStart(forms.SelfHandlingForm):
                 % share_name)
             return True
         except Exception:
-            exceptions.handle(request, _("Unable to migrate share %s.")
+            redirect = reverse("horizon:admin:shares:index")
-                              % share_name)
+            exceptions.handle(
+                request, _("Unable to migrate share %s.") % share_name,
+                redirect=redirect)
         return False
 
 

manila_ui/tests/dashboards/admin/shares/test_forms.py

@@ -273,6 +273,16 @@ class ManilaDashboardsAdminMigrationFormTests(base.APITestCase):
     @mock.patch('horizon.messages.success')
     def test_migration_start(self, mock_horizon_messages_success):
 
+        self.mock_object(
+            api, "share_network_list",
+            mock.Mock(return_value=[base.FakeEntity('sn1_id', 'sn1_name'),
+                                    base.FakeEntity('sn2_id', 'sn2_name')]))
+
+        self.mock_object(
+            api, "share_type_list",
+            mock.Mock(return_value=[base.FakeEntity('st1_id', 'st1_name'),
+                                    base.FakeEntity('st2_id', 'st2_name')]))
+
         form = forms.MigrationStart(self.request, **self._get_initial())
 
         data = {
@@ -280,8 +290,8 @@ class ManilaDashboardsAdminMigrationFormTests(base.APITestCase):
             'writable': True,
             'preserve_metadata': True,
             'nondisruptive': False,
-            'new_share_network_id': 'fake_net_id',
+            'new_share_network': 'fake_net_id',
-            'new_share_type_id': 'fake_type_id',
+            'new_share_type': 'fake_type_id',
             'host': 'fake_host',
         }
 
@@ -290,6 +300,9 @@ class ManilaDashboardsAdminMigrationFormTests(base.APITestCase):
         mock_horizon_messages_success.assert_called_once_with(
             self.request, mock.ANY)
 
+        api.share_network_list.assert_called_once_with(mock.ANY)
+        api.share_type_list.assert_called_once_with(mock.ANY)
+
     @mock.patch('horizon.messages.success')
     def test_migration_complete(self, mock_horizon_messages_success):
 

manila_ui/tests/dashboards/admin/shares/tests.py

@@ -137,6 +137,11 @@ class SharesTests(test.BaseAdminViewTests):
         share = test_data.share
         url = reverse('horizon:admin:shares:migration_start',
                       args=[share.id])
+        sn_choices = [test.FakeEntity('sn1_id', 'sn1_name'),
+                      test.FakeEntity('sn2_id', 'sn2_name')]
+
+        st_choices = [test.FakeEntity('st1_id', 'st1_name'),
+                      test.FakeEntity('st2_id', 'st2_name')]
         formData = {
             'share_id': share.id,
             'name': share.name,
@@ -145,14 +150,19 @@ class SharesTests(test.BaseAdminViewTests):
             'preserve_metadata': True,
             'force_host_assisted_migration': True,
             'nondisruptive': True,
-            'new_share_network_id': 'fake_net_id',
+            'new_share_network': 'sn2_id',
-            'new_share_type_id': 'fake_type_id',
+            'new_share_type': 'st2_id',
         }
-
         self.mock_object(
             api_manila, "share_get", mock.Mock(return_value=share))
-        self.mock_object(api_manila, "migration_start", mock.Mock(
+        self.mock_object(
-            side_effect=exc))
+            api_manila, "migration_start", mock.Mock(side_effect=exc))
+        self.mock_object(
+            api_manila, "share_network_list",
+            mock.Mock(return_value=sn_choices))
+        self.mock_object(
+            api_manila, "share_type_list",
+            mock.Mock(return_value=st_choices))
 
         res = self.client.post(url, formData)
 
@@ -165,17 +175,13 @@ class SharesTests(test.BaseAdminViewTests):
             writable=formData['writable'],
             preserve_metadata=formData['preserve_metadata'],
             nondisruptive=formData['nondisruptive'],
-            new_share_network_id=formData['new_share_network_id'],
+            new_share_network_id=formData['new_share_network'],
-            new_share_type_id=formData['new_share_type_id'])
+            new_share_type_id=formData['new_share_type'])
-
+        api_manila.share_network_list.assert_called_once_with(mock.ANY)
-        status_code = 200 if exc else 302
+        api_manila.share_type_list.assert_called_once_with(mock.ANY)
-        self.assertEqual(res.status_code, status_code)
+        self.assertEqual(302, res.status_code)
-        if not exc:
+        self.assertTemplateNotUsed(res, 'admin/shares/migration_start.html')
-            self.assertTemplateNotUsed(
-                res, 'admin/shares/migration_start.html')
         self.assertRedirectsNoFollow(res, INDEX_URL)
-        else:
-            self.assertTemplateUsed(res, 'admin/shares/migration_start.html')
 
     @ddt.data('migration_start', 'migration_cancel', 'migration_complete',
               'migration_get_progress')
@@ -187,6 +193,19 @@ class SharesTests(test.BaseAdminViewTests):
             api_manila, "share_get", mock.Mock(return_value=share))
         self.mock_object(api_manila, method)
 
+        if method == 'migration_start':
+            self.mock_object(
+                api_manila, "share_network_list",
+                mock.Mock(
+                    return_value=[test.FakeEntity('sn1_id', 'sn1_name'),
+                                  test.FakeEntity('sn2_id', 'sn2_name')]))
+
+            self.mock_object(
+                api_manila, "share_type_list",
+                mock.Mock(
+                    return_value=[test.FakeEntity('st1_id', 'st1_name'),
+                                  test.FakeEntity('st2_id', 'st2_name')]))
+
         res = self.client.get(url)
 
         api_manila.share_get.assert_called_once_with(mock.ANY, share.id)
@@ -196,6 +215,10 @@ class SharesTests(test.BaseAdminViewTests):
         self.assertEqual(res.status_code, 200)
         self.assertTemplateUsed(res, 'admin/shares/' + method + '.html')
 
+        if method == 'migration_start':
+            api_manila.share_network_list.assert_called_once_with(mock.ANY)
+            api_manila.share_type_list.assert_called_once_with(mock.ANY)
+
     @ddt.data('migration_start', 'migration_cancel', 'migration_complete',
               'migration_get_progress')
     def test_migration_start_get_share_exception(self, method):

manila_ui/tests/helpers.py

@@ -59,3 +59,9 @@ class APITestCase(ManilaTestsMixin, helpers.APITestCase):
         super(APITestCase, self).setUp()
         self._manilaclient = self.mock_object(api.manila, "manilaclient")
         self.manilaclient = self._manilaclient.return_value
+
+
+class FakeEntity(object):
+    def __init__(self, id, name):
+        self.id = id
+        self.name = name