Code review comment for lp:~dhillon-v10/launchpad/fix-bug-410331

Revision history for this message
Julian Edwards (julian-edwards) wrote :

Hi

Thanks for working on the fix!

There are quite a few problems with this branch, I'll take you through what you need to change to get this branch accepted:

1. Remove the XXX line - we only add those if there's an outstanding issue. For regular bug fixes they're just irrelevant.
2. You've taken my suggested default too literally! We want to substitute the words in the displayname string and default to meaningful names.
3. The code you've written won't do anything for the first PPA activated, something that you'd have noticed if there were tests :)
4. There are no tests! You should examine the file at
lib/lp/soyuz/stories/ppa/xx-ppa-workflow.txt and particularly at around line 57 where it demonstrates that the name is pre-filled for the first PPA activated.

The logic for the pre-filled displayname should be something like:

If the name field has data in it:
    "PPA named <name> for <owner's name>"
else:
    "PPA for <owner's name>"

where <owner's name> is self.context.owner.displayname

Regards
Julian.

review: Needs Resubmitting (code)

« Back to merge proposal