On Wed, 2010-05-26 at 16:12 +0000, Данило Шеган wrote: > Review: Needs Fixing > 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"). renamed all_pofiles to translation_files removed all_translation_templates since its content will be provided by a method. I have renamed the name used for exporting pofile and potemplate entries and collections. > 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. We can hide the "just_ids" in the exported API call. We can export getCurrentTranslationTemplates() method, with just_ids hidden and always set to False. I still think that exporting getTranslationTemplates() will make the API simpler (rather than exporting all 3 methods) and the client side check for active/inactive templates is not that complicated. > 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). I will raise this discussion over the lp-dev ML. I still have to read the code and see exactly why, where and how those snosphots are needed/used in order to ask a valid question. I think that we should land this branch and the start of next cycle. The first attempt should be without doNotSnapshot. If something goes wrong we can reconsider our options. > 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. See above comments. > 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. >