Code review comment for lp:~sinzui/launchpad/answers-ui-fixes

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Am 01.11.2009 06:30, Curtis Hovey schrieb:
> Curtis Hovey has proposed merging lp:~sinzui/launchpad/answers-ui-fixes into lp:launchpad/devel.
> This is my branch to fix faq and question 3.0 bugs.

Wonderful! Thank you for doing this, including the tedious work of
fixing all tests. Thank you. I am just curious, why you changed some
page title tests to leave out the greater context, like this:

> - Questions : Ubuntu
> + Question #11 : ...

since you did this in other places:

> - Questions : Ubuntu
> + FAQ #4 : Questions : Ubuntu

No big deal, but it actually seems more work to me to remove the context
and I just wonder if you did it consciously.

One more thing:

> === modified file 'lib/lp/answers/browser/tests/test_breadcrumbs.py'
> --- lib/lp/answers/browser/tests/test_breadcrumbs.py 2009-09-22 18:45:02 +0000
> +++ lib/lp/answers/browser/tests/test_breadcrumbs.py 2009-11-01 05:30:29 +0000
> @@ -9,6 +9,8 @@
> from canonical.launchpad.webapp.tests.breadcrumbs import (
> BaseBreadcrumbTestCase)
>
> +from lp.testing import login_person
> +
>
> class TestQuestionTargetProjectAndPersonBreadcrumbOnAnswersVHost(
> BaseBreadcrumbTestCase):
> @@ -55,5 +57,31 @@
> self.assertEquals(last_crumb.text, 'Questions')
>
>
> +class TestAnswersBreadcrumb(BaseBreadcrumbTestCase):
> + """Test Breadcrumbs for answer module objects."""
> +
> + def setUp(self):
> + super(TestAnswersBreadcrumb, self).setUp()
> + self.product = self.factory.makeProduct(name="mellon")
> + login_person(self.product.owner)
> + self.question = self.factory.makeQuestion(
> + target=self.product, title='Seeds are hard to chew')
> + self.question_url = canonical_url(self.question, rootsite='answers')

self.question and self.question_url are not needed in all tests, are
they? I think they should be local to test_question.

> + self.faq = self.factory.makeFAQ(target=self.product, title='Seedless')
> + self.faq_url = canonical_url(self.faq, rootsite='answers')

Same goes for self.faq and self.faq_url

> +
> + def test_question(self):

question = ...
question_url = ...

(You get my point ... ;-)

> + crumbs = self._getBreadcrumbs(
> + self.question_url, [self.root, self.product, self.question])
> + last_crumb = crumbs[-1]
> + self.assertEquals(last_crumb.text, 'Question #%d' % self.question.id)
> +
> + def test_faq(self):
> + crumbs = self._getBreadcrumbs(
> + self.faq_url, [self.root, self.product, self.faq])
> + last_crumb = crumbs[-1]
> + self.assertEquals(last_crumb.text, 'FAQ #%d' % self.faq.id)
> +
> +
> def test_suite():
> return unittest.TestLoader().loadTestsFromName(__name__)

 review approve code
 merge approved

Cheers,
Henning
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkrux6QACgkQBT3oW1L17iiQQwCg3MatFcXV44ulMsjsmf10Ihwx
AfsAoMHKgRGxivRfTfQij6B397j4/uIR
=dWuq
-----END PGP SIGNATURE-----