From bced12d63f3bac6c423a5b9c847910fc61c21039 Mon Sep 17 00:00:00 2001 From: Rodrigo Barbieri Date: Thu, 22 Sep 2016 19:52:34 -0300 Subject: [PATCH] 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 | 41 +++++++++----- .../dashboards/admin/shares/test_forms.py | 17 +++++- .../tests/dashboards/admin/shares/tests.py | 55 +++++++++++++------ manila_ui/tests/helpers.py | 6 ++ 4 files changed, 87 insertions(+), 32 deletions(-) diff --git a/manila_ui/dashboards/admin/shares/forms.py b/manila_ui/dashboards/admin/shares/forms.py index 14e4966d..8fc50115 100644 --- a/manila_ui/dashboards/admin/shares/forms.py +++ b/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( - max_length=255, label=_("ID of share network to be set in migrated " - "share"), required=False, - help_text=_("Input the ID of the share network where the share should" - " be migrated to.")) - new_share_type_id = forms.CharField( - max_length=255, label=_("ID of share type to be set in migrating " - "share"), required=False, - help_text=_("Input the ID of the share type which the migrating share" - " will be set to.")) + new_share_network = forms.ChoiceField( + label=_("New share network to be set in migrated share"), + required=False, + help_text=_("Input the new share network where the share should" + " be migrated to if you would like to change it.")) + new_share_type = forms.ChoiceField( + label=_("New share type to be set in migrating share"), required=False, + help_text=_("Input the new share type which the migrating share will " + "be set to if you would like to change the type.")) + + 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_type_id=data['new_share_type_id']) + new_share_network_id=data['new_share_network'], + 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.") - % share_name) + redirect = reverse("horizon:admin:shares:index") + exceptions.handle( + request, _("Unable to migrate share %s.") % share_name, + redirect=redirect) return False diff --git a/manila_ui/tests/dashboards/admin/shares/test_forms.py b/manila_ui/tests/dashboards/admin/shares/test_forms.py index e1511585..7be674f0 100644 --- a/manila_ui/tests/dashboards/admin/shares/test_forms.py +++ b/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_type_id': 'fake_type_id', + 'new_share_network': 'fake_net_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): diff --git a/manila_ui/tests/dashboards/admin/shares/tests.py b/manila_ui/tests/dashboards/admin/shares/tests.py index 880e5eed..a414db28 100644 --- a/manila_ui/tests/dashboards/admin/shares/tests.py +++ b/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_type_id': 'fake_type_id', + 'new_share_network': 'sn2_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( - side_effect=exc)) + self.mock_object( + 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_type_id=formData['new_share_type_id']) - - status_code = 200 if exc else 302 - self.assertEqual(res.status_code, status_code) - if not exc: - self.assertTemplateNotUsed( - res, 'admin/shares/migration_start.html') - self.assertRedirectsNoFollow(res, INDEX_URL) - else: - self.assertTemplateUsed(res, 'admin/shares/migration_start.html') + new_share_network_id=formData['new_share_network'], + new_share_type_id=formData['new_share_type']) + api_manila.share_network_list.assert_called_once_with(mock.ANY) + api_manila.share_type_list.assert_called_once_with(mock.ANY) + self.assertEqual(302, res.status_code) + self.assertTemplateNotUsed(res, 'admin/shares/migration_start.html') + self.assertRedirectsNoFollow(res, INDEX_URL) @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): diff --git a/manila_ui/tests/helpers.py b/manila_ui/tests/helpers.py index 29329542..38bca216 100644 --- a/manila_ui/tests/helpers.py +++ b/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