Code review comment for lp:~diegosarmentero/ubuntuone-windows-installer/setup-buttons

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

* Please call super() instead of QtGui.QCheckBox.__init__(self).

* Why re you removing the overlay shown when the setup button is clicked?

        self.setup_account.ui.set_up_button.clicked.connect(
            self.overlay.show)

* Since we're building strings with the hope they get translated, we always try to use formatting with keywords. So, instead of this

"By signing up to Ubuntu One you agree to our {0} and {1}"

we should use:

"By signing up to Ubuntu One you agree to our {terms_and_conditions} and {privacy_policy}"

and then call format like this:

terms = TERMS.format(terms_and_conditions=TERMS_LINK,
                     privacy_policy=PRIVACY_POLICY_LINK)
self.terms_checkbox = enhanced_check_box.EnhancedCheckBox(terms)

* Why are you calling

         self.terms_checkbox.setText(
            TERMS.format(TERMS_LINK, PRIVACY_POLICY_LINK)

again in initializePage? The terms legend was set on __init__, is there any specific issue that requires setting the text again?

* In the test_overlay_connection_setup_account, can you call self.ui.set_up_button.clicked.emit(False) instead of self.ui.button(QtGui.QWizard.CustomButton3).clicked.emit(False)?

* If you're patching self.patch(self.ui, 'wizard', FakeWizard) in every test method in SetupAccountTestCase, please add that to the setUp method to avoid duplication.

Great work on the tests for the new widget!

review: Needs Fixing

« Back to merge proposal