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

Proposed by Gary Lasker
Status: Merged
Merged at revision: 3130
Proposed branch: lp:~gary-lasker/software-center/update-to-latest-recommender-client
Merge into: lp: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:~gary-lasker/software-center/update-to-latest-recommender-client
Reviewer Review Type Date Requested Status
software-store-developers Pending
Review via email: mp+121499@code.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!

To post a comment you must log in.
Michael Vogt (mvo) wrote :

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

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

Gary Lasker (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.

Many thanks!

Michael Vogt (mvo) wrote :

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

Gary Lasker (gary-lasker) wrote :

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!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/backend/piston/sreclient_pristine.py'
2--- softwarecenter/backend/piston/sreclient_pristine.py 2012-03-16 20:12:57 +0000
3+++ softwarecenter/backend/piston/sreclient_pristine.py 2012-08-28 18:18:22 +0000
4@@ -41,9 +41,21 @@
5 scheme=PUBLIC_API_SCHEME)
6
7 @oauth_protected
8+ @validate_pattern('uuid', r'[\dabcdef]{32}')
9 @returns_json
10- def recommend_me(self):
11- return self._get('recommend_me/', scheme=AUTHENTICATED_API_SCHEME)
12+ def recommend_me(self, uuid):
13+ """Fetch personalized (authenticated) recommendations.
14+
15+ We still need to submit an UUID to link to the anon profile if
16+ it was submitted.
17+ """
18+ return self._get('recommend_me/%s/' % uuid,
19+ scheme=AUTHENTICATED_API_SCHEME)
20+
21+ @validate_pattern('uuid', r'[\dabcdef]{32}')
22+ def anon_recommend_me(self, uuid):
23+ return self._get('recommend_me/%s/' % uuid,
24+ scheme=AUTHENTICATED_API_SCHEME)
25
26 @validate_pattern('pkgname', '[^/]+')
27 @returns_json
28@@ -72,6 +84,18 @@
29 scheme=AUTHENTICATED_API_SCHEME)
30
31 @oauth_protected
32+ @validate('pkgname', str)
33+ @validate_pattern('action', '\w{3,20}')
34+ @returns_json
35+ def implicit_feedback(self, pkgname, action):
36+ data = {
37+ 'package_name': pkgname,
38+ 'action': action,
39+ }
40+ return self._post('implicit_feedback/', data=data,
41+ scheme=AUTHENTICATED_API_SCHEME)
42+
43+ @oauth_protected
44 @returns_json
45 @validate('remove', bool)
46 def remove_app(self, appname, remove):
47
48=== modified file 'softwarecenter/backend/recagent.py'
49--- softwarecenter/backend/recagent.py 2012-08-15 11:21:05 +0000
50+++ softwarecenter/backend/recagent.py 2012-08-28 18:18:22 +0000
51@@ -28,7 +28,7 @@
52
53 from softwarecenter.config import get_config
54 from softwarecenter.db.utils import get_installed_apps_list
55-from softwarecenter.utils import get_uuid
56+from softwarecenter.utils import get_recommender_uuid
57
58 LOG = logging.getLogger(__name__)
59
60@@ -123,7 +123,7 @@
61 if not recommender_uuid:
62 # generate a new uuid, but do not save it yet, this will
63 # be done later in _on_submit_profile_data
64- recommender_uuid = get_uuid()
65+ recommender_uuid = get_recommender_uuid()
66 installed_pkglist = [app.pkgname
67 for app in get_installed_apps_list(db)]
68 profile = self._generate_submit_profile_data(recommender_uuid,
69@@ -182,7 +182,9 @@
70 spawner.connect("data-available", self._on_recommend_me_data)
71 spawner.connect("error", lambda spawner, err: self.emit("error", err))
72 spawner.run_generic_piston_helper(
73- "SoftwareCenterRecommenderAPI", "recommend_me")
74+ "SoftwareCenterRecommenderAPI",
75+ "recommend_me",
76+ uuid=self.recommender_uuid)
77
78 def query_recommend_app(self, pkgname):
79 # build the command
80
81=== modified file 'softwarecenter/config.py'
82--- softwarecenter/config.py 2012-08-14 15:10:19 +0000
83+++ softwarecenter/config.py 2012-08-28 18:18:22 +0000
84@@ -110,8 +110,11 @@
85 None,
86 "Defines if apps should be started maximized")
87 recommender_uuid = property(
88- lambda self: self._generic_get("recommender_uuid"),
89- lambda self, value: self._generic_set("recommender_uuid", value),
90+ # remove any dashes for the case where a user has opted in before
91+ # we required UUIDs without dashes
92+ lambda self: self._generic_get("recommender_uuid").replace("-", ""),
93+ lambda self, value: self._generic_set("recommender_uuid",
94+ value),
95 None,
96 "The UUID generated for the recommendations")
97 recommender_profile_id = property(
98
99=== modified file 'softwarecenter/utils.py'
100--- softwarecenter/utils.py 2012-08-23 14:37:28 +0000
101+++ softwarecenter/utils.py 2012-08-28 18:18:22 +0000
102@@ -682,6 +682,13 @@
103 return str(uuid.uuid4())
104
105
106+def get_recommender_uuid():
107+ """ the recommender service requires a UUID that does not contain
108+ dashes
109+ """
110+ return get_uuid().replace("-", "")
111+
112+
113 def get_lock(path):
114 """ return a lock that can be released with release_lock on success
115 and -1 on failure
116
117=== modified file 'tests/test_utils.py'
118--- tests/test_utils.py 2012-08-24 08:49:37 +0000
119+++ tests/test_utils.py 2012-08-28 18:18:22 +0000
120@@ -34,6 +34,7 @@
121 get_oem_channel_descriptor,
122 get_title_from_html,
123 get_uuid,
124+ get_recommender_uuid,
125 is_no_display_desktop_file,
126 make_string_from_list,
127 release_filename_in_lists_from_deb_line,
128@@ -149,6 +150,10 @@
129 def test_get_uuid(self):
130 uuid = get_uuid()
131 self.assertTrue(uuid and len(uuid) > 0)
132+
133+ def test_get_recommender_uuid(self):
134+ uuid = get_recommender_uuid()
135+ self.assertTrue("-" not in uuid and len(uuid) > 0)
136
137 def test_clear_credentials(self):
138 clear_token_from_ubuntu_sso_sync("fo")

Subscribers

People subscribed via source and target branches