Merge lp:~dhillon-v10/launchpad/fix-bug-410331 into lp:launchpad/db-devel

[Vikram Dhillon]

[Vikram Dhillon (dhillon-v10)]

Adding a default display name in the file lib/lp/soyuz/browser/archive.py

[Julian Edwards (julian-edwards)]

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)

[Vikram Dhillon (dhillon-v10)]

Julian, then something like this perhaps: (the spacing might not come out right)

if self.context.archive is not None:
         return { 'displayname' : "PPA named " + self.context.owner.name + "for " + self.context.owner.displayname}
else:
         return { 'displayname' : "PPA for " + self.context.owner.displayname}

[Vikram Dhillon (dhillon-v10)]

Or better yet, something like this:

if self.context.archive is not None:
         return { 'displayname' : "PPA named X for " + self.context.owner.displayname}
else:
         return { 'displayname' : "PPA for " + self.context.owner.displayname}

[Julian Edwards (julian-edwards)]

Hi

This is still not going to work unfortunately :(

+ if self.context.archive is not None:
+ return { 'displayname' : "PPA named " + self.context.archive.name +
+ " for " + self.context.owner.displayname}
+ # Or something like "PPA named X for " +
+ # self.context.owner.displayname
+ else:
+ return { 'displayname' : "PPA for " + self.context.owner.displayname}

The context is not set up yet so this won't pull anything useful. In fact it will re-use the details on the already-existing default PPA. The else clause will also never get called because the code above the diff hunk returns early if the archive is None.

Did you test this locally at all?

Try something like this:

    def initial_values(self):
    """Set up default values for form fields."""
    # Suggest a default value of "ppa" for the name for the
    # first PPA activation.
    if self.context.archive is None:
        return {
            'name': 'ppa',
            'displayname': 'PPA for ' + self.context.owner.displayname
        }
    else:
        return {
            'name': '<change me>',
            'displayname': 'PPA named <change me> for ' + self.context.owner.displayname
        }

I'll do you a deal here - if you get this working in your local browser (if you followed the setup instructions here https://dev.launchpad.net/Running then you will have a http://launchpad.dev available) then I will write your tests for you (as explained in the comment I made above). Then you will see how to write them in the future!

J

[Julian Edwards (julian-edwards)]

review: Needs Fixing

[Vikram Dhillon (dhillon-v10)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 02/22/2010 06:25 AM, Julian Edwards wrote:
> Hi
>
> This is still not going to work unfortunately :(
>
> + if self.context.archive is not None:
> + return { 'displayname' : "PPA named " + self.context.archive.name +
> + " for " + self.context.owner.displayname}
> + # Or something like "PPA named X for " +
> + # self.context.owner.displayname
> + else:
> + return { 'displayname' : "PPA for " + self.context.owner.displayname}
>
> The context is not set up yet so this won't pull anything useful. In fact it will re-use the details on the already-existing default PPA. The else clause will also never get called because the code above the diff hunk returns early if the archive is None.
>
> Did you test this locally at all?
>
> Try something like this:
>
> def initial_values(self):
> """Set up default values for form fields."""
> # Suggest a default value of "ppa" for the name for the
> # first PPA activation.
> if self.context.archive is None:
> return {
> 'name': 'ppa',
> 'displayname': 'PPA for ' + self.context.owner.displayname
> }
> else:
> return {
> 'name': '<change me>',
> 'displayname': 'PPA named <change me> for ' + self.context.owner.displayname
> }
>
> I'll do you a deal here - if you get this working in your local browser (if you followed the setup instructions here https://dev.launchpad.net/Running then you will have a http://launchpad.dev available) then I will write your tests for you (as explained in the comment I made above). Then you will see how to write them in the future!
>
> J

Alright that's a good deal :) I do have a launchpad instance running
locally but I didn't test it out yet, sorry. Will work on it and also
test it this time, having some work here (AMC is on Wednesday, so have
to prepare for it) but I will get a working model by Wednesday.

- --
Regards,
Vikram Dhillon

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuC+DIACgkQcoBavQdRHczHwgCeI4flySsXQ18IlV/OsM5m5smI
DicAnio343cZDaIsPN9qxqa0jCpS1/mi
=5I/T
-----END PGP SIGNATURE-----

[lp:~dhillon-v10/launchpad/fix-bug-410331]

10289. By Vikram Dhillon on 2010-03-14

Fixed the default ppa naming

[Vikram Dhillon (dhillon-v10)]

Julian: It works now! all I had to do was just remove the owner part, other than that the code was fine :) Now time to write tests. Here's a screenshot: http://img101.imageshack.us/i/ppaactivation.png/

[Michael Nelson (michael.nelson)]

Hi Vikram,

Thanks for the nice little improvement.

For the case where the user already has a PPA, I wouldn't pre-populate the name/displayname at all, but would leave them blank (as in the previous implementation).

Also, if you are keen to have a go at writing a test for this, you could take a look at:

lib/lp/soyuz/browser/tests/test_archive_admin_view.py

which is an example unit test for another archive view. You could start by creating:
lib/lp/soyuz/browser/tests/test_archive_activate_view.py

and seeing if you can create a test to demonstrate the pre-populated display name when there are no previous ppas (it might be faster that waiting for bigjools to write the tests - as he's very busy atm - and will give a chance to learn more about writing tests, which is helpful generally to any project you work on). If you don't want to do that, let us know and we'll find some time to write the tests.

Thanks!

[Gavin Panella (allenap)]

Vikram, there has been no activity for a long time so I'm going to mark this as Rejected, but resubmit if you do feel like following up on it. Thanks for having a go at improving Launchpad.

Unmerged revisions

10289. By Vikram Dhillon on 2010-03-14

Fixed the default ppa naming

10288. By Vikram Dhillon on 2010-02-14

New display name style for a ppa

10287. By Vikram Dhillon on 2010-02-14

Removing the XXX line

10286. By Vikram Dhillon on 2010-02-08

Adding a default value to the display name when the ppa first activates.

=== modified file 'lib/lp/soyuz/browser/archive.py'
--- lib/lp/soyuz/browser/archive.py	2010-03-10 12:50:18 +0000
+++ lib/lp/soyuz/browser/archive.py	2010-03-14 01:14:37 +0000
@@ -1651,8 +1651,15 @@
         # Suggest a default value of "ppa" for the name for the
         # first PPA activation.
         if self.context.archive is None:
-            return {'name': 'ppa'}
-        return {}
+            return {
+                'name': 'ppa ',
+                'displayname': 'PPA for ' + self.context.displayname
+                }
+        else:
+            return {
+                'name': '<change me>',
+                'displayname': 'PPA named <change me> for '
+                }
 
     def setUpFields(self):
         """Override `LaunchpadFormView`.