Code review comment for lp:~chrismcginlay/ubuntuone-control-panel/fixes_715820

Revision history for this message
Natalia Bidart (nataliabidart) wrote :

Chris,

Thanks for working on this! The branch is good to go except for a few things that need fixing:

* the adding of tooltip=None in _update_status should be the last argument, to maintain compatibility of the function API.

* for completeness sake, you should add tooltips (and the matching tests) for the rest of the buttons (ENABLE, RESTART, START, STOP). See below for proper strings to use as tooltips.

* tooltip strings should be (always start with an action since is a button tooltip):

    CONNECT_TOOLTIP = _('Connect the file sync service with your personal cloud.')
    DISCONNECT_TOOLTIP = _('Disconnect the file sync service from your personal cloud.')
    ENABLE_TOOLTIP = _('Enable the file sync service.')
    RESTART_TOOLTIP = _('Restart the file sync service.')
    START_TOOLTIP = _('Start the file sync service.')
    STOP_TOOLTIP = _('Stop the file sync service.')

    DASHBOARD_BUTTON_TOOLTIP = _('View your personal details and service summary.')
    DEVICES_BUTTON_TOOLTIP = _('Manage devices registered with your personal cloud.')
    VOLUMES_BUTTON_TOOLTIP = _('Manage your cloud folders.')

* fix a couple of docstrings to be PEP-257 compliant:

        """The file sync status is disconnected.
        * The correct connection status is displayed.
        * The button has a tooltip.
        * The correct connect tooltip is set.

        """

should be

        """The file sync status is disconnected.

        * The correct connection status is displayed.
        * The button has a tooltip.
        * The correct connect tooltip is set.

        """

(same for docstring in test_on_file_sync_status_syncing).

* Instead of duplicate the adding of the assertions

        self.assertTrue(self.ui.button.get_has_tooltip())
        self.assertEqual(self.ui.button.get_tooltip_text(),
                         self.ui.DISCONNECT_TOOLTIP)

modify assert_status_correct to receive the tooltip and assert over it in only one place.

One again, thanks a lot! This is pretty close to be landed.

review: Needs Fixing

« Back to merge proposal