Merge lp:~camptocamp/lp-community-utils/nag-user-specific-info into lp:lp-community-utils

Proposed by Yannick Vaucher @ Camptocamp
Status: Merged
Merged at revision: 33
Proposed branch: lp:~camptocamp/lp-community-utils/nag-user-specific-info
Merge into: lp:lp-community-utils
Diff against target: 195 lines (+78/-13)
1 file modified
openerp-nag (+78/-13)
To merge this branch: bzr merge lp:~camptocamp/lp-community-utils/nag-user-specific-info
Reviewer Review Type Date Requested Status
Stefan Rijnhart (Opener) test Approve
Sandy Carter (http://www.savoirfairelinux.com) grammar Needs Fixing
Guewen Baconnier @ Camptocamp code review, test Approve
Alexandre Fayolle - camptocamp code review, test Approve
Review via email: mp+209665@code.launchpad.net

Description of the change

Option --me or --triage to show what I'm supposed to do with that bunch of MPs

To post a comment you must log in.
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Excellent!

That will be really really useful.

On the output, I thing a blank line could be nice between the sections.

I'm not so enthusiast on the option's name "--user_categ" though. Can I propose --triage? Better idea?

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

You can still use --me instead of user_categ.

But I agree user_categ isn't very good. Triage is better.

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Changes made thanks for the review

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

There are some inconsistencies in the sentences:

Sometimes the sentences refer to the MPs with a plural form and sometimes with a singular, I prefer the plural form.

Example:
I didn't reviewed those ones
vs
Nothing to do I've already reviewed it

For REVIEWED_UPDATED, I propose 'Check again, updated since the last "Need Fixing" vote', emphasizing on the *again* because the reviewer already reviewed them. I think it should apply for the "Need Information" vote too. In that case it could be 'Check again, updated since the last asking' (same text for both statuses).

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Oh, maybe we could remove the whole sentence " A committer should consider to merge the proposal" and "Someone should review the merge request" when the triage is used, because it's seems very redundant when they are already grouped.

Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

looks good, works fine. I'm almost +1 for merging : the --user_categ option is not a very good choice imo.

review: Needs Fixing (code review, test)
Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

@Alex oops mispelled the push branche. I pushed it again

29. By Yannick Vaucher @ Camptocamp

change phrases for more consistancy

30. By Yannick Vaucher @ Camptocamp

remove sentences when triage is enabled

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Ready for review here

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Yannick,

I think that the "Needs Information" statuses should appear in the REVIEWED_UPDATED section alongside the "Needs Fixing" proposals. Then, the sentence could be "Check again, updated since the last asking" to leave some ambiguity between the 2 statuses.

The change would be on line 115

    user_state = REVIEWED_UPDATED if v.date_created < last_commit and v.comment and v.comment.vote in ('Need Fixing', 'Needs Information') else REVIEWED_PENDING

review: Needs Information
Revision history for this message
Alexandre Fayolle - camptocamp (alexandre-fayolle-c2c) wrote :

LGTM

review: Approve (code review, test)
31. By Yannick Vaucher @ Camptocamp

Add needs information to updated status

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Thanks!

review: Approve (code review, test)
Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

l.61, l.163 Can we avoid committing commented out code and blank lines.

review: Needs Fixing (code review)
32. By Yannick Vaucher @ Camptocamp

remove added space line and clean comented code

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Thanks for the review Sandy

Fixed

Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

Looks, good, just a few pep8 issues:

openerp-nag:72:9: E126 continuation line over-indented for hanging indent
        MY_READY: "My MPs ready to land",
        ^
openerp-nag:73:9: E126 continuation line over-indented for hanging indent
        MY_PENDING: "My MPs needing some love",
        ^
openerp-nag:74:9: E126 continuation line over-indented for hanging indent
        REVIEWED_READY: "Ready to land, please Merge us!",
        ^
openerp-nag:75:9: E126 continuation line over-indented for hanging indent
        REVIEWED_UPDATED: "To check, updated since last need fixing vote",
        ^
openerp-nag:76:9: E126 continuation line over-indented for hanging indent
        REVIEWED_PENDING: "Nothing to do I've already reviewed those",
        ^
openerp-nag:77:9: E126 continuation line over-indented for hanging indent
        NOT_REVIEWED: "I didn't reviewed those ones",
        ^
openerp-nag:80:1: E302 expected 2 blank lines, found 1
def show_lp_object(obj):
^
                                                                       ^
openerp-nag:390:5: E303 too many blank lines (2)
    if args.triage:
    ^
openerp-nag:415:5: E303 too many blank lines (2)
    print('Votes legend:')

review: Needs Fixing (code review, pep8)
Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

A few English fixes needed too. I am not an expert, but I am pretty sure the comma should either be a period or a semicolon, or even a colon in some cases.
24 + MY_READY: "My MPs ready to land",
25 + MY_PENDING: "My MPs needing some love",
26 + REVIEWED_READY: "Ready to land, please Merge us!",
"Ready to land; please Merge us!"
27 + REVIEWED_UPDATED: "To check, updated since last need fixing vote",
"To check; updated since last needs fixing vote"
28 + REVIEWED_PENDING: "Nothing to do I've already reviewed those",
"Nothing to do; I've already reviewed those"
29 + NOT_REVIEWED: "I didn't reviewed those ones",
"I haven't reviewed these ones"

review: Needs Fixing (grammar)
Revision history for this message
Sandy Carter (http://www.savoirfairelinux.com) (sandy-carter) wrote :

28 + REVIEWED_PENDING: "Nothing to do I've already reviewed those",
"Nothing to do; I've already reviewed these"

33. By Yannick Vaucher @ Camptocamp

[PEP8] of user specific nag addition

34. By Yannick Vaucher @ Camptocamp

English fixes

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

Thanks again Sandy

English messages are improved and pep8 of added code is done

Revision history for this message
Stefan Rijnhart (Opener) (stefan-opener) wrote :

Cool feature!

review: Approve (test)
Revision history for this message
Sylvain LE GAL (GRAP) (sylvain-legal) wrote :

good feature. (and good project I just discovered!)

LGTM.

Regards.

review: Approve (code review, no test)
Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Sandy, is it ok for you now?

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

Yannick, you will need to rebase unfortunately, the head diverged.

35. By Yannick Vaucher @ Camptocamp

Merge from upsteam and resolve conflicts

Revision history for this message
Yannick Vaucher @ Camptocamp (yvaucher-c2c) wrote :

I resolved conflicts, ready to be merged

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'openerp-nag'
2--- openerp-nag 2014-04-25 11:48:45 +0000
3+++ openerp-nag 2014-04-30 10:17:01 +0000
4@@ -37,6 +37,7 @@
5 period of time
6 * Nagging about aging unanswered questions on launchpad answers
7 * Nagging about accepted merge proposals that have not been merged yet
8+ * Nagging by triaging MP in categories
9 """
10
11 from __future__ import unicode_literals, print_function
12@@ -61,10 +62,21 @@
13 # ...with the specified 'action'
14 # ...about a particular 'subject'
15 Nag = collections.namedtuple(
16- "Nag", "person action subject sort_class sort_priority sort_age project_name votes")
17+ "Nag", "person action subject sort_class sort_priority sort_age project_name votes sort_triage")
18
19 SORT_CLASS_BUG, SORT_CLASS_MERGE = range(2)
20
21+MY_READY, MY_PENDING, REVIEWED_READY, REVIEWED_UPDATED, REVIEWED_PENDING, NOT_REVIEWED = range(6)
22+
23+USER_CATEGORIES = {
24+ MY_READY: "My MPs ready to land",
25+ MY_PENDING: "My MPs needing some love",
26+ REVIEWED_READY: "Ready to land; please Merge us!",
27+ REVIEWED_UPDATED: "To check; updated since last need fixing vote",
28+ REVIEWED_PENDING: "Nothing to do; I've already reviewed these",
29+ NOT_REVIEWED: "I haven't reviewed these ones",
30+}
31+
32
33 def show_lp_object(obj):
34 print(obj)
35@@ -165,7 +177,7 @@
36 state, code in cls.__states__.iteritems())
37
38
39-def gen_project_nags(lp, policy, project_name):
40+def gen_project_nags(lp, policy, project_name, triage=False):
41 # TODO: Detect project groups and redirect the nag to all projects
42 # underneath that project
43 # Access the project that we care about
44@@ -173,11 +185,11 @@
45 project = lp.projects[project_name]
46 # Re-yield all the merge proposal nags
47 # NOTE: change to yield from in py3k3
48- for nag in gen_merge_proposal_nags(lp, policy, project):
49+ for nag in gen_merge_proposal_nags(lp, policy, project, triage):
50 yield nag
51
52
53-def gen_merge_proposal_nags(lp, policy, project):
54+def gen_merge_proposal_nags(lp, policy, project, triage=False):
55 # XXX: cannnot use utcnow as launchpad returns tz-aware objects here
56 now = datetime.datetime.now(tz=UTC)
57 logging.debug("Looking for merge proposals in project %r", project)
58@@ -208,16 +220,29 @@
59 sort_class=SORT_CLASS_MERGE,
60 sort_priority=None, # TODO: get from max(linked bugs)
61 sort_age=review_age.days,
62+ sort_triage=None,
63 project_name=project.name,
64 votes=votes,
65 )
66
67 # Nag about approved merges
68 age = (now - proposal.date_review_requested).days
69+
70+ user_state = None
71+ if triage:
72+ me = lp.me
73+ is_my_mp = proposal.registrant == me
74 if (votes.approve == votes.total(for_approval=True) and
75 (votes.approve >= policy.approvals_to_bypass or
76 votes.approve >= policy.min_approve and
77 age >= policy.days_before_merge)):
78+
79+ if triage:
80+ if is_my_mp:
81+ user_state = MY_READY
82+ else:
83+ user_state = REVIEWED_READY
84+
85 yield Nag(
86 person='A committer',
87 action="consider to merge the proposal",
88@@ -225,6 +250,7 @@
89 sort_class=SORT_CLASS_MERGE,
90 sort_priority=None, # TODO: get from max(linked bugs)
91 sort_age=review_age.days,
92+ sort_triage=user_state,
93 project_name=project.name,
94 votes=votes,
95 )
96@@ -232,6 +258,18 @@
97
98 # Nag about aging merge requests
99 if review_age.days >= policy.max_review_age:
100+
101+ if triage:
102+ if is_my_mp:
103+ user_state = MY_PENDING
104+ else:
105+ user_state = NOT_REVIEWED
106+ for v in proposal.votes:
107+ if v.reviewer == me:
108+ last_commit = proposal.source_branch.date_last_modified
109+ user_state = REVIEWED_UPDATED if v.date_created < last_commit and v.comment and v.comment.vote in ('Need Fixing', 'Needs Information') else REVIEWED_PENDING
110+ break
111+
112 yield Nag(
113 person='Someone',
114 action="review the merge request",
115@@ -239,6 +277,7 @@
116 sort_class=SORT_CLASS_MERGE,
117 sort_priority=None, # TODO: get from max(linked bugs)
118 sort_age=review_age.days,
119+ sort_triage=user_state,
120 project_name=project.name,
121 votes=votes,
122 )
123@@ -298,6 +337,10 @@
124 group.add_argument(
125 '--production', action='store_const', const='production',
126 dest='service_root', help="Use production launchpad instance")
127+ group.add_argument(
128+ '--triage', '--me', action='store_const', const='triage',
129+ help="Use authenticated user to triage merge proposal in categories"
130+ "based on their status")
131 parser.set_defaults(anonymous=True, service_root='production')
132 args = parser.parse_args()
133 # Get the policy
134@@ -305,6 +348,9 @@
135 # Reconfigure logging
136 if args.debug:
137 logging.basicConfig(level=logging.DEBUG)
138+ if args.triage:
139+ args.authenticated = True
140+ args.anonymous = False
141 # Access Lauchpad object as the current (system) user or anonymously
142 if args.anonymous:
143 lp = Launchpad.login_anonymously(consumer_name, args.service_root)
144@@ -331,23 +377,42 @@
145 progress = ProgressBar(widgets=widgets, maxval=len(project_names)).start()
146 for project_name in project_names:
147 logging.info("Looking for things to nag about under %s", project_name)
148- nags.extend(gen_project_nags(lp, policy, project_name))
149+ nags.extend(gen_project_nags(lp, policy, project_name, args.triage))
150 if ProgressBar:
151 progress.update(progress.currval + 1)
152 if ProgressBar:
153 progress.finish()
154
155- nags.sort(key=lambda nag: (nag.sort_class, nag.sort_priority,
156- -nag.sort_age))
157+ nags.sort(key=lambda nag: (nag.sort_triage, nag.sort_class,
158+ nag.sort_priority, -nag.sort_age))
159 print("=" * 80)
160 print("Done thinking, here's the nag list".center(80))
161 print("=" * 80)
162- for index1, nag in enumerate(nags, 1):
163- print("{index1:-2}: [age {age}] (votes {votes}) {person} should {action} {subject} "
164- "on the project {project}".format(
165- index1=index1, age=nag.sort_age, person=nag.person,
166- action=nag.action, subject=nag.subject, project=nag.project_name,
167- votes=nag.votes or ''))
168+
169+ # Nag with user categories
170+ if args.triage:
171+ for sort_triage in range(6):
172+ categ_nags = [nag for nag in enumerate(nags, 1)
173+ if nag[1].sort_triage == sort_triage]
174+ if not categ_nags:
175+ continue
176+ print("=" * 80)
177+ print(USER_CATEGORIES[sort_triage].center(80))
178+ print("MP: %d".center(80) % len(categ_nags))
179+ for index1, nag in categ_nags:
180+ print("{index1:-2}: [age {age}] (votes {votes}) {subject} "
181+ "{project}".format(
182+ index1=index1, age=(nag.sort_age and -nag.sort_age),
183+ subject=nag.subject, project=nag.project_name,
184+ votes=nag.votes or ''))
185+ print()
186+ else:
187+ for index1, nag in enumerate(nags, 1):
188+ print("{index1:-2}: [age {age}] (votes {votes}) {person} should {action} {subject} "
189+ "on the project {project}".format(
190+ index1=index1, age=nag.sort_age, person=nag.person,
191+ action=nag.action, subject=nag.subject, project=nag.project_name,
192+ votes=nag.votes or ''))
193
194 print('Votes legend:')
195 if args.anonymous:

Subscribers

People subscribed via source and target branches