Code review comment for lp:~ralsina/ubuntuone-control-panel/installer-option

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

* I wonder why you added a new method start_from_license instead of redefining setStartId in the QWizard. Any reason not to do that?

* I think that the test_first_page should not be removed, it may need some tweaks so it remains as:

    def test_first_page(self):
        """The first page is the correct one."""
        expected = self.ui.pages[self.ui.signin_page]
        self.assertEqual(self.ui.startId(), expected)

* Can you please fix the indentation of test_start_from_license so the whole 79-columns width are used? Something like (the following may need tweaking if we move to setStartId):

    def test_start_from_license(self):
        """Test the start_from_license method."""
        # Before, we start on sign_in
        assert self.ui.startId() == self.ui.pages[self.ui.signin_page]

        # After calling start_from_license, we start on license_page
        self.ui.start_from_license()
        self.assertEqual(self.ui.startId(), self.ui.pages[self.ui.license_page])

* Why did you need to change the page_id of the buttons dict in UbuntuOneWizardCloudToComputerTestCase, UbuntuOneWizardComputerToCloudTestCase and UbuntuOneWizardSettingsTestCase?

* Why the test_buttons_behavior is skipped for UbuntuOneWizardLicensePage? I see the comment you added but that does not explain to me why you need to redefine/skip it instead of making it pass. Also, why does the NextButton need to be a CommitButton?

* If we're leaving the license 'next' button to be a commit button, we don't need to store the string text of the Next button, so that can be safely removed.

review: Needs Fixing

« Back to merge proposal