Merge lp:~mvo/software-center/region-whitelist-lp1006570 into lp:software-center
- region-whitelist-lp1006570
- Merge into trunk
Proposed by
Michael Vogt
Status: | Merged |
---|---|
Merged at revision: | 3158 |
Proposed branch: | lp:~mvo/software-center/region-whitelist-lp1006570 |
Merge into: | lp:software-center |
Diff against target: |
217 lines (+93/-28) 4 files modified
softwarecenter/db/update.py (+29/-10) softwarecenter/region.py (+2/-0) tests/test_database.py (+60/-17) tests/utils.py (+2/-1) |
To merge this branch: | bzr merge lp:~mvo/software-center/region-whitelist-lp1006570 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Gary Lasker (community) | Approve | ||
Review via email: mp+123504@code.launchpad.net |
Commit message
Fix missing region whitelist support that is used by the software-
Description of the change
Add support for the region-whitelist feature requested in LP: #1006570
To post a comment you must log in.
Revision history for this message
Anthony Lenton (elachuni) wrote : | # |
- 3156. By Michael Vogt
-
use .lower() on the tags and update test to simulate passing uppercase tags (like the server is doing)
- 3157. By Michael Vogt
-
drive-by-cleanup
- 3158. By Michael Vogt
-
another small drive-by-cleanup that allows to give make_software_
center_ agent_app_ dict() a override dict - 3159. By Michael Vogt
-
trivial pep8 fixes
Revision history for this message
Michael Vogt (mvo) wrote : | # |
Thanks, good point, I fixed that now and updated the test.
Revision history for this message
Gary Lasker (gary-lasker) wrote : | # |
This looks fine. Approval also based on Anthony's comment above and Michael's subsequent updates per his comment.
Many thanks!
review:
Approve
Preview Diff
[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1 | === modified file 'softwarecenter/db/update.py' | |||
2 | --- softwarecenter/db/update.py 2012-08-23 14:37:28 +0000 | |||
3 | +++ softwarecenter/db/update.py 2012-09-10 09:45:22 +0000 | |||
4 | @@ -69,8 +69,16 @@ | |||
5 | 69 | 69 | ||
6 | 70 | 70 | ||
7 | 71 | from softwarecenter.db.pkginfo import get_pkg_info | 71 | from softwarecenter.db.pkginfo import get_pkg_info |
10 | 72 | from softwarecenter.distro import get_current_arch, get_foreign_architectures | 72 | from softwarecenter.distro import ( |
11 | 73 | from softwarecenter.region import get_region_cached, REGION_BLACKLIST_TAG | 73 | get_current_arch, |
12 | 74 | get_foreign_architectures, | ||
13 | 75 | ) | ||
14 | 76 | from softwarecenter.region import ( | ||
15 | 77 | get_region_cached, | ||
16 | 78 | REGION_BLACKLIST_TAG, | ||
17 | 79 | REGION_WHITELIST_TAG, | ||
18 | 80 | ) | ||
19 | 81 | |||
20 | 74 | 82 | ||
21 | 75 | # weights for the different fields | 83 | # weights for the different fields |
22 | 76 | WEIGHT_DESKTOP_NAME = 10 | 84 | WEIGHT_DESKTOP_NAME = 10 |
23 | @@ -380,19 +388,30 @@ | |||
24 | 380 | doc.add_term("AT" + app_type.lower()) | 388 | doc.add_term("AT" + app_type.lower()) |
25 | 381 | 389 | ||
26 | 382 | # (deb)tags (in addition to the pkgname debtags) | 390 | # (deb)tags (in addition to the pkgname debtags) |
33 | 383 | tags = self.get_value(AppInfoFields.TAGS) | 391 | tags_string = self.get_value(AppInfoFields.TAGS) |
34 | 384 | if tags: | 392 | if tags_string: |
35 | 385 | # register tags | 393 | # convert to list and register |
36 | 386 | for tag in tags.split(","): | 394 | tags = [tag.strip().lower() for tag in tags_string.split(",")] |
37 | 387 | doc.add_term("XT" + tag.strip()) | 395 | for tag in tags: |
38 | 388 | # ENFORCE region blacklist by not registering the app at all | 396 | doc.add_term("XT" + tag) |
39 | 397 | # ENFORCE region blacklist/whitelist by not registering | ||
40 | 398 | # the app at all | ||
41 | 389 | region = get_region_cached() | 399 | region = get_region_cached() |
42 | 390 | if region: | 400 | if region: |
43 | 391 | countrycode = region["countrycode"].lower() | 401 | countrycode = region["countrycode"].lower() |
44 | 402 | # blacklist | ||
45 | 392 | if "%s%s" % (REGION_BLACKLIST_TAG, countrycode) in tags: | 403 | if "%s%s" % (REGION_BLACKLIST_TAG, countrycode) in tags: |
48 | 393 | LOG.info("%r.make_doc: skipping region restricted app %r", | 404 | LOG.info("%r.make_doc: skipping region restricted app %r " |
49 | 394 | self.__class__.__name__, name) | 405 | "(blacklisted)", self.__class__.__name__, name) |
50 | 395 | return | 406 | return |
51 | 407 | # whitelist | ||
52 | 408 | for tag in tags: | ||
53 | 409 | if (tag.startswith(REGION_WHITELIST_TAG) and not | ||
54 | 410 | "%s%s" % (REGION_WHITELIST_TAG, countrycode) in tag): | ||
55 | 411 | LOG.info("%r.make_doc: skipping region restricted " | ||
56 | 412 | "app %r (region not whitelisted)", | ||
57 | 413 | self.__class__.__name__, name) | ||
58 | 414 | return | ||
59 | 396 | 415 | ||
60 | 397 | # popcon | 416 | # popcon |
61 | 398 | # FIXME: popularity not only based on popcon but also | 417 | # FIXME: popularity not only based on popcon but also |
62 | 399 | 418 | ||
63 | === modified file 'softwarecenter/region.py' | |||
64 | --- softwarecenter/region.py 2012-08-30 13:03:42 +0000 | |||
65 | +++ softwarecenter/region.py 2012-09-10 09:45:22 +0000 | |||
66 | @@ -38,6 +38,8 @@ | |||
67 | 38 | 38 | ||
68 | 39 | # blacklist this region | 39 | # blacklist this region |
69 | 40 | REGION_BLACKLIST_TAG = "blacklist-iso3166::" | 40 | REGION_BLACKLIST_TAG = "blacklist-iso3166::" |
70 | 41 | # or whitelist it | ||
71 | 42 | REGION_WHITELIST_TAG = "whitelist-iso3166::" | ||
72 | 41 | 43 | ||
73 | 42 | 44 | ||
74 | 43 | def get_region_name(countrycode): | 45 | def get_region_name(countrycode): |
75 | 44 | 46 | ||
76 | === modified file 'tests/test_database.py' | |||
77 | --- tests/test_database.py 2012-09-05 10:21:27 +0000 | |||
78 | +++ tests/test_database.py 2012-09-10 09:45:22 +0000 | |||
79 | @@ -47,7 +47,11 @@ | |||
80 | 47 | XapianValues, | 47 | XapianValues, |
81 | 48 | PkgStates, | 48 | PkgStates, |
82 | 49 | ) | 49 | ) |
84 | 50 | from softwarecenter.region import REGION_BLACKLIST_TAG, REGIONTAG | 50 | from softwarecenter.region import ( |
85 | 51 | REGION_BLACKLIST_TAG, | ||
86 | 52 | REGION_WHITELIST_TAG, | ||
87 | 53 | REGIONTAG, | ||
88 | 54 | ) | ||
89 | 51 | 55 | ||
90 | 52 | PKD_DIR = os.path.join(DATA_DIR, 'appdetails', 'var', 'lib', 'dpkg', 'status') | 56 | PKD_DIR = os.path.join(DATA_DIR, 'appdetails', 'var', 'lib', 'dpkg', 'status') |
91 | 53 | TEST_DB = os.path.join(DATA_DIR, "test.db") | 57 | TEST_DB = os.path.join(DATA_DIR, "test.db") |
92 | @@ -522,6 +526,12 @@ | |||
93 | 522 | def setUp(self): | 526 | def setUp(self): |
94 | 523 | self.db = get_test_db() | 527 | self.db = get_test_db() |
95 | 524 | 528 | ||
96 | 529 | def _get_app_details_from_override(self, override_dict): | ||
97 | 530 | app_dict = make_software_center_agent_app_dict() | ||
98 | 531 | app_dict.update(override_dict) | ||
99 | 532 | app_details = self._get_app_details_from_app_dict(app_dict) | ||
100 | 533 | return app_details | ||
101 | 534 | |||
102 | 525 | def _get_app_details_from_app_dict(self, app_dict): | 535 | def _get_app_details_from_app_dict(self, app_dict): |
103 | 526 | item = PistonResponseObject.from_dict(app_dict) | 536 | item = PistonResponseObject.from_dict(app_dict) |
104 | 527 | parser = SCAApplicationParser(item) | 537 | parser = SCAApplicationParser(item) |
105 | @@ -534,10 +544,8 @@ | |||
106 | 534 | # we need to patch os.path.exists as "AppDetails.channelname" will | 544 | # we need to patch os.path.exists as "AppDetails.channelname" will |
107 | 535 | # check if there is a matching channel description file on disk | 545 | # check if there is a matching channel description file on disk |
108 | 536 | os.path.exists.return_value = True | 546 | os.path.exists.return_value = True |
113 | 537 | # setup dict | 547 | app_details = self._get_app_details_from_override({ |
114 | 538 | app_dict = make_software_center_agent_app_dict() | 548 | "archive_root": "http://archive.canonical.com/"}) |
111 | 539 | app_dict["archive_root"] = "http://archive.canonical.com/" | ||
112 | 540 | app_details = self._get_app_details_from_app_dict(app_dict) | ||
115 | 541 | # ensure that archive.canonical.com archive roots are detected | 549 | # ensure that archive.canonical.com archive roots are detected |
116 | 542 | # as the partner channel | 550 | # as the partner channel |
117 | 543 | dist = softwarecenter.distro.get_distro().get_codename() | 551 | dist = softwarecenter.distro.get_distro().get_codename() |
118 | @@ -549,33 +557,68 @@ | |||
119 | 549 | # check if there is a matching channel description file on disk | 557 | # check if there is a matching channel description file on disk |
120 | 550 | os.path.exists.return_value = True | 558 | os.path.exists.return_value = True |
121 | 551 | # setup dict | 559 | # setup dict |
125 | 552 | app_dict = make_software_center_agent_app_dict() | 560 | app_details = self._get_app_details_from_override({ |
126 | 553 | app_dict["archive_root"] = "http://extras.ubuntu.com/" | 561 | "archive_root": "http://extras.ubuntu.com/"}) |
124 | 554 | app_details = self._get_app_details_from_app_dict(app_dict) | ||
127 | 555 | # ensure that archive.canonical.com archive roots are detected | 562 | # ensure that archive.canonical.com archive roots are detected |
128 | 556 | # as the partner channel | 563 | # as the partner channel |
129 | 557 | self.assertEqual(app_details.channelname, "ubuntu-extras") | 564 | self.assertEqual(app_details.channelname, "ubuntu-extras") |
130 | 558 | 565 | ||
131 | 559 | def test_date_no_published(self): | 566 | def test_date_no_published(self): |
135 | 560 | app_dict = make_software_center_agent_app_dict() | 567 | app_details = self._get_app_details_from_override({ |
136 | 561 | app_dict["date_published"] = "None" | 568 | "date_published": "None"}) |
134 | 562 | app_details = self._get_app_details_from_app_dict(app_dict) | ||
137 | 563 | # ensure that archive.canonical.com archive roots are detected | 569 | # ensure that archive.canonical.com archive roots are detected |
138 | 564 | # as the partner channel | 570 | # as the partner channel |
139 | 565 | self.assertEqual(app_details.date_published, "") | 571 | self.assertEqual(app_details.date_published, "") |
140 | 566 | # and again | 572 | # and again |
143 | 567 | app_dict["date_published"] = "2012-01-21 02:15:10.358926" | 573 | app_details = self._get_app_details_from_override({ |
144 | 568 | app_details = self._get_app_details_from_app_dict(app_dict) | 574 | "date_published": "2012-01-21 02:15:10.358926"}) |
145 | 569 | # ensure that archive.canonical.com archive roots are detected | 575 | # ensure that archive.canonical.com archive roots are detected |
146 | 570 | # as the partner channel | 576 | # as the partner channel |
147 | 571 | self.assertEqual(app_details.date_published, "2012-01-21 02:15:10") | 577 | self.assertEqual(app_details.date_published, "2012-01-21 02:15:10") |
148 | 572 | 578 | ||
149 | 573 | @patch("softwarecenter.db.update.get_region_cached") | 579 | @patch("softwarecenter.db.update.get_region_cached") |
150 | 574 | def test_region_blacklist(self, get_region_cached_mock): | 580 | def test_region_blacklist(self, get_region_cached_mock): |
155 | 575 | get_region_cached_mock.return_value = { "countrycode" : "es", | 581 | """Test that the region blacklist ignores blacklisted locations""" |
156 | 576 | } | 582 | get_region_cached_mock.return_value = { "countrycode" : "es", |
157 | 577 | app_dict = make_software_center_agent_app_dict() | 583 | } |
158 | 578 | app_dict["debtags"] = ["%s%s" % (REGION_BLACKLIST_TAG, "es")] | 584 | app_dict = make_software_center_agent_app_dict({ |
159 | 585 | "debtags": ["%s%s" % (REGION_BLACKLIST_TAG, "es")]}) | ||
160 | 586 | item = PistonResponseObject.from_dict(app_dict) | ||
161 | 587 | parser = SCAApplicationParser(item) | ||
162 | 588 | doc = parser.make_doc(self.db._aptcache) | ||
163 | 589 | self.assertEqual(doc, None) | ||
164 | 590 | |||
165 | 591 | @patch("softwarecenter.db.update.get_region_cached") | ||
166 | 592 | def test_region_blacklist_blacklists(self, get_region_cached_mock): | ||
167 | 593 | """Test that the region blacklist adds non-blacklisted locations""" | ||
168 | 594 | get_region_cached_mock.return_value = { "countrycode" : "de", | ||
169 | 595 | } | ||
170 | 596 | app_dict = make_software_center_agent_app_dict({ | ||
171 | 597 | "debtags": ["%s%s" % (REGION_BLACKLIST_TAG, "ES")]}) | ||
172 | 598 | item = PistonResponseObject.from_dict(app_dict) | ||
173 | 599 | parser = SCAApplicationParser(item) | ||
174 | 600 | doc = parser.make_doc(self.db._aptcache) | ||
175 | 601 | self.assertNotEqual(doc, None) | ||
176 | 602 | |||
177 | 603 | @patch("softwarecenter.db.update.get_region_cached") | ||
178 | 604 | def test_region_whitelist_whitelists(self, get_region_cached_mock): | ||
179 | 605 | """Test that the whitelist adds whitelisted locations""" | ||
180 | 606 | get_region_cached_mock.return_value = { "countrycode" : "es", | ||
181 | 607 | } | ||
182 | 608 | app_dict = make_software_center_agent_app_dict({ | ||
183 | 609 | "debtags": ["%s%s" % (REGION_WHITELIST_TAG, "ES")]}) | ||
184 | 610 | item = PistonResponseObject.from_dict(app_dict) | ||
185 | 611 | parser = SCAApplicationParser(item) | ||
186 | 612 | doc = parser.make_doc(self.db._aptcache) | ||
187 | 613 | self.assertNotEqual(doc, None) | ||
188 | 614 | |||
189 | 615 | @patch("softwarecenter.db.update.get_region_cached") | ||
190 | 616 | def test_region_whitelist_blacklists(self, get_region_cached_mock): | ||
191 | 617 | """Test that the whitelist ignores non-whitelist locations""" | ||
192 | 618 | get_region_cached_mock.return_value = { "countrycode" : "de", | ||
193 | 619 | } | ||
194 | 620 | app_dict = make_software_center_agent_app_dict({ | ||
195 | 621 | "debtags": ["%s%s" % (REGION_WHITELIST_TAG, "ES")]}) | ||
196 | 579 | # see _get_app_details_from_app_dict | 622 | # see _get_app_details_from_app_dict |
197 | 580 | item = PistonResponseObject.from_dict(app_dict) | 623 | item = PistonResponseObject.from_dict(app_dict) |
198 | 581 | parser = SCAApplicationParser(item) | 624 | parser = SCAApplicationParser(item) |
199 | 582 | 625 | ||
200 | === modified file 'tests/utils.py' | |||
201 | --- tests/utils.py 2012-09-06 00:49:18 +0000 | |||
202 | +++ tests/utils.py 2012-09-10 09:45:22 +0000 | |||
203 | @@ -267,7 +267,7 @@ | |||
204 | 267 | 267 | ||
205 | 268 | 268 | ||
206 | 269 | # factory stuff for the agent | 269 | # factory stuff for the agent |
208 | 270 | def make_software_center_agent_app_dict(): | 270 | def make_software_center_agent_app_dict(override_dict={}): |
209 | 271 | app_dict = { | 271 | app_dict = { |
210 | 272 | u'archive_root': 'http://private-ppa.launchpad.net/', | 272 | u'archive_root': 'http://private-ppa.launchpad.net/', |
211 | 273 | u'archive_id': u'commercial-ppa-uploaders/photobomb', | 273 | u'archive_id': u'commercial-ppa-uploaders/photobomb', |
212 | @@ -289,6 +289,7 @@ | |||
213 | 289 | '2011/08/64_Chainz.png', | 289 | '2011/08/64_Chainz.png', |
214 | 290 | u'categories': 'Game;LogicGame', | 290 | u'categories': 'Game;LogicGame', |
215 | 291 | } | 291 | } |
216 | 292 | app_dict.update(override_dict) | ||
217 | 292 | return app_dict | 293 | return app_dict |
218 | 293 | 294 | ||
219 | 294 | 295 |
Hi mvo!
Awesome, thanks for the fix. It probably makes sense to lowercase/uppercase the tags retrieved from the API, to make the feature case-insensitive. For example, in the API I see "whitelist- iso3166: :US" for an app atm, it would be great if that case just works.