Merge lp:~sinzui/launchpad/valid-targets-1 into lp:launchpad

[Curtis Hovey]

Restrict the target widget to pillars that use Launchpad.

    Launchpad bug: https://bugs.launchpad.net/bugs/857448
    Pre-implementation: jcsackett

LaunchpadTargetWidget is used for Questions and it does not
constrain the choice to pillars that use Launchpad. The widget lists
every distribution in the DB, but very few use Launchpad.

This issue is largely about the ui and the moment. Distributions can
change which support tracker they use, and the existing questions will
continue to exist. The existing constraint of any distro is correct,
but when I choose to retarget a question, it should only be to the distros
that currently use Lp. User can use the API to target a question's distro,
and we assume the user knows what he is doing.

Note that a previous attempt to restrict targets to the current state
of what is valid failed because historic data became invalid--you could
not load a bug if one of the bugtasks belonged to a project that did not
use Lp, or the package was never published.

--------------------------------------------------------------------

RULES

    * Create vocabularies that constrain the the pillars to those that
      use Launchpad, or the current distro so historic data is accepted.
    * Subclass the widget to use the appropriate question vocabulary

QA

    * Visit https://answers.qastaging.launchpad.net/ubuntu/+question/162149
    * Choose to retarget the question.
    * Verify the target widget field reads:
      This question is about
    * Verify the "product" is not used in the target widget's description.
    * Verify that Ubuntu and gentoo are listed.
    * Verify that archlinux and samsung are not listed.

    * Visit any question in the launchpad project.
    * Choose to edit the question.
    * Change the whiteboard and retarget the question to sikuli
    * Save the changes.
    * Verify there was not a form error.

LINT

    lib/lp/answers/configure.zcml
    lib/lp/answers/vocabulary.py
    lib/lp/answers/browser/question.py
    lib/lp/answers/browser/tests/test_question.py
    lib/lp/answers/interfaces/question.py
    lib/lp/answers/tests/test_vocabulary.py

IMPLEMENTATION

I saw 'product' was used in the edit form and found a related bug that
'project' is ambiguous in the form: I used MPT's suggested text and fixed
the field's description. See https://bugs.launchpad.net/launchpad/+bug/164424
    lib/lp/answers/interfaces/question.py

Added a vocabulary that constrains the distro to only those that use Lp.
    lib/lp/answers/configure.zcml
    lib/lp/answers/vocabulary.py
    lib/lp/answers/tests/test_vocabulary.py

Subclassed LaunchpadTargetWidget to use the new vocabulary.
    lib/lp/answers/browser/question.py
    lib/lp/answers/browser/tests/test_question.py

I realised that a form submission error reported by Matthew related to
permission changes implicit with retargeting. Question target changes must
always be the last change processed. See
https://bugs.launchpad.net/launchpad/+bug/117525
    lib/lp/answers/browser/question.py
    lib/lp/answers/browser/tests/test_question.py

[Brad Crittenden (bac)]

review: Approve (code)

[Brad Crittenden (bac)]

Hi Curtis,

This branch looks good and I like your approach.

You have a type here:
# The vocabulary contains does not contain the context if the

review: Approve (code)

=== modified file 'lib/lp/answers/browser/question.py'
--- lib/lp/answers/browser/question.py	2011-09-13 05:23:16 +0000
+++ lib/lp/answers/browser/question.py	2011-09-23 19:03:25 +0000
@@ -97,6 +97,7 @@
     IAnswersFrontPageSearchForm,
     IQuestionTarget,
     )
+from lp.answers.vocabulary import UsesAnswersDistributionVocabulary
 from lp.app.browser.launchpadform import (
     action,
     custom_widget,
@@ -725,6 +726,15 @@
     cancel_url = next_url
 
 
+class QuestionTargetWidget(LaunchpadTargetWidget):
+    """A targeting widget that is aware of pillars that use Answers."""
+
+    def getDistributionVocabulary(self):
+        distro = self.context.context.distribution
+        vocabulary = UsesAnswersDistributionVocabulary(distro)
+        return vocabulary
+
+
 class QuestionEditView(LaunchpadEditFormView):
     """View for editing a Question."""
     schema = IQuestion
@@ -735,7 +745,7 @@
 
     custom_widget('title', TextWidget, displayWidth=40)
     custom_widget('whiteboard', TextAreaWidget, height=5)
-    custom_widget('target', LaunchpadTargetWidget)
+    custom_widget('target', QuestionTargetWidget)
 
     @property
     def page_title(self):
@@ -762,7 +772,15 @@
     @action(_("Save Changes"), name="change")
     def change_action(self, action, data):
         """Update the Question from the request form data."""
+        # Target must be the last field processed because it implicitly
+        # changes the user's permissions.
+        target_data = {'target': self.context.target}
+        if 'target' in data:
+            target_data['target'] = data['target']
+            del data['target']
         self.updateContextFromData(data)
+        if target_data['target'] != self.context.target:
+            self.updateContextFromData(target_data)
 
     @property
     def next_url(self):

=== modified file 'lib/lp/answers/browser/tests/test_question.py'
--- lib/lp/answers/browser/tests/test_question.py	2011-01-28 21:11:07 +0000
+++ lib/lp/answers/browser/tests/test_question.py	2011-09-23 19:03:25 +0000
@@ -7,7 +7,10 @@
 
 __all__ = []
 
+from canonical.launchpad.webapp.servers import LaunchpadTestRequest
 from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.answers.browser.question import QuestionTargetWidget
+from lp.answers.interfaces.question import IQuestion
 from lp.answers.publisher import AnswersLayer
 from lp.testing import (
     login_person,
@@ -51,3 +54,84 @@
         self.assertEqual(1, len(view.errors))
         self.assertEqual(
             'The summary cannot exceed 250 characters.', view.errors[0])
+
+
+class QuestionEditViewTestCase(TestCaseWithFactory):
+    """Verify the behavior of the QuestionEditView."""
+
+    layer = DatabaseFunctionalLayer
+
+    def getForm(self, question):
+        if question.assignee is None:
+            assignee = ''
+        else:
+            assignee = question.assignee.name
+        return {
+            'field.title': question.title,
+            'field.description': question.description,
+            'field.language': question.language.code,
+            'field.assignee': assignee,
+            'field.target': 'product',
+            'field.target.distribution': '',
+            'field.target.package': '',
+            'field.target.product': question.target.name,
+            'field.whiteboard': question.whiteboard,
+            'field.actions.change': 'Change',
+            }
+
+    def test_retarget_with_other_changed(self):
+        # Retargeting must be the last change made to the question
+        # to ensure that user permission do not change while there
+        # are more changes to make.
+        target = self.factory.makeProduct()
+        question = self.factory.makeQuestion(target=target)
+        other_target = self.factory.makeProduct()
+        login_person(target.owner)
+        form = self.getForm(question)
+        form['field.whiteboard'] = 'comment'
+        form['field.target.product'] = other_target.name
+        view = create_initialized_view(
+            question, name='+edit', layer=AnswersLayer, form=form)
+        self.assertEqual([], view.errors)
+        self.assertEqual(other_target, question.target)
+        self.assertEqual('comment', question.whiteboard)
+
+
+class QuestionTargetWidgetTestCase(TestCaseWithFactory):
+    """Test that QuestionTargetWidgetTestCase behaves as expected."""
+    layer = DatabaseFunctionalLayer
+
+    def getWidget(self, question):
+        field = IQuestion['target']
+        bound_field = field.bind(question)
+        request = LaunchpadTestRequest()
+        return QuestionTargetWidget(bound_field, request)
+
+    def test_getDistributionVocabulary_with_product_question(self):
+        # The vocabulary does not contain distros that do not use
+        # launchpad to track answers.
+        distribution = self.factory.makeDistribution()
+        product = self.factory.makeProduct()
+        question = self.factory.makeQuestion(target=product)
+        target_widget = self.getWidget(question)
+        vocabulary = target_widget.getDistributionVocabulary()
+        self.assertEqual(None, vocabulary.distribution)
+        self.assertFalse(
+            distribution in vocabulary,
+            "Vocabulary contains distros that do not use Launchpad Answers.")
+
+    def test_getDistributionVocabulary_with_distribution_question(self):
+        # The vocabulary does not contain distros that do not use
+        # launchpad to track answers.
+        distribution = self.factory.makeDistribution()
+        other_distribution = self.factory.makeDistribution()
+        question = self.factory.makeQuestion(target=distribution)
+        target_widget = self.getWidget(question)
+        vocabulary = target_widget.getDistributionVocabulary()
+        self.assertEqual(distribution, vocabulary.distribution)
+        self.assertTrue(
+            distribution in vocabulary,
+            "Vocabulary missing context distribution.")
+        self.assertFalse(
+            other_distribution in vocabulary,
+            "Vocabulary contains distros that do not use Launchpad Answers.")

=== modified file 'lib/lp/answers/configure.zcml'
--- lib/lp/answers/configure.zcml	2011-08-12 04:17:29 +0000
+++ lib/lp/answers/configure.zcml	2011-09-23 19:03:25 +0000
@@ -182,6 +182,14 @@
     factory=".adapters.sourcepackage_to_faqtarget"
     />
 
+  <securedutility
+    name="UsesAnswersDistribution"
+    component=".vocabulary.UsesAnswersDistributionVocabulary"
+    provides="zope.schema.interfaces.IVocabularyFactory"
+    >
+    <allow interface="zope.schema.interfaces.IVocabularyFactory"/>
+  </securedutility>
+
   <!-- QuestionSet -->
   <class class=".model.question.QuestionSet">
     <allow interface=".interfaces.questioncollection.IQuestionSet" />

=== modified file 'lib/lp/answers/interfaces/question.py'
--- lib/lp/answers/interfaces/question.py	2011-09-22 20:50:40 +0000
+++ lib/lp/answers/interfaces/question.py	2011-09-23 19:03:25 +0000
@@ -157,8 +157,9 @@
         description=_('Up-to-date notes on the status of the question.'))
     # other attributes
     target = exported(Reference(
-        title=_('Project'), required=True, schema=IQuestionTarget,
-        description=_('The distribution, source package, or product the '
+        title=_('This question is about'), required=True,
+        schema=IQuestionTarget,
+        description=_('The distribution, source package, or project the '
                       'question pertains to.')),
         as_of="devel")
     faq = Object(

=== added file 'lib/lp/answers/tests/test_vocabulary.py'
--- lib/lp/answers/tests/test_vocabulary.py	1970-01-01 00:00:00 +0000
+++ lib/lp/answers/tests/test_vocabulary.py	2011-09-23 19:03:25 +0000
@@ -0,0 +1,69 @@
+# Copyright 2011 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test the answers domain vocabularies."""
+
+__metaclass__ = type
+
+from canonical.testing.layers import DatabaseFunctionalLayer
+from lp.answers.vocabulary import UsesAnswersDistributionVocabulary
+from lp.testing import (
+    person_logged_in,
+    TestCaseWithFactory,
+    )
+
+
+class UsesAnswersDistributionVocabularyTestCase(TestCaseWithFactory):
+    """Test that the vocabulary behaves as expected."""
+    layer = DatabaseFunctionalLayer
+
+    def test_init_with_distribution(self):
+        # When the context is adaptable to IDistribution, the distribution
+        # property is the distribution.
+        distribution = self.factory.makeDistribution()
+        vocabulary = UsesAnswersDistributionVocabulary(distribution)
+        self.assertEqual(distribution, vocabulary.context)
+        self.assertEqual(distribution, vocabulary.distribution)
+
+    def test_init_without_distribution(self):
+        # When the context is not adaptable to IDistribution, the
+        # distribution property is None
+        thing = self.factory.makeProduct()
+        vocabulary = UsesAnswersDistributionVocabulary(thing)
+        self.assertEqual(thing, vocabulary.context)
+        self.assertEqual(None, vocabulary.distribution)
+
+    def test_contains_distros_that_use_answers(self):
+        # The vocabulary contains distributions that also use
+        # Launchpad to track answers.
+        distro_less_answers = self.factory.makeDistribution()
+        distro_uses_answers = self.factory.makeDistribution()
+        with person_logged_in(distro_uses_answers.owner):
+            distro_uses_answers.official_answers = True
+        vocabulary = UsesAnswersDistributionVocabulary()
+        self.assertFalse(
+            distro_less_answers in vocabulary,
+            "Vocabulary contains distros that do not use Launchpad Answers.")
+        self.assertTrue(
+            distro_uses_answers in vocabulary,
+            "Vocabulary missing distros that use Launchpad Answers.")
+
+    def test_contains_context_distro(self):
+        # The vocabulary contains the context distro even it it does not
+        # use Launchpad to track answers. The distro may have tracked answers
+        # in the past so it is a legitimate choise for historic data.
+        distro_less_answers = self.factory.makeDistribution()
+        vocabulary = UsesAnswersDistributionVocabulary(distro_less_answers)
+        self.assertFalse(distro_less_answers.official_answers)
+        self.assertTrue(
+            distro_less_answers in vocabulary,
+            "Vocabulary missing context distro.")
+
+    def test_contains_missing_context(self):
+        # The vocabulary does not contain the context if the
+        # context is not adaptable to a distribution.
+        thing = self.factory.makeProduct()
+        vocabulary = UsesAnswersDistributionVocabulary(thing)
+        self.assertFalse(
+            thing in vocabulary,
+            "Vocabulary contains a non-distribution.")

=== modified file 'lib/lp/answers/vocabulary.py'
--- lib/lp/answers/vocabulary.py	2011-08-20 10:53:02 +0000
+++ lib/lp/answers/vocabulary.py	2011-09-23 19:03:25 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2011 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Named vocabularies defined by the Answers application."""
@@ -6,8 +6,11 @@
 __metaclass__ = type
 __all__ = [
     'FAQVocabulary',
+    'UsesAnswersDistributionVocabulary',
     ]
 
+from sqlobject import OR
+
 from zope.interface import implements
 from zope.schema.vocabulary import SimpleTerm
 
@@ -18,6 +21,8 @@
     )
 from lp.answers.interfaces.faq import IFAQ
 from lp.answers.interfaces.faqtarget import IFAQTarget
+from lp.registry.interfaces.distribution import IDistribution
+from lp.registry.vocabularies import DistributionVocabulary
 
 
 class FAQVocabulary(FilteredVocabularyBase):
@@ -58,7 +63,7 @@
     def getTermByToken(self, token):
         """See `IVocabularyTokenized`."""
         try:
-            faq_id = int(token)
+            int(token)
         except ValueError:
             raise LookupError(token)
         faq = self.context.getFAQ(token)
@@ -74,3 +79,28 @@
         """See `IHugeVocabulary`."""
         results = self.context.findSimilarFAQs(query)
         return CountableIterator(results.count(), results, self.toTerm)
+
+
+class UsesAnswersDistributionVocabulary(DistributionVocabulary):
+    """Distributions that use Launchpad to track questions.
+
+    If the context is a distribution, it is always included in the
+    vocabulary. Historic data is not invalidated if a distro stops
+    using Launchpad to track questions. This vocabulary offers the correct
+    choices of distributions at this moment.
+    """
+
+    def __init__(self, context=None):
+        super(UsesAnswersDistributionVocabulary, self).__init__(
+            context=context)
+        self.distribution = IDistribution(self.context, None)
+
+    @property
+    def _filter(self):
+        if self.distribution is None:
+            distro_id = 0
+        else:
+            distro_id = self.distribution.id
+        return OR(
+            self._table.q.official_answers == True,
+            self._table.id == distro_id)

=== modified file 'lib/lp/bugs/tests/test_vocabulary.py'
--- lib/lp/bugs/tests/test_vocabulary.py	2011-09-22 19:28:03 +0000
+++ lib/lp/bugs/tests/test_vocabulary.py	2011-09-23 19:03:25 +0000
@@ -60,7 +60,7 @@
             "Vocabulary missing context distro.")
 
     def test_contains_missing_context(self):
-        # The vocabulary contains does not contain the context if the
+        # The vocabulary does not contain the context if the
         # context is not adaptable to a distribution.
         thing = self.factory.makeProduct()
         vocabulary = UsesBugsDistributionVocabulary(thing)