Merge lp:~gary-lasker/software-center/offline-opt-in-lp965397-for-quantal into lp:software-center

Proposed by Gary Lasker
Status: Merged
Merged at revision: 3074
Proposed branch: lp:~gary-lasker/software-center/offline-opt-in-lp965397-for-quantal
Merge into: lp:software-center
Diff against target: 356 lines (+138/-90)
3 files modified
softwarecenter/backend/recagent.py (+17/-1)
softwarecenter/ui/gtk3/widgets/recommendations.py (+56/-26)
tests/gtk3/test_catview.py (+65/-63)
To merge this branch: bzr merge lp:~gary-lasker/software-center/offline-opt-in-lp965397-for-quantal
Reviewer Review Type Date Requested Status
Michael Vogt Approve
Review via email: mp+116961@code.launchpad.net

Commit message

* lp:~gary-lasker/software-center/offline-opt-in-lp965397:
   - implement offline recommendations opt-in behavior as specified
     in the Ubuntu Software Center specification (LP: #965397)

Description of the change

This is targeted for trunk only currently as it includes a new string.

This branch implements the offline recommendations opt-in behavior as specified in bug 965397 (and the corresponding section of the referenced spec). Note that all of the combinations of turning on and off recommendations with network available and not available should do the right thing. Note also that state of the recommendations menu item is consistently updated as well.

Unit tests are included.

Oh, also please note that I re-enabled two of the recommendations unit tests that had been turned off. For me, these work fine now, so I'm not sure why they were disabled initially, and I wonder whether the condition that necessited that is now resolved. mvo, please check these to make sure these work for you as well.

Thanks very much for your review!

To post a comment you must log in.
Gary Lasker (gary-lasker) wrote :

I'll bring in the comments from the first MP for 5.2 (https://code.launchpad.net/~gary-lasker/software-center/offline-opt-in-lp965397/+merge/116780), that one has been superceded by this one:

Michael Vogt (mvo) wrote 9 hours ago:

On Wed, Jul 25, 2012 at 11:13:20PM -0000, Gary Lasker wrote:
> This branch implements the offline recommendations opt-in behavior as specified in bug 965397 (and the corresponding section of the referenced spec). Note that all of the combinations of turning on and off recommendations with network available and not available should do the right thing. Note also that state of the recommendations menu item is consistently updated as well.
>
> Unit tests are included.

Thanks for the branch. This looks good.

Could you please target trunk? We want to upload it there first as its
a string change so we can not land this in 5.2 as it is, it would
require coordination with the translators again. Plus when this hits
quantal we will get the translations for free so puting the fix
into 5.2 later will be easier.

I noticed that its using a static check if the network is available or
not but does not react to network state signal changes. I think that
would be really nice from a UI point-of-view. Right now if the user
has no network and opts in he sees the message that recommendations will
appear when he is online next. When he/she goes online then the
message stays there which looks a little bit confusing. It works fine
when I close/restart software-center. Or is there a mechanism for
reacting to network changes that I overlooked maybe?

> Oh, also please note that I re-enabled two of the recommendations unit tests that had been turned off. For me, these work fine now, so I'm not sure why they were disabled initially, and I wonder whether the condition that necessited that is now resolved. mvo, please check these to make sure these work for you as well.

Interessting, that works for me now as well, might be some sort of
race that is not always triggered. I will leave them enabled :)

Cheers,
 Michael

Gary Lasker (gary-lasker) wrote :

Hi Michael, thanks for your review! Indeed, this change is not suitable for SRU (at this point) because of the string change, so I'll retarget for trunk.

Regarding adding a listener for the network coming online, I think I will make that change in a separate branch and propose it separately.

Gary Lasker (gary-lasker) wrote :

I've added the network listener functionality and the merge proposal for that (which depends on this branch as a prerequisite) can be found here:

  https://code.launchpad.net/~gary-lasker/software-center/offline-network-events/+merge/117000

Thanks again!

Michael Vogt (mvo) wrote :
Download full text (6.0 KiB)

On Thu, Jul 26, 2012 at 08:46:26PM -0000, Gary Lasker wrote:
> This is targeted for trunk only currently as it includes a new string.
[..]

Thanks for the update of this branch to trunk.

I reviewed this in detail now. Its a bit of a long reply, I'm sorry
for that. Pleae note that there is nothing wrong with the branch, it
can land as it is. It got a bit long because of various ideas I had
while reading the code and the diff and I wanted to share them. All of
this can be done in future branches if you prefer to land this as it
is. Please also note that those are suggestions, its fine to not agree
with them :)

Some comments inline:

> === modified file 'softwarecenter/backend/recagent.py'
> --- softwarecenter/backend/recagent.py 2012-04-02 18:23:20 +0000
> +++ softwarecenter/backend/recagent.py 2012-07-26 20:45:23 +0000
> @@ -95,6 +95,15 @@
> return hashlib.md5(str(profile)).hexdigest()
>
> @property
> + def opt_in_requested(self):
> + if self.config.has_option("general", "recommender_opt_in_requested"):
> + opt_in_requested = self.config.get("general",
> + "recommender_opt_in_requested")
> + if opt_in_requested == "True":
> + return True
[..]

The config parser has a "getboolean" method that can be used here, it
will automatically convert from various strings and you can simply
return the value of it which should make this easier.

> + def recommender_opt_in_requested(self, opt_in_requested):
> + if opt_in_requested:
> + self.config.set("general",
> + "recommender_opt_in_requested",
> + "True")
> + else:
> + self.config.set("general",
> + "recommender_opt_in_requested",
> + "False")

If we are sure we awlays gets booleans from opt_in_requested, then
this could probably be written in a more compat form as:

          self.config.set("general",
                          "recommender_opt_in_requested",
                          str(value))

(not that it matter much :).

[..]
> === modified file 'tests/gtk3/test_catview.py'
> --- tests/gtk3/test_catview.py 2012-07-13 07:39:30 +0000
> +++ tests/gtk3/test_catview.py 2012-07-26 20:45:23 +0000
> @@ -33,8 +33,17 @@
> def setUpClass(cls):
> cls.db = get_test_db()
>
> - def setUp(self):
> + @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
> + @patch('softwarecenter.ui.gtk3.widgets.recommendations.RecommenderAgent'
> + '.is_opted_in')
> + # patch out the agent query method to avoid making the actual server call
> + @patch('softwarecenter.ui.gtk3.widgets.recommendations.RecommenderAgent'
> + '.post_submit_profile')
> + def setUp(self, mock_query, mock_recommender_is_opted_in, mock_sso):
> self._cat = None
> + # patch the recommender to specify that we are not opted-in at
> + # the start of each test
> + mock_recommender_is_opted_in.return_value = False
> self.win = get_test_window_catview(self.db)
> self.addCleanup(self.win.des...

Read more...

Michael Vogt (mvo) :
review: Approve
3078. By Gary Lasker on 2012-07-31

use config's getboolean to simplify the recommender_opt_in_requested code

3079. By Gary Lasker on 2012-07-31

refactor test_catview so that the patch decorators needed for the recommendations tests are isolated to those tests

3080. By Gary Lasker on 2012-07-31

more minor test cleanup per review comments from mvo

Gary Lasker (gary-lasker) wrote :

Hi mvo! I made the first few changes per your recommendations and retested.

Regarding your comment:

> - could we simply assume that if there is a uuid generated, that
> means that the user requested a opt-in? or is the seperate
> recommender_opt_in_requested config needed (e.g. to make things
> simpler)? like just generate the uuid on opt-in?

The uuid is used to indicate a successful opt-in, so as such it is not set until the profile upload and initial recommendations download is complete. The recommender_opt_in_requested is specifically for the case where opt-in is requested but network connectivity is not available, so it is there only to track the case where an opt-in is pending. This is reset once the opt-in is successfully completed and the uuid generated.

The last couple of recommendations that you made are a bit more involved, so I will hold off on those for another branch.

Thanks as always for your detailed review!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/backend/recagent.py'
2--- softwarecenter/backend/recagent.py 2012-04-02 18:23:20 +0000
3+++ softwarecenter/backend/recagent.py 2012-07-31 22:07:20 +0000
4@@ -95,6 +95,15 @@
5 return hashlib.md5(str(profile)).hexdigest()
6
7 @property
8+ def opt_in_requested(self):
9+ if self.config.has_option("general", "recommender_opt_in_requested"):
10+ opt_in_requested = self.config.getboolean(
11+ "general",
12+ "recommender_opt_in_requested")
13+ return opt_in_requested
14+ return False
15+
16+ @property
17 def recommender_uuid(self):
18 if self.config.has_option("general", "recommender_uuid"):
19 recommender_uuid = self.config.get("general",
20@@ -118,6 +127,11 @@
21 def _set_recommender_uuid(self, uuid):
22 self.config.set("general", "recommender_uuid", uuid)
23
24+ def recommender_opt_in_requested(self, opt_in_requested):
25+ self.config.set("general",
26+ "recommender_opt_in_requested",
27+ str(opt_in_requested))
28+
29 def post_submit_profile(self, db):
30 """ This will post the users profile to the recommender server
31 and also generate the UUID for the user if that is not
32@@ -222,7 +236,7 @@
33 Return True is the user is currently opted-in to the recommender
34 service
35 """
36- if self.recommender_uuid:
37+ if self.recommender_uuid or self.opt_in_requested:
38 return True
39 else:
40 return False
41@@ -230,6 +244,8 @@
42 def opt_out(self):
43 self.config.set("general", "recommender_uuid", "")
44 self.config.set("general", "recommender_profile_id", "")
45+ if self.recommender_opt_in_requested:
46+ self.recommender_opt_in_requested(False)
47
48 def _on_server_status_data(self, spawner, piston_server_status):
49 self.emit("server-status", piston_server_status)
50
51=== modified file 'softwarecenter/ui/gtk3/widgets/recommendations.py'
52--- softwarecenter/ui/gtk3/widgets/recommendations.py 2012-06-21 08:29:33 +0000
53+++ softwarecenter/ui/gtk3/widgets/recommendations.py 2012-07-31 22:07:20 +0000
54@@ -144,12 +144,12 @@
55 }
56
57 TURN_ON_RECOMMENDATIONS_TEXT = _(u"Turn On Recommendations")
58- # FIXME: This will be the default text once LP: #986437 is approved and
59- # ready to be merged/SRU'd
60 RECOMMENDATIONS_OPT_IN_TEXT = _(u"To make recommendations, "
61 "Ubuntu Software Center "
62 "will occasionally send to Canonical a list "
63 "of software currently installed.")
64+ NO_NETWORK_RECOMMENDATIONS_TEXT = _(u"Recommendations will appear "
65+ "when next online.")
66
67 def __init__(self, catview):
68 RecommendationsPanel.__init__(self, catview)
69@@ -157,37 +157,60 @@
70 self.set_header_label(_(u"Recommended For You"))
71 self.recommended_for_you_content = None
72 if self.recommender_agent.is_opted_in():
73- self._try_sso_login()
74+ if network_state_is_connected():
75+ self._try_sso_login()
76+ else:
77+ self._show_no_network_view()
78 else:
79 self._show_opt_in_view()
80
81 def _show_opt_in_view(self):
82 # opt in box
83- vbox = Gtk.Box.new(Gtk.Orientation.VERTICAL, StockEms.MEDIUM)
84- vbox.set_border_width(StockEms.MEDIUM)
85- self.opt_in_vbox = vbox # for tests
86- self.recommended_for_you_content = vbox # hook it up to the rest
87-
88+ self.recommended_for_you_content = Gtk.Box.new(
89+ Gtk.Orientation.VERTICAL,
90+ StockEms.MEDIUM)
91+ self.recommended_for_you_content.set_border_width(StockEms.MEDIUM)
92 self.add(self.recommended_for_you_content)
93
94 # opt in button
95- button = Gtk.Button(_(self.TURN_ON_RECOMMENDATIONS_TEXT))
96- button.connect("clicked", self._on_opt_in_button_clicked)
97+ self.opt_in_button = Gtk.Button(_(self.TURN_ON_RECOMMENDATIONS_TEXT))
98+ self.opt_in_button.connect("clicked", self._on_opt_in_button_clicked)
99 hbox = Gtk.Box(Gtk.Orientation.HORIZONTAL)
100- hbox.pack_start(button, False, False, 0)
101- vbox.pack_start(hbox, False, False, 0)
102- self.opt_in_button = button # for tests
103+ hbox.pack_start(self.opt_in_button, False, False, 0)
104+ self.recommended_for_you_content.pack_start(hbox, False, False, 0)
105
106 # opt in text
107 text = _(self.RECOMMENDATIONS_OPT_IN_TEXT)
108- label = Gtk.Label()
109- label.set_use_markup(True)
110- markup = '<small>%s</small>'
111- label.set_name("subtle-label")
112- label.set_markup(markup % text)
113- label.set_alignment(0, 0.5)
114- label.set_line_wrap(True)
115- vbox.pack_start(label, False, False, 0)
116+ self.opt_in_label = self._create_opt_in_label(text)
117+ self.recommended_for_you_content.pack_start(self.opt_in_label,
118+ False, False, 0)
119+
120+ def _show_no_network_view(self):
121+ # display network not available message
122+ if not self.recommended_for_you_content:
123+ self.recommended_for_you_content = Gtk.Box.new(
124+ Gtk.Orientation.VERTICAL,
125+ StockEms.MEDIUM)
126+ self.recommended_for_you_content.set_border_width(StockEms.MEDIUM)
127+ self.add(self.recommended_for_you_content)
128+ text = _(self.NO_NETWORK_RECOMMENDATIONS_TEXT)
129+ self.opt_in_label = self._create_opt_in_label(text)
130+ self.recommended_for_you_content.pack_start(self.opt_in_label,
131+ False, False, 0)
132+ else:
133+ self.opt_in_button.hide()
134+ text = _(self.NO_NETWORK_RECOMMENDATIONS_TEXT)
135+ self.opt_in_label.set_markup(self.opt_in_label_markup % text)
136+
137+ def _create_opt_in_label(self, label_text):
138+ opt_in_label = Gtk.Label()
139+ opt_in_label.set_use_markup(True)
140+ self.opt_in_label_markup = '<small>%s</small>'
141+ opt_in_label.set_name("subtle-label")
142+ opt_in_label.set_markup(self.opt_in_label_markup % label_text)
143+ opt_in_label.set_alignment(0, 0.5)
144+ opt_in_label.set_line_wrap(True)
145+ return opt_in_label
146
147 def _on_opt_in_button_clicked(self, button):
148 self.opt_in_to_recommendations_service()
149@@ -199,7 +222,12 @@
150 # them here -- a spinner is shown for this process (the spec
151 # wants a progress bar, but we don't have access to real-time
152 # progress info)
153- self._try_sso_login()
154+ if network_state_is_connected():
155+ self._try_sso_login()
156+ else:
157+ self._show_no_network_view()
158+ self.recommender_agent.recommender_opt_in_requested(True)
159+ self.emit("recommendations-opt-in")
160
161 def opt_out_of_recommendations_service(self):
162 # tell the backend that the user has opted out
163@@ -239,10 +267,12 @@
164 # we are all squared up with SSO login, now we can proceed with the
165 # recommendations display, or the profile upload if this is an
166 # initial opt-in
167- if self.recommender_agent.is_opted_in():
168- self._update_recommended_for_you_content()
169- else:
170+ if not self.recommender_agent.recommender_uuid:
171 self._upload_user_profile_and_get_recommendations()
172+ if self.recommender_agent.recommender_opt_in_requested:
173+ self.recommender_agent.recommender_opt_in_requested(False)
174+ else:
175+ self._update_recommended_for_you_content()
176
177 def _whoami_error(self, ssologin, e):
178 self.spinner_notebook.hide_spinner()
179@@ -253,7 +283,7 @@
180 if not network_state_is_connected():
181 # if there is an error in the SSO whois, first just check if we
182 # have network access and if we do no, just hide the panel
183- self._hide_recommended_for_you_panel()
184+ self._show_no_network_view()
185 else:
186 # an error that is not network related indicates that the user's
187 # token has likely been revoked or invalidated on the server, for
188
189=== modified file 'tests/gtk3/test_catview.py'
190--- tests/gtk3/test_catview.py 2012-07-13 07:39:30 +0000
191+++ tests/gtk3/test_catview.py 2012-07-31 22:07:20 +0000
192@@ -40,7 +40,6 @@
193 self.notebook = self.win.get_child()
194 self.lobby = self.win.get_data("lobby")
195 self.subcat_view = self.win.get_data("subcat")
196- self.rec_panel = self.lobby.recommended_for_you_panel
197
198 def _on_category_selected(self, subcatview, category):
199 self._cat = category
200@@ -111,56 +110,72 @@
201
202 class RecommendationsTestCase(CatViewBaseTestCase):
203 """The test suite for the recommendations ."""
204-
205- # FIXME: reenable
206- @unittest.skip("Disabled because of race condition in test")
207+
208+ # we need to use a custom setUp method for the recommendations test cases
209+ # so that everything gets configured properly
210 @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
211- @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
212- def test_recommended_for_you_opt_in_display(self,
213- mock_get_recommender_opted_in, mock_sso):
214- # patch the recommender UUID value to ensure that we are not opted-in
215- # for this test
216- mock_get_recommender_opted_in.return_value = False
217-
218- self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
219- FramedHeaderBox.CONTENT)
220- self.assertTrue(self.rec_panel.opt_in_vbox.get_property("visible"))
221-
222- # FIXME: reenable
223- @unittest.skip("Disabled because of race condition in test")
224+ @patch('softwarecenter.ui.gtk3.widgets.recommendations.RecommenderAgent'
225+ '.is_opted_in')
226 # patch out the agent query method to avoid making the actual server call
227- @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
228- @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
229- @patch('softwarecenter.backend.recagent.RecommenderAgent'
230+ @patch('softwarecenter.ui.gtk3.widgets.recommendations.RecommenderAgent'
231 '.post_submit_profile')
232- def test_recommended_for_you_spinner_display(self,
233- mock_query, mock_get_recommender_opted_in, mock_sso):
234- # patch the recommender UUID value to insure that we are not opted-in
235- # for this test
236- mock_get_recommender_opted_in.return_value = False
237-
238+ def setUp(self, mock_query, mock_recommender_is_opted_in, mock_sso):
239+ self._cat = None
240+ # patch the recommender to specify that we are not opted-in at
241+ # the start of each test
242+ mock_recommender_is_opted_in.return_value = False
243+ self.win = get_test_window_catview(self.db)
244+ self.addCleanup(self.win.destroy)
245+ self.notebook = self.win.get_child()
246+ self.lobby = self.win.get_data("lobby")
247+ self.subcat_view = self.win.get_data("subcat")
248+ self.rec_panel = self.lobby.recommended_for_you_panel
249+
250+ def test_recommended_for_you_opt_in_display(self):
251+ self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
252+ FramedHeaderBox.CONTENT)
253+ self.assertTrue(
254+ self.rec_panel.recommended_for_you_content.get_property("visible"))
255+
256+ # ensure that we are showing the opt-in view
257+ self.assertTrue(self.rec_panel.opt_in_button.get_property("visible"))
258+ label_text = self.rec_panel.opt_in_label.get_text()
259+ self.assertEqual(label_text,
260+ self.rec_panel.RECOMMENDATIONS_OPT_IN_TEXT)
261+
262+ def test_recommended_for_you_spinner_display(self):
263 # click the opt-in button to initiate the process,
264 # this will show the spinner
265- self.rec_panel.opt_in_button.emit('clicked')
266+ self.rec_panel.opt_in_button.clicked()
267 do_events_with_sleep()
268 self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
269 SpinnerNotebook.SPINNER_PAGE)
270- self.assertTrue(self.rec_panel.opt_in_vbox.get_visible())
271-
272- # patch out the agent query method to avoid making the actual server call
273- @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
274- @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
275- @patch('softwarecenter.backend.recagent.RecommenderAgent'
276- '.post_submit_profile')
277- def test_recommended_for_you_display_recommendations(self,
278- mock_query, mock_get_recommender_opted_in, mock_sso):
279- # patch the recommender UUID value to insure that we are not opted-in
280- # for this test
281- mock_get_recommender_opted_in.return_value = False
282-
283+ self.assertTrue(
284+ self.rec_panel.recommended_for_you_content.get_property("visible"))
285+
286+ @patch('softwarecenter.ui.gtk3.widgets.recommendations'
287+ '.network_state_is_connected')
288+ def test_recommended_for_you_network_not_available(self,
289+ mock_network_state_is_connected):
290+ # simulate no network available
291+ mock_network_state_is_connected.return_value = False
292+ # click the opt-in button to initiate the process
293+ self.rec_panel.opt_in_button.clicked()
294+ do_events()
295+ self.rec_panel._update_recommended_for_you_content()
296+ do_events()
297+ self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
298+ FramedHeaderBox.CONTENT)
299+ # ensure that we are showing the network not available view
300+ self.assertFalse(self.rec_panel.opt_in_button.get_property("visible"))
301+ label_text = self.rec_panel.opt_in_label.get_text()
302+ self.assertEqual(label_text,
303+ self.rec_panel.NO_NETWORK_RECOMMENDATIONS_TEXT)
304+
305+ def test_recommended_for_you_display_recommendations(self):
306 # click the opt-in button to initiate the process,
307 # this will show the spinner
308- self.rec_panel.opt_in_button.emit('clicked')
309+ self.rec_panel.opt_in_button.clicked()
310 do_events()
311 self.rec_panel._update_recommended_for_you_content()
312 do_events()
313@@ -180,17 +195,7 @@
314 self.assertNotEqual(self._cat, None)
315 self.assertEqual(self._cat.name, "Recommended For You")
316
317- # patch out the agent query method to avoid making the actual server call
318- @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
319- @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
320- @patch('softwarecenter.backend.recagent.RecommenderAgent'
321- '.query_recommend_me')
322- def test_recommended_for_you_display_recommendations_not_opted_in(self,
323- mock_query, mock_get_recommender_opted_in, mock_sso):
324- # patch the recommender UUID value to insure that we are not opted-in
325- # for this test
326- mock_get_recommender_opted_in.return_value = False
327-
328+ def test_recommended_for_you_display_recommendations_not_opted_in(self):
329 # we want to work in the "subcat" view
330 self.notebook.next_page()
331
332@@ -199,17 +204,14 @@
333 "visible")
334 self.assertFalse(visible)
335
336- # patch out the agent query method to avoid making the actual server call
337- @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
338- @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
339- @patch('softwarecenter.backend.recagent.RecommenderAgent'
340- '.query_recommend_me')
341- def test_recommended_for_you_display_recommendations_opted_in(self,
342- mock_query, mock_get_recommender_opted_in, mock_sso):
343- # patch the recommender UUID value to insure that we are not opted-in
344- # for this test
345- mock_get_recommender_opted_in.return_value = True
346-
347+ def test_recommended_for_you_display_recommendations_opted_in(self):
348+ # click the opt-in button to initiate the process,
349+ # this will show the spinner
350+ self.rec_panel.opt_in_button.clicked()
351+ do_events()
352+ self.rec_panel._update_recommended_for_you_content()
353+ do_events()
354+
355 # we want to work in the "subcat" view
356 self.notebook.next_page()
357

Subscribers

People subscribed via source and target branches