Code review comment for lp:~james-w/launchpad/expose-blueprints

Thanks a lot for this branch. This is a branch I was planning to create as a way of introducing Benji to the web service, but instead we went through your branch and reviewed it.

I don't have any opinion on the whiteboard thing, but I'll make sure someone with an opinion looks at this branch.

= Questions about the exported objects =

1. Obviously you don't need to export everything to make this branch
useful, but I wonder why you exported ISpecification.goalstatus but
not ISpecification.goal.

2. "I added two new methods to IHasSpecifications, as if we exposed
the existing attributes we would serve the collections when just
getting the specification." Can you explain? There are many places
where we publish a collection associated with an entry. (Just within
IPerson we have 'languages', 'gpg_keys', 'pending_gpg_keys',
'wiki_names', 'irc_nicknames', etc.) Just getting the person doesn't
give you these collections--it gives you links to the collections.

3. Rather than publishing updateLifecycleStatus as a named operation, I
think you could publish it as the mutator for the 'completer' field. I
know that you're not publishing 'completer' right now, but I imagine
it will be published eventually, and publishing it now with a mutator
means that end-users will have to learn one less strangely-named named
operation.

The only catch is that updateLifecycleStatus() returns None if the
lifecycle is not in a state for being completed. It would be better to
return a 400 response code, which probably means raising a certain
exception. (I'm fuzzy on the details but I can help you with this when
the time comes.) We'd write a new method just for use in the web
service, which wrapped an updateLifecycleStatus() call with
appropriate exception-raising code.

(At the very least, call this operation something like 'complete', and
pass in the REQUEST_USER automatically--I don't think it makes much
sense to complete a blueprint on someone else's behalf.)

= Questions about the tests =

1. You changed the default priority for a specification created through
the factory to LOW, and then changed some tests that were assuming a
factory-created specification started out UNDEFINED. Is it easy to
leave the earlier behavior alone? Or are specifications with UNDEFINED
priority considered invalid, or don't show up in lists, or anything else?

2. You changed the factory's makeSpecification method to take a lot of
new arguments, which is fine, but you also changed
ISpecificationSet.new() to take the same arguments, which seems like
overkill. Can makeSpecification create a generic specification object
and then customize it?

3. Due to limitations of launchpadlib and the web service there's no
way around constructing the URL to a product when loading it in
launchpadlib. But, once you have that URL, is it possible to navigate
to a specific spec through launchpadlib object traversal? If so, I'd
like to see this in the test. If not, I think this branch should
publish some way of getting from an IHasSpecification to an
ISpecification, given the name--probably as a named operation.

4. In IHasSpecificationsTests, you could save a lot of setup code by
writing a helper methods that takes a list of (name, status) tuples
and creates corresponding specifications.

5. Your test_representation_contains_* methods test the web service
representation against the data used to create the specification
object. They should be testing the web service representation against
the data inside the specification object itself.

For instance,

  self.assertEqual(self.url, spec.specification_url)

should be:

  self.assertEqual(self.spec_object.specurl, spec.specification_url)

And what do we want to test here, really? lazr.restful is tested
separately, so we already know that export() works. We want to verify
that 1) export() was called on certain fields and not others, and 2)
when neccessary, export was called with extra arguments like
export_as. I would be satisfied with shorter tests that look at
lp_entries, lp_attributes, and lp_collections to make sure the proper
fields were exported: and then more detailed tests for any fields
whose export() calls took special arguments:

# Make sure that 'specurl' is published as 'specification_url'
self.assertEqual(self.spec_object.specurl, spec.specification_url)

Of course, it doesn't hurt to also test the identity between
spec_object.name and spec.name. If you want to do this, define the
code in a helper method and then invoke the method in every test
method, to make the amount of code and the risk of copy-paste errors
as small as possible.

7. Write a helper method for the
test_*_implements_IHasSpecification{s,Target} methods to avoid
copy-and-paste errors. (For instance,
test_distribution_implements_IHasSpecifications is a copy of
test_product_implements_IHasSpecifications, or was last time I
checked.)

8. The tests might run faster if you only created one launchpadlib
instance and shared it across test methods. Don't take my word for
this, though.

9. What do the changes in sprint-links.txt mean?