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
1=== modified file 'lib/lp/code/browser/branchmergeproposallisting.py'
2--- lib/lp/code/browser/branchmergeproposallisting.py 2009-10-20 02:02:18 +0000
3+++ lib/lp/code/browser/branchmergeproposallisting.py 2009-12-02 00:13:14 +0000
4@@ -97,6 +97,24 @@
5 """A vote from the specified reviewer."""
6 return self.context.getUsersVoteReference(self.proposal_reviewer)
7
8+ @property
9+ def sort_key(self):
10+ """The value to order by.
11+
12+ This defaults to date_review_requested, but there are occasions where
13+ this is not set if the proposal went directly from work in progress to
14+ approved. In this case the date_reviewed is used.
15+
16+ The value is always not None as proposals in needs review state will
17+ always have date_review_requested set, and approved proposals will
18+ always have date_reviewed set. These are the only two states that are
19+ shown in the active reviews page, so they can always be sorted on.
20+ """
21+ if self.context.date_review_requested is not None:
22+ return self.context.date_review_requested
23+ else:
24+ return self.context.date_reviewed
25+
26
27 class BranchMergeProposalListingBatchNavigator(TableBatchNavigator):
28 """Batch up the branch listings."""
29@@ -330,7 +348,7 @@
30 self.show_diffs = True
31 # Sort each collection...
32 for group in self.review_groups.values():
33- group.sort(key=attrgetter('date_review_requested'))
34+ group.sort(key=attrgetter('sort_key'))
35 self.proposal_count = len(proposals)
36
37 @cachedproperty
38
39=== modified file 'lib/lp/code/browser/tests/test_branchmergeproposallisting.py'
40--- lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-10-20 02:02:18 +0000
41+++ lib/lp/code/browser/tests/test_branchmergeproposallisting.py 2009-12-02 00:13:14 +0000
42@@ -16,7 +16,8 @@
43 from canonical.launchpad.webapp.servers import LaunchpadTestRequest
44 from canonical.testing import DatabaseFunctionalLayer
45 from lp.code.browser.branchmergeproposallisting import (
46- ActiveReviewsView, BranchMergeProposalListingView)
47+ ActiveReviewsView, BranchMergeProposalListingItem,
48+ BranchMergeProposalListingView)
49 from lp.code.enums import BranchMergeProposalStatus, CodeReviewVote
50 from lp.testing import ANONYMOUS, login, login_person, TestCaseWithFactory
51 from lp.testing.views import create_initialized_view
52@@ -255,6 +256,49 @@
53 reviewer, ActiveReviewsView.ARE_DOING)
54
55
56+class TestBranchMergeProposalListingItem(TestCaseWithFactory):
57+ """Tests specifically relating to the BranchMergeProposalListingItem."""
58+
59+ layer = DatabaseFunctionalLayer
60+
61+ def test_sort_key_needs_review(self):
62+ # If the proposal is in needs review, the sort_key will be the
63+ # date_review_requested.
64+ bmp = self.factory.makeBranchMergeProposal(
65+ date_created=datetime(2009,6,1,tzinfo=pytz.UTC))
66+ login_person(bmp.registrant)
67+ request_date = datetime(2009,7,1,tzinfo=pytz.UTC)
68+ bmp.requestReview(request_date)
69+ item = BranchMergeProposalListingItem(bmp, None, None)
70+ self.assertEqual(request_date, item.sort_key)
71+
72+ def test_sort_key_approved(self):
73+ # If the proposal is approved, the sort_key will default to the
74+ # date_review_requested.
75+ bmp = self.factory.makeBranchMergeProposal(
76+ date_created=datetime(2009,6,1,tzinfo=pytz.UTC))
77+ login_person(bmp.target_branch.owner)
78+ request_date = datetime(2009,7,1,tzinfo=pytz.UTC)
79+ bmp.requestReview(request_date)
80+ bmp.approveBranch(
81+ bmp.target_branch.owner, 'rev-id',
82+ datetime(2009,8,1,tzinfo=pytz.UTC))
83+ item = BranchMergeProposalListingItem(bmp, None, None)
84+ self.assertEqual(request_date, item.sort_key)
85+
86+ def test_sort_key_approved_from_wip(self):
87+ # If the proposal is approved and the review has been bypassed, the
88+ # date_reviewed is used.
89+ bmp = self.factory.makeBranchMergeProposal(
90+ date_created=datetime(2009,6,1,tzinfo=pytz.UTC))
91+ login_person(bmp.target_branch.owner)
92+ review_date = datetime(2009,8,1,tzinfo=pytz.UTC)
93+ bmp.approveBranch(
94+ bmp.target_branch.owner, 'rev-id', review_date)
95+ item = BranchMergeProposalListingItem(bmp, None, None)
96+ self.assertEqual(review_date, item.sort_key)
97+
98+
99 class ActiveReviewSortingTest(TestCaseWithFactory):
100 """Test the sorting of the active review groups."""
101
102
103=== modified file 'lib/lp/code/model/branchmergeproposal.py'
104--- lib/lp/code/model/branchmergeproposal.py 2009-11-27 02:39:17 +0000
105+++ lib/lp/code/model/branchmergeproposal.py 2009-12-02 00:13:14 +0000
106@@ -336,7 +336,6 @@
107 def setAsWorkInProgress(self):
108 """See `IBranchMergeProposal`."""
109 self._transitionToState(BranchMergeProposalStatus.WORK_IN_PROGRESS)
110- self.date_review_requested = None
111 self.reviewer = None
112 self.date_reviewed = None
113 self.reviewed_revision_id = None
114
115=== modified file 'lib/lp/code/model/tests/test_branchmergeproposals.py'
116--- lib/lp/code/model/tests/test_branchmergeproposals.py 2009-11-27 20:06:13 +0000
117+++ lib/lp/code/model/tests/test_branchmergeproposals.py 2009-12-02 00:13:14 +0000
118@@ -444,6 +444,13 @@
119 self.assertEqual(
120 proposal.date_created, proposal.date_review_requested)
121
122+ def test_date_not_reset_on_wip(self):
123+ # If a proposal has been in needs review state, and is moved back into
124+ # work in progress, the date_review_requested is not reset.
125+ proposal = self._createMergeProposal(needs_review=True)
126+ proposal.setAsWorkInProgress()
127+ self.assertIsNot(None, proposal.date_review_requested)
128+
129
130 class TestBranchMergeProposalQueueing(TestCase):
131 """Test the enqueueing and dequeueing of merge proposals."""

Subscribers

People subscribed via source and target branches

to status/vote changes: