Merge lp://staging/~bryce/launchpad/lp-617699-api into lp://staging/launchpad/db-devel

Hi Bryce,

This is good stuff, but needs some more work before it can land. From
a code point of view I think the most important points are:

  [3] Nothing is returned from a property getter, though I think the
      test for this is correct. I suspect the test hasn't been run ;)

  [8] Interface for Reference field needs updating. This is preventing
      the branch from building. Also, a simple test for accessing the
      field via the web API should be added, if one does not yet
      exist.

Gavin.

[1]

I think you'll need to get a database review for
database/schema/comments.sql, database/schema/patch-2208-19-0.sql and
database/schema/patch-2208-20-0.sql. I think stub and jml are the only
ones who can grant that.

[2]

+ distro_source_package = exported(

I was suprised to find that this spelling is more common than
distribution_source_package. No change needed here, just an
observation.

[3]

+ def _getDistroSourcePackage(self):
+ """Retrieves the corresponding source package"""
+ if self.distribution is None or self.source_package_name is None:
+ return None
+ self.distribution.getSourcePackage(
+ self.source_package_name)

This doesn't return the distribution source package, so I'm not sure
how test_link_distro_source_package succeeds!

Also, as it's intended solely for a property, I think it should be
named after the property, thus _get_distro_source_package(). I think
it's clearer if the method naming policy is bent a little here.

[4]

+ def _setDistroSourcePackage(self, distro_source_package):

Same naming suggestion as in [3]. It doesn't matter much though.

[5]

+ distro_source_package = property(_getDistroSourcePackage,
+ _setDistroSourcePackage)

Consider adding a doc parameter, even if it just says "See
`IBugTrackerComponent`".

[6]

{{{
./lib/lp/bugs/interfaces/bugtracker.py
     524: E501 line too long (82 characters)
     524: Line exceeds 78 characters.
./lib/lp/bugs/model/bugtracker.py
      92: 'SourcePackageName' imported but unused
      89: 'IDistributionSourcePackage' imported but unused
     770: E501 line too long (80 characters)
     775: E302 expected 2 blank lines, found 1
     770: Line exceeds 78 characters.
./lib/lp/bugs/tests/test_bugtracker_components.py
      16: E302 expected 2 blank lines, found 1
}}}

Please fix this lint.

[7]

make lint also makes the following complaints:

{{{
database/sampledata/current.sql
    database/sampledata/lintdata.sql differs from database/sampledata/current.sql.
    Patches to the schema, or manual edits to database/sampledata/current.sql
    do not match the dump of the launchpad_ftest_template database.
    If database/sampledata/lintdata.sql is correct, copy it to
    database/sampledata/current.sql.
    Otherwise update database/sampledata/current.sql and run:
        make schema
        make newsampledata
        cd database/sampledata
        cp newsampledata.sql database/sampledata/current.sql
    Run make schema again to update the test/dev database.

database/sampledata/current.sql
    database/sampledata/lintdata-dev.sql differs from database/sampledata/current-de...

> Hi Bryce,
>
> This is good stuff, but needs some more work before it can land.

Thank you for giving this a thorough look through. I've addressed all the issues, but have a question for #8.

> [1]
>
> I think you'll need to get a database review for
> database/schema/comments.sql, database/schema/patch-2208-19-0.sql and
> database/schema/patch-2208-20-0.sql. I think stub and jml are the only
> ones who can grant that.

Actually, those were done on separate branches which landed just today. I probably should have set this merge request as having those as prerequisites.

> [2]
>
> + distro_source_package = exported(
>
> I was suprised to find that this spelling is more common than
> distribution_source_package. No change needed here, just an
> observation.

I was too actually, but I went with this spelling because it helped
prevent overly long lines.

> [3]
>
> + def _getDistroSourcePackage(self):
> + """Retrieves the corresponding source package"""
> + if self.distribution is None or self.source_package_name is None:
> + return None
> + self.distribution.getSourcePackage(
> + self.source_package_name)
>
> This doesn't return the distribution source package, so I'm not sure
> how test_link_distro_source_package succeeds!
>
> Also, as it's intended solely for a property, I think it should be
> named after the property, thus _get_distro_source_package(). I think
> it's clearer if the method naming policy is bent a little here.

I've updated for both these issues, thanks. I don't know why the test
passes either; perhaps when a return type isn't specified, it returns
some value other than None?

> [4]
>
> + def _setDistroSourcePackage(self, distro_source_package):
>
> Same naming suggestion as in [3]. It doesn't matter much though.

Done

> [5]
>
> + distro_source_package = property(_getDistroSourcePackage,
> + _setDistroSourcePackage)
>
> Consider adding a doc parameter, even if it just says "See
> `IBugTrackerComponent`".

Done

> [6]
>
> {{{
> ./lib/lp/bugs/interfaces/bugtracker.py
> 524: E501 line too long (82 characters)
> 524: Line exceeds 78 characters.
> ./lib/lp/bugs/model/bugtracker.py
> 92: 'SourcePackageName' imported but unused
> 89: 'IDistributionSourcePackage' imported but unused
> 770: E501 line too long (80 characters)
> 775: E302 expected 2 blank lines, found 1
> 770: Line exceeds 78 characters.
> ./lib/lp/bugs/tests/test_bugtracker_components.py
> 16: E302 expected 2 blank lines, found 1
> }}}
>
> Please fix this lint.

Fixed

> [7]
>
> make lint also makes the following complaints:
>
> {{{
> database/sampledata/current.sql
> database/sampledata/lintdata.sql differs from
> database/sampledata/current.sql.
> Patches to the schema, or manual edits to database/sampledata/current.sql
> do not match the dump of the launchpad_ftest_template database.
> If database/sampledata/lintdata.sql is correct, copy it to
> database/sampledata/current.sql.
> Otherwise update database/sampledata/current.sql and run:
> make schema
> make newsampledata
> ...

> > [8]
> >
> > + distro_source_package = exported(
> > + Reference(
> > + Interface,
> >
> > This interface should be IDistributionSourcePackage, shouldn't it? I
> > think this is preventing the branch from building.
>
> Yeah I had trouble with this bit. When I include IDistributionSourcePackage
> I get this error:
>
> File "/home/bryce/launchpad/lp-
> branches/lp-617699-api/lib/lp/scripts/utilities/importfascist.py", line 190,
> in import_fascist
> module = original_import(name, globals, locals, fromlist, level)
> File "/home/bryce/launchpad/lp-
> branches/lp-617699-api/lib/lp/bugs/interfaces/bugtracker.py", line 69, in
> <module>
> from lp.bugs.interfaces.bugtask import (
> ImportError: cannot import name BugBranchSearch
>
> But including BugBranchSearch doesn't work either. I'll investigate this more
> tomorrow when I have fresh brains, but if you have suggestions I'd love to
> hear them.

That's probably a circular import problem. There's a way in Launchpad
for resolving this:

- Leave the definition of distro_source_package as
  Reference(Interface, ...),

- Add the following to
  lib/canonical/launchpad/interfaces/_schema_circular_imports.py:

      patch_reference_property(
          IBugTrackerComponent, "distro_source_package",
          IDistributionSourcePackage)

> > Are there any tests for using this attribute via the web API already
> > in the tree? If not, I think it's worth adding something simple.
>
> test_link_distro_source_package() is the only test I've done for this
> property, but I would like to do some with the web API - where would I place
> these tests, or where could I look for an example of how it should be done
> within launchpad?

I haven't done any API tests for a long time, so I'm not up to speed
on what the current best way to do it is, so here's what I know:

- Don't add new tests to lp/bugs/stories/webservice.

- lp/bugs/tests/test_bugs_webservice.py seems okay.

- I've seen people use a new lp.testing.ws_object() helper method in
  tests which says it returns the web service equivalent for a given
  object. Grepping for that might give some clues.

Ping me if you want some more help with this.

Gavin.

Hi Gavin,

After a lot of poking around in other corners of the codebase that do webservice testing, I've put together a reasonably comprehensive set of webservice tests for all the bug tracker component/component-group calls being exposed via the launchpadapi.

Unfortunately, I'm a bit stuck. Most of the tests are failing due either to 'Could not serialize object to JSON', or 'No url for object because object broke the chain', and for the life of me I can't seem to figure out what causes these errors - I'm not even sure I know what the error messages mean.

Do you have any ideas on what these errors mean or what might be causing them?

test I've been using for this branch is `./bin/test -t bugtracker_components`

Hi Bryce,

This was a bit of a 'mare :) Take a look at a bundle I prepared for
you:

  http://paste.ubuntu.com/512660/

It doesn't fix all the tests, but it gets you to a point where you can
probably fix the rest. As in, there are fewer wtf exceptions.

I'll run you through the changes I've made:

lib/canonical/launchpad/webapp/batching.py

  Adapter for some Storm classes that the web service was tripping
  over. Once I had the navigation stuff (later) done I was getting
  errors about not being able to adapt to IFiniteSequence. It seems
  that an adapter was only ever created for SQLObject joins.

lib/canonical/launchpad/webapp/configure.zcml

  Registration of the above adapter.

lib/lp/bugs/browser/bugtracker.py

  Added a navigation step for the bug tracker. I chose the following
  URL pattern, but it's easy to change:

    /bugs/bugtrackers/
      <bug-tracker-name>/
        +components/<component-group-name>/<component-name>

  This meant I also had to create a navigation class for component
  groups.

lib/lp/bugs/browser/configure.zcml

  Registration of the new navigation class, and canonical URL
  configuration for components and component groups.

lib/lp/bugs/configure.zcml

  The component_group attribute needed to be accessible.

lib/lp/bugs/interfaces/bugtracker.py

  Add component_group to the interface definition and fix the
  definition of the components attribute. On reflection, the
  component_group attribute doesn't actually need to be exported
  (though it does need to be in the interface).

lib/lp/bugs/tests/test_bugtracker_components.py

  The addition of lots of commits seems to make the tests work, or
  fail in a way that makes more sense.

lib/lp/testing/factory.py

  The factory methods should not be using hard-coded strings, so I
  fixed that.

And, yes, it's crazy :) There's a lot of stuff to do to get something
published. Fortunately it is actually worth it in the long run, well,
in my opinion.

Gavin.

Awesome, thanks I've merged these changes and cleaned up the remaining tests. Hopefully everything should be squared away, mind taking another look over when you get a chance?

Looks good :)