From 3d82d57353d1a29fb5b568ffaa93db86998e9d94 Mon Sep 17 00:00:00 2001 From: Radomir Dopieralski Date: Tue, 16 Mar 2021 17:09:21 +0100 Subject: [PATCH] Save instace_id inside Associate Floating IP workflow Also, rename instance_id field to proper port_id. Before this change, when instance_id was passed from the Instances page in the URL, the information would be lost on form submission, and if the form contained an error, the it would be redisplayed with the port drop-down containing all ports from all instances. This also made submitting this form slow, as the drop-down would be populated with all ports on form validation. It also creates a bug, where if there are more instances than Nova's pagination allows, the ports from newer instances would no longer appear in the drop-down, and the form would fail to validate, even though the choice of port on initial form was correct. Closes-bug: #1920010 Change-Id: I3ab26c19dc9ea1ed23fcff790d0db919039099eb --- .../dashboards/project/floating_ips/tests.py | 6 ++--- .../project/floating_ips/workflows.py | 26 ++++++++++++------- .../pages/project/network/floatingipspage.py | 6 ++--- 3 files changed, 22 insertions(+), 16 deletions(-) diff --git a/openstack_dashboard/dashboards/project/floating_ips/tests.py b/openstack_dashboard/dashboards/project/floating_ips/tests.py index 0bedcd9507..eca92dcd88 100644 --- a/openstack_dashboard/dashboards/project/floating_ips/tests.py +++ b/openstack_dashboard/dashboards/project/floating_ips/tests.py @@ -139,7 +139,7 @@ class FloatingIpViewTests(test.TestCase): self._get_fip_targets() self.mock_floating_ip_associate.return_value = None - form_data = {'instance_id': port_target_id, + form_data = {'port_id': port_target_id, 'ip_id': floating_ip.id} url = reverse('%s:associate' % NAMESPACE) res = self.client.post(url, form_data) @@ -168,7 +168,7 @@ class FloatingIpViewTests(test.TestCase): self.mock_floating_ip_associate.return_value = None next = reverse("horizon:project:instances:index") - form_data = {'instance_id': port_target_id, + form_data = {'port_id': port_target_id, 'next': next, 'ip_id': floating_ip.id} url = reverse('%s:associate' % NAMESPACE) @@ -197,7 +197,7 @@ class FloatingIpViewTests(test.TestCase): self._get_fip_targets() self.mock_floating_ip_associate.side_effect = self.exceptions.nova - form_data = {'instance_id': port_target_id, + form_data = {'port_id': port_target_id, 'ip_id': floating_ip.id} url = reverse('%s:associate' % NAMESPACE) res = self.client.post(url, form_data) diff --git a/openstack_dashboard/dashboards/project/floating_ips/workflows.py b/openstack_dashboard/dashboards/project/floating_ips/workflows.py index a2307883bc..5f3ac9825e 100644 --- a/openstack_dashboard/dashboards/project/floating_ips/workflows.py +++ b/openstack_dashboard/dashboards/project/floating_ips/workflows.py @@ -36,7 +36,11 @@ class AssociateIPAction(workflows.Action): coerce=filters.get_int_or_uuid, empty_value=None ) - instance_id = forms.ThemableChoiceField( + instance_id = forms.CharField( + widget=forms.widgets.HiddenInput(), + required=False, + ) + port_id = forms.ThemableChoiceField( label=_("Port to be associated") ) @@ -53,7 +57,8 @@ class AssociateIPAction(workflows.Action): # an association target is not an instance but a port, so we need # to get an association target based on a received instance_id # and set the initial value of instance_id ChoiceField. - q_instance_id = self.request.GET.get('instance_id') + q_instance_id = self.data.get('instance_id', + self.request.GET.get('instance_id')) q_port_id = self.request.GET.get('port_id') if policy.check((("network", "create_floatingip"),), @@ -61,20 +66,21 @@ class AssociateIPAction(workflows.Action): self.fields['ip_id'].widget.add_item_link = ALLOCATE_URL if q_instance_id: + self.initial['instance_id'] = q_instance_id targets = self._get_target_list(q_instance_id) # Setting the initial value here is required to avoid a situation # where instance_id passed in the URL is used as the initial value # unexpectedly. (This always happens if the form is invoked from # the instance table.) if targets: - self.initial['instance_id'] = targets[0].id + self.initial['port_id'] = targets[0].id else: - self.initial['instance_id'] = '' + self.initial['port_id'] = '' elif q_port_id: targets = self._get_target_list() for target in targets: if target.port_id == q_port_id: - self.initial['instance_id'] = target.id + self.initial['port_id'] = target.id break def populate_ip_id_choices(self, request, context): @@ -112,9 +118,9 @@ class AssociateIPAction(workflows.Action): redirect=redirect) return targets - # TODO(amotoki): [drop-nova-network] Rename instance_id to port_id - def populate_instance_id_choices(self, request, context): - q_instance_id = self.request.GET.get('instance_id') + def populate_port_id_choices(self, request, context): + q_instance_id = self.data.get('instance_id', + self.initial.get('instance_id')) # The reason of specifying an empty tuple when q_instance_id is None # is to make memoized_method _get_target_list work. Two calls of # _get_target_list from here and __init__ must have a same arguments. @@ -132,7 +138,7 @@ class AssociateIPAction(workflows.Action): class AssociateIP(workflows.Step): action_class = AssociateIPAction - contributes = ("ip_id", "instance_id", "ip_address") + contributes = ("ip_id", "instance_id", "ip_address", "port_id") def contribute(self, data, context): context = super().contribute(data, context) @@ -162,7 +168,7 @@ class IPAssociationWorkflow(workflows.Workflow): try: api.neutron.floating_ip_associate(request, data['ip_id'], - data['instance_id']) + data['port_id']) except neutron_exc.Conflict: msg = _('The requested instance port is already' ' associated with another floating IP.') diff --git a/openstack_dashboard/test/integration_tests/pages/project/network/floatingipspage.py b/openstack_dashboard/test/integration_tests/pages/project/network/floatingipspage.py index dcfbd29925..180b68f405 100644 --- a/openstack_dashboard/test/integration_tests/pages/project/network/floatingipspage.py +++ b/openstack_dashboard/test/integration_tests/pages/project/network/floatingipspage.py @@ -25,7 +25,7 @@ from openstack_dashboard.test.integration_tests.regions import tables class FloatingIPTable(tables.TableRegion): name = 'floating_ips' FLOATING_IP_ASSOCIATIONS = ( - ("ip_id", "instance_id")) + ("ip_id", "port_id")) @tables.bind_table_action('allocate') def allocate_ip(self, allocate_button): @@ -93,8 +93,8 @@ class FloatingipsPage(basepage.BaseNavigationPage): instance_ip=None): row = self._get_row_with_floatingip(floatingip) floatingip_form = self.floatingips_table.associate_ip(row) - floatingip_form.instance_id.text = "{}: {}".format(instance_name, - instance_ip) + floatingip_form.port_id.text = "{}: {}".format(instance_name, + instance_ip) floatingip_form.submit() def disassociate_floatingip(self, floatingip):