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

Proposed by Anthony Lenton on 2012-01-17
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 2012-01-17 Approve on 2012-01-18
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.
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
=== modified file 'softwarecenter/backend/reviews/__init__.py'
--- softwarecenter/backend/reviews/__init__.py 2012-01-06 05:43:34 +0000
+++ softwarecenter/backend/reviews/__init__.py 2012-01-17 21:32:24 +0000
@@ -253,7 +253,7 @@
253 return False253 return False
254254
255 def get_reviews(self, application, callback, page=1, language=None,255 def get_reviews(self, application, callback, page=1, language=None,
256 sort=0):256 sort=0, relaxed=False):
257 """run callback f(app, review_list) 257 """run callback f(app, review_list)
258 with list of review objects for the given258 with list of review objects for the given
259 db.database.Application object259 db.database.Application object
@@ -456,7 +456,8 @@
456 return random.choice(self.LOREM.split("\n\n"))456 return random.choice(self.LOREM.split("\n\n"))
457 def _random_summary(self):457 def _random_summary(self):
458 return random.choice(self.SUMMARIES)458 return random.choice(self.SUMMARIES)
459 def get_reviews(self, application, callback, page=1, language=None, sort=0):459 def get_reviews(self, application, callback, page=1, language=None,
460 sort=0, relaxed=False):
460 if not application in self._review_stats_cache:461 if not application in self._review_stats_cache:
461 self.get_review_stats(application)462 self.get_review_stats(application)
462 stats = self._review_stats_cache[application]463 stats = self._review_stats_cache[application]
@@ -628,7 +629,8 @@
628 self._review_stats_cache = {}629 self._review_stats_cache = {}
629 self._reviews_cache = {}630 self._reviews_cache = {}
630631
631 def get_reviews(self, application, callback, page=1, language=None, sort=0):632 def get_reviews(self, application, callback, page=1, language=None,
633 sort=0, relaxed=False):
632 callback(application, [])634 callback(application, [])
633635
634 def get_review_stats(self, application):636 def get_review_stats(self, application):
635637
=== modified file 'softwarecenter/backend/reviews/rnr.py'
--- softwarecenter/backend/reviews/rnr.py 2012-01-05 14:13:59 +0000
+++ softwarecenter/backend/reviews/rnr.py 2012-01-17 21:32:24 +0000
@@ -65,7 +65,7 @@
6565
66 # reviews66 # reviews
67 def get_reviews(self, translated_app, callback, page=1, 67 def get_reviews(self, translated_app, callback, page=1,
68 language=None, sort=0):68 language=None, sort=0, relaxed=False):
69 """ public api, triggers fetching a review and calls callback69 """ public api, triggers fetching a review and calls callback
70 when its ready70 when its ready
71 """71 """
@@ -77,23 +77,27 @@
77 if language is None:77 if language is None:
78 language = self.language78 language = self.language
79 # gather args for the helper79 # gather args for the helper
80 try:80 if relaxed:
81 origin = self.cache.get_origin(app.pkgname)81 origin = 'any'
82 except:82 distroseries = 'any'
83 # this can happen if e.g. the app has multiple origins, this83 else:
84 # will be handled later84 try:
85 origin = None85 origin = self.cache.get_origin(app.pkgname)
86 # special case for not-enabled PPAs86 except:
87 if not origin and self.db:87 # this can happen if e.g. the app has multiple origins, this
88 details = app.get_details(self.db)88 # will be handled later
89 ppa = details.ppaname89 origin = None
90 if ppa:90 # special case for not-enabled PPAs
91 origin = "lp-ppa-%s" % ppa.replace("/", "-")91 if not origin and self.db:
92 # if there is no origin, there is nothing to do92 details = app.get_details(self.db)
93 if not origin:93 ppa = details.ppaname
94 callback(app, [])94 if ppa:
95 return95 origin = "lp-ppa-%s" % ppa.replace("/", "-")
96 distroseries = self.distro.get_codename()96 # if there is no origin, there is nothing to do
97 if not origin:
98 callback(app, [])
99 return
100 distroseries = self.distro.get_codename()
97 # run the command and add watcher101 # run the command and add watcher
98 cmd = [os.path.join(softwarecenter.paths.datadir, PistonHelpers.GET_REVIEWS),102 cmd = [os.path.join(softwarecenter.paths.datadir, PistonHelpers.GET_REVIEWS),
99 "--language", language, 103 "--language", language,
100104
=== modified file 'softwarecenter/ui/gtk3/views/appdetailsview.py'
--- softwarecenter/ui/gtk3/views/appdetailsview.py 2012-01-17 17:22:11 +0000
+++ softwarecenter/ui/gtk3/views/appdetailsview.py 2012-01-17 21:32:24 +0000
@@ -751,6 +751,7 @@
751 # reviews751 # reviews
752 self._reviews_server_page = 1752 self._reviews_server_page = 1
753 self._reviews_server_language = None753 self._reviews_server_language = None
754 self._reviews_relaxed = False
754 self._review_sort_method = 0755 self._review_sort_method = 0
755 756
756 # switches757 # switches
@@ -814,12 +815,14 @@
814 815
815 def _on_review_sort_method_changed(self, uilist, sort_method):816 def _on_review_sort_method_changed(self, uilist, sort_method):
816 self._reviews_server_page = 1817 self._reviews_server_page = 1
818 self._reviews_relaxed = False
817 self._review_sort_method = sort_method819 self._review_sort_method = sort_method
818 self.reviews.clear()820 self.reviews.clear()
819 self._do_load_reviews()821 self._do_load_reviews()
820822
821 def _on_reviews_in_different_language_clicked(self, uilist, language):823 def _on_reviews_in_different_language_clicked(self, uilist, language):
822 self._reviews_server_page = 1824 self._reviews_server_page = 1
825 self._reviews_relaxed = False
823 self._reviews_server_language = language826 self._reviews_server_language = language
824 self.reviews.clear()827 self.reviews.clear()
825 self._do_load_reviews()828 self._do_load_reviews()
@@ -830,7 +833,8 @@
830 self.app, self._reviews_ready_callback, 833 self.app, self._reviews_ready_callback,
831 page=self._reviews_server_page,834 page=self._reviews_server_page,
832 language=self._reviews_server_language,835 language=self._reviews_server_language,
833 sort=self._review_sort_method)836 sort=self._review_sort_method,
837 relaxed=self._reviews_relaxed)
834838
835 def _review_update_single(self, action, review):839 def _review_update_single(self, action, review):
836 if action == 'replace':840 if action == 'replace':
@@ -863,6 +867,19 @@
863 if self.app.pkgname != app.pkgname:867 if self.app.pkgname != app.pkgname:
864 return868 return
865869
870 # Start fetching relaxed reviews if we retrieved no data
871 # (and if we weren't already relaxed)
872 if not reviews_data and not self._reviews_relaxed:
873 self._reviews_relaxed = True
874 self._reviews_server_page = 1
875 self.review_loader.get_reviews(
876 self.app, self._reviews_ready_callback,
877 page=self._reviews_server_page,
878 language=self._reviews_server_language,
879 sort=self._review_sort_method,
880 relaxed=self._reviews_relaxed)
881 return
882
866 # update the stats (if needed). the caching can make them883 # update the stats (if needed). the caching can make them
867 # wrong, so if the reviews we have in the list are more than the884 # wrong, so if the reviews we have in the list are more than the
868 # stats we update manually885 # stats we update manually
@@ -888,10 +905,22 @@
888 if action:905 if action:
889 self._review_update_single(action, single_review)906 self._review_update_single(action, single_review)
890 else:907 else:
891 curr_list = self.reviews.get_all_review_ids()908 curr_list = set(self.reviews.get_all_review_ids())
909 retrieved_a_new_review = False
892 for review in reviews_data:910 for review in reviews_data:
893 if not review.id in curr_list:911 if not review.id in curr_list:
912 retrieved_a_new_review = True
894 self.reviews.add_review(review)913 self.reviews.add_review(review)
914 if reviews_data and not retrieved_a_new_review:
915 # We retrieved data, but nothing new. Keep going.
916 self._reviews_server_page += 1
917 self.review_loader.get_reviews(
918 self.app, self._reviews_ready_callback,
919 page=self._reviews_server_page,
920 language=self._reviews_server_language,
921 sort=self._review_sort_method,
922 relaxed=self._reviews_relaxed)
923
895 self.reviews.configure_reviews_ui()924 self.reviews.configure_reviews_ui()
896925
897 def on_weblive_progress(self, weblive, progress):926 def on_weblive_progress(self, weblive, progress):
898927
=== modified file 'test/gtk3/test_appdetailsview.py'
--- test/gtk3/test_appdetailsview.py 2012-01-17 17:22:11 +0000
+++ test/gtk3/test_appdetailsview.py 2012-01-17 21:32:24 +0000
@@ -6,7 +6,7 @@
6from testutils import setup_test_env6from testutils import setup_test_env
7setup_test_env()7setup_test_env()
88
9from mock import Mock9from mock import Mock, patch
1010
11from softwarecenter.db.application import Application11from softwarecenter.db.application import Application
12from softwarecenter.testutils import get_mock_app_from_real_app, do_events12from softwarecenter.testutils import get_mock_app_from_real_app, do_events
@@ -170,6 +170,63 @@
170170
171 self.assertEqual(1, self.view._reviews_server_page)171 self.assertEqual(1, self.view._reviews_server_page)
172172
173 @patch('softwarecenter.backend.reviews.rnr.ReviewLoaderSpawningRNRClient'
174 '.get_reviews')
175 def test_no_reviews_returned_attempts_relaxing(self, mock_get_reviews):
176 """AppDetailsView._reviews_ready_callback will attempt to drop the
177 origin and distroseries restriction if no reviews are returned
178 with the restrictions in place.
179 """
180 self.view._do_load_reviews()
181
182 self.assertEqual(1, mock_get_reviews.call_count)
183 kwargs = mock_get_reviews.call_args[1]
184 self.assertEqual(False, kwargs['relaxed'])
185 self.assertEqual(1, kwargs['page'])
186
187 # Now we come back with no data
188 application, callback = mock_get_reviews.call_args[0]
189 callback(application, [])
190
191 self.assertEqual(2, mock_get_reviews.call_count)
192 kwargs = mock_get_reviews.call_args[1]
193 self.assertEqual(True, kwargs['relaxed'])
194 self.assertEqual(1, kwargs['page'])
195
196 @patch('softwarecenter.backend.reviews.rnr.ReviewLoaderSpawningRNRClient'
197 '.get_reviews')
198 def test_all_duplicate_reviews_keeps_going(self, mock_get_reviews):
199 """AppDetailsView._reviews_ready_callback will fetch another page if
200 all data returned was already displayed in the reviews list.
201 """
202 # Fixme: Do we have a test factory?
203 review = Mock()
204 review.rating = 3
205 review.date_created = "2011-01-01 18:00:00"
206 review.version = "1.0"
207 review.summary = 'some summary'
208 review.review_text = 'Some text'
209 review.reviewer_username = "name"
210 review.reviewer_displayname = "displayname"
211
212 reviews = [review]
213 self.view.reviews.reviews = reviews
214 self.view._do_load_reviews()
215
216 self.assertEqual(1, mock_get_reviews.call_count)
217 kwargs = mock_get_reviews.call_args[1]
218 self.assertEqual(False, kwargs['relaxed'])
219 self.assertEqual(1, kwargs['page'])
220
221 # Now we come back with no NEW data
222 application, callback = mock_get_reviews.call_args[0]
223 callback(application, reviews)
224
225 self.assertEqual(2, mock_get_reviews.call_count)
226 kwargs = mock_get_reviews.call_args[1]
227 self.assertEqual(False, kwargs['relaxed'])
228 self.assertEqual(2, kwargs['page'])
229
173230
174if __name__ == "__main__":231if __name__ == "__main__":
175 import logging232 import logging