Merge lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-connect-hacking into lp:~linaro-automation/launchpad-work-items-tracker/linaro
- linaro-roadmap-connect-hacking
- Merge into linaro
Status: | Merged |
---|---|
Merged at revision: | 307 |
Proposed branch: | lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-connect-hacking |
Merge into: | lp:~linaro-automation/launchpad-work-items-tracker/linaro |
Diff against target: |
473 lines (+188/-81) 8 files modified
collect_roadmap (+30/-11) generate-all (+7/-3) html-report (+15/-10) lpworkitems/database.py (+7/-1) lpworkitems/models_roadmap.py (+2/-0) report_tools.py (+88/-2) templates/roadmap_card.html (+20/-39) templates/roadmap_lane.html (+19/-15) |
To merge this branch: | bzr merge lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-connect-hacking |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Guilherme Salgado (community) | Approve | ||
Review via email: mp+82898@code.launchpad.net |
Commit message
Description of the change
Hi,
This branch contains some changes back and forth and is a bit big. I hope it is reviewable though, if it's not I'll work on breaking it down in smaller chunks.
The main thing this adds is some plumbing for card health checks and a first set of checks.
It also changes the way information is displayed after feedback at the Connect. There is more to do, but I'd like to get this landed so we can start making smaller changes step by step.
Thanks,
Mattias
Guilherme Salgado (salgado) wrote : | # |
Mattias Backman (mabac) wrote : | # |
On Mon, Nov 21, 2011 at 10:22 PM, Guilherme Salgado
<email address hidden> wrote:
> Hi Mattias,
>
> This looks quite good. I have a few questions below but I don't think
> I've ever touched this code so please forgive me if any of them are
> stupid. :)
>
> On Mon, 2011-11-21 at 15:23 +0000, Mattias Backman wrote:
> [...]
>>
>> This branch contains some changes back and forth and is a bit big. I
>> hope it is reviewable though, if it's not I'll work on breaking it
>> down in smaller chunks.
>
> It's not too big in fact, and although a very long diff is no fun, what
> is less fun is when there are lots of unrelated changes bundled together
> in a single branch. I hope that's not the case here! ;)
As you've seen it's a bit like that. :/
>
>>
>> The main thing this adds is some plumbing for card health checks and a
>> first set of checks.
>>
>> It also changes the way information is displayed after feedback at the
>> Connect. There is more to do, but I'd like to get this landed so we
>> can start making smaller changes step by step.
>
>> === modified file 'collect_roadmap'
>> --- collect_roadmap 2011-10-27 13:23:14 +0000
>> +++ collect_roadmap 2011-11-21 15:22:45 +0000
>> @@ -53,7 +53,7 @@
>> def kanban_
>> nodes = {}
>> root_node_id = None
>> - lanes_to_ignore = ['Legend', 'Deferred']
>> + lanes_to_ignore = ['Legend']
>>
>> # Iterate over all workflow_stages which may be in any order.
>> for workflow_stage in workflow_stages:
>> @@ -75,7 +75,7 @@
>> assert node['parent_id'] == root_node_id
>> model_lane = Lane(unicode_
>> node['id'])
>> - if model_lane.name == '2011Q3':
>> + if model_lane.name == '2011Q4':
>
> Would it be worth moving this to a constant?
We should actually move it to an external config of some sort. That
reminds me to talk to Joey about if this can be a flag on the lanes in
Kanbantool. I'll get back to you on that one.
>
>> model_lane.
>> else:
>> model_lane.
>> @@ -116,21 +116,30 @@
>> unicode_
>> model_card.status = unicode_
>> model_card.team = unicode_
>> - model_card.priority = u"%s" % task['task'
>> + model_card.priority = lookup_
>> model_card.size = unicode_
>> model_card.sponsor = unicode_
>>
>> external_link = task['task'
>> - if external_link is not None:
>> + if external_link is not None and external_link is not '':
>> + model_card.url = unicode_
>> + dbg('Getting Papyrs information from %s.' % external_link)
>> papyrs_data = papyrs_
>> model_car...
Guilherme Salgado (salgado) wrote : | # |
On Tue, 2011-11-22 at 12:47 +0000, Mattias Backman wrote:
> On Mon, Nov 21, 2011 at 10:22 PM, Guilherme Salgado
> <email address hidden> wrote:
[...]
> > It's not too big in fact, and although a very long diff is no fun, what
> > is less fun is when there are lots of unrelated changes bundled together
> > in a single branch. I hope that's not the case here! ;)
>
> As you've seen it's a bit like that. :/
It was ok, actually. :)
> >>
> >> The main thing this adds is some plumbing for card health checks and a
> >> first set of checks.
> >>
> >> It also changes the way information is displayed after feedback at the
> >> Connect. There is more to do, but I'd like to get this landed so we
> >> can start making smaller changes step by step.
> >
> >> === modified file 'collect_roadmap'
> >> --- collect_roadmap 2011-10-27 13:23:14 +0000
> >> +++ collect_roadmap 2011-11-21 15:22:45 +0000
> >> @@ -53,7 +53,7 @@
> >> def kanban_
> >> nodes = {}
> >> root_node_id = None
> >> - lanes_to_ignore = ['Legend', 'Deferred']
> >> + lanes_to_ignore = ['Legend']
> >>
> >> # Iterate over all workflow_stages which may be in any order.
> >> for workflow_stage in workflow_stages:
> >> @@ -75,7 +75,7 @@
> >> assert node['parent_id'] == root_node_id
> >> model_lane = Lane(unicode_
> >> node['id'])
> >> - if model_lane.name == '2011Q3':
> >> + if model_lane.name == '2011Q4':
> >
> > Would it be worth moving this to a constant?
>
> We should actually move it to an external config of some sort. That
> reminds me to talk to Joey about if this can be a flag on the lanes in
> Kanbantool. I'll get back to you on that one.
Ok, cool. If that's not possible then I'd be happy with a constant
defined at the top of this module.
> >> @@ -160,12 +169,22 @@
> >> last_heading = page_item['text']
> >> if page_item[
> >> if not has_found_
> >> - description = page_item['text']
> >> + description = page_item['html']
> >
> > Why are we using the html instead of the plaintext version now?
>
> We don't want to loose the formatting that whoever edits the Papyrs
> document adds. It's also a practical thing since the text version is
> just the html stripped of markup. So lists end up as a sentence that
> do not make sense and paragraph breaks are lost (since they are <br>
> which are not translated to line breaks).
Oh, ok, that makes sense.
>
> >
> >> has_found_
> >> - elif last_heading == 'Acceptance Criteria':
> >> - acceptance_criteria = page_item['text']
> >> -
> >> - return {'description': description, 'acceptance_
> >> + elif 'Acceptance Criteria' in last_heading:
> >> + acceptance_criteria = page_item['html']
> >> +
> >> + return {'description': get_first_
> >> + 'acceptance_
> >> +
> >> +
> >> +def get_first_par...
Mattias Backman (mabac) wrote : | # |
> On Tue, 2011-11-22 at 12:47 +0000, Mattias Backman wrote:
> > On Mon, Nov 21, 2011 at 10:22 PM, Guilherme Salgado
> > <email address hidden> wrote:
> [...]
> > > It's not too big in fact, and although a very long diff is no fun, what
> > > is less fun is when there are lots of unrelated changes bundled together
> > > in a single branch. I hope that's not the case here! ;)
> >
> > As you've seen it's a bit like that. :/
>
> It was ok, actually. :)
>
> > >>
> > >> The main thing this adds is some plumbing for card health checks and a
> > >> first set of checks.
> > >>
> > >> It also changes the way information is displayed after feedback at the
> > >> Connect. There is more to do, but I'd like to get this landed so we
> > >> can start making smaller changes step by step.
> > >
> > >> === modified file 'collect_roadmap'
> > >> --- collect_roadmap 2011-10-27 13:23:14 +0000
> > >> +++ collect_roadmap 2011-11-21 15:22:45 +0000
> > >> @@ -53,7 +53,7 @@
> > >> def kanban_
> > >> nodes = {}
> > >> root_node_id = None
> > >> - lanes_to_ignore = ['Legend', 'Deferred']
> > >> + lanes_to_ignore = ['Legend']
> > >>
> > >> # Iterate over all workflow_stages which may be in any order.
> > >> for workflow_stage in workflow_stages:
> > >> @@ -75,7 +75,7 @@
> > >> assert node['parent_id'] == root_node_id
> > >> model_lane = Lane(unicode_
> > >> node['id'])
> > >> - if model_lane.name == '2011Q3':
> > >> + if model_lane.name == '2011Q4':
> > >
> > > Would it be worth moving this to a constant?
> >
> > We should actually move it to an external config of some sort. That
> > reminds me to talk to Joey about if this can be a flag on the lanes in
> > Kanbantool. I'll get back to you on that one.
>
> Ok, cool. If that's not possible then I'd be happy with a constant
> defined at the top of this module.
Apparently there is no data for a Lane except for it's name. So I'll go with the constant and we can change it to an external config when we need to.
>
> > >> @@ -160,12 +169,22 @@
> > >> last_heading = page_item['text']
> > >> if page_item[
> > >> if not has_found_
> > >> - description = page_item['text']
> > >> + description = page_item['html']
> > >
> > > Why are we using the html instead of the plaintext version now?
> >
> > We don't want to loose the formatting that whoever edits the Papyrs
> > document adds. It's also a practical thing since the text version is
> > just the html stripped of markup. So lists end up as a sentence that
> > do not make sense and paragraph breaks are lost (since they are <br>
> > which are not translated to line breaks).
>
> Oh, ok, that makes sense.
>
> >
> > >
> > >> has_found_
> > >> - elif last_heading == 'Acceptance Criteria':
> > >> - acceptance_criteria = page_item['text']
> > >> -
> > >> - return {'description': description, 'acceptance_
> acceptance_
> > >> + elif...
Guilherme Salgado (salgado) wrote : | # |
On Wed, 2011-11-23 at 08:46 +0000, Mattias Backman wrote:
[...]
> > > >
> > > >> has_found_
> > > >> - elif last_heading == 'Acceptance Criteria':
> > > >> - acceptance_criteria = page_item['text']
> > > >> -
> > > >> - return {'description': description, 'acceptance_
> > acceptance_
> > > >> + elif 'Acceptance Criteria' in last_heading:
> > > >> + acceptance_criteria = page_item['html']
> > > >> +
> > > >> + return {'description': get_first_
> > > >> + 'acceptance_
> > get_first_
> > > >> +
> > > >> +
> > > >> +def get_first_
> > > >> + if text is None:
> > > >> + return None
> > > >> + # This might break, depending on what type of line breaks
> > > >> + # whoever authors the Papyrs document uses.
> > > >> + first_pararaph, _, _ = text.partition(
> > > >
> > > > Would it be possible to use something like BeautifulSoup to make this
> > > > more robust?
> > >
> > > Possibly. I can have a look at what it can do. This seemed to work but
> > > now I see that someone has added BRs just before a list too which gets
> > > ugly.
> >
> > What does the HTML you're parsing look like? On, say
> > https:/
> > actual content in a javascript function, so I'm assuming you're parsing
> > something else?
>
> Here's one example with a list: http://
> Sorry that it's all on one line. I get it from the json API, but it's
> not much better than scraping that javascript. Sometimes we get
> whitespace with a lot of formatting around it, just like when people
> edit html using MS Word.
Ok, apparently there's not much structure on the HTML; just some lists
and <br>s to force line breaks, so I think BeautifulSoup wouldn't help
much here.
> >
> > > >
> > > >> + return first_pararaph
> > > >>
> > > >>
> > > >> #######
> > > >
> > > >
> > > >> === modified file 'report_tools.py'
> > > >> --- report_tools.py 2011-10-24 15:08:02 +0000
> > > >> +++ report_tools.py 2011-11-21 15:22:45 +0000
[...]
> > > >
> > > > Having all the classes here make for a rather long function. Is there a
> > > > reason for keeping them here other than the fact that they're not used
> > > > anywhere else?
> > >
> > > I wasn't even sure that creating subclasses would be a good idea.
> > > Since i fear that we will see a lot more checks when PMs know what
> > > they want I guess these should go on a module instead.
> > >
> > > >
> > > >> +
> > > >> + health_checks = []
> > > >> + health_
> > > >> + health_
> > > >> + health_
> > > >> + health_
> > > >> + health_
> > > >
> > > > One neat thing we could do is use a class decorator on all HealthCheck
> > > > classes to register them on a global registry (a...
Mattias Backman (mabac) wrote : | # |
I have fixed the pep8 and constant suggestions and used Salgado's
magic to move the health checks to a separate module.
Some items (mostly visual stuff from kiko) carry over and have been
registered in a new bp.
On Thu, Nov 24, 2011 at 11:45 AM, <email address hidden> wrote:
> The proposal to merge lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-connect-hacking into lp:~linaro-infrastructure/launchpad-work-items-tracker/linaro has been updated.
>
> Status: Needs review => Merged
>
> For more details, see:
> https:/
> --
> https:/
> You are the owner of lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-connect-hacking.
>
Preview Diff
1 | === modified file 'collect_roadmap' |
2 | --- collect_roadmap 2011-10-27 13:23:14 +0000 |
3 | +++ collect_roadmap 2011-11-21 15:22:45 +0000 |
4 | @@ -53,7 +53,7 @@ |
5 | def kanban_import_lanes(collector, workflow_stages): |
6 | nodes = {} |
7 | root_node_id = None |
8 | - lanes_to_ignore = ['Legend', 'Deferred'] |
9 | + lanes_to_ignore = ['Legend'] |
10 | |
11 | # Iterate over all workflow_stages which may be in any order. |
12 | for workflow_stage in workflow_stages: |
13 | @@ -75,7 +75,7 @@ |
14 | assert node['parent_id'] == root_node_id |
15 | model_lane = Lane(unicode_or_None(node['name']), |
16 | node['id']) |
17 | - if model_lane.name == '2011Q3': |
18 | + if model_lane.name == '2011Q4': |
19 | model_lane.is_current = True |
20 | else: |
21 | model_lane.is_current = False |
22 | @@ -116,21 +116,30 @@ |
23 | unicode_or_None(task['task']['external_id'])) |
24 | model_card.status = unicode_or_None(task_status['name']) |
25 | model_card.team = unicode_or_None(card_type_name) |
26 | - model_card.priority = u"%s" % task['task']['priority'] |
27 | + model_card.priority = lookup_kanban_priority(task['task']['priority']) |
28 | model_card.size = unicode_or_None(task['task']['size_estimate']) |
29 | model_card.sponsor = unicode_or_None(task['task']['custom_field_1']) |
30 | |
31 | external_link = task['task']['external_link'] |
32 | - if external_link is not None: |
33 | + if external_link is not None and external_link is not '': |
34 | + model_card.url = unicode_or_None(external_link) |
35 | + dbg('Getting Papyrs information from %s.' % external_link) |
36 | papyrs_data = papyrs_import(collector, external_link) |
37 | model_card.description = unicode_or_None(papyrs_data['description']) |
38 | model_card.acceptance_criteria = unicode_or_None(papyrs_data['acceptance_criteria']) |
39 | - |
40 | collector.store_card(model_card) |
41 | else: |
42 | dbg("Task '%s' does not have a status." % task['task']['name']) |
43 | |
44 | |
45 | +def lookup_kanban_priority(numeric_priority): |
46 | + priority_lookup = { -1 : "low", |
47 | + 0 : "normal", |
48 | + 1 : "high" } |
49 | + assert numeric_priority in priority_lookup, ( |
50 | + "Priority '%s' is unknown." % numeric_priority) |
51 | + return unicode_or_None(priority_lookup[numeric_priority]) |
52 | + |
53 | def kanban_import(collector, cfg, board_id, api_token): |
54 | '''Collect roadmap items from KanbanTool into DB.''' |
55 | board_url = get_kanban_url('boards/%s.json' % board_id, api_token) |
56 | @@ -150,7 +159,7 @@ |
57 | acceptance_criteria = None |
58 | |
59 | page = get_json_data(requirement_url + '?json&auth_token=e8a2801e4f0f') |
60 | - |
61 | + assert page is not None, "Could not access Papyrs document %s." % requirement_url |
62 | page_text_items = page[0] |
63 | page_extra_items = page[1] |
64 | |
65 | @@ -160,12 +169,22 @@ |
66 | last_heading = page_item['text'] |
67 | if page_item['classname'] == 'Paragraph': |
68 | if not has_found_description: |
69 | - description = page_item['text'] |
70 | + description = page_item['html'] |
71 | has_found_description = True |
72 | - elif last_heading == 'Acceptance Criteria': |
73 | - acceptance_criteria = page_item['text'] |
74 | - |
75 | - return {'description': description, 'acceptance_criteria': acceptance_criteria} |
76 | + elif 'Acceptance Criteria' in last_heading: |
77 | + acceptance_criteria = page_item['html'] |
78 | + |
79 | + return {'description': get_first_paragraph(description), |
80 | + 'acceptance_criteria': get_first_paragraph(acceptance_criteria)} |
81 | + |
82 | + |
83 | +def get_first_paragraph(text): |
84 | + if text is None: |
85 | + return None |
86 | + # This might break, depending on what type of line breaks |
87 | + # whoever authors the Papyrs document uses. |
88 | + first_pararaph, _, _ = text.partition('<br>') |
89 | + return first_pararaph |
90 | |
91 | |
92 | ######################################################################## |
93 | |
94 | === modified file 'generate-all' |
95 | --- generate-all 2011-10-18 11:34:41 +0000 |
96 | +++ generate-all 2011-11-21 15:22:45 +0000 |
97 | @@ -159,9 +159,13 @@ |
98 | |
99 | # roadmap cards |
100 | for card in report_tools.cards(store): |
101 | - basename = os.path.join(opts.output_dir, 'roadmap-card-%s' % \ |
102 | - card.roadmap_id) |
103 | - report_tools.roadmap_cards(my_path, opts.database, basename, opts.config, card, root=opts.root) |
104 | + if card.roadmap_id != '': |
105 | + page_name = card.roadmap_id |
106 | + else: |
107 | + page_name = card.card_id |
108 | + basename = os.path.join(opts.output_dir, 'roadmap-card-%s' % page_name) |
109 | + report_tools.roadmap_cards(my_path, opts.database, basename, opts.config, |
110 | + card, root=opts.root) |
111 | |
112 | def copy_files(source_dir): |
113 | for filename in os.listdir(source_dir): |
114 | |
115 | === modified file 'html-report' |
116 | --- html-report 2011-10-27 12:20:51 +0000 |
117 | +++ html-report 2011-11-21 15:22:45 +0000 |
118 | @@ -474,20 +474,16 @@ |
119 | title = opts.title |
120 | |
121 | data = self.template_data(store, opts) |
122 | - lane = report_tools.lane(store, opts.lane).one() |
123 | + lane = report_tools.lane(store, opts.lane) |
124 | lanes = report_tools.lanes(store) |
125 | statuses = [] |
126 | for status, cards in report_tools.statuses(store, lane): |
127 | - cards_with_bp_counts = [] |
128 | + cards_with_bps = [] |
129 | for card in cards: |
130 | + report_tools.check_card_health(store, card) |
131 | blueprints = report_tools.card_blueprints_by_status(store, card.roadmap_id) |
132 | - count_string = '' |
133 | - for bp_status in blueprints.keys(): |
134 | - bp_count = len(blueprints[bp_status]) |
135 | - if bp_count > 0: |
136 | - count_string += '%s: %d ' % (bp_status, bp_count) |
137 | - cards_with_bp_counts.append({'card': card, 'count': count_string}) |
138 | - statuses.append(dict(name=status, cards=cards_with_bp_counts)) |
139 | + cards_with_bps.append({'card': card, 'blueprints': blueprints}) |
140 | + statuses.append(dict(name=status, cards=cards_with_bps)) |
141 | |
142 | data.update(dict(statuses=statuses)) |
143 | data.update(dict(page_type="roadmap_lane")) |
144 | @@ -503,7 +499,8 @@ |
145 | |
146 | data = self.template_data(store, opts) |
147 | card = report_tools.card(store, int(opts.card)).one() |
148 | - lane = report_tools.lane(store, None, id=card.lane_id).one() |
149 | + health_checks = report_tools.check_card_health(store, card) |
150 | + lane = report_tools.lane(store, None, id=card.lane_id) |
151 | |
152 | if not opts.title: |
153 | title = card.name |
154 | @@ -511,12 +508,20 @@ |
155 | title = opts.title |
156 | |
157 | blueprints = report_tools.card_blueprints_by_status(store, card.roadmap_id) |
158 | + blueprint_status_count = WorkitemTarget() |
159 | + blueprint_status_count.todo = len(blueprints['Planned']) |
160 | + blueprint_status_count.blocked = len(blueprints['Blocked']) |
161 | + blueprint_status_count.inprogress = len(blueprints['In Progress']) |
162 | + blueprint_status_count.postponed = 0 |
163 | + blueprint_status_count.done = len(blueprints['Completed']) |
164 | |
165 | data.update(dict(page_type="roadmap_card")) |
166 | data.update(dict(card_title=title)) |
167 | data.update(dict(card=card)) |
168 | + data.update(dict(health_checks=health_checks)) |
169 | data.update(dict(lane=lane.name)) |
170 | data.update(dict(blueprints=blueprints)) |
171 | + data.update(dict(blueprint_status_count=blueprint_status_count)) |
172 | |
173 | print report_tools.fill_template( |
174 | "roadmap_card.html", data, theme=opts.theme) |
175 | |
176 | === modified file 'lpworkitems/database.py' |
177 | --- lpworkitems/database.py 2011-10-13 15:31:42 +0000 |
178 | +++ lpworkitems/database.py 2011-11-21 15:22:45 +0000 |
179 | @@ -7,7 +7,7 @@ |
180 | store.execute('''CREATE TABLE version ( |
181 | db_layout_ref INT NOT NULL |
182 | )''') |
183 | - store.execute('''INSERT INTO version VALUES (13)''') |
184 | + store.execute('''INSERT INTO version VALUES (14)''') |
185 | |
186 | store.execute('''CREATE TABLE specs ( |
187 | name VARCHAR(255) PRIMARY KEY, |
188 | @@ -100,6 +100,8 @@ |
189 | store.execute('''CREATE TABLE card ( |
190 | name VARCHAR(200) NOT NULL, |
191 | card_id NOT NULL, |
192 | + url VARCHAR(200), |
193 | + is_healthy BOOLEAN, |
194 | status VARCHAR(50), |
195 | team VARCHAR(50), |
196 | priority VARCHAR(50), |
197 | @@ -228,6 +230,10 @@ |
198 | store.execute('ALTER TABLE card ADD COLUMN acceptance_criteria BLOB') |
199 | store.execute('ALTER TABLE lane ADD COLUMN is_current BOOLEAN') |
200 | store.execute('UPDATE version SET db_layout_ref = 13') |
201 | + if ver == 13: |
202 | + store.execute('ALTER TABLE card ADD COLUMN url VARCHAR(200)') |
203 | + store.execute('ALTER TABLE card ADD COLUMN is_healthy BOOLEAN') |
204 | + store.execute('UPDATE version SET db_layout_ref = 14') |
205 | |
206 | |
207 | def get_store(dbpath): |
208 | |
209 | === modified file 'lpworkitems/models_roadmap.py' |
210 | --- lpworkitems/models_roadmap.py 2011-10-13 15:31:42 +0000 |
211 | +++ lpworkitems/models_roadmap.py 2011-11-21 15:22:45 +0000 |
212 | @@ -21,6 +21,8 @@ |
213 | contact = Unicode() |
214 | description = Unicode() |
215 | acceptance_criteria = Unicode() |
216 | + url = Unicode() |
217 | + is_healthy = Bool() |
218 | |
219 | def __init__(self, name, card_id, lane_id, roadmap_id): |
220 | self.lane_id = lane_id |
221 | |
222 | === modified file 'report_tools.py' |
223 | --- report_tools.py 2011-10-24 15:08:02 +0000 |
224 | +++ report_tools.py 2011-11-21 15:22:45 +0000 |
225 | @@ -942,14 +942,15 @@ |
226 | |
227 | def lane(store, name, id=None): |
228 | if id is None: |
229 | - return store.find(Lane, Lane.name == unicode(name)) |
230 | + return store.find(Lane, Lane.name == unicode(name)).one() |
231 | else: |
232 | - return store.find(Lane, Lane.lane_id == id) |
233 | + return store.find(Lane, Lane.lane_id == id).one() |
234 | |
235 | |
236 | def lane_cards(store, lane): |
237 | return lane.cards |
238 | |
239 | + |
240 | def statuses(store, lane): |
241 | result = [] |
242 | for status in store.find(Card.status, |
243 | @@ -995,6 +996,91 @@ |
244 | return bp_by_status |
245 | |
246 | |
247 | +def check_card_health(store, card): |
248 | + class HealthCheck(object): |
249 | + NOT_APPLICABLE = 'n/a' |
250 | + OK = 'OK' |
251 | + NOT_OK = 'Not OK' |
252 | + name = 'Base check, not to be used' |
253 | + |
254 | + def applicable(self, card, store=None): |
255 | + raise NotImplementedError() |
256 | + |
257 | + def check(self, card, store=None): |
258 | + raise NotImplementedError() |
259 | + |
260 | + def execute(self, card, store=None): |
261 | + if self.applicable(card, store): |
262 | + if self.check(card, store): |
263 | + return self.OK |
264 | + else: |
265 | + return self.NOT_OK |
266 | + else: |
267 | + return self.NOT_APPLICABLE |
268 | + |
269 | + class DescriptionHealthCheck(HealthCheck): |
270 | + name = 'Has description' |
271 | + |
272 | + def applicable(self, card, store=None): |
273 | + return True |
274 | + |
275 | + def check(self, card, store=None): |
276 | + return card.description is not None |
277 | + |
278 | + class CriteriaHealthCheck(HealthCheck): |
279 | + name = 'Has acceptance criteria' |
280 | + |
281 | + def applicable(self, card, store=None): |
282 | + return True |
283 | + |
284 | + def check(self, card, store=None): |
285 | + return card.acceptance_criteria is not None |
286 | + |
287 | + class BlueprintsHealthCheck(HealthCheck): |
288 | + name = 'Has blueprints' |
289 | + |
290 | + def applicable(self, card, store): |
291 | + return card.status == 'Ready' |
292 | + |
293 | + def check(self, card, store): |
294 | + return len(card_blueprints(store, card.roadmap_id)) > 0 |
295 | + |
296 | + class BlueprintsBlockedHealthCheck(HealthCheck): |
297 | + name = 'Has no Blocked blueprints' |
298 | + |
299 | + def applicable(self, card, store): |
300 | + return card.status != 'Ready' |
301 | + |
302 | + def check(self, card, store): |
303 | + blueprints = card_blueprints_by_status(store, card.roadmap_id) |
304 | + return len(blueprints['Blocked']) == 0 |
305 | + |
306 | + class RoadmapIdHealthCheck(HealthCheck): |
307 | + name = 'Has a roadmap id' |
308 | + |
309 | + def applicable(self, card, store=None): |
310 | + return True |
311 | + |
312 | + def check(self, card, store=None): |
313 | + return card.roadmap_id != '' |
314 | + |
315 | + health_checks = [] |
316 | + health_checks.append(RoadmapIdHealthCheck()) |
317 | + health_checks.append(DescriptionHealthCheck()) |
318 | + health_checks.append(CriteriaHealthCheck()) |
319 | + health_checks.append(BlueprintsHealthCheck()) |
320 | + health_checks.append(BlueprintsBlockedHealthCheck()) |
321 | + |
322 | + performed_checks = [] |
323 | + card.is_healthy = True |
324 | + for check in health_checks: |
325 | + result = check.execute(card, store) |
326 | + if result == check.NOT_OK: |
327 | + card.is_healthy = False |
328 | + performed_checks.append({'name': check.name, |
329 | + 'result': result}) |
330 | + return performed_checks |
331 | + |
332 | def subteams(store, team): |
333 | result = store.execute('SELECT name from team_structure where team = ?', (unicode(team),)) |
334 | return [i[0] for i in result] |
335 | |
336 | === modified file 'templates/roadmap_card.html' |
337 | --- templates/roadmap_card.html 2011-10-31 16:12:14 +0000 |
338 | +++ templates/roadmap_card.html 2011-11-21 15:22:45 +0000 |
339 | @@ -14,8 +14,6 @@ |
340 | <h2>${card.status} in <a href="roadmap-${lane}.html">${lane}</a></h2> |
341 | <p> |
342 | <ul> |
343 | - <li>Description: ${card.description[:250]} |
344 | - <li>Acceptance criteria: ${card.acceptance_criteria[:250]} |
345 | <li>Sponsor: ${card.sponsor} |
346 | <li>Contact: ${card.contact} |
347 | <li>Priority: ${card.priority} |
348 | @@ -23,12 +21,15 @@ |
349 | <li>Team: ${card.team} |
350 | </ul> |
351 | |
352 | +<div style="text-align: center">Overall blueprint completion</div> |
353 | +${util.progress_bar(blueprint_status_count)} |
354 | + |
355 | +<h3>Description</h3> ${card.description if card.description is not None else '<i>No description could be found.</i>'} |
356 | +<p><a href="${card.url}">Read the full description</a>. |
357 | +<h3>Acceptance criteria</h3> ${card.acceptance_criteria if card.acceptance_criteria is not None else '<i>No acceptance criteria could be found.</i>'} |
358 | +<p><a href="${card.url}">Read the full acceptance criteria</a>. |
359 | <p> |
360 | -% for status in blueprints.keys(): |
361 | -% if blueprints[status] == []: |
362 | -<p><i>There are no ${status} blueprints linked to this card.</i> |
363 | -% else: |
364 | -<h3>${status}</h3> |
365 | +% if card.status in ['Ready', 'Delivered'] or blueprints.keys() > 0: |
366 | <table> |
367 | <thead> |
368 | <tr><th>Title</th> |
369 | @@ -39,47 +40,27 @@ |
370 | <th>Health check</th> |
371 | </tr> |
372 | </thead> |
373 | +% for status in blueprints.keys(): |
374 | % for bp in blueprints[status]: |
375 | <tr><td><a href="${bp.url}">${bp.name}</a></td> |
376 | <td>${bp.assignee_name}</td> |
377 | <td>${bp.priority}</td> |
378 | - <td>${bp.implementation}</td> |
379 | + <td>${status} |
380 | <td>${bp.milestone_name}</td> |
381 | - <td>xxx</td> |
382 | + <td></td> |
383 | </tr> |
384 | % endfor |
385 | +% endfor |
386 | </table> |
387 | % endif |
388 | -% endfor |
389 | |
390 | <h3>Health check</h3> |
391 | -<table> |
392 | - <tr><td>Has Description</td> |
393 | - <td> |
394 | -% if description == "": |
395 | - No |
396 | -% else: |
397 | - Yes |
398 | -% endif |
399 | - </td></tr> |
400 | - <tr><td>Has Acceptance Criteria</td> |
401 | - <td> |
402 | -% if acceptance_criteria == "": |
403 | - No |
404 | -% else: |
405 | - Yes |
406 | -% endif |
407 | - </td></tr> |
408 | - <tr><td>Has Blueprints</td> |
409 | - <td> |
410 | -% if blueprints == []: |
411 | - No |
412 | -% else: |
413 | - Yes |
414 | -% endif |
415 | - </td></tr> |
416 | - <tr><td>Is Ready</td> |
417 | - <td> |
418 | - TODO |
419 | - </td></tr> |
420 | + |
421 | +<table>${'<tr colspan="2"><td><font color="#FF0000"><b>Needs attention!</b></font></td></tr>' if not card.is_healthy else ''} |
422 | + % for result in health_checks: |
423 | + <tr ${'bgcolor="#FFAAAA"' if result['result'] == 'Not OK' else 'bgcolor="#FFFFFF"'}> |
424 | + <td>${result['name']}</td> |
425 | + <td>${result['result']}</td> |
426 | +</tr> |
427 | +% endfor |
428 | </table> |
429 | |
430 | === modified file 'templates/roadmap_lane.html' |
431 | --- templates/roadmap_lane.html 2011-10-31 16:12:14 +0000 |
432 | +++ templates/roadmap_lane.html 2011-11-21 15:22:45 +0000 |
433 | @@ -19,22 +19,26 @@ |
434 | |
435 | <h1>${title()}</h1> |
436 | <p> |
437 | +<table> |
438 | +<thead><tr><th>Card</th><th>Status</th><th>Team</th><th>Priority</th><th>Blueprints</th><th>Health</th></tr></thead> |
439 | % for status in statuses: |
440 | -<p><b>${status['name']}</b> |
441 | % for card_dict in status['cards']: |
442 | -<table width="45%"> |
443 | -<thead> |
444 | - <tr><th> |
445 | - <a href="roadmap-card-${card_dict['card'].roadmap_id}.html">${card_dict['card'].name}</a> |
446 | - </th></tr> |
447 | -</thead> |
448 | - <tr><td colspan=2>${card_dict['card'].description[:200]}</td></tr> |
449 | - <tr><td> |
450 | - ${card_dict['count']} |
451 | - </td><td> </td></tr> |
452 | - <tr><td>${card_dict['card'].team}</td><td align=right>Priority ${card_dict['card'].priority}</td></tr> |
453 | + <tr> |
454 | + <td> |
455 | + <a href="roadmap-card-${card_dict['card'].roadmap_id if card_dict['card'].roadmap_id != '' else card_dict['card'].card_id}.html">${card_dict['card'].name}</a> |
456 | + </td> |
457 | + <td>${status['name']}</td> |
458 | + <!--<td>${card_dict['card'].description} <a href="roadmap-card-${card_dict['card'].roadmap_id}.html">...</a></td> --> |
459 | + <td>${card_dict['card'].team}</td><td align=right>${card_dict['card'].priority}</td> |
460 | + <td> |
461 | +% for bp_status in card_dict['blueprints'].keys(): |
462 | + ${'%s: %d ' % (bp_status, len(card_dict['blueprints'][bp_status])) if len(card_dict['blueprints'][bp_status]) > 0 else ''} |
463 | +% endfor |
464 | + </td> |
465 | + <td> |
466 | + ${'<font color="#FF0000">Needs attention!</font>' if not card_dict['card'].is_healthy else ''} |
467 | + </td> |
468 | +% endfor |
469 | +% endfor |
470 | </table> |
471 | -<p><p> |
472 | -% endfor |
473 | -% endfor |
474 |
Hi Mattias,
This looks quite good. I have a few questions below but I don't think
I've ever touched this code so please forgive me if any of them are
stupid. :)
On Mon, 2011-11-21 at 15:23 +0000, Mattias Backman wrote:
[...]
>
> This branch contains some changes back and forth and is a bit big. I
> hope it is reviewable though, if it's not I'll work on breaking it
> down in smaller chunks.
It's not too big in fact, and although a very long diff is no fun, what
is less fun is when there are lots of unrelated changes bundled together
in a single branch. I hope that's not the case here! ;)
>
> The main thing this adds is some plumbing for card health checks and a
> first set of checks.
>
> It also changes the way information is displayed after feedback at the
> Connect. There is more to do, but I'd like to get this landed so we
> can start making smaller changes step by step.
> === modified file 'collect_roadmap' import_ lanes(collector , workflow_stages): or_None( node['name' ]),
> --- collect_roadmap 2011-10-27 13:23:14 +0000
> +++ collect_roadmap 2011-11-21 15:22:45 +0000
> @@ -53,7 +53,7 @@
> def kanban_
> nodes = {}
> root_node_id = None
> - lanes_to_ignore = ['Legend', 'Deferred']
> + lanes_to_ignore = ['Legend']
>
> # Iterate over all workflow_stages which may be in any order.
> for workflow_stage in workflow_stages:
> @@ -75,7 +75,7 @@
> assert node['parent_id'] == root_node_id
> model_lane = Lane(unicode_
> node['id'])
> - if model_lane.name == '2011Q3':
> + if model_lane.name == '2011Q4':
Would it be worth moving this to a constant?
> model_lane. is_current = True is_current = False or_None( task['task' ]['external_ id'])) or_None( task_status[ 'name'] ) or_None( card_type_ name) ]['priority' ] kanban_ priority( task['task' ]['priority' ]) or_None( task['task' ]['size_ estimate' ]) or_None( task['task' ]['custom_ field_1' ]) ]['external_ link'] or_None( external_ link) import( collector, external_link) description = unicode_ or_None( papyrs_ data['descripti on']) acceptance_ criteria = unicode_ or_None( papyrs_ data['acceptanc e_criteria' ]) store_card( model_card) ]['name' ]) kanban_ priority( numeric_ priority) :
> else:
> model_lane.
> @@ -116,21 +116,30 @@
> unicode_
> model_card.status = unicode_
> model_card.team = unicode_
> - model_card.priority = u"%s" % task['task'
> + model_card.priority = lookup_
> model_card.size = unicode_
> model_card.sponsor = unicode_
>
> external_link = task['task'
> - if external_link is not None:
> + if external_link is not None and external_link is not '':
> + model_card.url = unicode_
> + dbg('Getting Papyrs information from %s.' % external_link)
> papyrs_data = papyrs_
> model_card.
> model_card.
> -
> collector.
> else:
> dbg("Task '%s' does not have a status." % task['task'
>
>
> +def lookup_
> + priority...