Merge lp:~stevenk/launchpad/filebug-description-widget into lp:launchpad

[Steve Kowalik]

Also check the extra_description + comment for being > 50000 in the validation method for filing a bug.

As it stands, Apport uploads a blob using RootObject:+storeblob, and then passes the token from that blob in the +filebug URL. This blob may contain an extra_description, which is nailed onto the end of the comment. As it stood, the validation method in FileBugViewBase would check the length of the comment, and if it was over 50001, it would error. However, it didn't check anything about extra_description, so that the validation would pass, the extra_description would be added on, and then Storm would de-pram its toys when we try and create a Bug row with a 50001 character description.

Claw this branch back to net-neutral by doing a fair whack of whitespace cleanup.

[Ian Booth (wallyworld)]

Change line 27 to > 50000 and it's goo to go

review: Approve

=== modified file 'lib/lp/bugs/browser/bugtarget.py'
--- lib/lp/bugs/browser/bugtarget.py	2012-10-11 12:41:43 +0000
+++ lib/lp/bugs/browser/bugtarget.py	2012-11-06 05:59:19 +0000
@@ -255,8 +255,7 @@
             self.context.pillar.getDefaultBugInformationType() in
             PRIVATE_INFORMATION_TYPES)
         json_dump_information_types(
-            cache,
-            self.context.pillar.getAllowedBugInformationTypes())
+            cache, self.context.pillar.getAllowedBugInformationTypes())
 
         bugtask_status_data = vocabulary_to_choice_edit_items(
             BugTaskStatus, include_description=True, css_class_prefix='status',
@@ -385,11 +384,19 @@
             widget_error = self.widgets.get('comment')._error
             if widget_error and isinstance(widget_error.errors, TooLong):
                 self.setFieldError('comment',
-                    'The description is too long. If you have lots '
+                    'The description is too long. If you have lots of '
                     'text to add, attach a file to the bug instead.')
             elif not comment or widget_error is not None:
                 self.setFieldError(
                     'comment', "Provide details about the issue.")
+            # Check if there is an extra description, and if it is too long.
+            if self.extra_data.extra_description:
+                if len("%s\n\n%s" % (
+                    comment, self.extra_data.extra_description)) > 50000:
+                    self.setFieldError(
+                        'comment', 'The description and the additional '
+                        'information is too long. If you have lots of text '
+                        'to add, attach a file to the bug instead.')
         # Check a bug has been selected when the user wants to
         # subscribe to an existing bug.
         elif self.this_is_my_bug_action.submitted():

=== modified file 'lib/lp/bugs/browser/tests/test_bugtarget_filebug.py'
--- lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-10-08 22:59:03 +0000
+++ lib/lp/bugs/browser/tests/test_bugtarget_filebug.py	2012-11-06 05:59:19 +0000
@@ -3,7 +3,6 @@
 
 __metaclass__ = type
 
-
 from textwrap import dedent
 from BeautifulSoup import BeautifulSoup
 from lazr.restful.interfaces import IJSONRequestCache
@@ -368,11 +367,7 @@
 
     def filebug_via_view(self, information_type=None,
                          bug_sharing_policy=None, extra_data_token=None):
-        form = {
-            'field.title': 'A bug',
-            'field.comment': 'A comment',
-            'field.actions.submit_bug': 'Submit Bug Request',
-        }
+        form = self.get_form()
         if information_type:
             form['field.information_type'] = information_type
         product = self.factory.makeProduct(official_malone=True)
@@ -695,6 +690,40 @@
              'The file "attachment2" was attached to the bug report.'],
             notifications)
 
+    def test_submit_comment_with_large_extra_description(self):
+        # Submitting a large blob will set a sensible error.
+        string = 'y' * 50001
+        token = self.process_extra_data("""\
+            MIME-Version: 1.0
+            Content-type: multipart/mixed; boundary=boundary
+
+            --boundary
+            Content-disposition: inline
+            Content-type: text/plain; charset=utf-8
+
+            %s
+
+            --boundary
+            Content-disposition: inline
+            Content-type: text/plain; charset=utf-8
+
+            A bug comment.
+
+            --boundary--
+            """ % string)
+        form = {
+            'field.title': 'Test title',
+            'field.comment': 'Test comment',
+            'field.actions.submit_bug': 'Submit Bug Report'}
+        view = self.create_initialized_view(form)
+        view.publishTraverse(view.request, token)
+        view.validate(self.get_form())
+        expected = [
+            u'The description and the additional information is too long. '
+            'If you have lots of text to add, attach a file to the bug '
+            'instead.']
+        self.assertContentEqual(expected, view.errors)
+
 
 class TestFileBugForNonBugSupervisors(TestCaseWithFactory):
 
@@ -816,10 +845,8 @@
                         'help': '', 'disabled': False,
                         'css_class': 'status' + item.name}
             bugtask_status_data.append(new_item)
-        self.assertEqual(
-            bugtask_status_data, cache['bugtask_status_data'])
-        excluded_importances = [
-            BugTaskImportance.UNKNOWN]
+        self.assertEqual(bugtask_status_data, cache['bugtask_status_data'])
+        excluded_importances = [BugTaskImportance.UNKNOWN]
         bugtask_importance_data = []
         for item in BugTaskImportance:
             item = item.value

=== modified file 'lib/lp/bugs/model/bug.py'
--- lib/lp/bugs/model/bug.py	2012-10-25 05:09:07 +0000
+++ lib/lp/bugs/model/bug.py	2012-11-06 05:59:19 +0000
@@ -330,8 +330,7 @@
     # db field names
     name = StringCol(unique=True, default=None)
     title = StringCol(notNull=True)
-    description = StringCol(notNull=False,
-                            default=None)
+    description = StringCol(notNull=False, default=None)
     owner = ForeignKey(
         dbName='owner', foreignKey='Person',
         storm_validator=validate_public_person, notNull=True)
@@ -433,16 +432,14 @@
         """See `IBug`."""
         return Store.of(self).find(
             Person, BugAffectsPerson.person == Person.id,
-            BugAffectsPerson.affected,
-            BugAffectsPerson.bug == self)
+            BugAffectsPerson.affected, BugAffectsPerson.bug == self)
 
     @property
     def users_unaffected(self):
         """See `IBug`."""
         return Store.of(self).find(
             Person, BugAffectsPerson.person == Person.id,
-            Not(BugAffectsPerson.affected),
-            BugAffectsPerson.bug == self)
+            Not(BugAffectsPerson.affected), BugAffectsPerson.bug == self)
 
     @property
     def user_ids_affected_with_dupes(self):
@@ -466,8 +463,7 @@
     def users_affected_with_dupes(self):
         """See `IBug`."""
         return Store.of(self).find(
-            Person,
-            Person.id.is_in(self.user_ids_affected_with_dupes))
+            Person, Person.id.is_in(self.user_ids_affected_with_dupes))
 
     @property
     def users_affected_count_with_dupes(self):
@@ -751,12 +747,9 @@
     @cachedproperty
     def initial_message(self):
         """See `IBug`."""
-        store = Store.of(self)
-        messages = store.find(
-            Message,
-            BugMessage.bug == self,
-            BugMessage.message == Message.id).order_by('id')
-        return messages.first()
+        return Store.of(self).find(
+            Message, BugMessage.bug == self,
+            BugMessage.message == Message.id).order_by('id').first()
 
     @cachedproperty
     def official_tags(self):
@@ -766,13 +759,10 @@
         from lp.registry.model.product import Product
         table = OfficialBugTag
         table = LeftJoin(
-            table,
-            Distribution,
+            table, Distribution,
             OfficialBugTag.distribution_id == Distribution.id)
         table = LeftJoin(
-            table,
-            Product,
-            OfficialBugTag.product_id == Product.id)
+            table, Product, OfficialBugTag.product_id == Product.id)
         # When this method is typically called it already has the necessary
         # info in memory, so rather than rejoin with Product etc, we do this
         # bit in Python. If reviewing performance here feel free to change.
@@ -903,17 +893,12 @@
         return self.personIsSubscribedToDuplicate(person)
 
     def _getMutes(self, person):
-        store = Store.of(self)
-        mutes = store.find(
-            BugMute,
-            BugMute.bug == self,
-            BugMute.person == person)
-        return mutes
+        return Store.of(self).find(
+            BugMute, BugMute.bug == self, BugMute.person == person)
 
     def isMuted(self, person):
         """See `IBug`."""
-        mutes = self._getMutes(person)
-        return not mutes.is_empty()
+        return not self._getMutes(person).is_empty()
 
     def mute(self, person, muted_by):
         """See `IBug`."""
@@ -928,19 +913,15 @@
         if mutes.is_empty():
             mute = BugMute(person, self)
             Store.of(mute).flush()
-        else:
-            # It's already muted, pass.
-            pass
 
     def unmute(self, person, unmuted_by):
         """See `IBug`."""
-        store = Store.of(self)
         if person is None:
             # This may be a webservice request.
             person = unmuted_by
         mutes = self._getMutes(person)
         if not mutes.is_empty():
-            store.remove(mutes.one())
+            Store.of(self).remove(mutes.one())
         return self.getSubscriptionForPerson(person)
 
     @property
@@ -1087,8 +1068,7 @@
     def getSubscriptionForPerson(self, person):
         """See `IBug`."""
         return Store.of(self).find(
-            BugSubscription,
-            BugSubscription.person == person,
+            BugSubscription, BugSubscription.person == person,
             BugSubscription.bug == self).one()
 
     def getAlsoNotifiedSubscribers(self, recipients=None, level=None):
@@ -1152,8 +1132,8 @@
             recipients = self.getBugNotificationRecipients(
                 level=BugNotificationLevel.COMMENTS)
         getUtility(IBugNotificationSet).addNotification(
-             bug=self, is_comment=True,
-             message=message, recipients=recipients, activity=activity)
+             bug=self, is_comment=True, message=message, recipients=recipients,
+             activity=activity)
 
     def addChange(self, change, recipients=None, deferred=False,
                   update_heat=True):
@@ -1329,9 +1309,7 @@
 
     def hasBranch(self, branch):
         """See `IBug`."""
-        branch = BugBranch.selectOneBy(branch=branch, bug=self)
-
-        return branch is not None
+        return BugBranch.selectOneBy(branch=branch, bug=self) is not None
 
     def linkBranch(self, branch, registrant):
         """See `IBug`."""
@@ -1358,18 +1336,15 @@
 
     def getVisibleLinkedBranches(self, user, eager_load=False):
         """Return all the branches linked to the bug that `user` can see."""
-        all_branches = getUtility(IAllBranches)
-        linked_branches = list(all_branches.visibleByUser(
+        linked_branches = list(getUtility(IAllBranches).visibleByUser(
             user).linkedToBugs([self]).getBranches(eager_load=eager_load))
         if len(linked_branches) == 0:
             return EmptyResultSet()
         else:
-            store = Store.of(self)
             branch_ids = [branch.id for branch in linked_branches]
-            return store.find(
+            return Store.of(self).find(
                 BugBranch,
-                BugBranch.bug == self,
-                In(BugBranch.branchID, branch_ids))
+                BugBranch.bug == self, In(BugBranch.branchID, branch_ids))
 
     @cachedproperty
     def has_cves(self):
@@ -1522,8 +1497,7 @@
         result = Store.of(self).find((BugMessage, Message, MessageChunk),
             Message.id == MessageChunk.messageID,
             BugMessage.messageID == Message.id,
-            BugMessage.bug == self.id,
-            *ranges)
+            BugMessage.bug == self.id, *ranges)
         result.order_by(BugMessage.index, MessageChunk.sequence)
 
         def eager_load_owners(rows):
@@ -1839,8 +1813,7 @@
     @cachedproperty
     def _cached_tags(self):
         return list(Store.of(self).find(
-            BugTag.tag,
-            BugTag.bugID == self.id).order_by(BugTag.tag))
+            BugTag.tag, BugTag.bugID == self.id).order_by(BugTag.tag))
 
     def _setTags(self, tags):
         """Set the tags from a list of strings."""
@@ -1883,8 +1856,7 @@
         if user is None:
             return None
         else:
-            return Store.of(self).get(
-                BugAffectsPerson, (self.id, user.id))
+            return Store.of(self).get(BugAffectsPerson, (self.id, user.id))
 
     def isUserAffected(self, user):
         """See `IBug`."""
@@ -1927,8 +1899,7 @@
         # update the affected status.
         if dupe_bug_ids:
             Store.of(self).find(
-                BugAffectsPerson,
-                BugAffectsPerson.person == user,
+                BugAffectsPerson, BugAffectsPerson.person == user,
                 BugAffectsPerson.bugID.is_in(dupe_bug_ids),
             ).set(affected=affected)
             for dupe in self.duplicates:
@@ -2087,8 +2058,7 @@
         store = Store.of(self)
         subscriptions = store.find(
             BugSubscription,
-            BugSubscription.bug == self,
-            BugSubscription.person == person)
+            BugSubscription.bug == self, BugSubscription.person == person)
         return not subscriptions.is_empty()
 
     def personIsAlsoNotifiedSubscriber(self, person):
@@ -2115,14 +2085,10 @@
             return False
         if person is None:
             return False
-        store = Store.of(self)
-        subscriptions_from_dupes = store.find(
-            BugSubscription,
-            Bug.duplicateof == self,
+        return not Store.of(self).find(
+            BugSubscription, Bug.duplicateof == self,
             BugSubscription.bug_id == Bug.id,
-            BugSubscription.person == person)
-
-        return not subscriptions_from_dupes.is_empty()
+            BugSubscription.person == person).is_empty()
 
     def _reconcileAccess(self):
         # reconcile_access_for_artifact will only use the pillar list if