One more thing:
in post_layer_save() we are pulling in metadata from GeoServer and
GeoNetwork, then saving again? Why not do this in a pre_layer_save hook?
--
David Winslow
OpenGeo - http://opengeo.org/
On 07/09/2010 11:39 AM, David Winslow wrote:
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
