Here are my notes from reviewing this code (I did a partial review
yesterday so I only had to review the latest 5 changesets or so and add
to my notes.) I've already expressed my architectural concerns, but as
discussed in the other thread about this we're going to hold off on
refactoring this stuff until we have a better idea of the real-world needs.
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"
* 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())
* the LabelledInput class from forms.py is no longer needed
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?
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.)
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)
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.
I don't think we should merge this onto master before addressing these
issues.
--
David Winslow
OpenGeo - http://opengeo.org/
On 07/09/2010 09:54 AM, Ariel Nunez wrote:
I have finished my functional review of the metadata branch and
verified it addresses the following tickets:
http://projects.opengeo.org/CAPRA/ticket/345
http://projects.opengeo.org/CAPRA/ticket/486
http://projects.opengeo.org/CAPRA/ticket/492
http://projects.opengeo.org/CAPRA/ticket/498
http://projects.opengeo.org/CAPRA/ticket/500
http://projects.opengeo.org/CAPRA/ticket/501
http://projects.opengeo.org/CAPRA/ticket/564
You can find more information about the changes and the commands to
run it in my previous post about it to the list [0]
For now I would like to point out a few rough edges I found during the
development:
1. *Updating the shp for a given layer gives a HTTP 403* response and
does not even reach the layerController view. All the code to delete
from Geoserver and Geonetwork as well as inserting new records to both
of them is working, fixing this operation is just a matter of figuring
out why that 403 error is popping out and then making sure all the
operations are called in the right order.
2. *uuids, unit tests and idempotent-iveness of interactions with
geonetwork. * This one alone deserves another mail where I would
addres the duplication of geonetwork records, the testing setup and
some idea.
3. *layerController.* Currently we have the following scheme for the
layer operations:
/data/base:district?describe -> edit metadata
/data/base:district?update -> Udate layer data
/data/base:district?remove -> Remove layer
/data/base:district?style -> Change default style
We are accessing the QUERY STRING and performing the routing
inside the layerController view, I suggest we change that approach to
a more elegant use of django routers and use the opportunity to add
named urls so one can reference for example the metadata editing url
for a given layer in a template as well as make debugging easier (for
example in the case of the update operation that is broken at the
moment). Also, the colons ":" are special chars in a url and we should
not use them so liberally[1] (although I admit it could work), I would
suggest we either uriencode them (ugly!) or change them for something
else, for example:
/data/base/district/describe
/data/base/district/update
/data/base/district/remove
/data/base/district/style
Since base and district already have a hierarchical relationship I
would see no problem in separating them with slashes like this.
This is all for now, I will address the uuid topic in a separate
email and hope a good soul (David? Seb?) can review my changes and
help me get them into the master branch.
Ariel.
[0]
http://librelist.com/browser//geonode/2010/7/6/ariel-s-update-metadata-branch/#4e85a475ddbc734becf818536492547e
[1] http://www.w3.org/Addressing/URL/4_URI_Recommentations.html
