launchpad-reviewers team mailing list archive

[Curtis Hovey <curtis.hovey@xxxxxxxxxxxxx>]

Curtis Hovey has proposed merging lp:~sinzui/launchpad/dsp-question-permissions-0 into lp:launchpad.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #695206 in Launchpad itself: "Cannot reject some duplicate questions"
  https://bugs.launchpad.net/launchpad/+bug/695206

For more details, see:
https://code.launchpad.net/~sinzui/launchpad/dsp-question-permissions-0/+merge/58705

Fix the distribution answer contact permissions on dsp questions.

    Launchpad bug: https://bugs.launchpad.net/bugs/695206
    Pre-implementation: no one

Answer contact for Ubuntu cannot (reject) questions targeted to Ubuntu
packages. This is a regression caused by a performance refactoring
in the IQuestion security checker.

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

RULES

    * Add a test for for this specific case.
    * Update security.AppendQuestion(). The method matches the user's
      question targets (answer contact for) to the current question target.
      The method may want to adapt the question target to an IDistribution or
      use self.obj.distribution to also do the check.


QA

    * As an answer contact for Ubuntu, reject a question on a source package.


LINT

    lib/canonical/launchpad/security.py
    lib/lp/answers/tests/test_question_workflow.py



TEST

    ./bin/test -vv -t testRejectPermission


IMPLEMENTATION

Revised the existing reject permission test and security.py to hush lint.
I made the existing test use a proper assert that can raise a message on
fail. Added a new test for the dsp case.
    lib/canonical/launchpad/security.py
    lib/lp/answers/tests/test_question_workflow.py
-- 
https://code.launchpad.net/~sinzui/launchpad/dsp-question-permissions-0/+merge/58705
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~sinzui/launchpad/dsp-question-permissions-0 into lp:launchpad.

=== modified file 'lib/canonical/launchpad/security.py'
--- lib/canonical/launchpad/security.py	2011-04-19 23:46:22 +0000
+++ lib/canonical/launchpad/security.py	2011-04-21 15:49:26 +0000
@@ -961,6 +961,7 @@
                 user.inTeam(self.obj.distribution.owner) or
                 user.in_admin)
 
+
 class EditDistributionSourcePackage(AuthorizationBase):
     """DistributionSourcePackage is not editable.
 
@@ -1731,12 +1732,16 @@
         if AdminQuestion.checkAuthenticated(self, user):
             return True
         question_target = self.obj.target
+        if IDistributionSourcePackage.providedBy(question_target):
+            question_targets = (question_target, question_target.distribution)
+        else:
+            question_targets = (question_target, )
         questions_person = IQuestionsPerson(user.person)
         for target in questions_person.getDirectAnswerQuestionTargets():
-            if target == question_target:
+            if target in question_targets:
                 return True
         for target in questions_person.getTeamAnswerQuestionTargets():
-            if target == question_target:
+            if target in question_targets:
                 return True
         return False
 

=== modified file 'lib/lp/answers/tests/test_question_workflow.py'
--- lib/lp/answers/tests/test_question_workflow.py	2010-10-03 15:30:06 +0000
+++ lib/lp/answers/tests/test_question_workflow.py	2011-04-21 15:49:26 +0000
@@ -695,7 +695,7 @@
         question1_answer = self.question.giveAnswer(
             self.answerer, 'Really, just do it!')
         question2 = self.ubuntu.newQuestion(self.owner, 'Help 2', 'Help me!')
-        question2_answer = question2.giveAnswer(self.answerer, 'Do that!')
+        question2.giveAnswer(self.answerer, 'Do that!')
         answerRefused = False
         try:
             question2.confirmAnswer('That worked!', answer=question1_answer)
@@ -920,18 +920,25 @@
         # Answer contacts must speak a language
         self.answerer.addLanguage(getUtility(ILanguageSet)['en'])
         self.question.target.addAnswerContact(self.answerer)
-        clear_cache() # clear authorization cache for check_permission
-        # this is a test to prove that the getattr succeeds
-        getattr(self.question, 'reject')
-
-        login_person(self.admin)
-        # this is a test to prove that the getattr succeeds
-        getattr(self.question, 'reject')
-
-
-def test_suite():
-    return unittest.TestLoader().loadTestsFromName(__name__)
-
-
-if __name__ == '__main__':
-    unittest.main()
+        # clear authorization cache for check_permission
+        clear_cache()
+        self.assertTrue(
+            getattr(self.question, 'reject'),
+            "Answer contact cannot reject question.")
+        login_person(self.admin)
+        self.assertTrue(
+            getattr(self.question, 'reject'),
+            "Admin cannot reject question.")
+
+    def testRejectPermission_indirect_answer_contact(self):
+        # Indirect answer contacts (for a distribution) can reject
+        # distribuiton source package questions.
+        login_person(self.admin)
+        dsp = self.ubuntu.getSourcePackage('mozilla-firefox')
+        self.question.target = dsp
+        login_person(self.answerer)
+        self.answerer.addLanguage(getUtility(ILanguageSet)['en'])
+        self.ubuntu.addAnswerContact(self.answerer)
+        self.assertTrue(
+            getattr(self.question, 'reject'),
+            "Answer contact cannot reject question.")