Inconsistent logic for Pause/Suspend action buttons

Bug #925395 reported by Tihomir Trifonov on 2012-02-02
6
This bug affects 1 person
Affects Status Importance Assigned to Milestone
OpenStack Dashboard (Horizon)
Low
Tihomir Trifonov

Bug Description

It seems something was changed in the BatchAction logic.

Here is the code for TogglePause(the same is for ToggleSuspend):

class TogglePause(tables.BatchAction):
    name = "pause"
    action_present = _("Pause")
    action_past = _("Paused")
    data_type_singular = _("Instance")
    data_type_plural = _("Instances")

    def allowed(self, request, instance=None):
        if not instance:
            return True
        self.paused = instance.status == "PAUSED"
        if self.paused:
            self.action_present = _("Unpause")
            self.action_past = _("Unpaused")
        return instance.status in ACTIVE_STATES

    def action(self, request, obj_id):
        if getattr(self, 'paused', False):
            api.server_pause(request, obj_id)
        else:
            api.server_unpause(request, obj_id)

1.
def allowed(..):
    self.action_present = _("Unpause")

As seems in code, action_present is only used in __init__() of base class, and changing it in allowed() method has no effect on the final result in UI.
In the base class has a method called def update(), which is supposed to handle cases like toggle buttons. But it is not used here.
Also, allowed() returns True if instance is None ? As long as there is a valid VM instance, this variable will be never None.
And return instance.status in ACTIVE_STATES - will return true only if the status is ACTIVE. And False if it is 'PAUSED'. So we rename the button label to 'Unpause', but it will be never displayed.

2. def action(..)

        if getattr(self, 'paused', False):
            api.server_pause(request, obj_id)

According to me that means - if self.instance == "PAUSED" then server_pause() ? Should'n be it the opposite?

3. self.action_past - should be toggled after server_unpause() is called as it is dependant on the action.

Attached is a proposed fix for that. If you find it useful, I can commit and create a review with it.

Tihomir Trifonov (ttrifonov) wrote :
Devin Carlen (devcamcar) on 2012-02-02
Changed in horizon:
status: New → Confirmed
importance: Undecided → Low
milestone: none → essex-4
Changed in horizon:
assignee: nobody → Tihomir Trifonov (ttrifonov)

Fix proposed to branch: master
Review: https://review.openstack.org/3697

Changed in horizon:
status: Confirmed → In Progress

Fix proposed to branch: master
Review: https://review.openstack.org/3723

Reviewed: https://review.openstack.org/3723
Committed: http://github.com/openstack/horizon/commit/7369311aad6c22c6dbdf77c6a6f6a3c6e09c98ae
Submitter: Jenkins
Branch: master

commit 7369311aad6c22c6dbdf77c6a6f6a3c6e09c98ae
Author: Tihomir Trifonov <email address hidden>
Date: Fri Feb 3 01:45:08 2012 +0200

    Fixes logic for toggle Pause/Suspend actions
    Fixes bug 925395. Added functionality in BatchAction to support
    multiple actions. The verbose_names are accordingly changed
    in update method. It is only required that the current
    action index is set in the control.

    Patch 2: Applied code changes to newly added classes

    Patch 3: Refactored logic, added unit tests for batch actions and for verbose_names on all actions

    Patch 4: Resolved conflicts with newly added code in horizon/horizon/tests/table_tests.py

    Change-Id: I7cbdce25b8ae027ff1a153b3379add1b66b8aa76

Changed in horizon:
status: In Progress → Fix Committed
Thierry Carrez (ttx) on 2012-02-29
Changed in horizon:
status: Fix Committed → Fix Released
Thierry Carrez (ttx) on 2012-04-05
Changed in horizon:
milestone: essex-4 → 2012.1
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers