Ubuntu Software Center

Merge lp://staging/~gary-lasker/software-center/update-to-latest-recommender-client into lp://staging/software-center

update-to-latest-recommender-client
Merge into trunk

Proposed by Gary Lasker on 2012-08-27

Status:	Merged
Merged at revision:	3130
Proposed branch:	lp://staging/~gary-lasker/software-center/update-to-latest-recommender-client
Merge into:	lp://staging/software-center
Diff against target:	138 lines (+48/-7) 5 files modified softwarecenter/backend/piston/sreclient_pristine.py (+26/-2) softwarecenter/backend/recagent.py (+5/-3) softwarecenter/config.py (+5/-2) softwarecenter/utils.py (+7/-0) tests/test_utils.py (+5/-0)
To merge this branch:	bzr merge lp://staging/~gary-lasker/software-center/update-to-latest-recommender-client
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
software-store-developers		2012-08-27	Pending
Review via email: mp+121499@code.staging.launchpad.net

Commit message

* lp:~gary-lasker/software-center/update-to-latest-recommender-client:
   - update to the latest recommender client API and replaces the
     deprecated recommend_me call with the new recommend_me call that
     includes the UUID

Description of the change

This branch updates the USC client to the latest version of the recommender API as found in lp:~canonical-ca-hackers/ubuntu-recommender/ubuntu-recommender-client. As part of the update we've moved from the deprecated recommend-me call that did not include a UUID to the updated recommend me call that does include a UUID. In making this update I discovered that the server does not support UUID values with dashes in them, so this condition is handled now for both for new UUIDs generated in all future opt-ins as well as for all previous UUID values in the field that still have dashes in them.

This branch also includes the new implicit_feedback call but does not use it yet. This will be used in a separate branch that is currently in-progress and that will depend on this one.

Many thanks for taking the time to review this!

Revision history for this message

Michael Vogt (mvo) wrote on 2012-08-28:

On Mon, Aug 27, 2012 at 09:20:29PM -0000, Gary Lasker wrote:
> Gary Lasker has proposed merging lp:~gary-lasker/software-center/update-to-latest-recommender-client into lp:software-center.

Thanks for your branch, that looks good. A small test would be good,
to ensure to not break this again in some future refactor. Should we
as simply as:

def test_uuid_conversion(self):
self.config.recommender_uuid = "xx-yy-zz"
self.assertEqual(self.config.recommender_uuid, "xxyyzz")

[..]
> === modified file 'softwarecenter/backend/recagent.py'
> --- softwarecenter/backend/recagent.py 2012-08-15 11:21:05 +0000
> +++ softwarecenter/backend/recagent.py 2012-08-27 21:19:21 +0000
> @@ -123,7 +123,7 @@
> if not recommender_uuid:
> # generate a new uuid, but do not save it yet, this will
> # be done later in _on_submit_profile_data
> - recommender_uuid = get_uuid()
> + recommender_uuid = get_uuid().replace("-", "")

My gut feeling is that the get_uuid() call should do the replace,
maybe as a new generate_recommender_uuid() call? Just so that its all
in one place. So that if the call is re-used there is no suprise that
the result is different from what the recommender agent expects.

Cheers,
Michael

lp://staging/~gary-lasker/software-center/update-to-latest-recommender-client updated on 2012-08-28

3104. By Gary Lasker on 2012-08-28: add a custom get_recommender_uuid call and use it
3105. By Gary Lasker on 2012-08-28: pep8

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-28:

Hi Michael, and thanks for your review. Your suggestions are good and I've added a separate get_recommender_uuid in utils.py and added a unit test for it.

Many thanks!

Revision history for this message

Michael Vogt (mvo) wrote on 2012-08-29:

On Tue, Aug 28, 2012 at 06:19:29PM -0000, Gary Lasker wrote:
> Hi Michael, and thanks for your review. Your suggestions are good and I've added a separate get_recommender_uuid in utils.py and added a unit test for it.
> You are subscribed to branch lp:software-center.

Thanks for this, I merged this now and added the test for the config
conversion that I put in the previous comment as well. I hope you
don't mind. It will ensure that on any future changes of the config
code this functionality keeps working. I also put the branch name of
the recommender-client into the changelog to make it easier for me to
remember the location.

Cheers,
Michael

Revision history for this message

Gary Lasker (gary-lasker) wrote on 2012-08-29:

Ah, the config bit, thanks for the test for that! Of course it is much better to have that. And it's definitely useful to have the branch name in the changelog 'cuz it's not straightforward to find otherwise.

Many thanks, Michael!