Code review comment for lp:~rvb/maas/cluster-own-ui4

Revision history for this message
Raphaël Badin (rvb) wrote :

> The change in the man page is a good catch! I just wrote that, and I hadn't
> made the connection. In general, thanks for the changes you've made. I know
> I'm being difficult as always. :) I'll make up for that by letting you have
> your way with the en-masse accept/reject actions.
>
> In no-bootimages-warning.html, pre-existing text says “cluster's listing page”
> — should that apostrophe be there?

Right, changed to "Cluster listing page."

> Could you give make_title_entry a quick docstring? Doesn't have to be much,
> just enough to place it in context.

Done.

> In the ‘post’ method, maybe move the comments about what case each ‘if’ block
> handles, into the block? You'd get something like:
>
> if 'mass_accept_submit' in request.POST:
> # Accept clusters en masse.
> number = NodeGroup.objects.accept_all_pending()
> messages.info(request, "Accepted %d cluster(s)." % number)
> return HttpResponseRedirect(reverse('cluster-list'))

Good point, done.

> Finally, I don't actually see the unit tests for make_title_entry. Missing
> commit? Not pushed? Diff not updated..?

I guess I forgot to push the revision, done now.

Thanks again for the review.

« Back to merge proposal