Code review comment for lp:~adiroiban/launchpad/bug-525371

Hi Adi,

Ok, I realized there are more things to fix first. We've discussed them
over on IRC, but I will repeat myself a bit for reference.

 review needs-fixing

Attribute names should include as little gettextisms as possible:
"all_pofiles" and "all_potemplates" should be "translation_files" and
"translation_templates" (this one is already almost alright in the
branch: I am not sure about the "all").

And further on that topic, we should only show most relevant translation
templates: these are usually only the current, non-obsolete ones.
Obsolete templates are useful only for management purposes, and if we
want, we can export them either separately, or export all of

  getCurrentTranslationTemplates
  getObsoleteTranslationTemplates
  getTranslationTemplates

There is no need to wrap either of the methods in a property in order to
export them. What's else, these methods expose some implementation
details (like "just_ids" parameter, needed for performance reasons
elsewhere) and we should not get them exported as such.

Since I want to see this branch move in soon, I don't want to dwell on
cleaning up these right now.

Now, performance reasons are very important to consider here. We may
have to investigate whether `doNotSnapshot` is needed, and how batching
works over Collection results ([1] suggests it splits results into
batches of 75, but we'd have to check that). If API framework splits
the results into batches and only serializes a single batch (vs
serializing a whole result set), it'd be ok even if we don't use
doNotSnapshot (unless it turns out to be too slow once it hits
edge/staging, when we'll need to fix it).

Also, at the moment, we should export only the most useful list of
templates (because most of this code needs slight refactoring, we should
not export all methods and then stop exporting them soon after). At the
moment, for the "reporting" use case, I believe only "current" templates
make sense to be exported. Now, exporting it with just_ids parameter is
fine as well, and we can fix it with the refactoring in the future.

Next, we did mention how it should already allow read-write access if
you've got sufficient privileges. In my tests that has turned out to be
false. This is definitely not a blocker (it's ok if we start with
read-only API), but we at least need to understand why this is so and
how is write access created (so we don't do it by accident).

Example script I tried out (after doing 'make run' in this branch, run
this in a python console and log in with sufficient privileges, eg.
<email address hidden>:test):

  cachedir = "/home/danilo/.launchpadlib/cache/"
  from launchpadlib.launchpad import Launchpad
  launchpad = Launchpad.login_with('dev testing', 'dev', cachedir)
  ubuntu = launchpad.distributions['ubuntu']
  hoary = ubuntu.series[2]
  pots = hoary.all_translation_templates
  pot = pots[0]
  pot.translation_domain = 'something_else'
  pot.lp_save()

So, suggested course of action to get this landed:

 0. fix "all_pofile" attribute name
 1. Check if we get batching of results on API method calls/attributes
for free (without needing to do anything else)
 2. If 1 is true, check if batching snapshots the entire result set or
just the current batch
 3. if answer to 2 is "entire result set", use "doNotSnapshot"
 4. Export getCurrentTranslationTemplates directly (or
getTranslationTemplates if you feel that's really the right choice right
now)
 5. Confirm that privileges are indeed read-only, and what it takes to
make them writeable as well

[1] https://dev.launchpad.net/API/ImplementingAPIs