Code review comment for lp://staging/~mbp/launchpad/323000-affecting-user

Revision history for this message
Raphaƫl Badin (rvb) wrote :

Hi Martin, simple change but a really useful one I reckon. And thank you for removing the "List" in front of all the links, this was really only visual noise.

This looks good to land but I've a few nitpicks:

[1]

The code duplication is indeed a little bit frightening within that file. Maybe it's worth adding an XXX along with a tech-debt bug?

[2]

88 +class PersonAffectingBugTaskSearchListingView(RelevantMilestonesMixin,
89 + BugTaskSearchListingView):

This is really a detail but could you please fix the indentation?

[3]

155 +class TestPersonAffectingBugTaskSearchListingView(
156 + BugTaskViewsTestBase, TestCaseWithFactory):
157 + """Tests for PersonAffectingBugTaskSearchListingView."""
158 +
159 + view_name = '+affectingbugs'
160 +
161 + def setUp(self):
162 + super(TestPersonAffectingBugTaskSearchListingView, self).setUp()
163 + # Bugs filed by this user are marked as affecting them by default, so
164 + # the bug we filed is returned.
165 + self.expected_for_search_unbatched = [
166 + self.owned_bug.default_bugtask,
167 + ]

I understand that owned bugs are also affecting bugs but maybe it would be worth populating a variable named self.affecting_bug and to create a test to make sure that affecting bugs (not owned bugs) are displayed.

review: Approve (code*)

« Back to merge proposal