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_import_lanes(collector, workflow_stages):
>>      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_or_None(node['name']),
>>                            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.is_current = True
>>          else:
>>              model_lane.is_current = False
>> @@ -116,21 +116,30 @@
>>                                    unicode_or_None(task['task']['external_id']))
>>                  model_card.status = unicode_or_None(task_status['name'])
>>                  model_card.team = unicode_or_None(card_type_name)
>> -                model_card.priority = u"%s" % task['task']['priority']
>> +                model_card.priority = lookup_kanban_priority(task['task']['priority'])
>>                  model_card.size = unicode_or_None(task['task']['size_estimate'])
>>                  model_card.sponsor = unicode_or_None(task['task']['custom_field_1'])
>>
>>                  external_link = task['task']['external_link']
>> -                if external_link is not None:
>> +                if external_link is not None and external_link is not '':
>> +                    model_card.url = unicode_or_None(external_link)
>> +                    dbg('Getting Papyrs information from %s.' % external_link)
>>                      papyrs_data = papyrs_import(collector, external_link)
>>                      model_card.description = unicode_or_None(papyrs_data['description'])
>>                      model_card.acceptance_criteria = unicode_or_None(papyrs_data['acceptance_criteria'])
>> -
>>                  collector.store_card(model_card)
>>          else:
>>              dbg("Task '%s' does not have a status." % task['task']['name'])
>>
>>
>> +def lookup_kanban_priority(numeric_priority):
>> +    priority_lookup = { -1 : "low",
>> +                         0 : "normal",
>> +                         1 : "high" }
>
> PEP-8 nitpick: no spaces before/after }/{, or before the semicolon.

Arrg of course. Thanks!

>
>> +    assert numeric_priority in priority_lookup, (
>> +        "Priority '%s' is unknown." % numeric_priority)
>> +    return unicode_or_None(priority_lookup[numeric_priority])
>> +
>>  def kanban_import(collector, cfg, board_id, api_token):
>>      '''Collect roadmap items from KanbanTool into DB.'''
>>      board_url = get_kanban_url('boards/%s.json' % board_id, api_token)
>> @@ -150,7 +159,7 @@
>>      acceptance_criteria = None
>>
>>      page = get_json_data(requirement_url + '?json&auth_token=e8a2801e4f0f')
>> -
>> +    assert page is not None, "Could not access Papyrs document %s." % requirement_url
>>      page_text_items = page[0]
>>      page_extra_items = page[1]
>>
>> @@ -160,12 +169,22 @@
>>              last_heading = page_item['text']
>>          if page_item['classname'] == 'Paragraph':
>>              if not has_found_description:
>> -                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).

>
>>                  has_found_description = True
>> -            elif last_heading == 'Acceptance Criteria':
>> -                acceptance_criteria = page_item['text']
>> -
>> -    return {'description': description, 'acceptance_criteria': acceptance_criteria}
>> +            elif 'Acceptance Criteria' in last_heading:
>> +                acceptance_criteria = page_item['html']
>> +
>> +    return {'description': get_first_paragraph(description),
>> +            'acceptance_criteria': get_first_paragraph(acceptance_criteria)}
>> +
>> +
>> +def get_first_paragraph(text):
>> +    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('<br>')
>
> 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.

>
>> +    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
>> @@ -995,6 +996,91 @@
>>      return bp_by_status
>>
>>
>> +def check_card_health(store, card):
>> +    class HealthCheck(object):
>> +        NOT_APPLICABLE = 'n/a'
>> +        OK = 'OK'
>> +        NOT_OK = 'Not OK'
>> +        name = 'Base check, not to be used'
>> +
>> +        def applicable(self, card, store=None):
>> +            raise NotImplementedError()
>> +
>> +        def check(self, card, store=None):
>> +            raise NotImplementedError()
>> +
>> +        def execute(self, card, store=None):
>> +            if self.applicable(card, store):
>> +                if self.check(card, store):
>> +                    return self.OK
>> +                else:
>> +                    return self.NOT_OK
>> +            else:
>> +                return self.NOT_APPLICABLE
>> +
>> +    class DescriptionHealthCheck(HealthCheck):
>> +        name = 'Has description'
>> +
>> +        def applicable(self, card, store=None):
>> +            return True
>> +
>> +        def check(self, card, store=None):
>> +            return card.description is not None
>> +
>> +    class CriteriaHealthCheck(HealthCheck):
>> +        name = 'Has acceptance criteria'
>> +
>> +        def applicable(self, card, store=None):
>> +            return True
>> +
>> +        def check(self, card, store=None):
>> +            return card.acceptance_criteria is not None
>> +
>> +    class BlueprintsHealthCheck(HealthCheck):
>> +        name = 'Has blueprints'
>> +
>> +        def applicable(self, card, store):
>> +            return card.status == 'Ready'
>> +
>> +        def check(self, card, store):
>> +            return len(card_blueprints(store, card.roadmap_id)) > 0
>> +
>> +    class BlueprintsBlockedHealthCheck(HealthCheck):
>> +        name = 'Has no Blocked blueprints'
>> +
>> +        def applicable(self, card, store):
>> +            return card.status != 'Ready'
>> +
>> +        def check(self, card, store):
>> +            blueprints = card_blueprints_by_status(store, card.roadmap_id)
>> +            return len(blueprints['Blocked']) == 0
>> +
>> +    class RoadmapIdHealthCheck(HealthCheck):
>> +        name = 'Has a roadmap id'
>> +
>> +        def applicable(self, card, store=None):
>> +            return True
>> +
>> +        def check(self, card, store=None):
>> +            return card.roadmap_id != ''
>
> 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_checks.append(RoadmapIdHealthCheck())
>> +    health_checks.append(DescriptionHealthCheck())
>> +    health_checks.append(CriteriaHealthCheck())
>> +    health_checks.append(BlueprintsHealthCheck())
>> +    health_checks.append(BlueprintsBlockedHealthCheck())
>
> One neat thing we could do is use a class decorator on all HealthCheck
> classes to register them on a global registry (a module-level python
> list, really) and then use that here. That way one just needs to inherit
> from HealthCheck and use the decorator to add a health check. It could
> be a nice thing to have, but definitely not essential.

It could make this a lot cleaner and it's going to be important to
make changes and additions really clean since the requirements are not
really clear yet. So with this I could just put all the checks in a
separate module and then just import the module?

>
>> +
>> +    performed_checks = []
>> +    card.is_healthy = True
>> +    for check in health_checks:
>> +        result = check.execute(card, store)
>> +        if result == check.NOT_OK:
>> +            card.is_healthy = False
>> +        performed_checks.append({'name': check.name,
>> +                                 'result': result})
>
> I guess it doesn't make sense to try and store those failed health
> checks in the DB?

It would make sense if I would run the health checks when collecting
the data rather than when generating the pages. Let me see if there's
any reason why we shouldn't do that.

>
>> +    return performed_checks
>> +
>>  def subteams(store, team):
>>      result = store.execute('SELECT name from team_structure where team = ?', (unicode(team),))
>>      return [i[0] for i in result]
>>
>
>
> --
> https://code.launchpad.net/~mabac/launchpad-work-items-tracker/linaro-roadmap-connect-hacking/+merge/82898
> You are the owner of lp:~mabac/launchpad-work-items-tracker/linaro-roadmap-connect-hacking.
>