launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #05062
[Merge] lp:~sinzui/launchpad/valid-targets-1 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/valid-targets-1 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #117525 in Launchpad itself: "Retargeting and setting whiteboard are forbidden when done simultaneously"
https://bugs.launchpad.net/launchpad/+bug/117525
Bug #164424 in Launchpad itself: "Confusing for "Project:" to have "Distribution" and "Project" choices"
https://bugs.launchpad.net/launchpad/+bug/164424
Bug #857448 in Launchpad itself: "Questions can be targeted to distros that do not use Lp Answers"
https://bugs.launchpad.net/launchpad/+bug/857448
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/valid-targets-1/+merge/76781
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
--
https://code.launchpad.net/~sinzui/launchpad/valid-targets-1/+merge/76781
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/valid-targets-1 into lp:launchpad.
=== 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 17:16:24 +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 17:16:24 +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 17:16:24 +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 17:16:24 +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 17:16:24 +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 contains 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 17:16:24 +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)
