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_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.

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['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).

Oh, ok, that makes sense.

> 
> >
> >>                  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.

What does the HTML you're parsing look like? On, say
https://linaro-public.papyrs.com/public/4552/KWG2011-AMP-IPC/, I see the
actual content in a javascript function, so I'm assuming you're parsing
something else?

> >
> >> +    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?

Yes, you could move all classes to a separate module and then just
import the registry here. Just ping me if you'd like a hand with the
decorator or anything else.

> >> +
> >> +    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.

On second thought I think it probably doesn't make sense as the health
checks are supposed to change, right?