Code review comment for lp:~gary-lasker/software-center/recommended-installed-feedback

On Tue, Aug 28, 2012 at 05:46:23AM -0000, Gary Lasker wrote:
> Gary Lasker has proposed merging lp:~gary-lasker/software-center/recommended-installed-feedback into lp:software-center with lp:~gary-lasker/software-center/update-to-latest-recommender-client as a prerequisite.

Thanks for working on this branch. I'm very happy to see this feature
coming.

[..]
> Note that this branch depends on the lp:~gary-lasker/software-center/update-to-latest-recommender-client branch as that branch adds the new recommender client code from lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client. Also note that I made one small change to sreclient to fix the validate decorator for the pkgname argument to implicit_feedback so that it matches the validator for the other calls that use pkgname as an argument.

Thanks for this, if you fixed the sreclient_pristine.py, please also
send the diff to the server team (if you haven't done already) so that
they add it to their
lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client
branch.

> Finally, I added a unit test for the new call in test_recagent.py, however like a few of the other tests there, the test is not currently working with the server. Therefore, it remains disabled as the other tests are. I'll look at this tomorrow to try to see what the issue is with these tests.
[..]

Maybe the staging server had problems. I just tested enabling them and
its working, but that may well be that its now fixed. While looking at
the test code I noticed that there is a bit of duplication in
there, I pushed a branch at
lp:~mvo/software-center/recagent-test-cleanup that removes most of
that.

I also noticed that the branch is missing a test for the new
functionality, it would be nice to add one to
tests/gtk3/test_recommendations_widget.py. Something like activate a
app, trigger a install transaction and verify that on finish the
self.recommender_agent.post_implicit_feedback() call was done.

> @@ -244,7 +262,7 @@
>
> def _on_submit_anon_profile_data(self, spawner,
> piston_submit_anon_profile):
> - self.emit("submit-anon_profile", piston_submit_anon_profile)
> + self.emit("submit-anon-profile-finished", piston_submit_anon_profile)

Nice that this is more consistent now (plus that it will actually work).

> === modified file 'softwarecenter/enums.py'
> --- softwarecenter/enums.py 2012-08-23 14:37:28 +0000
> +++ softwarecenter/enums.py 2012-08-28 05:45:23 +0000
> @@ -299,6 +299,9 @@
> LOBBY_RECOMMENDATIONS_CAROUSEL_LIMIT = 9
> DETAILS_RECOMMENDATIONS_CAROUSEL_LIMIT = 4
>
> +# action values for recommendations implicit feedback
> +RECOMMENDATIONS_FEEDBACK_INSTALLED = "installed"
> +

That looks like we could use:

class RecommenderFeedback:
      INSTALLED = "installed"

(so that later "REMOVED" can be added there as well and its more
consitent with the rest of enums.py).

[..]
> def _on_application_activated(self, catview, app):
> self.emit("application-activated", app)
> + # we only track installed items of the user has opted-in to the
> + # recommendations service
> + if self.recommender_agent.is_opted_in():
> + self.recommended_apps_viewed.append(app.pkgname)

self.recommended_apps_viewed should probably be a set (as we don't
want duplication and order is not important here).

Should self.recommended_apps_viewed we cleared when we get new data,
e.g on _on_app_recommendations_agent_refresh (and the equivalent
function for the categories). Or should this be persistent? I'm don't
know the details of the spec well enough :) I guess it comes down to
if we should consider it a recommendation that are reached
"indirectly", e.g. click on it "app A", move back to main view search
for something else and then reach "app A" again later (without
following a recommendation) if installing that should be counted or
not. I don't really mind either way, but would like to know.

> + def _on_transaction_started(self, backend, pkgname, appname, trans_id,
> + trans_type):
> + if (trans_type != TransactionTypes.INSTALL and
> + pkgname in self.recommended_apps_viewed):
> + # if the transaction is not an installation we don't want to
> + # track it as a recommended item
> + self.recommended_apps_viewed.remove(pkgname)

Could the check for the transaction type simply be folded into the
_on_transaction_finished() ? So that there is only a single signal
type to watch?

> + def _on_transaction_finished(self, backend, result):
> + if result.pkgname in self.recommended_apps_viewed:
> + self.recommended_apps_viewed.remove(result.pkgname)
> + if network_state_is_connected():
> + # no need to monitor for an error, etc., for this
> + self.recommender_agent.post_implicit_feedback(
> + result.pkgname,
> + RECOMMENDATIONS_FEEDBACK_INSTALLED)

If the error condition does not matter, then we can probably also skip
the network check.

> === modified file 'tests/test_recagent.py'
> --- tests/test_recagent.py 2012-06-11 14:50:53 +0000
> +++ tests/test_recagent.py 2012-08-28 05:45:23 +0000
> + @unittest.skip('disabled for now as the server is not quite working')
[..]

Nice update to use the skip decorator.

Cheers,
 Michael