Code review comment for lp:~mwhudson/lava-scheduler/email-on-health-check-failure

On Thu, 05 Apr 2012 01:49:18 -0000, Michael Hudson-Doyle <email address hidden> wrote:
> On Thu, 05 Apr 2012 00:49:18 -0000, Zygmunt Krynicki <email address hidden> wrote:
> > Review: Approve
> >
> > 149 + if len(description) > 80:
> > 150 + description = '...' + description[-78:]
> >
> > Why the last 78 characters? Note that will make the full line 81 + newline characters long
>
> Just a bit arbitrary really. Can bump up the limit to say 200, which
> would allow every description in the database today, but avoid complete
> silliness.

I've done this.

> > 112 + 'lava_scheduler_app/job_summary_mail.txt',
> > 113 + {
> > 114 + 'job': self,
> > 115 + })
> >
> > Maybe you can fit that on one line?
>
> Yep.
>
> > 178 +{% for run in job.results_bundle.test_runs.all %} | {{ run.test.test_id|ljust:20 }} | {{ run.get_summary_results.pass|default:0|rjust:6 }} | {{ run.get_summary_results.total|default:0|rjust:6 }} |
> >
> > On the note that this makes the template uglier. Recent django has
> > variables and other goodies in the templates and I'm sure you can
> > rewrite that to be readable if you want.
>
> Ah, you mean {% with %} and so on? It helps a bit, but having to have
> everything on the one line is a bit painful. I've improved this a bit,
> am happy to take any more suggestions :-)
>
> I also need to make sure we get full URLs in the emails, currently
> they're just the /scheduler/job/16280 sort of thing.

And this.

And I've tested it. Can you take a look at how I construct the URLs,
and if that looks OK, please merge it?

See you on the 16th!

Cheers,
mwh