Hi David, thanks for the quick feedback, please find my responses inline. On Fri, Jul 9, 2010 at 10:39 AM, David Winslow <[email protected]> wrote:
> [snip] > I do have some lower-level concerns about the code. > > There's some left-overs floating around: > * models.py imports uggettext as _ and later overrides it with "def _(x): > return x" > Roger that, I will fix it asap. > * extra gn.login/logout around the main loop in LayerManager.slurp() > (side note - it would be nice to avoid logging in/out repeatedly for this; > that's tripling the number of HTTP requests we have to make to GeoNetwork in > a slurp()) > Good point too, I'll take a look at the current login/logout approach and minimize it. > * the LabelledInput class from forms.py is no longer needed > I remember grepping for it and discovering the same, I'll remove it. Layer has a many-to-many relationship with Contacts. I seem to remember > discussing using two foreign-key fields to accomplish that, since the schema > we got from the World Bank specifies that a metadata document contains > exactly one owner and one metadata maintainer. Did I miss something? > In general a minimim of two contacts is needed (Point Of Contact and Metadata Author, however the World Bank spec defines additional roles, especially important ones are the 'user' one and the 'originator'. Generally, mixing in formatting changes with content changes is a pain for > reviewers (this is especially egregious in the various CSW request > templates, where a few real changes in a 500-line file were obscured by an > indent applied to the entire thing.) > I agree with your assertion in general and plead guilty of doing it a few times, in the case you are mentioning, it was more a refactor than an indent: the current 'transaction_insert.xml' template had a lot of code that was also needed in 'transaction_update.xml' and I had to abstract it into 'full_metadata.xml' and import it in both. Although even in this case, I should have performed the move first and the changes in a future changeset so your recommendation still applies. My bad, I'll take that into account in the future. > > SUPPLEMENTAL has two P's in English (let's try to keep the API spelled > correctly so orthography nerds like me don't flip out.) > (DEFAULT_SUPLEMENTAL_INFORMATION) > Sorry, that bit me twice while using that field in lowercase too (forgetting to add one p). It also seems a little odd that accessing a layer's metadata information > (poc) may have the side-effect of creating a Contact in the database. I > would think it would be better to just leave it alone if no poc is > configured. The default value should also probably be pulling an existing > user rather than making one from scratch. Maybe the admin user with the > lowest user-id would work? Or we can just require admins to create a > superuser and put the id in settings.py. > Well, I don't think it's a good idea to link a random user (even if it is the admin) with a contact, we can create a contact without having to create (or link to) a user. I think I can satisfy your concern by creating a fixture with two defaults contacts, one for poc and one for metadata and changing the get_or_create for a plain get. That alternative does not require to create anything on the fly or put anything in settings.py. The fixture will be named initial_data.json so it's picked up after every syncdb. > I don't think we should merge this onto master before addressing these > issues. > I totally agree, I'll adress these issues today and report back here. Thanks again, Ariel
