Update metrics on transfer/owner change #810
Conversation
| 'owner', | ||
| 'organization', | ||
| ], | ||
| 'queryset_class': OwnedQuerySet, |
There was a problem hiding this comment.
I don't think we use that? Maybe inheriting from Owned.meta['queryset_class'] is possible but quite ugly :/
There was a problem hiding this comment.
This is Mongoengine behavior: child classes inherit this queryset (at least used in tests), unless this is explicitely overriden.
udata/models/owned.py
Outdated
| import logging | ||
|
|
||
| from blinker import signal | ||
|
|
There was a problem hiding this comment.
Same coding style used in every module: import grouped by packages if not builtin.
udata/models/owned.py
Outdated
|
|
||
| def owned_pre_save(sender, document, **kwargs): | ||
| ''' | ||
| Owned pre_save handler |
There was a problem hiding this comment.
Not sure that comment is useful? (redundant with method name?)
udata/models/owned.py
Outdated
|
|
||
| def owned_post_save(sender, document, **kwargs): | ||
| ''' | ||
| Owned post_save handler |
| @@ -99,10 +101,89 @@ def test_request_transfer_to_same_organization(self): | |||
| self.assert_transfer_started(dataset, org, org, comment) | |||
|
|
|||
|
|
|||
| class TransferAcceptTest(TestCase, DBTestMixin): | |||
| @@ -49,6 +46,8 @@ def __init__(self, app=None): | |||
| self.ValidationError = ValidationError | |||
| self.ObjectId = ObjectId | |||
| self.DBRef = DBRef | |||
| self.Owned = Owned | |||
There was a problem hiding this comment.
I don't get why we need these two lines?
There was a problem hiding this comment.
This allows, as every other db mixins, to be used as db.Owned and db.OwnedQuerySet.
db act as a facade pattern everywhere.
| 'resources.id', | ||
| 'resources.urlhash', | ||
| ], | ||
| ] + db.Owned.meta['indexes'], |
There was a problem hiding this comment.
I wonder if db.Owned.INDEXES is easier to understand. Here it looks to be dynamic.
There was a problem hiding this comment.
But where does it come from ?
The meta['indexes'] the only way (I know) to get the mixin default indexes.
It relies on mongoengine default (documented) behavior in which meta is a dictionary
There was a problem hiding this comment.
Alright, I thought it was udata-related. My bad.
udata/models/owned.py
Outdated
| def owned_pre_save(sender, document, **kwargs): | ||
| ''' | ||
| Owned pre_save handler | ||
| Need to fetch original owner before the new one erase it. |
There was a problem hiding this comment.
for on_owner_change to transfer metrics too
There was a problem hiding this comment.
I don't understand :/
udata/models/owned.py
Outdated
| ''' | ||
| Owned post_save handler | ||
| Dispatch the `Owned.on_owner_change` signal | ||
| once the document has been saved. |
|
|
||
| class DomainModel(UDataDocument): | ||
| '''Placeholder for inheritance''' | ||
| pass |
There was a problem hiding this comment.
Just extracted from models/__init__.py as it was. I think the only usecase is isinstance(document, DomainModel)
There was a problem hiding this comment.
In that case, why not directly isinstance(document, UDataDocument)?
This PR fix metrics not being updated after transfer for initial owner (organization or user):
models/__init__.pyinto modules (models/queryset.py,models/document.py)Tests require #809 to pass