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

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

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.

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!

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

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!