Ubuntu Software Center

Merge lp:~elachuni/software-center/relax-origin-distroseries into lp:software-center

  1. relax-origin-distroseries
  2. Merge into trunk
Proposed by Anthony Lenton
Status: Merged
Merged at revision: 2675
Proposed branch: lp:~elachuni/software-center/relax-origin-distroseries
Merge into: lp:software-center
Diff against target: 248 lines (+116/-24)
4 files modified
softwarecenter/backend/reviews/__init__.py (+5/-3)
softwarecenter/backend/reviews/rnr.py (+22/-18)
softwarecenter/ui/gtk3/views/appdetailsview.py (+31/-2)
test/gtk3/test_appdetailsview.py (+58/-1)
To merge this branch: bzr merge lp:~elachuni/software-center/relax-origin-distroseries
Reviewer Review Type Date Requested Status
Michael Vogt Approve
Review via email: mp+88942@code.launchpad.net

Description of the change

Overview
========
This branch adds a 'relaxed' argument to all review loaders' get_reviews methods, to relax the origin and distroseries restriction.

Details
=======
If get_reviews() is called with relaxed=True, reviews are fetched with origin='any' and distroseries='any'.

With this branch in place, if the user selects "Any language" in the language drop down, the total number of reviews retrieved (if "Fetch more reviews" is clicked enough!) should always be equal to the total number reported by the ratings stats.

Reviews for the specific origin and distroseries will always be displayed first, and only then (if the user keeps requesting more reviews) the widget will retrieve and display reviews for other origins and distroseries.

To post a comment you must log in.
Revision history for this message
Michael Vogt (mvo) wrote :

Thanks, this looks very good, especially your use of the @patch decorator is inspiring and we should use it
more. About the test data factory, we don't have one currently but it would be good to add one.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/backend/reviews/__init__.py'
2--- softwarecenter/backend/reviews/__init__.py 2012-01-06 05:43:34 +0000
3+++ softwarecenter/backend/reviews/__init__.py 2012-01-17 21:32:24 +0000
4@@ -253,7 +253,7 @@
5 return False
6
7 def get_reviews(self, application, callback, page=1, language=None,
8- sort=0):
9+ sort=0, relaxed=False):
10 """run callback f(app, review_list)
11 with list of review objects for the given
12 db.database.Application object
13@@ -456,7 +456,8 @@
14 return random.choice(self.LOREM.split("\n\n"))
15 def _random_summary(self):
16 return random.choice(self.SUMMARIES)
17- def get_reviews(self, application, callback, page=1, language=None, sort=0):
18+ def get_reviews(self, application, callback, page=1, language=None,
19+ sort=0, relaxed=False):
20 if not application in self._review_stats_cache:
21 self.get_review_stats(application)
22 stats = self._review_stats_cache[application]
23@@ -628,7 +629,8 @@
24 self._review_stats_cache = {}
25 self._reviews_cache = {}
26
27- def get_reviews(self, application, callback, page=1, language=None, sort=0):
28+ def get_reviews(self, application, callback, page=1, language=None,
29+ sort=0, relaxed=False):
30 callback(application, [])
31
32 def get_review_stats(self, application):
33
34=== modified file 'softwarecenter/backend/reviews/rnr.py'
35--- softwarecenter/backend/reviews/rnr.py 2012-01-05 14:13:59 +0000
36+++ softwarecenter/backend/reviews/rnr.py 2012-01-17 21:32:24 +0000
37@@ -65,7 +65,7 @@
38
39 # reviews
40 def get_reviews(self, translated_app, callback, page=1,
41- language=None, sort=0):
42+ language=None, sort=0, relaxed=False):
43 """ public api, triggers fetching a review and calls callback
44 when its ready
45 """
46@@ -77,23 +77,27 @@
47 if language is None:
48 language = self.language
49 # gather args for the helper
50- try:
51- origin = self.cache.get_origin(app.pkgname)
52- except:
53- # this can happen if e.g. the app has multiple origins, this
54- # will be handled later
55- origin = None
56- # special case for not-enabled PPAs
57- if not origin and self.db:
58- details = app.get_details(self.db)
59- ppa = details.ppaname
60- if ppa:
61- origin = "lp-ppa-%s" % ppa.replace("/", "-")
62- # if there is no origin, there is nothing to do
63- if not origin:
64- callback(app, [])
65- return
66- distroseries = self.distro.get_codename()
67+ if relaxed:
68+ origin = 'any'
69+ distroseries = 'any'
70+ else:
71+ try:
72+ origin = self.cache.get_origin(app.pkgname)
73+ except:
74+ # this can happen if e.g. the app has multiple origins, this
75+ # will be handled later
76+ origin = None
77+ # special case for not-enabled PPAs
78+ if not origin and self.db:
79+ details = app.get_details(self.db)
80+ ppa = details.ppaname
81+ if ppa:
82+ origin = "lp-ppa-%s" % ppa.replace("/", "-")
83+ # if there is no origin, there is nothing to do
84+ if not origin:
85+ callback(app, [])
86+ return
87+ distroseries = self.distro.get_codename()
88 # run the command and add watcher
89 cmd = [os.path.join(softwarecenter.paths.datadir, PistonHelpers.GET_REVIEWS),
90 "--language", language,
91
92=== modified file 'softwarecenter/ui/gtk3/views/appdetailsview.py'
93--- softwarecenter/ui/gtk3/views/appdetailsview.py 2012-01-17 17:22:11 +0000
94+++ softwarecenter/ui/gtk3/views/appdetailsview.py 2012-01-17 21:32:24 +0000
95@@ -751,6 +751,7 @@
96 # reviews
97 self._reviews_server_page = 1
98 self._reviews_server_language = None
99+ self._reviews_relaxed = False
100 self._review_sort_method = 0
101
102 # switches
103@@ -814,12 +815,14 @@
104
105 def _on_review_sort_method_changed(self, uilist, sort_method):
106 self._reviews_server_page = 1
107+ self._reviews_relaxed = False
108 self._review_sort_method = sort_method
109 self.reviews.clear()
110 self._do_load_reviews()
111
112 def _on_reviews_in_different_language_clicked(self, uilist, language):
113 self._reviews_server_page = 1
114+ self._reviews_relaxed = False
115 self._reviews_server_language = language
116 self.reviews.clear()
117 self._do_load_reviews()
118@@ -830,7 +833,8 @@
119 self.app, self._reviews_ready_callback,
120 page=self._reviews_server_page,
121 language=self._reviews_server_language,
122- sort=self._review_sort_method)
123+ sort=self._review_sort_method,
124+ relaxed=self._reviews_relaxed)
125
126 def _review_update_single(self, action, review):
127 if action == 'replace':
128@@ -863,6 +867,19 @@
129 if self.app.pkgname != app.pkgname:
130 return
131
132+ # Start fetching relaxed reviews if we retrieved no data
133+ # (and if we weren't already relaxed)
134+ if not reviews_data and not self._reviews_relaxed:
135+ self._reviews_relaxed = True
136+ self._reviews_server_page = 1
137+ self.review_loader.get_reviews(
138+ self.app, self._reviews_ready_callback,
139+ page=self._reviews_server_page,
140+ language=self._reviews_server_language,
141+ sort=self._review_sort_method,
142+ relaxed=self._reviews_relaxed)
143+ return
144+
145 # update the stats (if needed). the caching can make them
146 # wrong, so if the reviews we have in the list are more than the
147 # stats we update manually
148@@ -888,10 +905,22 @@
149 if action:
150 self._review_update_single(action, single_review)
151 else:
152- curr_list = self.reviews.get_all_review_ids()
153+ curr_list = set(self.reviews.get_all_review_ids())
154+ retrieved_a_new_review = False
155 for review in reviews_data:
156 if not review.id in curr_list:
157+ retrieved_a_new_review = True
158 self.reviews.add_review(review)
159+ if reviews_data and not retrieved_a_new_review:
160+ # We retrieved data, but nothing new. Keep going.
161+ self._reviews_server_page += 1
162+ self.review_loader.get_reviews(
163+ self.app, self._reviews_ready_callback,
164+ page=self._reviews_server_page,
165+ language=self._reviews_server_language,
166+ sort=self._review_sort_method,
167+ relaxed=self._reviews_relaxed)
168+
169 self.reviews.configure_reviews_ui()
170
171 def on_weblive_progress(self, weblive, progress):
172
173=== modified file 'test/gtk3/test_appdetailsview.py'
174--- test/gtk3/test_appdetailsview.py 2012-01-17 17:22:11 +0000
175+++ test/gtk3/test_appdetailsview.py 2012-01-17 21:32:24 +0000
176@@ -6,7 +6,7 @@
177 from testutils import setup_test_env
178 setup_test_env()
179
180-from mock import Mock
181+from mock import Mock, patch
182
183 from softwarecenter.db.application import Application
184 from softwarecenter.testutils import get_mock_app_from_real_app, do_events
185@@ -170,6 +170,63 @@
186
187 self.assertEqual(1, self.view._reviews_server_page)
188
189+ @patch('softwarecenter.backend.reviews.rnr.ReviewLoaderSpawningRNRClient'
190+ '.get_reviews')
191+ def test_no_reviews_returned_attempts_relaxing(self, mock_get_reviews):
192+ """AppDetailsView._reviews_ready_callback will attempt to drop the
193+ origin and distroseries restriction if no reviews are returned
194+ with the restrictions in place.
195+ """
196+ self.view._do_load_reviews()
197+
198+ self.assertEqual(1, mock_get_reviews.call_count)
199+ kwargs = mock_get_reviews.call_args[1]
200+ self.assertEqual(False, kwargs['relaxed'])
201+ self.assertEqual(1, kwargs['page'])
202+
203+ # Now we come back with no data
204+ application, callback = mock_get_reviews.call_args[0]
205+ callback(application, [])
206+
207+ self.assertEqual(2, mock_get_reviews.call_count)
208+ kwargs = mock_get_reviews.call_args[1]
209+ self.assertEqual(True, kwargs['relaxed'])
210+ self.assertEqual(1, kwargs['page'])
211+
212+ @patch('softwarecenter.backend.reviews.rnr.ReviewLoaderSpawningRNRClient'
213+ '.get_reviews')
214+ def test_all_duplicate_reviews_keeps_going(self, mock_get_reviews):
215+ """AppDetailsView._reviews_ready_callback will fetch another page if
216+ all data returned was already displayed in the reviews list.
217+ """
218+ # Fixme: Do we have a test factory?
219+ review = Mock()
220+ review.rating = 3
221+ review.date_created = "2011-01-01 18:00:00"
222+ review.version = "1.0"
223+ review.summary = 'some summary'
224+ review.review_text = 'Some text'
225+ review.reviewer_username = "name"
226+ review.reviewer_displayname = "displayname"
227+
228+ reviews = [review]
229+ self.view.reviews.reviews = reviews
230+ self.view._do_load_reviews()
231+
232+ self.assertEqual(1, mock_get_reviews.call_count)
233+ kwargs = mock_get_reviews.call_args[1]
234+ self.assertEqual(False, kwargs['relaxed'])
235+ self.assertEqual(1, kwargs['page'])
236+
237+ # Now we come back with no NEW data
238+ application, callback = mock_get_reviews.call_args[0]
239+ callback(application, reviews)
240+
241+ self.assertEqual(2, mock_get_reviews.call_count)
242+ kwargs = mock_get_reviews.call_args[1]
243+ self.assertEqual(False, kwargs['relaxed'])
244+ self.assertEqual(2, kwargs['page'])
245+
246
247 if __name__ == "__main__":
248 import logging