Launchpad itself

Merge lp:~thumper/launchpad/fix-date-review-requested into lp:launchpad/db-devel

  1. fix-date-review-requested
  2. Merge into db-devel
Proposed by Tim Penhey
Status: Merged
Approved by: Tim Penhey
Approved revision: not available
Merged at revision: not available
Proposed branch: lp:~thumper/launchpad/fix-date-review-requested
Merge into: lp:launchpad/db-devel
Diff against target: 131 lines (+71/-3)
4 files modified
lib/lp/code/browser/branchmergeproposallisting.py (+19/-1)
lib/lp/code/browser/tests/test_branchmergeproposallisting.py (+45/-1)
lib/lp/code/model/branchmergeproposal.py (+0/-1)
lib/lp/code/model/tests/test_branchmergeproposals.py (+7/-0)
To merge this branch: bzr merge lp:~thumper/launchpad/fix-date-review-requested
Reviewer Review Type Date Requested Status
Guilherme Salgado (community) release-critical Approve
Brad Crittenden (community) code Approve
Review via email: mp+15464@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Tim Penhey (thumper) wrote :

Followed up on the outage last night for the Bazaar team. The active reviews page always expected the date_review_requested to be non-None.

However there are some edge cases where the proposal may be Approved but not reviewed. In this case it makes sense that the date_review_requested is None. The browser class has been updated to effectively coalesce the sort order to choose date_review_requested, and if that is None, use date_reviewed.

Also fixes the model level code where setting a branch to work in progress doesn't reset the date_review_requested so as to protect against accidental setting.

Tests:
  TestBranchMergeProposalListingItem
  ActiveReviewSortingTest
  TestBranchMergeProposalRequestReview

Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

I think it looks basically fine, and it must be nice to have found it :)

I wonder if 'sort_key' would be a better name than 'sort_order'?

It's wasn't completely obvious to me that sort_order will not return None all the time -- can you add a comment saying something like "because only approved or ready for review proposals are in the list this always returns non-None". Except more grammatically.

Revision history for this message
Brad Crittenden (bac) wrote :

I agree with Michael's comments.

review: Approve (code)
Revision history for this message
Guilherme Salgado (salgado) :
review: Approve (release-critical)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
--- lib/lp/code/browser/branchmergeproposallisting.py 2009-10-20 02:02:18 +0000
+++ lib/lp/code/browser/branchmergeproposallisting.py 2009-12-02 00:13:14 +0000
@@ -97,6 +97,24 @@
97 """A vote from the specified reviewer."""97 """A vote from the specified reviewer."""
98 return self.context.getUsersVoteReference(self.proposal_reviewer)98 return self.context.getUsersVoteReference(self.proposal_reviewer)
9999
100 @property
101 def sort_key(self):
102 """The value to order by.
103
104 This defaults to date_review_requested, but there are occasions where
105 this is not set if the proposal went directly from work in progress to
106 approved. In this case the date_reviewed is used.
107
108 The value is always not None as proposals in needs review state will
109 always have date_review_requested set, and approved proposals will
110 always have date_reviewed set. These are the only two states that are
111 shown in the active reviews page, so they can always be sorted on.
112 """
113 if self.context.date_review_requested is not None:
114 return self.context.date_review_requested
115 else:
116 return self.context.date_reviewed
117
100118
101class BranchMergeProposalListingBatchNavigator(TableBatchNavigator):119class BranchMergeProposalListingBatchNavigator(TableBatchNavigator):
102 """Batch up the branch listings."""120 """Batch up the branch listings."""
@@ -330,7 +348,7 @@
330 self.show_diffs = True348 self.show_diffs = True
331 # Sort each collection...349 # Sort each collection...
332 for group in self.review_groups.values():350 for group in self.review_groups.values():
333 group.sort(key=attrgetter('date_review_requested'))351 group.sort(key=attrgetter('sort_key'))
334 self.proposal_count = len(proposals)352 self.proposal_count = len(proposals)
335353
336 @cachedproperty354 @cachedproperty
337355
=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-10-20 02:02:18 +0000
+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-12-02 00:13:14 +0000
@@ -16,7 +16,8 @@
16from canonical.launchpad.webapp.servers import LaunchpadTestRequest16from canonical.launchpad.webapp.servers import LaunchpadTestRequest
17from canonical.testing import DatabaseFunctionalLayer17from canonical.testing import DatabaseFunctionalLayer
18from lp.code.browser.branchmergeproposallisting import (18from lp.code.browser.branchmergeproposallisting import (
19 ActiveReviewsView, BranchMergeProposalListingView)19 ActiveReviewsView, BranchMergeProposalListingItem,
20 BranchMergeProposalListingView)
20from lp.code.enums import BranchMergeProposalStatus, CodeReviewVote21from lp.code.enums import BranchMergeProposalStatus, CodeReviewVote
21from lp.testing import ANONYMOUS, login, login_person, TestCaseWithFactory22from lp.testing import ANONYMOUS, login, login_person, TestCaseWithFactory
22from lp.testing.views import create_initialized_view23from lp.testing.views import create_initialized_view
@@ -255,6 +256,49 @@
255 reviewer, ActiveReviewsView.ARE_DOING)256 reviewer, ActiveReviewsView.ARE_DOING)
256257
257258
259class TestBranchMergeProposalListingItem(TestCaseWithFactory):
260 """Tests specifically relating to the BranchMergeProposalListingItem."""
261
262 layer = DatabaseFunctionalLayer
263
264 def test_sort_key_needs_review(self):
265 # If the proposal is in needs review, the sort_key will be the
266 # date_review_requested.
267 bmp = self.factory.makeBranchMergeProposal(
268 date_created=datetime(2009,6,1,tzinfo=pytz.UTC))
269 login_person(bmp.registrant)
270 request_date = datetime(2009,7,1,tzinfo=pytz.UTC)
271 bmp.requestReview(request_date)
272 item = BranchMergeProposalListingItem(bmp, None, None)
273 self.assertEqual(request_date, item.sort_key)
274
275 def test_sort_key_approved(self):
276 # If the proposal is approved, the sort_key will default to the
277 # date_review_requested.
278 bmp = self.factory.makeBranchMergeProposal(
279 date_created=datetime(2009,6,1,tzinfo=pytz.UTC))
280 login_person(bmp.target_branch.owner)
281 request_date = datetime(2009,7,1,tzinfo=pytz.UTC)
282 bmp.requestReview(request_date)
283 bmp.approveBranch(
284 bmp.target_branch.owner, 'rev-id',
285 datetime(2009,8,1,tzinfo=pytz.UTC))
286 item = BranchMergeProposalListingItem(bmp, None, None)
287 self.assertEqual(request_date, item.sort_key)
288
289 def test_sort_key_approved_from_wip(self):
290 # If the proposal is approved and the review has been bypassed, the
291 # date_reviewed is used.
292 bmp = self.factory.makeBranchMergeProposal(
293 date_created=datetime(2009,6,1,tzinfo=pytz.UTC))
294 login_person(bmp.target_branch.owner)
295 review_date = datetime(2009,8,1,tzinfo=pytz.UTC)
296 bmp.approveBranch(
297 bmp.target_branch.owner, 'rev-id', review_date)
298 item = BranchMergeProposalListingItem(bmp, None, None)
299 self.assertEqual(review_date, item.sort_key)
300
301
258class ActiveReviewSortingTest(TestCaseWithFactory):302class ActiveReviewSortingTest(TestCaseWithFactory):
259 """Test the sorting of the active review groups."""303 """Test the sorting of the active review groups."""
260304
261305
=== modified file 'lib/lp/code/model/branchmergeproposal.py'
--- lib/lp/code/model/branchmergeproposal.py 2009-11-27 02:39:17 +0000
+++ lib/lp/code/model/branchmergeproposal.py 2009-12-02 00:13:14 +0000
@@ -336,7 +336,6 @@
336 def setAsWorkInProgress(self):336 def setAsWorkInProgress(self):
337 """See `IBranchMergeProposal`."""337 """See `IBranchMergeProposal`."""
338 self._transitionToState(BranchMergeProposalStatus.WORK_IN_PROGRESS)338 self._transitionToState(BranchMergeProposalStatus.WORK_IN_PROGRESS)
339 self.date_review_requested = None
340 self.reviewer = None339 self.reviewer = None
341 self.date_reviewed = None340 self.date_reviewed = None
342 self.reviewed_revision_id = None341 self.reviewed_revision_id = None
343342
=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-11-27 20:06:13 +0000
+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-02 00:13:14 +0000
@@ -444,6 +444,13 @@
444 self.assertEqual(444 self.assertEqual(
445 proposal.date_created, proposal.date_review_requested)445 proposal.date_created, proposal.date_review_requested)
446446
447 def test_date_not_reset_on_wip(self):
448 # If a proposal has been in needs review state, and is moved back into
449 # work in progress, the date_review_requested is not reset.
450 proposal = self._createMergeProposal(needs_review=True)
451 proposal.setAsWorkInProgress()
452 self.assertIsNot(None, proposal.date_review_requested)
453
447454
448class TestBranchMergeProposalQueueing(TestCase):455class TestBranchMergeProposalQueueing(TestCase):
449 """Test the enqueueing and dequeueing of merge proposals."""456 """Test the enqueueing and dequeueing of merge proposals."""

Subscribers

People subscribed via source and target branches

to status/vote changes: