From a9d6c7886c9d3d884b2b8d9dff0e0d20c1f96457 Mon Sep 17 00:00:00 2001 From: wangliangyu Date: Thu, 15 Nov 2018 10:20:04 +0800 Subject: [PATCH] Make update status dialog less confusing Now, we give the init status of the volume when resetting status. The first glance is the current status, instead of "Select an new status", and we could do nothing with the status will confuse user. The same with snapshot. According to the above situation and Akihiro's suggestion: - Use "Select on new status" as the default label - List all possible values for status *including the current status* - Add "(current)" to the current value in the dropdown list Change-Id: I61e642e2fd8ddd7498911272678e57b60b60a3b8 Closes-Bug: #1803475 --- .../dashboards/admin/snapshots/forms.py | 20 ++++++++++--------- .../dashboards/admin/snapshots/tests.py | 9 --------- .../dashboards/admin/volumes/forms.py | 8 +++----- .../dashboards/admin/volumes/tests.py | 10 ---------- 4 files changed, 14 insertions(+), 33 deletions(-) diff --git a/openstack_dashboard/dashboards/admin/snapshots/forms.py b/openstack_dashboard/dashboards/admin/snapshots/forms.py index 85bb082451..2a36057e99 100644 --- a/openstack_dashboard/dashboards/admin/snapshots/forms.py +++ b/openstack_dashboard/dashboards/admin/snapshots/forms.py @@ -33,23 +33,25 @@ STATUS_CHOICES = tuple( def populate_status_choices(current_status, status_choices): - status_choices = [status for status in status_choices - if status[0] != current_status] - status_choices.insert(0, ("", _("Select a new status"))) + choices = [] + for status in status_choices: + if status[0] == current_status: + choices.append((status[0], _("%s (current)") % status[1])) + else: + choices.append(status) - return status_choices + choices.insert(0, ("", _("Select a new status"))) + return choices class UpdateStatus(forms.SelfHandlingForm): status = forms.ThemableChoiceField(label=_("Status")) def __init__(self, request, *args, **kwargs): - # Obtain the localized status to use as initial value, has to be done - # before super() otherwise the initial value will get overwritten back - # to the raw value + # Initial values have to be operated before super() otherwise the + # initial values will get overwritten back to the raw value current_status = kwargs['initial']['status'] - choices = dict(STATUS_CHOICES) - kwargs['initial']['status'] = choices[current_status] + kwargs['initial'].pop('status') super(UpdateStatus, self).__init__(request, *args, **kwargs) diff --git a/openstack_dashboard/dashboards/admin/snapshots/tests.py b/openstack_dashboard/dashboards/admin/snapshots/tests.py index 3bcd69ae07..200045e182 100644 --- a/openstack_dashboard/dashboards/admin/snapshots/tests.py +++ b/openstack_dashboard/dashboards/admin/snapshots/tests.py @@ -20,7 +20,6 @@ from openstack_dashboard.api import cinder from openstack_dashboard.api import keystone from openstack_dashboard.test import helpers as test -from openstack_dashboard.dashboards.admin.snapshots import forms from openstack_dashboard.dashboards.admin.snapshots import tables INDEX_URL = 'horizon:admin:snapshots:index' @@ -223,14 +222,6 @@ class VolumeSnapshotsViewTests(test.BaseAdminViewTests): self.mock_volume_snapshot_get.assert_called_once_with( test.IsHttpRequest(), snapshot.id) - def test_get_snapshot_status_choices_without_current(self): - current_status = 'available' - status_choices = forms.populate_status_choices(current_status, - forms.STATUS_CHOICES) - self.assertEqual(len(status_choices), len(forms.STATUS_CHOICES)) - self.assertNotIn(current_status, - [status[0] for status in status_choices]) - @test.create_mocks({cinder: ('volume_snapshot_get',)}) def test_update_volume_status_get(self): snapshot = self.cinder_volume_snapshots.first() diff --git a/openstack_dashboard/dashboards/admin/volumes/forms.py b/openstack_dashboard/dashboards/admin/volumes/forms.py index e91bf194f0..1b4cd79293 100644 --- a/openstack_dashboard/dashboards/admin/volumes/forms.py +++ b/openstack_dashboard/dashboards/admin/volumes/forms.py @@ -217,12 +217,10 @@ class UpdateStatus(forms.SelfHandlingForm): status = forms.ThemableChoiceField(label=_("Status")) def __init__(self, request, *args, **kwargs): - # Obtain the localized status to use as initial value, has to be done - # before super() otherwise the initial value will get overwritten back - # to the raw value + # Initial values have to be operated before super() otherwise the + # initial values will get overwritten back to the raw value current_status = kwargs['initial']['status'] - choices = dict(STATUS_CHOICES) - kwargs['initial']['status'] = choices[current_status] + kwargs['initial'].pop('status') super(UpdateStatus, self).__init__(request, *args, **kwargs) diff --git a/openstack_dashboard/dashboards/admin/volumes/tests.py b/openstack_dashboard/dashboards/admin/volumes/tests.py index 1cc91bbf0d..3ada0ed55f 100644 --- a/openstack_dashboard/dashboards/admin/volumes/tests.py +++ b/openstack_dashboard/dashboards/admin/volumes/tests.py @@ -24,8 +24,6 @@ from openstack_dashboard.dashboards.project.volumes \ import tables as volume_tables from openstack_dashboard.test import helpers as test -from openstack_dashboard.dashboards.admin.snapshots import forms - DETAIL_URL = ('horizon:admin:volumes:detail') INDEX_URL = reverse('horizon:admin:volumes:index') @@ -365,14 +363,6 @@ class VolumeTests(test.BaseAdminViewTests): test.IsHttpRequest(), volume.id, host, False) self.assertRedirectsNoFollow(res, INDEX_URL) - def test_get_volume_status_choices_without_current(self): - current_status = 'available' - status_choices = forms.populate_status_choices(current_status, - forms.STATUS_CHOICES) - self.assertEqual(len(status_choices), len(forms.STATUS_CHOICES)) - self.assertNotIn(current_status, - [status[0] for status in status_choices]) - @test.create_mocks({api.cinder: ['volume_get']}) def test_update_volume_status_get(self): volume = self.cinder_volumes.get(name='v2_volume')