Re: [geonode] Metadata branch ready for review.
- From:
- Ariel Nunez
- Date:
- 2010-07-12 @ 15:53
David,
Thanks to your feedback I have performed the following tasks and pushed them
to the metadata branch:
On Fri, Jul 9, 2010 at 10:39 AM, David Winslow <dwinslow@opengeo.org> wrote:
> [...]
>
> There's some left-overs floating around:
> * models.py imports uggettext as _ and later overrides it with "def _(x):
> return x"
>
fixed.
> * 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())
>
Geonetwork object is now being cached like the gs_catalog.
* the LabelledInput class from forms.py is no longer needed
>
removed.
> 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?
>
You are correct we discussed using two foreign-key fields, but I felt
strongly inclined towards the many-to-many option while implementing the
Role model and that's why I sent that mail before with the breakdown of the
models.
I have double checked with Galen and more than two contacts can be attached,
the schema specifies those two contacts as a minimum. The API I implemented
allows accessing the poc and metadata_author objects as layer properties so
the use is not complicated by having the more liberal many-to-many
relationship and we get the benefit of being able to use the
layer/contact/role/permission architecture to allow some level of per-object
permissions based on the contact role.
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)
>
Fixed, mea culpa.
> 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.
>
Fixed by adding a initial_data.json fixture with the two default contacts
and changing the get_or_create to a simple get
>
> I don't think we should merge this onto master before addressing these
> issues.
>
We are one step closer now :)
Ariel.
Re: [geonode] Metadata branch ready for review.
- From:
- David Winslow
- Date:
- 2010-07-12 @ 16:49
On 07/12/2010 11:53 AM, Ariel Nunez wrote:
>
> I have double checked with Galen and more than two contacts can be
> attached, the schema specifies those two contacts as a minimum. The
> API I implemented allows accessing the poc and metadata_author objects
> as layer properties so the use is not complicated by having the more
> liberal many-to-many relationship and we get the benefit of being able
> to use the layer/contact/role/permission architecture to allow some
> level of per-object permissions based on the contact role.
These shortcut methods will error if, for whatever reason a layer, has
more than one poc or metadata_author. It doesn't seem unreasonable to
expect multiple of either, so if we are going this way then we should
probably ditch the properties and make developers deal with the idea
that there might be more than one party playing any particular role for
any particular layer. Otherwise, we should add a uniqueness constraint
on ContactRole on (layer, role).
<snip cause="already in the repo!">bit about using a fixture for
roles</snip>
Everything else looks good to me too[1]. Let's figure out how we want
to handle contacts before merging this into master though.
[1] Well, we might have some issues with caching logged-in-ness across
all GeoServer/GeoNetwork access later, but we can deal with that when/if
it becomes a problem. It's not a new issue anyway.
--
David Winslow
OpenGeo - http://opengeo.org/
Re: [geonode] Metadata branch ready for review.
- From:
- Sebastian Benthall
- Date:
- 2010-07-12 @ 22:23
>
> > I have double checked with Galen and more than two contacts can be
> > attached, the schema specifies those two contacts as a minimum. The
> > API I implemented allows accessing the poc and metadata_author objects
> > as layer properties so the use is not complicated by having the more
> > liberal many-to-many relationship and we get the benefit of being able
> > to use the layer/contact/role/permission architecture to allow some
> > level of per-object permissions based on the contact role.
>
> These shortcut methods will error if, for whatever reason a layer, has
> more than one poc or metadata_author. It doesn't seem unreasonable to
> expect multiple of either, so if we are going this way then we should
> probably ditch the properties and make developers deal with the idea
> that there might be more than one party playing any particular role for
> any particular layer. Otherwise, we should add a uniqueness constraint
> on ContactRole on (layer, role).
>
Is there a way to write the shortcut method in a way that it doesn't fail
when there are multiple authors? Maybe return them all in a collection, or
else return just one of them?
I am concerned about some related user interface recommendations according
to which in certain places (on the Data grid, for example). According to
these designs, these rows have an "Author" or "Owner" column that has a
singular name in it. So far, nobody has specified the logic for choosing
one of many points of contact or authors as the primary owner of a resource.
What I'm trying to figure out is whether this is a flaw in our design, or
whether there is some kind of appropriate logic for determining this. Maybe
a hierarchy of the "roles" used in the metadata could be used to select the
person who is _most_ responsible for the resource. This could easily be an
orthogonal point, but it could also be a point of inspiration.
<snip cause="already in the repo!">bit about using a fixture for
> roles</snip>
>
> Everything else looks good to me too[1]. Let's figure out how we want
> to handle contacts before merging this into master though.
> [1] Well, we might have some issues with caching logged-in-ness across
> all GeoServer/GeoNetwork access later, but we can deal with that when/if
> it becomes a problem. It's not a new issue anyway.
>
> --
> David Winslow
> OpenGeo - http://opengeo.org/
>
>
>
--
Sebastian Benthall
OpenGeo - http://opengeo.org
Re: [geonode] Metadata branch ready for review.
- From:
- David Winslow
- Date:
- 2010-07-13 @ 14:16
On 07/12/2010 06:23 PM, Sebastian Benthall wrote:
>
> > I have double checked with Galen and more than two contacts can be
> > attached, the schema specifies those two contacts as a minimum. The
> > API I implemented allows accessing the poc and metadata_author
> objects
> > as layer properties so the use is not complicated by having the more
> > liberal many-to-many relationship and we get the benefit of
> being able
> > to use the layer/contact/role/permission architecture to allow some
> > level of per-object permissions based on the contact role.
>
> These shortcut methods will error if, for whatever reason a layer, has
> more than one poc or metadata_author. It doesn't seem unreasonable to
> expect multiple of either, so if we are going this way then we should
> probably ditch the properties and make developers deal with the idea
> that there might be more than one party playing any particular
> role for
> any particular layer. Otherwise, we should add a uniqueness
> constraint
> on ContactRole on (layer, role).
>
>
> Is there a way to write the shortcut method in a way that it doesn't
> fail when there are multiple authors? Maybe return them all in a
> collection, or else return just one of them?
In fact we can return anything from a property's getter that we can from
a full-fledged method. At this point though, we are talking about the
difference between:
layer.contact_set.find(role=AUTHOR)
and
layer.poc_author
I'd prefer to just expose the already polished Django ORM methods rather
than try to dumb things down any further.
More explicitly, I am:
-0 on having a property that returns a collection
-1 on having a property that returns some arbitrary single element of
the collection
>
> I am concerned about some related user interface recommendations
> according to which in certain places (on the Data grid, for example).
> According to these designs, these rows have an "Author" or "Owner"
> column that has a singular name in it. So far, nobody has specified
> the logic for choosing one of many points of contact or authors as the
> primary owner of a resource. What I'm trying to figure out is whether
> this is a flaw in our design, or whether there is some kind of
> appropriate logic for determining this. Maybe a hierarchy of the
> "roles" used in the metadata could be used to select the person who is
> _most_ responsible for the resource. This could easily be an
> orthogonal point, but it could also be a point of inspiration.
The UI design discussion should be separate from the API design discussion.
> <snip cause="already in the repo!">bit about using a fixture for
> roles</snip>
>
> Everything else looks good to me too[1]. Let's figure out how we want
> to handle contacts before merging this into master though.
> [1] Well, we might have some issues with caching logged-in-ness
> across
> all GeoServer/GeoNetwork access later, but we can deal with that
> when/if
> it becomes a problem. It's not a new issue anyway.
>
> --
> David Winslow
> OpenGeo - http://opengeo.org/
>
>
>
>
>
> --
> Sebastian Benthall
> OpenGeo - http://opengeo.org
>
Re: [geonode] Metadata branch ready for review.
- From:
- Ariel Nunez
- Date:
- 2010-07-13 @ 14:22
I have added a property that returns a collection (of pocs and mds), it is
in the metadata branch since yesterday.
Re: [geonode] Metadata branch ready for review.
- From:
- David Winslow
- Date:
- 2010-07-13 @ 16:14
On 07/13/2010 10:22 AM, Ariel Nunez wrote:
> I have added a property that returns a collection (of pocs and mds),
> it is in the metadata branch since yesterday.
Ariel and I just discussed these properties in the channel. We
discussed requirements a bit -
* Some roles, like "admin" and "user," should allow multiple
associations between a particular layer and contact
* Other roles, like "author" and "point of contact", should allow
only a single association between a particular layer and contact.
So we will be allowing multiplicity at the database level, and enforcing
uniqueness in the model's clean() method for those roles that require
it. The UI will continue to display one fixed field each for the author
and poc contacts, with a more general 'associated contact' editor to
come in a future iteration.
With this in mind, my objections to the .poc and .author properties go
away, so the revised metadata management branch should be hitting master
sometime today.
--
David Winslow
OpenGeo - http://opengeo.org/
Re: [geonode] Metadata branch ready for review.
- From:
- Ariel Nunez
- Date:
- 2010-07-09 @ 17:43
Hi David, thanks for the quick feedback, please find my responses inline.
On Fri, Jul 9, 2010 at 10:39 AM, David Winslow <dwinslow@opengeo.org> 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
Re: [geonode] Metadata branch ready for review.
- From:
- David Winslow
- Date:
- 2010-07-09 @ 16:01
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
>>
>
Re: [geonode] Metadata branch ready for review.
- From:
- Ariel Nunez
- Date:
- 2010-07-09 @ 17:50
>
> 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?
>
I tried many ways before being able to make this work, here is the
breakdown:
1. We need to save the layer in geoserver before being able to get the
bounding box and the other data that geoserver generates.
2. We need to save the layer in geonetwork before being able to pull data
from geonetwork, in this case distribution_url and some of the other data
set in the template from geoserver and layer info.
3. We need to have the poc defined before saving to geonetwork or the record
would be invalid ... but ... we need to have the layer instance saved to the
database before being able to assign a poc (requiring a post_save hook).
I am open to suggestions as I played with pre_init, post_init, pre_save,
post_save hooks until the letters in the screen became ants and started
dancing around. An outsider pair of eyes are always fresher.
Ariel.