Code review comment for lp:~gary-lasker/software-center/offline-opt-in-lp965397-for-quantal

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.destroy)
> self.notebook = self.win.get_child()

I really like moving the patch decoratos from the individual tests to
the setUp() function, I think that is a nice change, I wonder if this
is the best place though. This is adding the patches to the
CaViewBaseTestCase which is used for various other testcases that are
unreleated to the recommendations functionatlity. Would it be possible
to move this into a more targeted setUp() call like
RecommendationstestCase ? E.g. something like

class RecommendationstestCase()

    @patch1
    @patch2
    @pach3
    def setUp(self, mock_1, mock_2, mock_3):
        mock_1.return_value ="what is needed"
        ...
        super(RecommendationstestCase, self).__init__()

so that its more isolated?

> self.rec_panel.opt_in_button.emit('clicked')

This could also be written as button.clicked() (not that it matters :)

> + @patch('softwarecenter.ui.gtk3.widgets.recommendations'
> + '.network_state_is_connected')
> + def test_recommended_for_you_network_not_available(self,
> + mock_network_state_is_connected):
[..]

Very nice, I really like this test!

> -
> + 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.emit('clicked')
> + do_events()
> + self.rec_panel._update_recommended_for_you_content()
> + do_events()
[..]

Unreleated to this branch I noticed that
  test_recommended_for_you_display_recommendations_opted_in()
and
  test_recommended_for_you_display_recommendations()

share the same "make a fake callback from the agent" code, its not
much but I wonder if it would make sense to move it into a small
helper that just takes different top-level rec_panel or rec_cat_panel?
Might be a nice way of extracting some common code as the two tests
look similar in some ways.

Some other random thoughts:
 - 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?

More random random thoughts:
- I wonder if the "recommendations-opt-{in,out}" signals would be
  better placed in the RecommendationsAgent? E.g. functions there
  like "opt_{in,out}" that do the magic and sends the signals. The
  reason it occured to me is that then we would have a "star topology"
  with the RecommenerAgent in the middle where e.g. the menu in
  app.py does not have to know anything about the
  RecommendationsPanelLobby (nicer seperation between GUI and
  backend), just about the RecommenderAgent (which it
  does already). The RecommendationsPanelLobby would now just have to
  subscript to the signals instead of sending them itself.

Cheers,
 Michael