launchpad-reviewers team mailing list archive
-
launchpad-reviewers team
-
Mailing list archive
-
Message #14013
[Merge] lp:~stevenk/launchpad/filebug-description-widget into lp:launchpad
Steve Kowalik has proposed merging lp:~stevenk/launchpad/filebug-description-widget into lp:launchpad.
Requested reviews:
Launchpad code reviewers (launchpad-reviewers)
For more details, see:
https://code.launchpad.net/~stevenk/launchpad/filebug-description-widget/+merge/133009
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.
--
https://code.launchpad.net/~stevenk/launchpad/filebug-description-widget/+merge/133009
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~stevenk/launchpad/filebug-description-widget into lp:launchpad.
=== 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:08: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 an extra description, and if that is too long.
+ if self.extra_data.extra_description:
+ if len("%s\n\n%s" % (
+ comment, self.extra_data.extra_description)) > 50001:
+ 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:08: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:08: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
Follow ups
