launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #03702
[Merge] lp:~sinzui/launchpad/answers-api-4 into lp:launchpad
Curtis Hovey has proposed merging lp:~sinzui/launchpad/answers-api-4 into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
Related bugs:
Bug #786297 in Launchpad itself: "Consolidate and export question errors"
https://bugs.launchpad.net/launchpad/+bug/786297
For more details, see:
https://code.launchpad.net/~sinzui/launchpad/answers-api-4/+merge/61872
Consolidate and export question errors.
Launchpad bug: https://bugs.launchpad.net/bugs/786297
Pre-implementation: no one
Consolidate answers errors and export all question model errors to the
webservice.
--------------------------------------------------------------------
RULES
* Move InvalidQuestionStateError and other errors to errors.py
and export them.
* Replace all asserts with errors.
* Export all errors
QA
This cannot be QAed since The browser view do not let users create
oopses, and the methods that could raise the errors are not exported.
LINT
lib/lp/answers/errors.py
lib/lp/answers/doc/faq.txt
lib/lp/answers/doc/workflow.txt
lib/lp/answers/interfaces/question.py
lib/lp/answers/interfaces/questiontarget.py
lib/lp/answers/model/question.py
lib/lp/answers/tests/test_question_webservice.py
lib/lp/answers/tests/test_question_workflow.py
lib/lp/testing/views.py
TEST
./bin/test -vv -t ErrorsTestCase -m lp.answers
IMPLEMENTATION
Moved InvalidQuestionStateError to the errors module. I revise the doc string
because the error does not mean the question *is* in an invalid state, but
that it *would* be in an invalid state.
lib/lp/answers/errors.py
lib/lp/answers/interfaces/question.py
Replaced assertions in model/question.py with specific errors defined in the
errors module. Updated existing tests.
lib/lp/answers/errors.py
lib/lp/answers/doc/faq.txt
lib/lp/answers/doc/workflow.txt
lib/lp/answers/interfaces/questiontarget.py
lib/lp/answers/model/question.py
lib/lp/answers/tests/test_question_workflow.py
Added a helper to test that errors are exported. Tests do not need to
duplicate model tests and hope that some lazr.restful magic happens.
Webservice error are simple adaptions, just like browser errors.
lib/lp/testing/views.py
Exported the new errors.
lib/lp/answers/errors.py
lib/lp/answers/tests/test_question_webservice.py
--
https://code.launchpad.net/~sinzui/launchpad/answers-api-4/+merge/61872
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/answers-api-4 into lp:launchpad.
=== modified file 'lib/lp/answers/doc/faq.txt'
--- lib/lp/answers/doc/faq.txt 2011-05-09 16:35:10 +0000
+++ lib/lp/answers/doc/faq.txt 2011-05-21 20:01:37 +0000
@@ -343,7 +343,7 @@
... no_priv, firefox_faq, 'See the FAQ.')
Traceback (most recent call last):
...
- AssertionError: cannot call linkFAQ() with already linked FAQ
+ FAQTargetError: Cannot call linkFAQ() with already linked FAQ.
A FAQ can be linked to a 'solved' question, in which case, the status is
not changed.
=== modified file 'lib/lp/answers/doc/workflow.txt'
--- lib/lp/answers/doc/workflow.txt 2011-05-13 16:50:50 +0000
+++ lib/lp/answers/doc/workflow.txt 2011-05-21 20:01:37 +0000
@@ -760,4 +760,4 @@
>>> question.setStatus(stub, QuestionStatus.OPEN, reject_message)
Traceback (most recent call last):
...
- AssertionError...
+ NotMessageOwnerError...
=== modified file 'lib/lp/answers/errors.py'
--- lib/lp/answers/errors.py 2011-05-09 15:53:20 +0000
+++ lib/lp/answers/errors.py 2011-05-21 20:01:37 +0000
@@ -4,6 +4,12 @@
__metaclass__ = type
__all__ = [
'AddAnswerContactError',
+ 'FAQTargetError',
+ 'InvalidQuestionStateError',
+ 'NotAnswerContactError',
+ 'NotMessageOwnerError',
+ 'NotQuestionOwnerError',
+ 'QuestionTargetError',
]
import httplib
@@ -18,3 +24,37 @@
language.
"""
webservice_error(httplib.BAD_REQUEST)
+
+
+class FAQTargetError(ValueError):
+ """The target must be an `IFAQTarget`."""
+ webservice_error(httplib.BAD_REQUEST)
+
+
+class InvalidQuestionStateError(ValueError):
+ """Error raised when the question is in an invalid state.
+
+ Error raised when a workflow action cannot be executed because the
+ question would be in an invalid state.
+ """
+ webservice_error(httplib.BAD_REQUEST)
+
+
+class NotAnswerContactError(ValueError):
+ """The person must be an answer contact."""
+ webservice_error(httplib.BAD_REQUEST)
+
+
+class NotMessageOwnerError(ValueError):
+ """The person be the the message owner."""
+ webservice_error(httplib.BAD_REQUEST)
+
+
+class NotQuestionOwnerError(ValueError):
+ """The person be the the question owner."""
+ webservice_error(httplib.BAD_REQUEST)
+
+
+class QuestionTargetError(ValueError):
+ """The target must be an `IQueastionTarget`."""
+ webservice_error(httplib.BAD_REQUEST)
=== modified file 'lib/lp/answers/interfaces/question.py'
--- lib/lp/answers/interfaces/question.py 2011-05-14 03:06:32 +0000
+++ lib/lp/answers/interfaces/question.py 2011-05-21 20:01:37 +0000
@@ -8,7 +8,6 @@
__metaclass__ = type
__all__ = [
- 'InvalidQuestionStateError',
'IQuestion',
'IQuestionAddMessageForm',
'IQuestionChangeStatusForm',
@@ -58,14 +57,6 @@
from lp.services.worlddata.interfaces.language import ILanguage
-class InvalidQuestionStateError(Exception):
- """Error raised when the question is in an invalid state.
-
- Error raised when a workflow action cannot be executed because the
- question is in an invalid state.
- """
-
-
class IQuestion(IHasOwner):
"""A single question, often a support request."""
=== modified file 'lib/lp/answers/interfaces/questiontarget.py'
--- lib/lp/answers/interfaces/questiontarget.py 2011-05-13 20:04:47 +0000
+++ lib/lp/answers/interfaces/questiontarget.py 2011-05-21 20:01:37 +0000
@@ -195,8 +195,8 @@
:param subscribed_by: The user making the change.
:return: True if the person was added, False if the person already is
an answer contact.
- :raises ValueError: When the person or team does no have a preferred
- language.
+ :raises AddAnswerContactError: When the person or team does no have a
+ preferred language.
"""
@operation_parameters(
=== modified file 'lib/lp/answers/model/question.py'
--- lib/lp/answers/model/question.py 2011-05-17 01:49:32 +0000
+++ lib/lp/answers/model/question.py 2011-05-21 20:01:37 +0000
@@ -68,10 +68,7 @@
from canonical.launchpad.interfaces.launchpad import ILaunchpadCelebrities
from lp.services.messages.interfaces.message import IMessage
from lp.answers.interfaces.faq import IFAQ
-from lp.answers.interfaces.question import (
- InvalidQuestionStateError,
- IQuestion,
- )
+from lp.answers.interfaces.question import IQuestion
from lp.answers.interfaces.questioncollection import IQuestionSet
from lp.answers.enums import (
QuestionAction,
@@ -83,6 +80,12 @@
)
from lp.answers.errors import (
AddAnswerContactError,
+ FAQTargetError,
+ InvalidQuestionStateError,
+ NotAnswerContactError,
+ NotMessageOwnerError,
+ NotQuestionOwnerError,
+ QuestionTargetError,
)
from lp.answers.interfaces.questiontarget import IQuestionTarget
from lp.answers.model.answercontact import AnswerContact
@@ -228,8 +231,9 @@
def _settarget(self, question_target):
"""See Question.target."""
- assert IQuestionTarget.providedBy(question_target), (
- "The target must be an IQuestionTarget")
+ if not IQuestionTarget.providedBy(question_target):
+ raise QuestionTargetError(
+ "The target must be an IQuestionTarget")
if IProduct.providedBy(question_target):
self.product = question_target
self.distribution = None
@@ -315,7 +319,8 @@
@notify_question_modified()
def requestInfo(self, user, question, datecreated=None):
"""See `IQuestion`."""
- assert user != self.owner, "Owner cannot use requestInfo()."
+ if user == self.owner:
+ raise NotQuestionOwnerError("Owner cannot use requestInfo().")
if not self.can_request_info:
raise InvalidQuestionStateError(
"Question status != OPEN, NEEDSINFO, or ANSWERED")
@@ -381,10 +386,12 @@
def linkFAQ(self, user, faq, comment, datecreated=None):
"""See `IQuestion`."""
if faq is not None:
- assert IFAQ.providedBy(faq), (
- "faq parameter must provide IFAQ or be None")
- assert self.faq != faq, (
- 'cannot call linkFAQ() with already linked FAQ')
+ if not IFAQ.providedBy(faq):
+ raise FAQTargetError(
+ "faq parameter must provide IFAQ or be None.")
+ if self.faq == faq:
+ raise FAQTargetError(
+ 'Cannot call linkFAQ() with already linked FAQ.')
self.faq = faq
if self.can_give_answer:
return self._giveAnswer(user, comment, datecreated)
@@ -415,8 +422,9 @@
"There is no answer that can be confirmed")
if answer:
assert answer in self.messages
- assert answer.owner != self.owner, (
- 'Use giveAnswer() when solving own question.')
+ if answer.owner == self.owner:
+ raise NotQuestionOwnerError(
+ 'Use giveAnswer() when solving own question.')
msg = self._newMessage(
self.owner, comment, datecreated=datecreated,
@@ -451,8 +459,9 @@
@notify_question_modified()
def reject(self, user, comment, datecreated=None):
"""See `IQuestion`."""
- assert self.canReject(user), (
- 'User "%s" cannot reject the question.' % user.displayname)
+ if not self.canReject(user):
+ raise NotAnswerContactError(
+ 'User "%s" cannot reject the question.' % user.displayname)
if self.status == QuestionStatus.INVALID:
raise InvalidQuestionStateError("Question is already rejected.")
msg = self._newMessage(
@@ -604,8 +613,9 @@
:update_question_dates: A bool.
"""
if IMessage.providedBy(content):
- assert owner == content.owner, (
- 'The IMessage has the wrong owner.')
+ if owner != content.owner:
+ raise NotMessageOwnerError(
+ 'The IMessage has the wrong owner.')
msg = content
else:
if subject is None:
=== modified file 'lib/lp/answers/tests/test_question_webservice.py'
--- lib/lp/answers/tests/test_question_webservice.py 2011-04-27 15:46:45 +0000
+++ lib/lp/answers/tests/test_question_webservice.py 2011-05-21 20:01:37 +0000
@@ -12,9 +12,22 @@
from zope.component import getUtility
from canonical.launchpad.testing.pages import LaunchpadWebServiceCaller
-from canonical.testing.layers import DatabaseFunctionalLayer
+from canonical.testing.layers import (
+ DatabaseFunctionalLayer,
+ FunctionalLayer,
+ )
+from lp.answers.errors import (
+ AddAnswerContactError,
+ FAQTargetError,
+ InvalidQuestionStateError,
+ NotAnswerContactError,
+ NotMessageOwnerError,
+ NotQuestionOwnerError,
+ QuestionTargetError,
+ )
from lp.registry.interfaces.person import IPersonSet
from lp.testing import (
+ TestCase,
TestCaseWithFactory,
celebrity_logged_in,
launchpadlib_for,
@@ -22,6 +35,41 @@
person_logged_in,
ws_object,
)
+from lp.testing.views import create_webservice_error_view
+
+
+class ErrorsTestCase(TestCase):
+ """Test answers errors are exported as HTTPErrors."""
+
+ layer = FunctionalLayer
+
+ def test_AddAnswerContactError(self):
+ error_view = create_webservice_error_view(AddAnswerContactError())
+ self.assertEqual(400, error_view.status)
+
+ def test_FAQTargetError(self):
+ error_view = create_webservice_error_view(FAQTargetError())
+ self.assertEqual(400, error_view.status)
+
+ def test_InvalidQuestionStateError(self):
+ error_view = create_webservice_error_view(InvalidQuestionStateError())
+ self.assertEqual(400, error_view.status)
+
+ def test_NotAnswerContactError(self):
+ error_view = create_webservice_error_view(NotAnswerContactError())
+ self.assertEqual(400, error_view.status)
+
+ def test_NotMessageOwnerError(self):
+ error_view = create_webservice_error_view(NotMessageOwnerError())
+ self.assertEqual(400, error_view.status)
+
+ def test_NotQuestionOwnerError(self):
+ error_view = create_webservice_error_view(NotQuestionOwnerError())
+ self.assertEqual(400, error_view.status)
+
+ def test_QuestionTargetError(self):
+ error_view = create_webservice_error_view(QuestionTargetError())
+ self.assertEqual(400, error_view.status)
class TestQuestionRepresentation(TestCaseWithFactory):
=== modified file 'lib/lp/answers/tests/test_question_workflow.py'
--- lib/lp/answers/tests/test_question_workflow.py 2011-05-09 18:57:18 +0000
+++ lib/lp/answers/tests/test_question_workflow.py 2011-05-21 20:01:37 +0000
@@ -39,14 +39,15 @@
from canonical.launchpad.webapp.authorization import clear_cache
from canonical.launchpad.webapp.interfaces import ILaunchBag
from canonical.testing.layers import DatabaseFunctionalLayer
-from lp.answers.interfaces.question import (
- InvalidQuestionStateError,
- IQuestion,
- )
+from lp.answers.interfaces.question import IQuestion
from lp.answers.enums import (
QuestionAction,
QuestionStatus,
)
+from lp.answers.errors import (
+ InvalidQuestionStateError,
+ NotQuestionOwnerError,
+ )
from lp.answers.interfaces.questionmessage import IQuestionMessage
from lp.registry.interfaces.distribution import IDistributionSet
from lp.registry.interfaces.person import (
@@ -361,7 +362,7 @@
def test_requestInfoFromOwnerIsInvalid(self):
"""Test that the question owner cannot use requestInfo."""
self.assertRaises(
- AssertionError, self.question.requestInfo,
+ NotQuestionOwnerError, self.question.requestInfo,
self.owner, 'Why should I care?', datecreated=self.nowPlus(1))
def test_requestInfoFromInvalidStates(self):
=== modified file 'lib/lp/testing/views.py'
--- lib/lp/testing/views.py 2011-05-17 20:45:44 +0000
+++ lib/lp/testing/views.py 2011-05-21 20:01:37 +0000
@@ -23,6 +23,7 @@
from canonical.config import config
from canonical.launchpad.layers import setFirstLayer
+from canonical.launchpad.webapp.servers import WebServiceTestRequest
from canonical.launchpad.webapp.interfaces import (
ICanonicalUrlData,
IPlacelessAuthUtility,
@@ -106,3 +107,9 @@
folder = os.path.join(config.root, 'lib/')
export_subdirectories = True
+
+
+def create_webservice_error_view(error):
+ """Return a view of the error with a webservice request."""
+ request = WebServiceTestRequest()
+ return getMultiAdapter((error, request), name='index.html')
Follow ups
