Merge lp:~lifeless/launchpad/bug-607935 into lp:launchpad
- bug-607935
- Merge into devel
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Robert Collins | ||||
Approved revision: | no longer in the source branch. | ||||
Merged at revision: | 12398 | ||||
Proposed branch: | lp:~lifeless/launchpad/bug-607935 | ||||
Merge into: | lp:launchpad | ||||
Diff against target: |
940 lines (+266/-223) 12 files modified
lib/lp/bugs/browser/bugcomment.py (+39/-38) lib/lp/bugs/browser/bugtask.py (+84/-77) lib/lp/bugs/browser/tests/bug-views.txt (+5/-4) lib/lp/bugs/browser/tests/test_bugcomment.py (+33/-31) lib/lp/bugs/configure.zcml (+1/-1) lib/lp/bugs/doc/bug.txt (+22/-4) lib/lp/bugs/doc/bugcomment.txt (+18/-30) lib/lp/bugs/interfaces/bug.py (+9/-2) lib/lp/bugs/interfaces/bugmessage.py (+1/-0) lib/lp/bugs/model/bug.py (+50/-32) lib/lp/bugs/templates/bugcomment-macros.pt (+1/-1) lib/lp/bugs/templates/bugtask-index.pt (+3/-3) |
||||
To merge this branch: | bzr merge lp:~lifeless/launchpad/bug-607935 | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Stuart Bishop (community) | Approve | ||
Review via email: mp+49915@code.launchpad.net |
Commit message
[r=stub][bug=607935] Reduce overhead when showing only some bug comments.
Description of the change
In this iteration I start doing range requests on BugMessage.index and no longer read in every bug message for every page load.
This required refactoring the stack by which such messages are obtained - we really should ditch BugComment I think and just use BugMessage directly (having it delegate to Message for the message fields). But thats a different project.
Anyhow, there were some properties on the bug task view that required access to all messages to work - e.g. the concept of 'how many bug comments are visible with ?show=all present' requires us to load the text of every bug message because of the algorithm in play. I've taken the view that our users will value speed of page loading over absolute precision and adjusted these to show the total messages in the system, rather than the total that would be shown. This is massively faster.
The largest expected benefit from this branch is less Disk IO - e.g. in bug one, we should skip messages 41->1280 or so. Thats a good 5 seconds of IO when it occurs - and it occurs frequently.
As a drive by I fixed the bug message grouping logic to be stable on bugmessage.index if/when two bug comments have the same datetime - which can happen in tests (there was a fragile test failing 1/3rds of the time for me).
I also fixed the logic for putting 'show all message' fillers in to show them where any hidden message is, though still guarded by the overall check for whether we're bulk-hiding messages. This was needed to avoid going up to two identical queries to get different ranges of comments.
Preview Diff
1 | === modified file 'lib/lp/bugs/browser/bugcomment.py' | |||
2 | --- lib/lp/bugs/browser/bugcomment.py 2010-12-20 13:39:41 +0000 | |||
3 | +++ lib/lp/bugs/browser/bugcomment.py 2011-02-16 20:08:14 +0000 | |||
4 | @@ -55,41 +55,33 @@ | |||
5 | 55 | COMMENT_ACTIVITY_GROUPING_WINDOW = timedelta(minutes=5) | 55 | COMMENT_ACTIVITY_GROUPING_WINDOW = timedelta(minutes=5) |
6 | 56 | 56 | ||
7 | 57 | 57 | ||
10 | 58 | def build_comments_from_chunks(chunks, bugtask, truncate=False): | 58 | def build_comments_from_chunks(bugtask, truncate=False, slice_info=None): |
11 | 59 | """Build BugComments from MessageChunks.""" | 59 | """Build BugComments from MessageChunks. |
12 | 60 | |||
13 | 61 | :param truncate: Perform truncation of large messages. | ||
14 | 62 | :param slice_info: If not None, an iterable of slices to retrieve. | ||
15 | 63 | """ | ||
16 | 64 | chunks = bugtask.bug.getMessagesForView(slice_info=slice_info) | ||
17 | 65 | # This would be better as part of indexed_messages eager loading. | ||
18 | 60 | comments = {} | 66 | comments = {} |
23 | 61 | index = 0 | 67 | for bugmessage, message, chunk in chunks: |
24 | 62 | for chunk in chunks: | 68 | bug_comment = comments.get(message.id) |
21 | 63 | message_id = chunk.message.id | ||
22 | 64 | bug_comment = comments.get(message_id) | ||
25 | 65 | if bug_comment is None: | 69 | if bug_comment is None: |
30 | 66 | bug_comment = BugComment( | 70 | bug_comment = BugComment(bugmessage.index, message, bugtask, |
31 | 67 | index, chunk.message, bugtask) | 71 | visible=bugmessage.visible) |
32 | 68 | comments[message_id] = bug_comment | 72 | comments[message.id] = bug_comment |
33 | 69 | index += 1 | 73 | # This code path is currently only used from BugTask view which |
34 | 74 | # have already loaded all the bug watches. If we start lazy loading | ||
35 | 75 | # those, or not needing them we will need to batch lookup watches | ||
36 | 76 | # here. | ||
37 | 77 | if bugmessage.bugwatchID is not None: | ||
38 | 78 | bug_comment.bugwatch = bugmessage.bugwatch | ||
39 | 79 | bug_comment.synchronized = ( | ||
40 | 80 | bugmessage.remote_comment_id is not None) | ||
41 | 70 | bug_comment.chunks.append(chunk) | 81 | bug_comment.chunks.append(chunk) |
42 | 71 | 82 | ||
43 | 72 | # Set up the bug watch for all the imported comments. We do it | ||
44 | 73 | # outside the for loop to avoid issuing one db query per comment. | ||
45 | 74 | imported_bug_messages = getUtility(IBugMessageSet).getImportedBugMessages( | ||
46 | 75 | bugtask.bug) | ||
47 | 76 | for bug_message in imported_bug_messages: | ||
48 | 77 | message_id = bug_message.message.id | ||
49 | 78 | comments[message_id].bugwatch = bug_message.bugwatch | ||
50 | 79 | comments[message_id].synchronized = ( | ||
51 | 80 | bug_message.remote_comment_id is not None) | ||
52 | 81 | |||
53 | 82 | for bug_message in bugtask.bug.bug_messages: | ||
54 | 83 | comment = comments.get(bug_message.messageID, None) | ||
55 | 84 | # XXX intellectronica 2009-04-22, bug=365092: Currently, there are | ||
56 | 85 | # some bug messages for which no chunks exist in the DB, so we need to | ||
57 | 86 | # make sure that we skip them, since the corresponding message wont | ||
58 | 87 | # have been added to the comments dictionary in the section above. | ||
59 | 88 | if comment is not None: | ||
60 | 89 | comment.visible = bug_message.visible | ||
61 | 90 | |||
62 | 91 | for comment in comments.values(): | 83 | for comment in comments.values(): |
64 | 92 | # Once we have all the chunks related to a comment set up, | 84 | # Once we have all the chunks related to a comment populated, |
65 | 93 | # we get the text set up for display. | 85 | # we get the text set up for display. |
66 | 94 | comment.setupText(truncate=truncate) | 86 | comment.setupText(truncate=truncate) |
67 | 95 | return comments | 87 | return comments |
68 | @@ -101,20 +93,28 @@ | |||
69 | 101 | Generates a stream of comment instances (with the activity grouped within) | 93 | Generates a stream of comment instances (with the activity grouped within) |
70 | 102 | or `list`s of grouped activities. | 94 | or `list`s of grouped activities. |
71 | 103 | 95 | ||
73 | 104 | :param comments: An iterable of `BugComment` instances. | 96 | :param comments: An iterable of `BugComment` instances, which should be |
74 | 97 | sorted by index already. | ||
75 | 105 | :param activities: An iterable of `BugActivity` instances. | 98 | :param activities: An iterable of `BugActivity` instances. |
76 | 106 | """ | 99 | """ |
77 | 107 | window = COMMENT_ACTIVITY_GROUPING_WINDOW | 100 | window = COMMENT_ACTIVITY_GROUPING_WINDOW |
78 | 108 | 101 | ||
79 | 109 | comment_kind = "comment" | 102 | comment_kind = "comment" |
80 | 103 | if comments: | ||
81 | 104 | max_index = comments[-1].index + 1 | ||
82 | 105 | else: | ||
83 | 106 | max_index = 0 | ||
84 | 110 | comments = ( | 107 | comments = ( |
86 | 111 | (comment.datecreated, comment.owner, comment_kind, comment) | 108 | (comment.datecreated, comment.index, comment.owner, comment_kind, comment) |
87 | 112 | for comment in comments) | 109 | for comment in comments) |
88 | 113 | activity_kind = "activity" | 110 | activity_kind = "activity" |
89 | 114 | activity = ( | 111 | activity = ( |
91 | 115 | (activity.datechanged, activity.person, activity_kind, activity) | 112 | (activity.datechanged, max_index, activity.person, activity_kind, activity) |
92 | 116 | for activity in activities) | 113 | for activity in activities) |
94 | 117 | events = sorted(chain(comments, activity), key=itemgetter(0, 1)) | 114 | # when an action and a comment happen at the same time, the action comes |
95 | 115 | # second, when two events are tied the comment index is used to | ||
96 | 116 | # disambiguate. | ||
97 | 117 | events = sorted(chain(comments, activity), key=itemgetter(0, 1, 2)) | ||
98 | 118 | 118 | ||
99 | 119 | def gen_event_windows(events): | 119 | def gen_event_windows(events): |
100 | 120 | """Generate event windows. | 120 | """Generate event windows. |
101 | @@ -123,12 +123,12 @@ | |||
102 | 123 | an integer, and is incremented each time the windowing conditions are | 123 | an integer, and is incremented each time the windowing conditions are |
103 | 124 | triggered. | 124 | triggered. |
104 | 125 | 125 | ||
107 | 126 | :param events: An iterable of `(date, actor, kind, event)` tuples in | 126 | :param events: An iterable of `(date, ignored, actor, kind, event)` |
108 | 127 | order. | 127 | tuples in order. |
109 | 128 | """ | 128 | """ |
110 | 129 | window_comment, window_actor = None, None | 129 | window_comment, window_actor = None, None |
111 | 130 | window_index, window_end = 0, None | 130 | window_index, window_end = 0, None |
113 | 131 | for date, actor, kind, event in events: | 131 | for date, _, actor, kind, event in events: |
114 | 132 | window_ended = ( | 132 | window_ended = ( |
115 | 133 | # A window may contain only one comment. | 133 | # A window may contain only one comment. |
116 | 134 | (window_comment is not None and kind is comment_kind) or | 134 | (window_comment is not None and kind is comment_kind) or |
117 | @@ -174,7 +174,7 @@ | |||
118 | 174 | """ | 174 | """ |
119 | 175 | implements(IBugComment) | 175 | implements(IBugComment) |
120 | 176 | 176 | ||
122 | 177 | def __init__(self, index, message, bugtask, activity=None): | 177 | def __init__(self, index, message, bugtask, activity=None, visible=True): |
123 | 178 | self.index = index | 178 | self.index = index |
124 | 179 | self.bugtask = bugtask | 179 | self.bugtask = bugtask |
125 | 180 | self.bugwatch = None | 180 | self.bugwatch = None |
126 | @@ -195,6 +195,7 @@ | |||
127 | 195 | self.activity = activity | 195 | self.activity = activity |
128 | 196 | 196 | ||
129 | 197 | self.synchronized = False | 197 | self.synchronized = False |
130 | 198 | self.visible = visible | ||
131 | 198 | 199 | ||
132 | 199 | @property | 200 | @property |
133 | 200 | def show_for_admin(self): | 201 | def show_for_admin(self): |
134 | 201 | 202 | ||
135 | === modified file 'lib/lp/bugs/browser/bugtask.py' | |||
136 | --- lib/lp/bugs/browser/bugtask.py 2011-02-11 18:15:28 +0000 | |||
137 | +++ lib/lp/bugs/browser/bugtask.py 2011-02-16 20:08:14 +0000 | |||
138 | @@ -293,20 +293,30 @@ | |||
139 | 293 | return title.strip() | 293 | return title.strip() |
140 | 294 | 294 | ||
141 | 295 | 295 | ||
143 | 296 | def get_comments_for_bugtask(bugtask, truncate=False): | 296 | def get_comments_for_bugtask(bugtask, truncate=False, for_display=False, |
144 | 297 | slice_info=None): | ||
145 | 297 | """Return BugComments related to a bugtask. | 298 | """Return BugComments related to a bugtask. |
146 | 298 | 299 | ||
147 | 299 | This code builds a sorted list of BugComments in one shot, | 300 | This code builds a sorted list of BugComments in one shot, |
148 | 300 | requiring only two database queries. It removes the titles | 301 | requiring only two database queries. It removes the titles |
149 | 301 | for those comments which do not have a "new" subject line | 302 | for those comments which do not have a "new" subject line |
150 | 303 | |||
151 | 304 | :param for_display: If true, the zeroth comment is given an empty body so | ||
152 | 305 | that it will be filtered by get_visible_comments. | ||
153 | 306 | :param slice_info: If not None, defines a list of slices of the comments to | ||
154 | 307 | retrieve. | ||
155 | 302 | """ | 308 | """ |
158 | 303 | chunks = bugtask.bug.getMessageChunks() | 309 | comments = build_comments_from_chunks(bugtask, truncate=truncate, |
159 | 304 | comments = build_comments_from_chunks(chunks, bugtask, truncate=truncate) | 310 | slice_info=slice_info) |
160 | 311 | # TODO: further fat can be shaved off here by limiting the attachments we | ||
161 | 312 | # query to those that slice_info would include. | ||
162 | 305 | for attachment in bugtask.bug.attachments_unpopulated: | 313 | for attachment in bugtask.bug.attachments_unpopulated: |
163 | 306 | message_id = attachment.message.id | 314 | message_id = attachment.message.id |
164 | 307 | # All attachments are related to a message, so we can be | 315 | # All attachments are related to a message, so we can be |
165 | 308 | # sure that the BugComment is already created. | 316 | # sure that the BugComment is already created. |
167 | 309 | assert message_id in comments, message_id | 317 | if message_id not in comments: |
168 | 318 | # We are not showing this message. | ||
169 | 319 | break | ||
170 | 310 | if attachment.type == BugAttachmentType.PATCH: | 320 | if attachment.type == BugAttachmentType.PATCH: |
171 | 311 | comments[message_id].patches.append(attachment) | 321 | comments[message_id].patches.append(attachment) |
172 | 312 | else: | 322 | else: |
173 | @@ -321,6 +331,12 @@ | |||
174 | 321 | # this comment has a new title, so make that the rolling focus | 331 | # this comment has a new title, so make that the rolling focus |
175 | 322 | current_title = comment.title | 332 | current_title = comment.title |
176 | 323 | comment.display_title = True | 333 | comment.display_title = True |
177 | 334 | if for_display and comments and comments[0].index==0: | ||
178 | 335 | # We show the text of the first comment as the bug description, | ||
179 | 336 | # or via the special link "View original description", but we want | ||
180 | 337 | # to display attachments filed together with the bug in the | ||
181 | 338 | # comment list. | ||
182 | 339 | comments[0].text_for_display = '' | ||
183 | 324 | return comments | 340 | return comments |
184 | 325 | 341 | ||
185 | 326 | 342 | ||
186 | @@ -344,7 +360,8 @@ | |||
187 | 344 | 360 | ||
188 | 345 | # These two lines are here to fill the ValidPersonOrTeamCache cache, | 361 | # These two lines are here to fill the ValidPersonOrTeamCache cache, |
189 | 346 | # so that checking owner.is_valid_person, when rendering the link, | 362 | # so that checking owner.is_valid_person, when rendering the link, |
191 | 347 | # won't issue a DB query. | 363 | # won't issue a DB query. Note that this should be obsolete now with |
192 | 364 | # getMessagesForView improvements. | ||
193 | 348 | commenters = set(comment.owner for comment in visible_comments) | 365 | commenters = set(comment.owner for comment in visible_comments) |
194 | 349 | getUtility(IPersonSet).getValidPersons(commenters) | 366 | getUtility(IPersonSet).getValidPersons(commenters) |
195 | 350 | 367 | ||
196 | @@ -686,7 +703,8 @@ | |||
197 | 686 | # This line of code keeps the view's query count down, | 703 | # This line of code keeps the view's query count down, |
198 | 687 | # possibly using witchcraft. It should be rewritten to be | 704 | # possibly using witchcraft. It should be rewritten to be |
199 | 688 | # useful or removed in favour of making other queries more | 705 | # useful or removed in favour of making other queries more |
201 | 689 | # efficient. | 706 | # efficient. The witchcraft is because the subscribers are accessed |
202 | 707 | # in the initial page load, so the data is actually used. | ||
203 | 690 | if self.user is not None: | 708 | if self.user is not None: |
204 | 691 | list(bug.getSubscribersForPerson(self.user)) | 709 | list(bug.getSubscribersForPerson(self.user)) |
205 | 692 | 710 | ||
206 | @@ -789,14 +807,8 @@ | |||
207 | 789 | @cachedproperty | 807 | @cachedproperty |
208 | 790 | def comments(self): | 808 | def comments(self): |
209 | 791 | """Return the bugtask's comments.""" | 809 | """Return the bugtask's comments.""" |
218 | 792 | comments = get_comments_for_bugtask(self.context, truncate=True) | 810 | return get_comments_for_bugtask(self.context, truncate=True, |
219 | 793 | # We show the text of the first comment as the bug description, | 811 | for_display=True) |
212 | 794 | # or via the special link "View original description", but we want | ||
213 | 795 | # to display attachments filed together with the bug in the | ||
214 | 796 | # comment list. | ||
215 | 797 | comments[0].text_for_display = '' | ||
216 | 798 | assert len(comments) > 0, "A bug should have at least one comment." | ||
217 | 799 | return comments | ||
220 | 800 | 812 | ||
221 | 801 | @cachedproperty | 813 | @cachedproperty |
222 | 802 | def interesting_activity(self): | 814 | def interesting_activity(self): |
223 | @@ -834,11 +846,22 @@ | |||
224 | 834 | + config.malone.comments_list_truncate_newest_to | 846 | + config.malone.comments_list_truncate_newest_to |
225 | 835 | < config.malone.comments_list_max_length) | 847 | < config.malone.comments_list_max_length) |
226 | 836 | 848 | ||
229 | 837 | recent_comments = self.visible_recent_comments_for_display | 849 | if not self.visible_comments_truncated_for_display: |
230 | 838 | oldest_comments = self.visible_oldest_comments_for_display | 850 | comments=self.comments |
231 | 851 | else: | ||
232 | 852 | # the comment function takes 0-offset counts where comment 0 is | ||
233 | 853 | # the initial description, so we need to add one to the limits | ||
234 | 854 | # to adjust. | ||
235 | 855 | oldest_count = 1 + self.visible_initial_comments | ||
236 | 856 | new_count = 1 + self.total_comments-self.visible_recent_comments | ||
237 | 857 | comments = get_comments_for_bugtask( | ||
238 | 858 | self.context, truncate=True, for_display=True, | ||
239 | 859 | slice_info=[ | ||
240 | 860 | slice(None, oldest_count), slice(new_count, None)]) | ||
241 | 861 | visible_comments = get_visible_comments(comments) | ||
242 | 839 | 862 | ||
243 | 840 | event_groups = group_comments_with_activity( | 863 | event_groups = group_comments_with_activity( |
245 | 841 | comments=chain(oldest_comments, recent_comments), | 864 | comments=visible_comments, |
246 | 842 | activities=self.interesting_activity) | 865 | activities=self.interesting_activity) |
247 | 843 | 866 | ||
248 | 844 | def group_activities_by_target(activities): | 867 | def group_activities_by_target(activities): |
249 | @@ -881,77 +904,61 @@ | |||
250 | 881 | 904 | ||
251 | 882 | events = map(event_dict, event_groups) | 905 | events = map(event_dict, event_groups) |
252 | 883 | 906 | ||
255 | 884 | # Insert blank if we're showing only a subset of the comment list. | 907 | # Insert blanks if we're showing only a subset of the comment list. |
256 | 885 | if len(recent_comments) > 0: | 908 | if self.visible_comments_truncated_for_display: |
257 | 886 | # Find the oldest recent comment in the event list. | 909 | # Find the oldest recent comment in the event list. |
265 | 887 | oldest_recent_comment = recent_comments[0] | 910 | index = 0 |
266 | 888 | for index, event in enumerate(events): | 911 | prev_comment = None |
267 | 889 | if event.get("comment") is oldest_recent_comment: | 912 | while index < len(events): |
268 | 890 | num_hidden = ( | 913 | event = events[index] |
269 | 891 | len(self.visible_comments) | 914 | comment = event.get("comment") |
270 | 892 | - len(oldest_comments) | 915 | if prev_comment is None: |
271 | 893 | - len(recent_comments)) | 916 | prev_comment = comment |
272 | 917 | index += 1 | ||
273 | 918 | continue | ||
274 | 919 | if comment is None: | ||
275 | 920 | index += 1 | ||
276 | 921 | continue | ||
277 | 922 | if prev_comment.index + 1 != comment.index: | ||
278 | 923 | # There is a gap here, record it. | ||
279 | 894 | separator = { | 924 | separator = { |
282 | 895 | 'date': oldest_recent_comment.datecreated, | 925 | 'date': prev_comment.datecreated, |
283 | 896 | 'num_hidden': num_hidden, | 926 | 'num_hidden': comment.index - prev_comment.index |
284 | 897 | } | 927 | } |
285 | 898 | events.insert(index, separator) | 928 | events.insert(index, separator) |
288 | 899 | break | 929 | index += 1 |
289 | 900 | 930 | prev_comment = comment | |
290 | 931 | index += 1 | ||
291 | 901 | return events | 932 | return events |
292 | 902 | 933 | ||
337 | 903 | @cachedproperty | 934 | @property |
338 | 904 | def visible_comments(self): | 935 | def visible_initial_comments(self): |
339 | 905 | """All visible comments. | 936 | """How many initial comments are being shown.""" |
340 | 906 | 937 | return config.malone.comments_list_truncate_oldest_to | |
341 | 907 | See `get_visible_comments` for the definition of a "visible" | 938 | |
342 | 908 | comment. | 939 | @property |
343 | 909 | """ | 940 | def visible_recent_comments(self): |
344 | 910 | return get_visible_comments(self.comments) | 941 | """How many recent comments are being shown.""" |
345 | 911 | 942 | return config.malone.comments_list_truncate_newest_to | |
346 | 912 | @cachedproperty | 943 | |
347 | 913 | def visible_oldest_comments_for_display(self): | 944 | @cachedproperty |
304 | 914 | """The list of oldest visible comments to be rendered. | ||
305 | 915 | |||
306 | 916 | This considers truncating the comment list if there are tons | ||
307 | 917 | of comments, but also obeys any explicitly requested ways to | ||
308 | 918 | display comments (currently only "all" is recognised). | ||
309 | 919 | """ | ||
310 | 920 | show_all = (self.request.form_ng.getOne('comments') == 'all') | ||
311 | 921 | max_comments = config.malone.comments_list_max_length | ||
312 | 922 | if show_all or len(self.visible_comments) <= max_comments: | ||
313 | 923 | return self.visible_comments | ||
314 | 924 | else: | ||
315 | 925 | oldest_count = config.malone.comments_list_truncate_oldest_to | ||
316 | 926 | return self.visible_comments[:oldest_count] | ||
317 | 927 | |||
318 | 928 | @cachedproperty | ||
319 | 929 | def visible_recent_comments_for_display(self): | ||
320 | 930 | """The list of recent visible comments to be rendered. | ||
321 | 931 | |||
322 | 932 | If the number of comments is beyond the maximum threshold, this | ||
323 | 933 | returns the most recent few comments. If we're under the threshold, | ||
324 | 934 | then visible_oldest_comments_for_display will be returning the bugs, | ||
325 | 935 | so this routine will return an empty set to avoid duplication. | ||
326 | 936 | """ | ||
327 | 937 | show_all = (self.request.form_ng.getOne('comments') == 'all') | ||
328 | 938 | max_comments = config.malone.comments_list_max_length | ||
329 | 939 | total = len(self.visible_comments) | ||
330 | 940 | if show_all or total <= max_comments: | ||
331 | 941 | return [] | ||
332 | 942 | else: | ||
333 | 943 | start = total - config.malone.comments_list_truncate_newest_to | ||
334 | 944 | return self.visible_comments[start:total] | ||
335 | 945 | |||
336 | 946 | @property | ||
348 | 947 | def visible_comments_truncated_for_display(self): | 945 | def visible_comments_truncated_for_display(self): |
349 | 948 | """Whether the visible comment list is truncated for display.""" | 946 | """Whether the visible comment list is truncated for display.""" |
352 | 949 | return (len(self.visible_comments) > | 947 | show_all = (self.request.form_ng.getOne('comments') == 'all') |
353 | 950 | len(self.visible_oldest_comments_for_display)) | 948 | if show_all: |
354 | 949 | return False | ||
355 | 950 | max_comments = config.malone.comments_list_max_length | ||
356 | 951 | return self.total_comments > max_comments | ||
357 | 952 | |||
358 | 953 | @cachedproperty | ||
359 | 954 | def total_comments(self): | ||
360 | 955 | """We count all comments because the db cannot do visibility yet.""" | ||
361 | 956 | return self.context.bug.bug_messages.count() - 1 | ||
362 | 951 | 957 | ||
363 | 952 | def wasDescriptionModified(self): | 958 | def wasDescriptionModified(self): |
364 | 953 | """Return a boolean indicating whether the description was modified""" | 959 | """Return a boolean indicating whether the description was modified""" |
366 | 954 | return self.comments[0].text_contents != self.context.bug.description | 960 | return (self.context.bug.indexed_messages[0].text_contents != |
367 | 961 | self.context.bug.description) | ||
368 | 955 | 962 | ||
369 | 956 | @cachedproperty | 963 | @cachedproperty |
370 | 957 | def linked_branches(self): | 964 | def linked_branches(self): |
371 | 958 | 965 | ||
372 | === modified file 'lib/lp/bugs/browser/tests/bug-views.txt' | |||
373 | --- lib/lp/bugs/browser/tests/bug-views.txt 2010-12-21 20:57:11 +0000 | |||
374 | +++ lib/lp/bugs/browser/tests/bug-views.txt 2011-02-16 20:08:14 +0000 | |||
375 | @@ -206,16 +206,17 @@ | |||
376 | 206 | True | 206 | True |
377 | 207 | 207 | ||
378 | 208 | The displayable comments for a bug can be obtained from the view | 208 | The displayable comments for a bug can be obtained from the view |
380 | 209 | property visible_oldest_comments_for_display. | 209 | property activity_and_comments. |
381 | 210 | 210 | ||
383 | 211 | >>> viewable_comments = ubuntu_bugview.visible_oldest_comments_for_display | 211 | >>> comments = [event.get('comment') for event in |
384 | 212 | ... ubuntu_bugview.activity_and_comments if event.get('comment')] | ||
385 | 212 | 213 | ||
386 | 213 | Because we omit the first comment, and because the third comment is | 214 | Because we omit the first comment, and because the third comment is |
387 | 214 | indentical to the second, we really only display one comment: | 215 | indentical to the second, we really only display one comment: |
388 | 215 | 216 | ||
390 | 216 | >>> print len(viewable_comments) | 217 | >>> print len(comments) |
391 | 217 | 1 | 218 | 1 |
393 | 218 | >>> [(c.index, c.owner.name, c.text_contents) for c in viewable_comments] | 219 | >>> [(c.index, c.owner.name, c.text_contents) for c in comments] |
394 | 219 | [(1, u'name16', u'I can reproduce this bug.')] | 220 | [(1, u'name16', u'I can reproduce this bug.')] |
395 | 220 | 221 | ||
396 | 221 | (Unregister our listener, since we no longer need it.) | 222 | (Unregister our listener, since we no longer need it.) |
397 | 222 | 223 | ||
398 | === modified file 'lib/lp/bugs/browser/tests/test_bugcomment.py' | |||
399 | --- lib/lp/bugs/browser/tests/test_bugcomment.py 2010-12-21 15:31:17 +0000 | |||
400 | +++ lib/lp/bugs/browser/tests/test_bugcomment.py 2011-02-16 20:08:14 +0000 | |||
401 | @@ -19,11 +19,11 @@ | |||
402 | 19 | 19 | ||
403 | 20 | class BugActivityStub: | 20 | class BugActivityStub: |
404 | 21 | 21 | ||
406 | 22 | def __init__(self, datechanged, person=None): | 22 | def __init__(self, datechanged, owner=None): |
407 | 23 | self.datechanged = datechanged | 23 | self.datechanged = datechanged |
411 | 24 | if person is None: | 24 | if owner is None: |
412 | 25 | person = PersonStub() | 25 | owner = PersonStub() |
413 | 26 | self.person = person | 26 | self.person = owner |
414 | 27 | 27 | ||
415 | 28 | def __repr__(self): | 28 | def __repr__(self): |
416 | 29 | return "BugActivityStub(%r, %r)" % ( | 29 | return "BugActivityStub(%r, %r)" % ( |
417 | @@ -32,16 +32,18 @@ | |||
418 | 32 | 32 | ||
419 | 33 | class BugCommentStub: | 33 | class BugCommentStub: |
420 | 34 | 34 | ||
422 | 35 | def __init__(self, datecreated, owner=None): | 35 | def __init__(self, datecreated, index, owner=None): |
423 | 36 | self.datecreated = datecreated | 36 | self.datecreated = datecreated |
424 | 37 | if owner is None: | 37 | if owner is None: |
425 | 38 | owner = PersonStub() | 38 | owner = PersonStub() |
426 | 39 | self.owner = owner | 39 | self.owner = owner |
427 | 40 | self.activity = [] | 40 | self.activity = [] |
428 | 41 | self.index = index | ||
429 | 41 | 42 | ||
430 | 42 | def __repr__(self): | 43 | def __repr__(self): |
433 | 43 | return "BugCommentStub(%r, %r)" % ( | 44 | return "BugCommentStub(%r, %d, %r)" % ( |
434 | 44 | self.datecreated.strftime('%Y-%m-%d--%H%M'), self.owner) | 45 | self.datecreated.strftime('%Y-%m-%d--%H%M'), |
435 | 46 | self.index, self.owner) | ||
436 | 45 | 47 | ||
437 | 46 | 48 | ||
438 | 47 | class PersonStub: | 49 | class PersonStub: |
439 | @@ -61,8 +63,8 @@ | |||
440 | 61 | def setUp(self): | 63 | def setUp(self): |
441 | 62 | super(TestGroupCommentsWithActivities, self).setUp() | 64 | super(TestGroupCommentsWithActivities, self).setUp() |
442 | 63 | self.now = datetime.now(utc) | 65 | self.now = datetime.now(utc) |
445 | 64 | self.timestamps = ( | 66 | self.time_index = ( |
446 | 65 | self.now + timedelta(minutes=counter) | 67 | (self.now + timedelta(minutes=counter), counter) |
447 | 66 | for counter in count(1)) | 68 | for counter in count(1)) |
448 | 67 | 69 | ||
449 | 68 | def group(self, comments, activities): | 70 | def group(self, comments, activities): |
450 | @@ -79,7 +81,7 @@ | |||
451 | 79 | # When no activities are passed in, and the comments passed in don't | 81 | # When no activities are passed in, and the comments passed in don't |
452 | 80 | # have any common actors, no grouping is possible. | 82 | # have any common actors, no grouping is possible. |
453 | 81 | comments = [ | 83 | comments = [ |
455 | 82 | BugCommentStub(next(self.timestamps)) | 84 | BugCommentStub(*next(self.time_index)) |
456 | 83 | for number in xrange(5)] | 85 | for number in xrange(5)] |
457 | 84 | self.assertEqual( | 86 | self.assertEqual( |
458 | 85 | comments, self.group(comments=comments, activities=[])) | 87 | comments, self.group(comments=comments, activities=[])) |
459 | @@ -88,7 +90,7 @@ | |||
460 | 88 | # When no comments are passed in, and the activities passed in don't | 90 | # When no comments are passed in, and the activities passed in don't |
461 | 89 | # have any common actors, no grouping is possible. | 91 | # have any common actors, no grouping is possible. |
462 | 90 | activities = [ | 92 | activities = [ |
464 | 91 | BugActivityStub(next(self.timestamps)) | 93 | BugActivityStub(next(self.time_index)[0]) |
465 | 92 | for number in xrange(5)] | 94 | for number in xrange(5)] |
466 | 93 | self.assertEqual( | 95 | self.assertEqual( |
467 | 94 | [[activity] for activity in activities], self.group( | 96 | [[activity] for activity in activities], self.group( |
468 | @@ -97,13 +99,13 @@ | |||
469 | 97 | def test_no_common_actor(self): | 99 | def test_no_common_actor(self): |
470 | 98 | # When each activities and comment given has a different actor then no | 100 | # When each activities and comment given has a different actor then no |
471 | 99 | # grouping is possible. | 101 | # grouping is possible. |
476 | 100 | activity1 = BugActivityStub(next(self.timestamps)) | 102 | activity1 = BugActivityStub(next(self.time_index)[0]) |
477 | 101 | comment1 = BugCommentStub(next(self.timestamps)) | 103 | comment1 = BugCommentStub(*next(self.time_index)) |
478 | 102 | activity2 = BugActivityStub(next(self.timestamps)) | 104 | activity2 = BugActivityStub(next(self.time_index)[0]) |
479 | 103 | comment2 = BugCommentStub(next(self.timestamps)) | 105 | comment2 = BugCommentStub(*next(self.time_index)) |
480 | 104 | 106 | ||
481 | 105 | activities = set([activity1, activity2]) | 107 | activities = set([activity1, activity2]) |
483 | 106 | comments = set([comment1, comment2]) | 108 | comments = list([comment1, comment2]) |
484 | 107 | 109 | ||
485 | 108 | self.assertEqual( | 110 | self.assertEqual( |
486 | 109 | [[activity1], comment1, [activity2], comment2], | 111 | [[activity1], comment1, [activity2], comment2], |
487 | @@ -113,8 +115,8 @@ | |||
488 | 113 | # An activity shortly after a comment by the same person is grouped | 115 | # An activity shortly after a comment by the same person is grouped |
489 | 114 | # into the comment. | 116 | # into the comment. |
490 | 115 | actor = PersonStub() | 117 | actor = PersonStub() |
493 | 116 | comment = BugCommentStub(next(self.timestamps), actor) | 118 | comment = BugCommentStub(*next(self.time_index), owner=actor) |
494 | 117 | activity = BugActivityStub(next(self.timestamps), actor) | 119 | activity = BugActivityStub(next(self.time_index)[0], owner=actor) |
495 | 118 | grouped = self.group(comments=[comment], activities=[activity]) | 120 | grouped = self.group(comments=[comment], activities=[activity]) |
496 | 119 | self.assertEqual([comment], grouped) | 121 | self.assertEqual([comment], grouped) |
497 | 120 | self.assertEqual([activity], comment.activity) | 122 | self.assertEqual([activity], comment.activity) |
498 | @@ -123,8 +125,8 @@ | |||
499 | 123 | # An activity shortly before a comment by the same person is grouped | 125 | # An activity shortly before a comment by the same person is grouped |
500 | 124 | # into the comment. | 126 | # into the comment. |
501 | 125 | actor = PersonStub() | 127 | actor = PersonStub() |
504 | 126 | activity = BugActivityStub(next(self.timestamps), actor) | 128 | activity = BugActivityStub(next(self.time_index)[0], owner=actor) |
505 | 127 | comment = BugCommentStub(next(self.timestamps), actor) | 129 | comment = BugCommentStub(*next(self.time_index), owner=actor) |
506 | 128 | grouped = self.group(comments=[comment], activities=[activity]) | 130 | grouped = self.group(comments=[comment], activities=[activity]) |
507 | 129 | self.assertEqual([comment], grouped) | 131 | self.assertEqual([comment], grouped) |
508 | 130 | self.assertEqual([activity], comment.activity) | 132 | self.assertEqual([activity], comment.activity) |
509 | @@ -133,9 +135,9 @@ | |||
510 | 133 | # Activities shortly before and after a comment are grouped into the | 135 | # Activities shortly before and after a comment are grouped into the |
511 | 134 | # comment's activity. | 136 | # comment's activity. |
512 | 135 | actor = PersonStub() | 137 | actor = PersonStub() |
516 | 136 | activity1 = BugActivityStub(next(self.timestamps), actor) | 138 | activity1 = BugActivityStub(next(self.time_index)[0], owner=actor) |
517 | 137 | comment = BugCommentStub(next(self.timestamps), actor) | 139 | comment = BugCommentStub(*next(self.time_index), owner=actor) |
518 | 138 | activity2 = BugActivityStub(next(self.timestamps), actor) | 140 | activity2 = BugActivityStub(next(self.time_index)[0], owner=actor) |
519 | 139 | grouped = self.group( | 141 | grouped = self.group( |
520 | 140 | comments=[comment], activities=[activity1, activity2]) | 142 | comments=[comment], activities=[activity1, activity2]) |
521 | 141 | self.assertEqual([comment], grouped) | 143 | self.assertEqual([comment], grouped) |
522 | @@ -146,7 +148,7 @@ | |||
523 | 146 | # Anything outside of that window is considered separate. | 148 | # Anything outside of that window is considered separate. |
524 | 147 | actor = PersonStub() | 149 | actor = PersonStub() |
525 | 148 | activities = [ | 150 | activities = [ |
527 | 149 | BugActivityStub(next(self.timestamps), actor) | 151 | BugActivityStub(next(self.time_index)[0], owner=actor) |
528 | 150 | for count in xrange(8)] | 152 | for count in xrange(8)] |
529 | 151 | grouped = self.group(comments=[], activities=activities) | 153 | grouped = self.group(comments=[], activities=activities) |
530 | 152 | self.assertEqual(2, len(grouped)) | 154 | self.assertEqual(2, len(grouped)) |
531 | @@ -156,8 +158,8 @@ | |||
532 | 156 | def test_two_comments_by_common_actor(self): | 158 | def test_two_comments_by_common_actor(self): |
533 | 157 | # Only one comment will ever appear in a group. | 159 | # Only one comment will ever appear in a group. |
534 | 158 | actor = PersonStub() | 160 | actor = PersonStub() |
537 | 159 | comment1 = BugCommentStub(next(self.timestamps), actor) | 161 | comment1 = BugCommentStub(*next(self.time_index), owner=actor) |
538 | 160 | comment2 = BugCommentStub(next(self.timestamps), actor) | 162 | comment2 = BugCommentStub(*next(self.time_index), owner=actor) |
539 | 161 | grouped = self.group(comments=[comment1, comment2], activities=[]) | 163 | grouped = self.group(comments=[comment1, comment2], activities=[]) |
540 | 162 | self.assertEqual([comment1, comment2], grouped) | 164 | self.assertEqual([comment1, comment2], grouped) |
541 | 163 | 165 | ||
542 | @@ -165,11 +167,11 @@ | |||
543 | 165 | # Activity gets associated with earlier comment when all other factors | 167 | # Activity gets associated with earlier comment when all other factors |
544 | 166 | # are unchanging. | 168 | # are unchanging. |
545 | 167 | actor = PersonStub() | 169 | actor = PersonStub() |
551 | 168 | activity1 = BugActivityStub(next(self.timestamps), actor) | 170 | activity1 = BugActivityStub(next(self.time_index)[0], owner=actor) |
552 | 169 | comment1 = BugCommentStub(next(self.timestamps), actor) | 171 | comment1 = BugCommentStub(*next(self.time_index), owner=actor) |
553 | 170 | activity2 = BugActivityStub(next(self.timestamps), actor) | 172 | activity2 = BugActivityStub(next(self.time_index)[0], owner=actor) |
554 | 171 | comment2 = BugCommentStub(next(self.timestamps), actor) | 173 | comment2 = BugCommentStub(*next(self.time_index), owner=actor) |
555 | 172 | activity3 = BugActivityStub(next(self.timestamps), actor) | 174 | activity3 = BugActivityStub(next(self.time_index)[0], owner=actor) |
556 | 173 | grouped = self.group( | 175 | grouped = self.group( |
557 | 174 | comments=[comment1, comment2], | 176 | comments=[comment1, comment2], |
558 | 175 | activities=[activity1, activity2, activity3]) | 177 | activities=[activity1, activity2, activity3]) |
559 | 176 | 178 | ||
560 | === modified file 'lib/lp/bugs/configure.zcml' | |||
561 | --- lib/lp/bugs/configure.zcml 2011-02-14 00:15:22 +0000 | |||
562 | +++ lib/lp/bugs/configure.zcml 2011-02-16 20:08:14 +0000 | |||
563 | @@ -734,7 +734,7 @@ | |||
564 | 734 | hasBranch | 734 | hasBranch |
565 | 735 | security_related | 735 | security_related |
566 | 736 | tags | 736 | tags |
568 | 737 | getMessageChunks | 737 | getMessagesForView |
569 | 738 | isSubscribedToDupes | 738 | isSubscribedToDupes |
570 | 739 | getSubscribersFromDuplicates | 739 | getSubscribersFromDuplicates |
571 | 740 | getSubscriptionsFromDuplicates | 740 | getSubscriptionsFromDuplicates |
572 | 741 | 741 | ||
573 | === modified file 'lib/lp/bugs/doc/bug.txt' | |||
574 | --- lib/lp/bugs/doc/bug.txt 2011-02-02 09:55:25 +0000 | |||
575 | +++ lib/lp/bugs/doc/bug.txt 2011-02-16 20:08:14 +0000 | |||
576 | @@ -1116,15 +1116,15 @@ | |||
577 | 1116 | ------------ | 1116 | ------------ |
578 | 1117 | 1117 | ||
579 | 1118 | A bug comment is actually made up of a number of chunks. The | 1118 | A bug comment is actually made up of a number of chunks. The |
582 | 1119 | IBug.getMessageChunks() method allows you to retreive these chunks in a | 1119 | IBug.getMessagesForView() method allows you to get all the data needed to |
583 | 1120 | single shot. | 1120 | show messages in the bugtask index template in one shot. |
584 | 1121 | 1121 | ||
585 | 1122 | >>> from canonical.ftests.pgsql import CursorWrapper | 1122 | >>> from canonical.ftests.pgsql import CursorWrapper |
586 | 1123 | >>> CursorWrapper.record_sql = True | 1123 | >>> CursorWrapper.record_sql = True |
587 | 1124 | >>> queries = len(CursorWrapper.last_executed_sql) | 1124 | >>> queries = len(CursorWrapper.last_executed_sql) |
588 | 1125 | 1125 | ||
591 | 1126 | >>> chunks = bug_two.getMessageChunks() | 1126 | >>> chunks = bug_two.getMessagesForView(None) |
592 | 1127 | >>> for chunk in sorted(chunks, key=lambda x:x.id): | 1127 | >>> for _, _1, chunk in sorted(chunks, key=lambda x:x[2].id): |
593 | 1128 | ... (chunk.id, chunk.message.id, chunk.message.owner.id, | 1128 | ... (chunk.id, chunk.message.id, chunk.message.owner.id, |
594 | 1129 | ... chunk.content[:30]) | 1129 | ... chunk.content[:30]) |
595 | 1130 | (4, 1, 16, u'Problem exists between chair a') | 1130 | (4, 1, 16, u'Problem exists between chair a') |
596 | @@ -1141,6 +1141,24 @@ | |||
597 | 1141 | >>> len(CursorWrapper.last_executed_sql) - queries <= 3 | 1141 | >>> len(CursorWrapper.last_executed_sql) - queries <= 3 |
598 | 1142 | True | 1142 | True |
599 | 1143 | 1143 | ||
600 | 1144 | getMessagesForView supports slicing operations: | ||
601 | 1145 | |||
602 | 1146 | >>> def message_ids(slices): | ||
603 | 1147 | ... chunks = bug_two.getMessagesForView(slices) | ||
604 | 1148 | ... return sorted(set( | ||
605 | 1149 | ... bugmessage.index for bugmessage, _, _1 in chunks)) | ||
606 | 1150 | >>> message_ids([slice(1, 2)]) | ||
607 | 1151 | [1] | ||
608 | 1152 | |||
609 | 1153 | We use this to get the first N and last M messages in big bugs: | ||
610 | 1154 | >>> message_ids([slice(None, 1), slice(2, None)]) | ||
611 | 1155 | [0, 2] | ||
612 | 1156 | |||
613 | 1157 | We also support a negative lookup though the bug view does not use that at the | ||
614 | 1158 | moment: | ||
615 | 1159 | >>> message_ids([slice(None, 1), slice(-1, None)]) | ||
616 | 1160 | [0, 2] | ||
617 | 1161 | |||
618 | 1144 | Bugs have a special attribute, `indexed_messages` which returns the collection | 1162 | Bugs have a special attribute, `indexed_messages` which returns the collection |
619 | 1145 | of messages, each decorated with the index of that message in its context | 1163 | of messages, each decorated with the index of that message in its context |
620 | 1146 | (the bug) and the primary bug task. This is used for providing an efficient | 1164 | (the bug) and the primary bug task. This is used for providing an efficient |
621 | 1147 | 1165 | ||
622 | === modified file 'lib/lp/bugs/doc/bugcomment.txt' | |||
623 | --- lib/lp/bugs/doc/bugcomment.txt 2010-12-21 15:07:26 +0000 | |||
624 | +++ lib/lp/bugs/doc/bugcomment.txt 2011-02-16 20:08:14 +0000 | |||
625 | @@ -14,7 +14,7 @@ | |||
626 | 14 | The bug's description starts out identical to its first comment. In the course | 14 | The bug's description starts out identical to its first comment. In the course |
627 | 15 | of a bug's life, the description may be updated, but the first comment stays | 15 | of a bug's life, the description may be updated, but the first comment stays |
628 | 16 | intact. To improve readability, we never display the first comment in the bug | 16 | intact. To improve readability, we never display the first comment in the bug |
630 | 17 | page, and this is why visible_oldest_comments_for_display doesn't include it: | 17 | page, and this is why the event stream elides it doesn't include it: |
631 | 18 | 18 | ||
632 | 19 | >>> bug_ten = getUtility(IBugSet).get(10) | 19 | >>> bug_ten = getUtility(IBugSet).get(10) |
633 | 20 | >>> bug_ten_bugtask = bug_ten.bugtasks[0] | 20 | >>> bug_ten_bugtask = bug_ten.bugtasks[0] |
634 | @@ -22,8 +22,10 @@ | |||
635 | 22 | >>> bug_view = getMultiAdapter( | 22 | >>> bug_view = getMultiAdapter( |
636 | 23 | ... (bug_ten_bugtask, LaunchpadTestRequest()), name='+index') | 23 | ... (bug_ten_bugtask, LaunchpadTestRequest()), name='+index') |
637 | 24 | >>> bug_view.initialize() | 24 | >>> bug_view.initialize() |
640 | 25 | >>> rendered_comments = bug_view.visible_oldest_comments_for_display | 25 | >>> def bug_comments(bug_view): |
641 | 26 | >>> [bug_comment.index for bug_comment in rendered_comments] | 26 | ... return [event.get('comment') for event in |
642 | 27 | ... bug_view.activity_and_comments if event.get('comment')] | ||
643 | 28 | >>> [bug_comment.index for bug_comment in bug_comments(bug_view)] | ||
644 | 27 | [1] | 29 | [1] |
645 | 28 | 30 | ||
646 | 29 | In the case of bug 10, the first comment is identical to the bug's | 31 | In the case of bug 10, the first comment is identical to the bug's |
647 | @@ -42,11 +44,10 @@ | |||
648 | 42 | stored as bug attchments of the first comment. Similary, the first | 44 | stored as bug attchments of the first comment. Similary, the first |
649 | 43 | comment of bugs imported from other bug trackers may have attachments. | 45 | comment of bugs imported from other bug trackers may have attachments. |
650 | 44 | We display these attachments in the comment section of the Web UI, | 46 | We display these attachments in the comment section of the Web UI, |
653 | 45 | hence visible_oldest_comments_for_display contains the first comment, if | 47 | hence the activity stream contains the first comment, if it has attachments. |
652 | 46 | it has attachments. | ||
654 | 47 | 48 | ||
655 | 48 | Currently, the first comment of bug 11 has no attachments, hence | 49 | Currently, the first comment of bug 11 has no attachments, hence |
657 | 49 | BugTaskView.visible_oldest_comments_for_display does not return the | 50 | BugTaskView.activity_and_comments does not return the |
658 | 50 | first comment. | 51 | first comment. |
659 | 51 | 52 | ||
660 | 52 | >>> bug_11 = getUtility(IBugSet).get(11) | 53 | >>> bug_11 = getUtility(IBugSet).get(11) |
661 | @@ -54,12 +55,12 @@ | |||
662 | 54 | >>> bug_11_view = getMultiAdapter( | 55 | >>> bug_11_view = getMultiAdapter( |
663 | 55 | ... (bug_11_bugtask, LaunchpadTestRequest()), name='+index') | 56 | ... (bug_11_bugtask, LaunchpadTestRequest()), name='+index') |
664 | 56 | >>> bug_11_view.initialize() | 57 | >>> bug_11_view.initialize() |
666 | 57 | >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display | 58 | >>> rendered_comments = bug_comments(bug_11_view) |
667 | 58 | >>> [bug_comment.index for bug_comment in rendered_comments] | 59 | >>> [bug_comment.index for bug_comment in rendered_comments] |
668 | 59 | [1, 2, 3, 4, 5, 6] | 60 | [1, 2, 3, 4, 5, 6] |
669 | 60 | 61 | ||
670 | 61 | If we add an attachment to the first comment, this comment is included | 62 | If we add an attachment to the first comment, this comment is included |
672 | 62 | in visible_oldest_comments_for_display... | 63 | in activity_and_comments... |
673 | 63 | 64 | ||
674 | 64 | >>> import StringIO | 65 | >>> import StringIO |
675 | 65 | >>> login("test@canonical.com") | 66 | >>> login("test@canonical.com") |
676 | @@ -71,7 +72,7 @@ | |||
677 | 71 | >>> bug_11_view = getMultiAdapter( | 72 | >>> bug_11_view = getMultiAdapter( |
678 | 72 | ... (bug_11_bugtask, LaunchpadTestRequest()), name='+index') | 73 | ... (bug_11_bugtask, LaunchpadTestRequest()), name='+index') |
679 | 73 | >>> bug_11_view.initialize() | 74 | >>> bug_11_view.initialize() |
681 | 74 | >>> rendered_comments = bug_11_view.visible_oldest_comments_for_display | 75 | >>> rendered_comments = bug_comments(bug_11_view) |
682 | 75 | >>> [bug_comment.index for bug_comment in rendered_comments] | 76 | >>> [bug_comment.index for bug_comment in rendered_comments] |
683 | 76 | [0, 1, 2, 3, 4, 5, 6] | 77 | [0, 1, 2, 3, 4, 5, 6] |
684 | 77 | >>> | 78 | >>> |
685 | @@ -112,13 +113,13 @@ | |||
686 | 112 | comments have been truncated: | 113 | comments have been truncated: |
687 | 113 | 114 | ||
688 | 114 | >>> [(bug_comment.index, bug_comment.was_truncated) | 115 | >>> [(bug_comment.index, bug_comment.was_truncated) |
690 | 115 | ... for bug_comment in bug_view.visible_oldest_comments_for_display] | 116 | ... for bug_comment in bug_comments(bug_view)] |
691 | 116 | [(1, True), (2, True)] | 117 | [(1, True), (2, True)] |
692 | 117 | 118 | ||
693 | 118 | Let's take a closer look at one of the truncated comments. We can | 119 | Let's take a closer look at one of the truncated comments. We can |
694 | 119 | display the truncated text using text_for_display: | 120 | display the truncated text using text_for_display: |
695 | 120 | 121 | ||
697 | 121 | >>> comment_one = bug_view.visible_oldest_comments_for_display[0] | 122 | >>> comment_one = bug_comments(bug_view)[0] |
698 | 122 | >>> print comment_one.text_for_display #doctest: -ELLIPSIS | 123 | >>> print comment_one.text_for_display #doctest: -ELLIPSIS |
699 | 123 | This would be a real... | 124 | This would be a real... |
700 | 124 | 125 | ||
701 | @@ -222,7 +223,7 @@ | |||
702 | 222 | ... (bug_three_bugtask, LaunchpadTestRequest()), name='+index') | 223 | ... (bug_three_bugtask, LaunchpadTestRequest()), name='+index') |
703 | 223 | >>> bug_view.initialize() | 224 | >>> bug_view.initialize() |
704 | 224 | >>> [(c.index, c.title, c.text_for_display) | 225 | >>> [(c.index, c.title, c.text_for_display) |
706 | 225 | ... for c in bug_view.visible_oldest_comments_for_display] | 226 | ... for c in bug_comments(bug_view)] |
707 | 226 | [(1, u'Hi', u'Hello there'), | 227 | [(1, u'Hi', u'Hello there'), |
708 | 227 | (3, u'Ho', u'Hello there'), | 228 | (3, u'Ho', u'Hello there'), |
709 | 228 | (5, u'Ho', u'Hello there'), | 229 | (5, u'Ho', u'Hello there'), |
710 | @@ -236,9 +237,8 @@ | |||
711 | 236 | comments: visible_comments_truncated_for_display. | 237 | comments: visible_comments_truncated_for_display. |
712 | 237 | 238 | ||
713 | 238 | This is normally false, but for bugs with lots of comments, the | 239 | This is normally false, but for bugs with lots of comments, the |
717 | 239 | visible_comments_truncated_for_display property becomes True and the | 240 | visible_comments_truncated_for_display property becomes True and the activity |
718 | 240 | visible_oldest_comments_for_display list is truncated to just the oldest | 241 | stream has the middle comments elided. |
716 | 241 | comments. | ||
719 | 242 | 242 | ||
720 | 243 | The configuration keys comments_list_max_length, | 243 | The configuration keys comments_list_max_length, |
721 | 244 | comments_list_truncate_oldest_to, and comments_list_truncate_newest_to | 244 | comments_list_truncate_oldest_to, and comments_list_truncate_newest_to |
722 | @@ -272,24 +272,14 @@ | |||
723 | 272 | >>> bug = factory.makeBug() | 272 | >>> bug = factory.makeBug() |
724 | 273 | >>> add_comments(bug, 9) | 273 | >>> add_comments(bug, 9) |
725 | 274 | 274 | ||
728 | 275 | If we create a view for this, we can see that all 9 comments are | 275 | If we create a view for this, we can see that truncation is disabled. |
727 | 276 | visible, and we can see that the list has not been truncated. | ||
729 | 277 | 276 | ||
730 | 278 | >>> bug_view = getMultiAdapter( | 277 | >>> bug_view = getMultiAdapter( |
731 | 279 | ... (bug.default_bugtask, LaunchpadTestRequest()), name='+index') | 278 | ... (bug.default_bugtask, LaunchpadTestRequest()), name='+index') |
732 | 280 | >>> bug_view.initialize() | 279 | >>> bug_view.initialize() |
733 | 281 | |||
734 | 282 | >>> len(bug_view.visible_oldest_comments_for_display) | ||
735 | 283 | 9 | ||
736 | 284 | >>> bug_view.visible_comments_truncated_for_display | 280 | >>> bug_view.visible_comments_truncated_for_display |
737 | 285 | False | 281 | False |
738 | 286 | 282 | ||
739 | 287 | When comments aren't being truncated and empty set will be returned by | ||
740 | 288 | visible_recent_comments_for_display. | ||
741 | 289 | |||
742 | 290 | >>> len(bug_view.visible_recent_comments_for_display) | ||
743 | 291 | 0 | ||
744 | 292 | |||
745 | 293 | Add two more comments, and the list will be truncated to only 8 total. | 283 | Add two more comments, and the list will be truncated to only 8 total. |
746 | 294 | 284 | ||
747 | 295 | >>> add_comments(bug, 2) | 285 | >>> add_comments(bug, 2) |
748 | @@ -300,9 +290,9 @@ | |||
749 | 300 | 290 | ||
750 | 301 | >>> bug_view.visible_comments_truncated_for_display | 291 | >>> bug_view.visible_comments_truncated_for_display |
751 | 302 | True | 292 | True |
753 | 303 | >>> len(bug_view.visible_oldest_comments_for_display) | 293 | >>> bug_view.visible_initial_comments |
754 | 304 | 3 | 294 | 3 |
756 | 305 | >>> len(bug_view.visible_recent_comments_for_display) | 295 | >>> bug_view.visible_recent_comments |
757 | 306 | 5 | 296 | 5 |
758 | 307 | 297 | ||
759 | 308 | The display of all comments can be requested with a form parameter. | 298 | The display of all comments can be requested with a form parameter. |
760 | @@ -312,8 +302,6 @@ | |||
761 | 312 | ... (bug.default_bugtask, request), name='+index') | 302 | ... (bug.default_bugtask, request), name='+index') |
762 | 313 | >>> bug_view.initialize() | 303 | >>> bug_view.initialize() |
763 | 314 | 304 | ||
764 | 315 | >>> len(bug_view.visible_oldest_comments_for_display) | ||
765 | 316 | 11 | ||
766 | 317 | >>> bug_view.visible_comments_truncated_for_display | 305 | >>> bug_view.visible_comments_truncated_for_display |
767 | 318 | False | 306 | False |
768 | 319 | 307 | ||
769 | 320 | 308 | ||
770 | === modified file 'lib/lp/bugs/interfaces/bug.py' | |||
771 | --- lib/lp/bugs/interfaces/bug.py 2011-02-16 13:42:51 +0000 | |||
772 | +++ lib/lp/bugs/interfaces/bug.py 2011-02-16 20:08:14 +0000 | |||
773 | @@ -695,8 +695,15 @@ | |||
774 | 695 | remote bug tracker, if it's an imported comment. | 695 | remote bug tracker, if it's an imported comment. |
775 | 696 | """ | 696 | """ |
776 | 697 | 697 | ||
779 | 698 | def getMessageChunks(): | 698 | def getMessagesForView(slice_info): |
780 | 699 | """Return MessageChunks corresponding to comments made on this bug""" | 699 | """Return BugMessage,Message,MessageChunks for renderinger. |
781 | 700 | |||
782 | 701 | This eager loads message.owner validity associated with the | ||
783 | 702 | bugmessages. | ||
784 | 703 | |||
785 | 704 | :param slice_info: Either None or a list of slices to constraint the | ||
786 | 705 | returned rows. The step parameter in each slice is ignored. | ||
787 | 706 | """ | ||
788 | 700 | 707 | ||
789 | 701 | def getNullBugTask(product=None, productseries=None, | 708 | def getNullBugTask(product=None, productseries=None, |
790 | 702 | sourcepackagename=None, distribution=None, | 709 | sourcepackagename=None, distribution=None, |
791 | 703 | 710 | ||
792 | === modified file 'lib/lp/bugs/interfaces/bugmessage.py' | |||
793 | --- lib/lp/bugs/interfaces/bugmessage.py 2011-01-31 08:08:04 +0000 | |||
794 | +++ lib/lp/bugs/interfaces/bugmessage.py 2011-02-16 20:08:14 +0000 | |||
795 | @@ -51,6 +51,7 @@ | |||
796 | 51 | message = Object(schema=IMessage, title=u"The message.") | 51 | message = Object(schema=IMessage, title=u"The message.") |
797 | 52 | bugwatch = Object(schema=IBugWatch, | 52 | bugwatch = Object(schema=IBugWatch, |
798 | 53 | title=u"A bugwatch to which the message pertains.") | 53 | title=u"A bugwatch to which the message pertains.") |
799 | 54 | bugwatchID = Int(title=u'The bugwatch id.', readonly=True) | ||
800 | 54 | remote_comment_id = TextLine( | 55 | remote_comment_id = TextLine( |
801 | 55 | title=u"The id this comment has in the bugwatch's bug tracker.") | 56 | title=u"The id this comment has in the bugwatch's bug tracker.") |
802 | 56 | visible = Bool(title=u"This message is visible or not.", required=False, | 57 | visible = Bool(title=u"This message is visible or not.", required=False, |
803 | 57 | 58 | ||
804 | === modified file 'lib/lp/bugs/model/bug.py' | |||
805 | --- lib/lp/bugs/model/bug.py 2011-02-14 00:15:22 +0000 | |||
806 | +++ lib/lp/bugs/model/bug.py 2011-02-16 20:08:14 +0000 | |||
807 | @@ -437,6 +437,8 @@ | |||
808 | 437 | @property | 437 | @property |
809 | 438 | def indexed_messages(self): | 438 | def indexed_messages(self): |
810 | 439 | """See `IMessageTarget`.""" | 439 | """See `IMessageTarget`.""" |
811 | 440 | # Note that this is a decorated result set, so will cache its value (in | ||
812 | 441 | # the absence of slices) | ||
813 | 440 | return self._indexed_messages(include_content=True) | 442 | return self._indexed_messages(include_content=True) |
814 | 441 | 443 | ||
815 | 442 | def _indexed_messages(self, include_content=False, include_parents=True): | 444 | def _indexed_messages(self, include_content=False, include_parents=True): |
816 | @@ -1369,39 +1371,55 @@ | |||
817 | 1369 | """See `IBug`.""" | 1371 | """See `IBug`.""" |
818 | 1370 | return self._question_from_bug | 1372 | return self._question_from_bug |
819 | 1371 | 1373 | ||
821 | 1372 | def getMessageChunks(self): | 1374 | def getMessagesForView(self, slice_info): |
822 | 1373 | """See `IBug`.""" | 1375 | """See `IBug`.""" |
854 | 1374 | query = """ | 1376 | # Note that this function and indexed_messages have significant overlap |
855 | 1375 | Message.id = MessageChunk.message AND | 1377 | # and could stand to be refactored. |
856 | 1376 | BugMessage.message = Message.id AND | 1378 | slices = [] |
857 | 1377 | BugMessage.bug = %s | 1379 | if slice_info is not None: |
858 | 1378 | """ % sqlvalues(self) | 1380 | # NB: This isn't a full implementation of the slice protocol, |
859 | 1379 | 1381 | # merely the bits needed by BugTask:+index. | |
860 | 1380 | chunks = MessageChunk.select(query, | 1382 | for slice in slice_info: |
861 | 1381 | clauseTables=["BugMessage", "Message"], | 1383 | if not slice.start: |
862 | 1382 | # XXX: kiko 2006-09-16 bug=60745: | 1384 | assert slice.stop > 0, slice.stop |
863 | 1383 | # There is an issue that presents itself | 1385 | slices.append(BugMessage.index < slice.stop) |
864 | 1384 | # here if we prejoin message.owner: because Message is | 1386 | elif not slice.stop: |
865 | 1385 | # already in the clauseTables, the SQL generated joins | 1387 | if slice.start < 0: |
866 | 1386 | # against message twice and that causes the results to | 1388 | # If the high index is N, a slice of -1: should |
867 | 1387 | # break. | 1389 | # return index N - so we need to add one to the |
868 | 1388 | prejoinClauseTables=["Message"], | 1390 | # range. |
869 | 1389 | # Note the ordering by Message.id here; while datecreated in | 1391 | slices.append(BugMessage.index >= SQL( |
870 | 1390 | # production is never the same, it can be in the test suite. | 1392 | "(select max(index) from " |
871 | 1391 | orderBy=["Message.datecreated", "Message.id", | 1393 | "bugmessage where bug=%s) + 1 - %s" % ( |
872 | 1392 | "MessageChunk.sequence"]) | 1394 | sqlvalues(self.id, -slice.start)))) |
873 | 1393 | chunks = list(chunks) | 1395 | else: |
874 | 1394 | 1396 | slices.append(BugMessage.index >= slice.start) | |
875 | 1395 | # Since we can't prejoin, cache all people at once so we don't | 1397 | else: |
876 | 1396 | # have to do it while rendering, which is a big deal for bugs | 1398 | slices.append(And(BugMessage.index >= slice.start, |
877 | 1397 | # with a million comments. | 1399 | BugMessage.index < slice.stop)) |
878 | 1398 | owner_ids = set() | 1400 | if slices: |
879 | 1399 | for chunk in chunks: | 1401 | ranges = [Or(*slices)] |
880 | 1400 | if chunk.message.ownerID: | 1402 | else: |
881 | 1401 | owner_ids.add(str(chunk.message.ownerID)) | 1403 | ranges = [] |
882 | 1402 | list(Person.select("ID in (%s)" % ",".join(owner_ids))) | 1404 | # We expect: |
883 | 1403 | 1405 | # 1 bugmessage -> 1 message -> small N chunks. For now, using a wide | |
884 | 1404 | return chunks | 1406 | # query seems fine as we have to join out from bugmessage anyway. |
885 | 1407 | result = Store.of(self).find((BugMessage, Message, MessageChunk), | ||
886 | 1408 | Message.id==MessageChunk.messageID, | ||
887 | 1409 | BugMessage.messageID==Message.id, | ||
888 | 1410 | BugMessage.bug==self.id, | ||
889 | 1411 | *ranges) | ||
890 | 1412 | result.order_by(BugMessage.index, MessageChunk.sequence) | ||
891 | 1413 | def eager_load_owners(rows): | ||
892 | 1414 | owners = set() | ||
893 | 1415 | for row in rows: | ||
894 | 1416 | owners.add(row[1].ownerID) | ||
895 | 1417 | owners.discard(None) | ||
896 | 1418 | if not owners: | ||
897 | 1419 | return | ||
898 | 1420 | PersonSet().getPrecachedPersonsFromIDs(owners, | ||
899 | 1421 | need_validity=True) | ||
900 | 1422 | return DecoratedResultSet(result, pre_iter_hook=eager_load_owners) | ||
901 | 1405 | 1423 | ||
902 | 1406 | def getNullBugTask(self, product=None, productseries=None, | 1424 | def getNullBugTask(self, product=None, productseries=None, |
903 | 1407 | sourcepackagename=None, distribution=None, | 1425 | sourcepackagename=None, distribution=None, |
904 | 1408 | 1426 | ||
905 | === modified file 'lib/lp/bugs/templates/bugcomment-macros.pt' | |||
906 | --- lib/lp/bugs/templates/bugcomment-macros.pt 2010-07-01 21:53:22 +0000 | |||
907 | +++ lib/lp/bugs/templates/bugcomment-macros.pt 2011-02-16 20:08:14 +0000 | |||
908 | @@ -66,7 +66,7 @@ | |||
909 | 66 | class="sprite retry" | 66 | class="sprite retry" |
910 | 67 | style="white-space: nowrap"> | 67 | style="white-space: nowrap"> |
911 | 68 | view all <span | 68 | view all <span |
913 | 69 | tal:replace="view/visible_comments/count:len" | 69 | tal:replace="view/total_comments" |
914 | 70 | /> comments</a> | 70 | /> comments</a> |
915 | 71 | </td> | 71 | </td> |
916 | 72 | </tr> | 72 | </tr> |
917 | 73 | 73 | ||
918 | === modified file 'lib/lp/bugs/templates/bugtask-index.pt' | |||
919 | --- lib/lp/bugs/templates/bugtask-index.pt 2011-02-15 18:16:28 +0000 | |||
920 | +++ lib/lp/bugs/templates/bugtask-index.pt 2011-02-16 20:08:14 +0000 | |||
921 | @@ -320,16 +320,16 @@ | |||
922 | 320 | tal:condition="view/visible_comments_truncated_for_display"> | 320 | tal:condition="view/visible_comments_truncated_for_display"> |
923 | 321 | <div class="informational message"> | 321 | <div class="informational message"> |
924 | 322 | Displaying first <span | 322 | Displaying first <span |
926 | 323 | tal:replace="view/visible_oldest_comments_for_display/count:len">23</span> | 323 | tal:replace="view/visible_initial_comments">23</span> |
927 | 324 | and last <span | 324 | and last <span |
929 | 325 | tal:replace="view/visible_recent_comments_for_display/count:len">32</span> | 325 | tal:replace="view/visible_recent_comments">32</span> |
930 | 326 | comments. | 326 | comments. |
931 | 327 | <tal:what-next | 327 | <tal:what-next |
932 | 328 | define="view_all_href | 328 | define="view_all_href |
933 | 329 | string:${context/fmt:url}?comments=all"> | 329 | string:${context/fmt:url}?comments=all"> |
934 | 330 | <a href="#" tal:attributes="href view_all_href"> | 330 | <a href="#" tal:attributes="href view_all_href"> |
935 | 331 | View all <span | 331 | View all <span |
937 | 332 | tal:replace="view/visible_comments/count:len" /> | 332 | tal:replace="view/total_comments" /> |
938 | 333 | comments</a> or <a href="#" tal:attributes="href | 333 | comments</a> or <a href="#" tal:attributes="href |
939 | 334 | view_all_href">add a comment</a>. | 334 | view_all_href">add a comment</a>. |
940 | 335 | </tal:what-next> | 335 | </tal:what-next> |
Mainly fine.
- Remove the commented out eager bugwatch loader if it isn't needed. from_chunks( ) explaining that bugwatches will have been preloaded at this point so the code is not retrieving them one at a time from the db. iew's new slice behavior, in particular checking for off-by-one errors.
- Add a comment in build_comments_
- I see no tests for getMessagesForV