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

[Gary Lasker]

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

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!

[Gary Lasker (gary-lasker)]

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

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

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

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

[Michael Vogt (mvo)]

review: Approve

[lp:~gary-lasker/software-center/offline-opt-in-lp965397-for-quantal]

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

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!

=== modified file 'softwarecenter/backend/recagent.py'
--- softwarecenter/backend/recagent.py	2012-04-02 18:23:20 +0000
+++ softwarecenter/backend/recagent.py	2012-07-31 22:07:20 +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.getboolean(
+                                               "general",
+                                               "recommender_opt_in_requested")
+            return opt_in_requested
+        return False
+
+    @property
     def recommender_uuid(self):
         if self.config.has_option("general", "recommender_uuid"):
             recommender_uuid = self.config.get("general",
@@ -118,6 +127,11 @@
     def _set_recommender_uuid(self, uuid):
         self.config.set("general", "recommender_uuid", uuid)
 
+    def recommender_opt_in_requested(self, opt_in_requested):
+        self.config.set("general",
+                        "recommender_opt_in_requested",
+                        str(opt_in_requested))
+
     def post_submit_profile(self, db):
         """ This will post the users profile to the recommender server
             and also generate the UUID for the user if that is not
@@ -222,7 +236,7 @@
         Return True is the user is currently opted-in to the recommender
         service
         """
-        if self.recommender_uuid:
+        if self.recommender_uuid or self.opt_in_requested:
             return True
         else:
             return False
@@ -230,6 +244,8 @@
     def opt_out(self):
         self.config.set("general", "recommender_uuid", "")
         self.config.set("general", "recommender_profile_id", "")
+        if self.recommender_opt_in_requested:
+            self.recommender_opt_in_requested(False)
 
     def _on_server_status_data(self, spawner, piston_server_status):
         self.emit("server-status", piston_server_status)

=== modified file 'softwarecenter/ui/gtk3/widgets/recommendations.py'
--- softwarecenter/ui/gtk3/widgets/recommendations.py	2012-06-21 08:29:33 +0000
+++ softwarecenter/ui/gtk3/widgets/recommendations.py	2012-07-31 22:07:20 +0000
@@ -144,12 +144,12 @@
         }
 
     TURN_ON_RECOMMENDATIONS_TEXT = _(u"Turn On Recommendations")
-    # FIXME: This will be the default text once LP: #986437 is approved and
-    #        ready to be merged/SRU'd
     RECOMMENDATIONS_OPT_IN_TEXT = _(u"To make recommendations, "
                  "Ubuntu Software Center "
                  "will occasionally send to Canonical a list "
                  "of software currently installed.")
+    NO_NETWORK_RECOMMENDATIONS_TEXT = _(u"Recommendations will appear "
+                                         "when next online.")
 
     def __init__(self, catview):
         RecommendationsPanel.__init__(self, catview)
@@ -157,37 +157,60 @@
         self.set_header_label(_(u"Recommended For You"))
         self.recommended_for_you_content = None
         if self.recommender_agent.is_opted_in():
-            self._try_sso_login()
+            if network_state_is_connected():
+                self._try_sso_login()
+            else:
+                self._show_no_network_view()
         else:
             self._show_opt_in_view()
 
     def _show_opt_in_view(self):
         # opt in box
-        vbox = Gtk.Box.new(Gtk.Orientation.VERTICAL, StockEms.MEDIUM)
-        vbox.set_border_width(StockEms.MEDIUM)
-        self.opt_in_vbox = vbox  # for tests
-        self.recommended_for_you_content = vbox  # hook it up to the rest
-
+        self.recommended_for_you_content = Gtk.Box.new(
+                Gtk.Orientation.VERTICAL,
+                StockEms.MEDIUM)
+        self.recommended_for_you_content.set_border_width(StockEms.MEDIUM)
         self.add(self.recommended_for_you_content)
 
         # opt in button
-        button = Gtk.Button(_(self.TURN_ON_RECOMMENDATIONS_TEXT))
-        button.connect("clicked", self._on_opt_in_button_clicked)
+        self.opt_in_button = Gtk.Button(_(self.TURN_ON_RECOMMENDATIONS_TEXT))
+        self.opt_in_button.connect("clicked", self._on_opt_in_button_clicked)
         hbox = Gtk.Box(Gtk.Orientation.HORIZONTAL)
-        hbox.pack_start(button, False, False, 0)
-        vbox.pack_start(hbox, False, False, 0)
-        self.opt_in_button = button  # for tests
+        hbox.pack_start(self.opt_in_button, False, False, 0)
+        self.recommended_for_you_content.pack_start(hbox, False, False, 0)
 
         # opt in text
         text = _(self.RECOMMENDATIONS_OPT_IN_TEXT)
-        label = Gtk.Label()
-        label.set_use_markup(True)
-        markup = '<small>%s</small>'
-        label.set_name("subtle-label")
-        label.set_markup(markup % text)
-        label.set_alignment(0, 0.5)
-        label.set_line_wrap(True)
-        vbox.pack_start(label, False, False, 0)
+        self.opt_in_label = self._create_opt_in_label(text)
+        self.recommended_for_you_content.pack_start(self.opt_in_label,
+                                                    False, False, 0)
+
+    def _show_no_network_view(self):
+        # display network not available message
+        if not self.recommended_for_you_content:
+            self.recommended_for_you_content = Gtk.Box.new(
+                Gtk.Orientation.VERTICAL,
+                StockEms.MEDIUM)
+            self.recommended_for_you_content.set_border_width(StockEms.MEDIUM)
+            self.add(self.recommended_for_you_content)
+            text = _(self.NO_NETWORK_RECOMMENDATIONS_TEXT)
+            self.opt_in_label = self._create_opt_in_label(text)
+            self.recommended_for_you_content.pack_start(self.opt_in_label,
+                                                        False, False, 0)
+        else:
+            self.opt_in_button.hide()
+            text = _(self.NO_NETWORK_RECOMMENDATIONS_TEXT)
+            self.opt_in_label.set_markup(self.opt_in_label_markup % text)
+
+    def _create_opt_in_label(self, label_text):
+        opt_in_label = Gtk.Label()
+        opt_in_label.set_use_markup(True)
+        self.opt_in_label_markup = '<small>%s</small>'
+        opt_in_label.set_name("subtle-label")
+        opt_in_label.set_markup(self.opt_in_label_markup % label_text)
+        opt_in_label.set_alignment(0, 0.5)
+        opt_in_label.set_line_wrap(True)
+        return opt_in_label
 
     def _on_opt_in_button_clicked(self, button):
         self.opt_in_to_recommendations_service()
@@ -199,7 +222,12 @@
         # them here -- a spinner is shown for this process (the spec
         # wants a progress bar, but we don't have access to real-time
         # progress info)
-        self._try_sso_login()
+        if network_state_is_connected():
+            self._try_sso_login()
+        else:
+            self._show_no_network_view()
+            self.recommender_agent.recommender_opt_in_requested(True)
+            self.emit("recommendations-opt-in")
 
     def opt_out_of_recommendations_service(self):
         # tell the backend that the user has opted out
@@ -239,10 +267,12 @@
         # we are all squared up with SSO login, now we can proceed with the
         # recommendations display, or the profile upload if this is an
         # initial opt-in
-        if self.recommender_agent.is_opted_in():
-            self._update_recommended_for_you_content()
-        else:
+        if not self.recommender_agent.recommender_uuid:
             self._upload_user_profile_and_get_recommendations()
+            if self.recommender_agent.recommender_opt_in_requested:
+                self.recommender_agent.recommender_opt_in_requested(False)
+        else:
+            self._update_recommended_for_you_content()
 
     def _whoami_error(self, ssologin, e):
         self.spinner_notebook.hide_spinner()
@@ -253,7 +283,7 @@
         if not network_state_is_connected():
             # if there is an error in the SSO whois, first just check if we
             # have network access and if we do no, just hide the panel
-            self._hide_recommended_for_you_panel()
+            self._show_no_network_view()
         else:
             # an error that is not network related indicates that the user's
             # token has likely been revoked or invalidated on the server, for

=== 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-31 22:07:20 +0000
@@ -40,7 +40,6 @@
         self.notebook = self.win.get_child()
         self.lobby = self.win.get_data("lobby")
         self.subcat_view = self.win.get_data("subcat")
-        self.rec_panel = self.lobby.recommended_for_you_panel
 
     def _on_category_selected(self, subcatview, category):
         self._cat = category
@@ -111,56 +110,72 @@
 
 class RecommendationsTestCase(CatViewBaseTestCase):
     """The test suite for the recommendations ."""
-
-    # FIXME: reenable
-    @unittest.skip("Disabled because of race condition in test")
+    
+    # we need to use a custom setUp method for the recommendations test cases
+    # so that everything gets configured properly
     @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
-    def test_recommended_for_you_opt_in_display(self,
-            mock_get_recommender_opted_in, mock_sso):
-        # patch the recommender UUID value to ensure that we are not opted-in
-        # for this test
-        mock_get_recommender_opted_in.return_value = False
-
-        self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
-                         FramedHeaderBox.CONTENT)
-        self.assertTrue(self.rec_panel.opt_in_vbox.get_property("visible"))
-
-    # FIXME: reenable
-    @unittest.skip("Disabled because of race condition in test")
+    @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.get_sso_backend')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent'
+    @patch('softwarecenter.ui.gtk3.widgets.recommendations.RecommenderAgent'
            '.post_submit_profile')
-    def test_recommended_for_you_spinner_display(self,
-            mock_query, mock_get_recommender_opted_in, mock_sso):
-        # patch the recommender UUID value to insure that we are not opted-in
-        # for this test
-        mock_get_recommender_opted_in.return_value = False
-
+    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.destroy)
+        self.notebook = self.win.get_child()
+        self.lobby = self.win.get_data("lobby")
+        self.subcat_view = self.win.get_data("subcat")
+        self.rec_panel = self.lobby.recommended_for_you_panel
+
+    def test_recommended_for_you_opt_in_display(self):
+        self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
+                         FramedHeaderBox.CONTENT)
+        self.assertTrue(
+            self.rec_panel.recommended_for_you_content.get_property("visible"))
+
+        # ensure that we are showing the opt-in view
+        self.assertTrue(self.rec_panel.opt_in_button.get_property("visible"))
+        label_text = self.rec_panel.opt_in_label.get_text()
+        self.assertEqual(label_text,
+                         self.rec_panel.RECOMMENDATIONS_OPT_IN_TEXT)
+
+    def test_recommended_for_you_spinner_display(self):
         # click the opt-in button to initiate the process,
         # this will show the spinner
-        self.rec_panel.opt_in_button.emit('clicked')
+        self.rec_panel.opt_in_button.clicked()
         do_events_with_sleep()
         self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
                          SpinnerNotebook.SPINNER_PAGE)
-        self.assertTrue(self.rec_panel.opt_in_vbox.get_visible())
-
-    # patch out the agent query method to avoid making the actual server call
-    @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent'
-           '.post_submit_profile')
-    def test_recommended_for_you_display_recommendations(self,
-            mock_query, mock_get_recommender_opted_in, mock_sso):
-        # patch the recommender UUID value to insure that we are not opted-in
-        # for this test
-        mock_get_recommender_opted_in.return_value = False
-
+        self.assertTrue(
+            self.rec_panel.recommended_for_you_content.get_property("visible"))
+
+    @patch('softwarecenter.ui.gtk3.widgets.recommendations'
+           '.network_state_is_connected')
+    def test_recommended_for_you_network_not_available(self,
+                mock_network_state_is_connected):
+        # simulate no network available
+        mock_network_state_is_connected.return_value = False
+        # click the opt-in button to initiate the process
+        self.rec_panel.opt_in_button.clicked()
+        do_events()
+        self.rec_panel._update_recommended_for_you_content()
+        do_events()
+        self.assertEqual(self.rec_panel.spinner_notebook.get_current_page(),
+                         FramedHeaderBox.CONTENT)
+        # ensure that we are showing the network not available view
+        self.assertFalse(self.rec_panel.opt_in_button.get_property("visible"))
+        label_text = self.rec_panel.opt_in_label.get_text()
+        self.assertEqual(label_text,
+                         self.rec_panel.NO_NETWORK_RECOMMENDATIONS_TEXT)
+
+    def test_recommended_for_you_display_recommendations(self):
         # click the opt-in button to initiate the process,
         # this will show the spinner
-        self.rec_panel.opt_in_button.emit('clicked')
+        self.rec_panel.opt_in_button.clicked()
         do_events()
         self.rec_panel._update_recommended_for_you_content()
         do_events()
@@ -180,17 +195,7 @@
         self.assertNotEqual(self._cat, None)
         self.assertEqual(self._cat.name, "Recommended For You")
 
-    # patch out the agent query method to avoid making the actual server call
-    @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent'
-           '.query_recommend_me')
-    def test_recommended_for_you_display_recommendations_not_opted_in(self,
-            mock_query, mock_get_recommender_opted_in, mock_sso):
-        # patch the recommender UUID value to insure that we are not opted-in
-        # for this test
-        mock_get_recommender_opted_in.return_value = False
-
+    def test_recommended_for_you_display_recommendations_not_opted_in(self):
         # we want to work in the "subcat" view
         self.notebook.next_page()
 
@@ -199,17 +204,14 @@
             "visible")
         self.assertFalse(visible)
 
-    # patch out the agent query method to avoid making the actual server call
-    @patch('softwarecenter.ui.gtk3.widgets.recommendations.get_sso_backend')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent.is_opted_in')
-    @patch('softwarecenter.backend.recagent.RecommenderAgent'
-           '.query_recommend_me')
-    def test_recommended_for_you_display_recommendations_opted_in(self,
-            mock_query, mock_get_recommender_opted_in, mock_sso):
-        # patch the recommender UUID value to insure that we are not opted-in
-        # for this test
-        mock_get_recommender_opted_in.return_value = True
-
+    def test_recommended_for_you_display_recommendations_opted_in(self):
+        # click the opt-in button to initiate the process,
+        # this will show the spinner
+        self.rec_panel.opt_in_button.clicked()
+        do_events()
+        self.rec_panel._update_recommended_for_you_content()
+        do_events()
+        
         # we want to work in the "subcat" view
         self.notebook.next_page()