Merge lp:~ralsina/ubuntuone-control-panel/installer-option into lp:ubuntuone-control-panel

Proposed by Roberto Alsina
Status: Merged
Approved by: Roberto Alsina
Approved revision: 308
Merged at revision: 295
Proposed branch: lp:~ralsina/ubuntuone-control-panel/installer-option
Merge into: lp:ubuntuone-control-panel
Prerequisite: lp:~nataliabidart/ubuntuone-control-panel/license-page
Diff against target: 335 lines (+62/-45)
8 files modified
ubuntuone/controlpanel/gui/qt/controlpanel.py (+8/-0)
ubuntuone/controlpanel/gui/qt/gui.py (+8/-4)
ubuntuone/controlpanel/gui/qt/main/__init__.py (+6/-1)
ubuntuone/controlpanel/gui/qt/main/tests/test_main.py (+9/-3)
ubuntuone/controlpanel/gui/qt/tests/test_controlpanel.py (+10/-0)
ubuntuone/controlpanel/gui/qt/tests/test_start.py (+1/-1)
ubuntuone/controlpanel/gui/qt/tests/test_wizard.py (+11/-23)
ubuntuone/controlpanel/gui/qt/wizard.py (+9/-13)
To merge this branch: bzr merge lp:~ralsina/ubuntuone-control-panel/installer-option
Reviewer Review Type Date Requested Status
Natalia Bidart Approve
Diego Sarmentero (community) Approve
Review via email: mp+98503@code.launchpad.net

Commit message

- Made the license page from the wizard be shown (only when called with --installer) (LP: #933697).

Description of the change

- Added the license page to the wizard (only when called with --installer) (LP: #933697).

To test IRL:

* remove credentials
* If you start u1cp, you will get the "sign in/sign up" page
* Start u1cp again with the --installer option, and you should get a license page, for which the "next" page is "signin / signup"

To post a comment you must log in.
Revision history for this message
Diego Sarmentero (diegosarmentero) wrote :

+1

review: Approve
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
Revision history for this message
Roberto Alsina (ralsina) 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?

Moed start_from_license into the controlpanel as suggested.

>
> * 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)

That was already covered by the new test_start_from_license test (had that exact assert).
Readded this test now since start_from_license moved, and moved the test for start_from_license
to control panel tests.

> * 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])

Ermmmm I like the way I wrote it? It's wider now anyway because the names are longer.

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

Because they didn't work. Those are page IDs, and since now the license page is
added as first page, all those tests failed because they IDs moved.

> * 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?

It has to be a CommitButton so the user can't go back to the license page.
The CommitButton doesn't trigger the same signals as NextButton, and can't find
anywhere in the docs what we may expect there. I changed the test slightly so that
we can check the text of the buttons even if they don't trigger anything by
passing None as signal name.

> * 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.

Can't remove it, causes errors later on. Moved it so it doesn't appear to be part
of the license_page setup.

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

> > * 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.
>
> Can't remove it, causes errors later on. Moved it so it doesn't appear to be
> part
> of the license_page setup.

The line:

self.next_button_text = self.button(self.NextButton).text()

was added by me so we could restore the text in the NextButton in another page, since before we were cutomizing the text in the NextButton for the license page.

As how this branch is, the NextButton text is never changed, so there is no need to keep that hack in place. You can indeed remove it, but you have to removed from all the methods that is being used on (I tried this myself).

Thanks!

review: Needs Fixing
Revision history for this message
Roberto Alsina (ralsina) wrote :

> As how this branch is, the NextButton text is never changed, so there is no
> need to keep that hack in place. You can indeed remove it, but you have to
> removed from all the methods that is being used on (I tried this myself).

Ok, removing it everywhere in revno 308

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

Looks great! Also works as expected.

review: Approve
Revision history for this message
Ubuntu One Auto Pilot (otto-pilot) wrote :
Download full text (94.2 KiB)

The attempt to merge lp:~ralsina/ubuntuone-control-panel/installer-option into lp:ubuntuone-control-panel failed. Below is the output from the failed tests.

*** Running test suite for ubuntuone/controlpanel ***
ubuntuone.controlpanel.utils.tests.test_utils
  ExceptionHandlingTestCase
    test_exception_to_error_dict ... [OK]
    test_failure_to_error_dict ... [OK]
  GetDataFileTestCase
    test_get_data_file ... [OK]
  GetProjectDirTestCase
    test_get_project_dir_none_exists ... [OK]
    test_get_project_dir_relative ... [OK]
  GetProjectDirWithConstantsTestCase
    test_get_project_dir ... [OK]
    test_get_project_dir_none_exists ... [OK]
    test_get_project_dir_relative ... [OK]
ubuntuone.controlpanel.tests
  TestCase
    runTest ... [OK]
ubuntuone.controlpanel.utils.tests.test_linux
  AutoupdaterTestCase
    test_are_updates_present ... [OK]
    test_perform_update ... [OK]
  DefaultFoldersTestCase
    test_default_folders_bad_encoding ... [OK]
    test_default_folders_empty_file ... [OK]
    test_default_folders_non_ascii ... [OK]
    test_default_folders_not_file ... [OK]
    test_default_folders_only_comments ... [OK]
    test_default_folders_parsed ... [OK]
    test_default_folders_syntax_error ... [OK]
ubuntuone.controlpanel.tests
  TestCase
    runTest ... [OK]
ubuntuone.controlpanel.tests.test_replication_client
  ReplicationsTestCase
    test_exclude ... [OK]
    test_exclude_name_in_exclusions ... [OK]
    test_exclude_name_not_in_replications ... [OK]
    test_get_exclusions ... [OK]
    test_get_replications ... [OK]
    test_no_pairing_record ... [OK]
    test_replicate ... [OK]
    test_replicate_name_not_in_exclusions ... [OK]
    test_replicate_name_not_in_replications ... [OK]
ubuntuone.controlpanel.tests
  TestCase
    runTest ... [OK]
ubuntuone.controlpanel.tests.test_sd_client
  AutoconnectTestCase
    test_disable ... [OK]
    test_disable_throws_an_error ... [...

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'ubuntuone/controlpanel/gui/qt/controlpanel.py'
2--- ubuntuone/controlpanel/gui/qt/controlpanel.py 2012-03-19 20:55:52 +0000
3+++ ubuntuone/controlpanel/gui/qt/controlpanel.py 2012-03-22 12:49:18 +0000
4@@ -170,3 +170,11 @@
5 def on_wizard_finished(self, status):
6 """Move to controlpanel if wizard ended successfully."""
7 self.on_credentials_found()
8+
9+ @log_call(logger.info)
10+ def start_from_license(self):
11+ """Use the license page as first page."""
12+ # license
13+ self.ui.wizard.setStartId(self.ui.wizard.pages[
14+ self.ui.wizard.license_page])
15+ self.ui.wizard.restart()
16
17=== modified file 'ubuntuone/controlpanel/gui/qt/gui.py'
18--- ubuntuone/controlpanel/gui/qt/gui.py 2012-03-06 21:33:23 +0000
19+++ ubuntuone/controlpanel/gui/qt/gui.py 2012-03-22 12:49:18 +0000
20@@ -35,7 +35,7 @@
21 class MainWindow(QtGui.QMainWindow):
22 """The Main Window of the Control Panel."""
23
24- def __init__(self, close_callback=None):
25+ def __init__(self, close_callback=None, installer=False):
26 """Initialize this instance with the UI layout."""
27 super(MainWindow, self).__init__()
28 self.ui = mainwindow_ui.Ui_MainWindow()
29@@ -46,6 +46,9 @@
30 triggered=self.close)
31 self.quit_action.setShortcuts(["Ctrl+q", "Ctrl+w"])
32 self.addAction(self.quit_action)
33+ self.installer = installer
34+ if installer:
35+ self.ui.control_panel.start_from_license()
36 if USE_LIBUNITY:
37 self.entry = Unity.LauncherEntry.get_for_desktop_id(U1_DOTDESKTOP)
38 else:
39@@ -84,14 +87,15 @@
40 # pylint: enable=C0103
41
42
43-def start(close_callback, minimized=False, with_icon=False):
44+def start(close_callback, minimized=False, with_icon=False, installer=False):
45 """Show the UI elements."""
46 # pylint: disable=W0404, F0401
47 if not minimized:
48 if with_icon or minimized:
49- window = MainWindow()
50+ window = MainWindow(installer=installer)
51 else:
52- window = MainWindow(close_callback=close_callback)
53+ window = MainWindow(close_callback=close_callback,
54+ installer=installer)
55 app = QtGui.QApplication.instance()
56 style = QtGui.QStyle.alignedRect(
57 QtCore.Qt.LeftToRight, QtCore.Qt.AlignCenter,
58
59=== modified file 'ubuntuone/controlpanel/gui/qt/main/__init__.py'
60--- ubuntuone/controlpanel/gui/qt/main/__init__.py 2012-03-16 21:09:40 +0000
61+++ ubuntuone/controlpanel/gui/qt/main/__init__.py 2012-03-22 12:49:18 +0000
62@@ -55,6 +55,9 @@
63 default=False,
64 help="Start Ubuntu One "
65 "with an icon in the notification area.")
66+ result.add_argument("--installer", dest="installer", action="store_true",
67+ default=False,
68+ help="Show the license agreement page first.")
69 return result
70
71
72@@ -82,6 +85,7 @@
73 switch_to = args.switch_to
74 minimized = args.minimized
75 with_icon = args.with_icon
76+ installer = args.installer
77 source.main(app)
78
79 data = []
80@@ -92,7 +96,8 @@
81
82 # Unused variable 'window', 'icon', pylint: disable=W0612
83 icon, window = start(lambda: source.main_quit(app),
84- minimized=minimized, with_icon=with_icon)
85+ minimized=minimized, with_icon=with_icon,
86+ installer=installer)
87 window.switch_to(switch_to)
88 # pylint: enable=W0612
89 if icon:
90
91=== modified file 'ubuntuone/controlpanel/gui/qt/main/tests/test_main.py'
92--- ubuntuone/controlpanel/gui/qt/main/tests/test_main.py 2012-03-16 21:14:20 +0000
93+++ ubuntuone/controlpanel/gui/qt/main/tests/test_main.py 2012-03-22 12:49:18 +0000
94@@ -132,19 +132,19 @@
95 """Ensure the binary name is not given to argparse."""
96 main.main(["foo", "bar", sys.argv[0], "--minimized"])
97 self.assertEqual(self.start.args[1],
98- {'minimized': True, 'with_icon': False})
99+ {'minimized': True, 'with_icon': False, 'installer': False})
100
101 def test_minimized_option(self):
102 """Ensure the --minimized option is parsed and passed correctly."""
103 main.main([sys.argv[0], "--minimized"])
104 self.assertEqual(self.start.args[1],
105- {'minimized': True, 'with_icon': False})
106+ {'minimized': True, 'with_icon': False, 'installer': False})
107
108 def test_with_icon_option(self):
109 """Ensure the --minimized option is parsed and passed correctly."""
110 main.main([sys.argv[0], "--with-icon"])
111 self.assertEqual(self.start.args[1],
112- {'minimized': False, 'with_icon': True})
113+ {'minimized': False, 'with_icon': True, 'installer': False})
114
115 def test_all_styles_load(self):
116 """Ensure the platform style is loaded."""
117@@ -160,6 +160,12 @@
118 main.main([sys.argv[0], "--switch-to", "folders"])
119 self.assertEqual(self.start.window.tabname, "folders")
120
121+ def test_installer_option(self):
122+ """Ensure the --installer option is parsed and passed correctly."""
123+ main.main([sys.argv[0], "--installer"])
124+ self.assertEqual(self.start.args[1],
125+ {'minimized': False, 'with_icon': False, 'installer': True})
126+
127 def test_translator(self):
128 """Ensure the Qt translator is loaded."""
129 main.main([sys.argv[0]])
130
131=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_controlpanel.py'
132--- ubuntuone/controlpanel/gui/qt/tests/test_controlpanel.py 2012-03-19 15:50:31 +0000
133+++ ubuntuone/controlpanel/gui/qt/tests/test_controlpanel.py 2012-03-22 12:49:18 +0000
134@@ -186,6 +186,16 @@
135 remote = self.ui.ui.folders_tab.remote_folders
136 self.assertFalse(remote)
137
138+ def test_start_from_license(self):
139+ """Ensure we change the starting page correctly."""
140+ # Before, we start on sign_in
141+ self.assertEqual(self.ui.ui.wizard.startId(),
142+ self.ui.ui.wizard.pages[self.ui.ui.wizard.signin_page])
143+ # After, we start on license_page
144+ self.ui.start_from_license()
145+ self.assertEqual(self.ui.ui.wizard.startId(),
146+ self.ui.ui.wizard.pages[self.ui.ui.wizard.license_page])
147+
148
149 class ExternalLinkButtonsTestCase(ControlPanelTestCase):
150 """The link in the go-to-web buttons are correct."""
151
152=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_start.py'
153--- ubuntuone/controlpanel/gui/qt/tests/test_start.py 2012-02-17 14:42:57 +0000
154+++ ubuntuone/controlpanel/gui/qt/tests/test_start.py 2012-03-22 12:49:18 +0000
155@@ -78,7 +78,7 @@
156 gui.start(close_callback=self.close_cb,
157 with_icon=True, minimized=False)
158 kwargs = {'close_callback': self.close_cb, 'window': self.main_window}
159- self.assertEqual(self.main_window.args, [((), {})])
160+ self.assertEqual(self.main_window.args, [((), {'installer': False})])
161 self.assertEqual(self.tray_icon.args, [((), kwargs)])
162
163 def test_both_false(self):
164
165=== modified file 'ubuntuone/controlpanel/gui/qt/tests/test_wizard.py'
166--- ubuntuone/controlpanel/gui/qt/tests/test_wizard.py 2012-03-20 15:14:24 +0000
167+++ ubuntuone/controlpanel/gui/qt/tests/test_wizard.py 2012-03-22 12:49:18 +0000
168@@ -153,11 +153,9 @@
169
170 class_ui = gui.UbuntuOneWizard
171 confirm_response = gui.QtGui.QDialog.Accepted
172- show_license = False
173
174 @defer.inlineCallbacks
175 def setUp(self):
176- self.patch(self.class_ui, 'show_license', self.show_license)
177 yield super(UbuntuOneWizardTestCase, self).setUp()
178 self.patch(self.ui.confirm_dialog, 'exec_',
179 lambda: self.confirm_response)
180@@ -186,11 +184,7 @@
181
182 def test_first_page(self):
183 """The first page is the correct one."""
184- if self.show_license:
185- expected = self.ui.pages[self.ui.license_page]
186- else:
187- expected = self.ui.pages[self.ui.signin_page]
188-
189+ expected = self.ui.pages[self.ui.signin_page]
190 self.assertEqual(self.ui.startId(), expected)
191
192 def test_done_accepted(self):
193@@ -202,12 +196,6 @@
194 self.assertEqual(self._called, ((gui.QtGui.QDialog.Accepted,), {}))
195
196
197-class LicensedUbuntuOneWizardTestCase(UbuntuOneWizardTestCase):
198- """Test the LicensedUbuntuOneWizard."""
199-
200- show_license = True
201-
202-
203 class UbuntuOneWizardSignInTestCase(UbuntuOneWizardTestCase):
204 """Test the SignInPage wizard page."""
205
206@@ -267,12 +255,13 @@
207 if text is not None:
208 self.assertEqual(unicode(button.text()), text)
209
210- getattr(self.ui, signal).connect(self._set_called)
211- button.click()
212+ if signal:
213+ getattr(self.ui, signal).connect(self._set_called)
214+ button.click()
215
216- self.assertEqual(self._called, (signal_args, {}),
217- msg % (name, signal, signal_args))
218- self._called = False
219+ self.assertEqual(self._called, (signal_args, {}),
220+ msg % (name, signal, signal_args))
221+ self._called = False
222
223 def test_done_rejected(self):
224 """When the wizard was cancelled and user confirmed, finish."""
225@@ -300,7 +289,7 @@
226 class UbuntuOneWizardCloudToComputerTestCase(UbuntuOneWizardSignInTestCase):
227 """Test the CloudToComputerPage wizard page."""
228
229- buttons = {'NextButton': (None, 'currentIdChanged', (3,))}
230+ buttons = {'NextButton': (None, 'currentIdChanged', (4,))}
231 page_name = 'cloud_folders'
232 stage_name = 'folders'
233
234@@ -308,7 +297,7 @@
235 class UbuntuOneWizardSettingsTestCase(UbuntuOneWizardSignInTestCase):
236 """Test the CloudToComputerPage wizard page."""
237
238- buttons = {'BackButton': (None, 'currentIdChanged', (0,))}
239+ buttons = {'BackButton': (None, 'currentIdChanged', (1,))}
240 page_name = 'settings'
241 stage_name = 'folders'
242
243@@ -318,7 +307,7 @@
244
245 buttons = {
246 'FinishButton': (None, 'finished', (gui.QtGui.QDialog.Accepted,)),
247- 'BackButton': (None, 'currentIdChanged', (0,)),
248+ 'BackButton': (None, 'currentIdChanged', (1,)),
249 }
250 page_name = 'local_folders'
251 stage_name = 'sync'
252@@ -341,11 +330,10 @@
253 """Test the LicensePage wizard page."""
254
255 buttons = {
256- 'NextButton': (gui.LICENSE_AGREE, 'currentIdChanged', (1,)),
257+ 'CommitButton': (gui.LICENSE_AGREE, None, ()),
258 'CancelButton': (gui.LICENSE_DISAGREE, 'rejected', ()),
259 }
260 page_name = 'license'
261- show_license = True
262 stage_name = 'install'
263
264
265
266=== modified file 'ubuntuone/controlpanel/gui/qt/wizard.py'
267--- ubuntuone/controlpanel/gui/qt/wizard.py 2012-03-20 15:11:56 +0000
268+++ ubuntuone/controlpanel/gui/qt/wizard.py 2012-03-22 12:49:18 +0000
269@@ -167,8 +167,6 @@
270 class UbuntuOneWizard(cache.Cache, QtGui.QWizard):
271 """The Ubuntu One wizard."""
272
273- show_license = False # do not change unless you know what you're doing
274-
275 def __init__(self, *args, **kwargs):
276 super(UbuntuOneWizard, self).__init__(*args, **kwargs)
277 self.pages = {}
278@@ -185,9 +183,8 @@
279
280 # license
281 self.license_page = LicensePage()
282- self.next_button_text = self.button(self.NextButton).text()
283- if self.show_license:
284- self.addPage(self.license_page)
285+ self.license_page.setCommitPage(True)
286+ self.addPage(self.license_page)
287
288 # sign in
289 self.signin_page = SignInPage()
290@@ -213,7 +210,7 @@
291 self.addPage(self.local_folders_page)
292
293 self._next_id = self.pages[self.signin_page]
294- self.next()
295+ self.setStartId(self._next_id)
296
297 # pylint: disable=C0103
298
299@@ -231,12 +228,14 @@
300 button_layout = button_to = button = stage = None
301
302 if page is self.license_page:
303- button_layout = [self.Stretch, self.CancelButton, self.NextButton]
304- button = self.button(self.NextButton)
305+ button_layout = [self.Stretch, self.CancelButton,
306+ self.CommitButton]
307+ button = self.button(self.CommitButton)
308 button_to = self.button(self.CancelButton)
309 stage = self.side_widget.install_stage
310+ self._next_id = self.pages[self.signin_page]
311
312- self.setButtonText(self.NextButton, LICENSE_AGREE)
313+ self.setButtonText(self.CommitButton, LICENSE_AGREE)
314 self.setButtonText(self.CancelButton, LICENSE_DISAGREE)
315
316 elif page is self.signin_page:
317@@ -250,8 +249,6 @@
318 self.setTabOrder(self.signin_page.panel.ui.login_button, button)
319
320 elif page is self.cloud_folders_page:
321- self.setButtonText(self.NextButton, self.next_button_text)
322-
323 button_layout = [self.Stretch, self.NextButton]
324 button = self.cloud_folders_page.panel.ui.check_settings_button
325 button_to = self.button(self.NextButton)
326@@ -339,8 +336,7 @@
327 if response == QtGui.QDialog.Accepted:
328 logger.warning('UbuntuOneWizard: user canceled setup.')
329 self.rejected.emit()
330- elif (self.show_license and
331- self.currentId() == self.pages[self.license_page]):
332+ elif (self.currentId() == self.pages[self.license_page]):
333 response = self.confirm_dialog.exec_()
334 if response == QtGui.QDialog.Accepted:
335 logger.warning('UbuntuOneWizard: user wants to uninstall.')

Subscribers

People subscribed via source and target branches