From d8260136454765ad69fc1c82ea2fb8366252e2e9 Mon Sep 17 00:00:00 2001 From: "John N. Milner" Date: Wed, 7 Jul 2021 21:36:19 -0400 Subject: [PATCH] Apply changes from first day of reviewing #3057 with @noliveleger --- kpi/deployment_backends/kobocat_backend.py | 41 +++++++------- kpi/exceptions.py | 13 ----- kpi/filters.py | 6 ++- kpi/interfaces/sync_backend_media.py | 13 +---- kpi/models/asset.py | 56 +------------------ kpi/models/asset_file.py | 24 +-------- kpi/models/object_permission.py | 2 +- kpi/models/paired_data.py | 62 +++++++++------------- kpi/utils/query_parser/query_parser.py | 1 + 9 files changed, 58 insertions(+), 160 deletions(-) diff --git a/kpi/deployment_backends/kobocat_backend.py b/kpi/deployment_backends/kobocat_backend.py index e596095473..3a8967e1c7 100644 --- a/kpi/deployment_backends/kobocat_backend.py +++ b/kpi/deployment_backends/kobocat_backend.py @@ -61,8 +61,8 @@ class KobocatDeploymentBackend(BaseDeploymentBackend): ] SYNCED_DATA_FILE_TYPES = { - AssetFile.FORM_MEDIA: AssetFile.BACKEND_DATA_TYPE, - AssetFile.PAIRED_DATA: PairedData.BACKEND_DATA_TYPE, + AssetFile.FORM_MEDIA: 'media', + AssetFile.PAIRED_DATA: 'paired_data', } def bulk_assign_mapped_perms(self): @@ -810,36 +810,37 @@ def sync_media_files(self, file_type: str = AssetFile.FORM_MEDIA): queryset = self._get_metadata_queryset(file_type=file_type) - for obj in queryset: + for media_file in queryset: - uniq = obj.backend_uniqid + backend_media_id = media_file.backend_media_id # File does not exist in KC - if uniq not in kc_filenames: - if obj.deleted_at is None: + if backend_media_id not in kc_filenames: + if media_file.deleted_at is None: # New file - self.__save_kc_metadata(obj) + self.__save_kc_metadata(media_file) else: # Orphan, delete it - obj.delete(force=True) + media_file.delete(force=True) continue # Existing file - if uniq in kc_filenames: - kc_file = kc_files[uniq] - if obj.deleted_at is None: + if backend_media_id in kc_filenames: + kc_file = kc_files[backend_media_id] + if media_file.deleted_at is None: # If md5 differs, we need to re-upload it. - if obj.md5_hash != kc_file['md5']: + if media_file.md5_hash != kc_file['md5']: self.__delete_kc_metadata(kc_file) - self.__save_kc_metadata(obj) + self.__save_kc_metadata(media_file) elif kc_file['from_kpi']: - self.__delete_kc_metadata(kc_file, obj) + self.__delete_kc_metadata(kc_file, media_file) else: # Remote file has been uploaded directly to KC. We # cannot delete it, but we need to vacuum KPI. - obj.delete(force=True) - # Skip deletion of key corresponding to `uniq` in `kc_files` - # to avoid unique constraint failure in case user deleted + media_file.delete(force=True) + # Skip deletion of key corresponding to `backend_media_id` + # in `kc_files` to avoid unique constraint failure in case + # user deleted # and re-uploaded the same file in a row between # two deployments # Example: @@ -855,7 +856,7 @@ def sync_media_files(self, file_type: str = AssetFile.FORM_MEDIA): # Remove current filename from `kc_files`. # All files which will remain in this dict (after this loop) # will be considered obsolete and will be deleted - del kc_files[uniq] + del kc_files[backend_media_id] # Remove KC orphan files previously uploaded through KPI for kc_file in kc_files.values(): @@ -1226,9 +1227,9 @@ def __save_kc_metadata(self, file_: SyncBackendMediaInterface): kwargs = { 'data': { - 'data_value': file_.backend_data_value, + 'data_value': file_.backend_media_id, 'xform': self.xform_id, - 'data_type': file_.backend_data_type, + 'data_type': self.SYNCED_DATA_FILE_TYPES[file_.file_type], 'from_kpi': True, 'data_filename': file_.filename, 'data_file_type': file_.mimetype, diff --git a/kpi/exceptions.py b/kpi/exceptions.py index 2e60338155..311700dfba 100644 --- a/kpi/exceptions.py +++ b/kpi/exceptions.py @@ -55,10 +55,6 @@ class ImportAssetException(Exception): pass -class ImportAssetException(Exception): - pass - - class InvalidSearchException(exceptions.APIException): status_code = 400 default_detail = _('Invalid search. Please try again') @@ -112,15 +108,6 @@ class ObjectDeploymentDoesNotExist(exceptions.APIException): default_code = 'deployment_does_not_exist' -class PairedParentException(Exception): - - def __init__(self, - message=_('Parent is not set. ' - 'Must call `asset.get_paired_data()` first')): - self.message = message - super().__init__(self.message) - - class ReadOnlyModelError(Exception): def __init__(self, msg='This model is read only', *args, **kwargs): diff --git a/kpi/filters.py b/kpi/filters.py index 8c8127eb1a..bb4f4923cf 100644 --- a/kpi/filters.py +++ b/kpi/filters.py @@ -169,6 +169,8 @@ def _get_queryset_for_data_sharing_enabled( asset_ids = perms.values('asset') + # `SearchFilter` handles futher filtering to include only assets with + # `data_sharing__enabled` (`self.DATA_SHARING_PARAMETER`) set to true return queryset.filter(pk__in=asset_ids) def _get_discoverable(self, queryset): @@ -256,7 +258,9 @@ def _get_queryset_for_discoverable_child_assets( if parent_obj.has_perm(get_anonymous_user(), PERM_DISCOVER_ASSET): self._return_queryset = True - return queryset.filter(pk__in=self._get_publics()) + return queryset.filter( + pk__in=self._get_publics(), parent=parent_obj + ) return queryset diff --git a/kpi/interfaces/sync_backend_media.py b/kpi/interfaces/sync_backend_media.py index 5ef6c0bc96..87c0e75bf2 100644 --- a/kpi/interfaces/sync_backend_media.py +++ b/kpi/interfaces/sync_backend_media.py @@ -9,19 +9,8 @@ class SyncBackendMediaInterface: """ - # Type of file sent to back end during synchronization - BACKEND_DATA_TYPE = None - - @property - def backend_data_value(self): - raise AbstractPropertyError - - @property - def backend_data_type(self): - raise AbstractPropertyError - @property - def backend_uniqid(self): + def backend_media_id(self): raise AbstractPropertyError def delete(self, **kwargs): diff --git a/kpi/models/asset.py b/kpi/models/asset.py index 2316dd23f6..7db9fb9bed 100644 --- a/kpi/models/asset.py +++ b/kpi/models/asset.py @@ -59,7 +59,6 @@ from kpi.exceptions import ( BadPermissionsException, DeploymentDataException, - PairedParentException, ) from kpi.fields import ( KpiUidField, @@ -505,7 +504,7 @@ class Asset(ObjectPermissionMixin, # ToDo Move the field to another table with one-to-one relationship _deployment_data = JSONBField(default=dict) - # JSON with subset of fields and allowed users to use it + # JSON with subset of fields to share # { # 'enable': True, # 'fields': [] # shares all when empty @@ -851,40 +850,6 @@ def get_label_for_permission( ) return label - def get_paired_parent(self, paired_data_uid: str) -> Union['Asset', None]: - - # Validate `paired_data_uid`, i.e., must exist in `self.paired_data` - parent_uid = None - for key, values in self.paired_data.items(): - if values['paired_data_uid'] == paired_data_uid: - parent_uid = key - break - - if not parent_uid: - return None - - try: - parent_asset = Asset.objects.get(uid=parent_uid) - except Asset.DoesNotExist: - return None - - # Data sharing must be enabled on the parent - parent_data_sharing = parent_asset.data_sharing - if not parent_data_sharing.get('enabled'): - return None - - # Validate `self.owner` is still allowed to see parent data - allowed_users = parent_data_sharing.get('users', []) - if allowed_users and self.owner.username not in allowed_users: - return None - - if not parent_asset.has_deployment: - return None - - self.__parent_paired_asset = parent_asset - - return parent_asset - def get_partial_perms( self, user_id: int, with_filters: bool = False ) -> Union[list, dict, None]: @@ -961,25 +926,6 @@ def latest_version(self): except IndexError: return None - @property - def paired_data_fields(self): - try: - parent_asset = self.__parent_paired_asset - except AttributeError: - raise PairedParentException - - parent_fields = parent_asset.data_sharing['fields'] - asset_fields = self.paired_data[parent_asset.uid]['fields'] - - if parent_fields and asset_fields: - return list(set(parent_fields).intersection(set(asset_fields))) - - if not asset_fields: - return parent_fields - - if not parent_fields: - return asset_fields - @staticmethod def optimize_queryset_for_list(queryset): """ Used by serializers to improve performance when listing assets """ diff --git a/kpi/models/asset_file.py b/kpi/models/asset_file.py index a1d0edb0f0..25055fddf1 100644 --- a/kpi/models/asset_file.py +++ b/kpi/models/asset_file.py @@ -36,8 +36,6 @@ class AssetFile(models.Model, FORM_MEDIA = 'form_media' PAIRED_DATA = 'paired_data' - BACKEND_DATA_TYPE = 'media' - TYPE_CHOICES = ( (MAP_LAYER, MAP_LAYER), (FORM_MEDIA, FORM_MEDIA), @@ -81,9 +79,9 @@ class AssetFile(models.Model, synced_with_backend = models.BooleanField(default=False) @property - def backend_data_value(self): + def backend_media_id(self): """ - Implements `SyncBackendMediaInterface.backend_data_value()` + Implements `SyncBackendMediaInterface.backend_media_id()` """ return ( self.metadata['redirect_url'] @@ -91,24 +89,6 @@ def backend_data_value(self): else self.filename ) - @property - def backend_data_type(self): - """ - Implements `SyncBackendMediaInterface.backend_data_type()` - """ - return self.BACKEND_DATA_TYPE - - @property - def backend_uniqid(self): - """ - Implements `SyncBackendMediaInterface.backend_uniqid()` - """ - return ( - self.metadata['filename'] - if not self.is_remote_url - else self.metadata['redirect_url'] - ) - def delete(self, using=None, keep_parents=False, force=False): # Delete object and files on storage if `force` is True or file type # is anything else than 'form_media' diff --git a/kpi/models/object_permission.py b/kpi/models/object_permission.py index 7694dd5c62..4af87592bf 100644 --- a/kpi/models/object_permission.py +++ b/kpi/models/object_permission.py @@ -811,7 +811,7 @@ def has_perm(self, user_obj: User, perm: str) -> bool: return False return result - def has_perms(self, user_obj: User, perms: list, all_: bool = False) -> bool: # noqa + def has_perms(self, user_obj: User, perms: list, all_: bool = True) -> bool: # noqa """ Returns True or False whether user `user_obj` has several permissions (`perms`) on current object. diff --git a/kpi/models/paired_data.py b/kpi/models/paired_data.py index 7c543aa28f..de6afe9b1b 100644 --- a/kpi/models/paired_data.py +++ b/kpi/models/paired_data.py @@ -21,7 +21,10 @@ class PairedData(OpenRosaManifestInterface, SyncBackendMediaInterface): - BACKEND_DATA_TYPE = 'paired_data' + # `filename` implements: + # - `OpenRosaManifestInterface.filename()` + # - `SyncBackendMediaInterface.filename()` + filename = None def __init__( self, @@ -32,12 +35,26 @@ def __init__( paired_data_uid: str = None, hash_: str = None, ): + """ + Usually, this constructor should NOT be called directly except when + creating a new paired data relationship. To retrieve existing pairings, + use `PairedData.objects(asset)` instead. + + When the submission data from `source_asset_or_uid` is collected into + a single XML file, it is attached to `asset` using `filename`, which + the content of `asset` can then reference via `xml-external` (see + https://xlsform.org/en/#external-xml-data). Specify a list of fields + from the source asset to include using `fields`, or pass an empty list + to include all fields. + TODO: document `hash_` (it is currently used to set a value that would + never match the hash of real content, to force synchronization) + """ try: self.source_uid = source_asset_or_uid.uid except AttributeError: self.source_uid = source_asset_or_uid self.asset = asset - self.__filename = filename + self.filename = filename self.fields = fields if not paired_data_uid: @@ -54,23 +71,9 @@ def __str__(self): return f'' @property - def backend_data_value(self): + def backend_media_id(self): """ - Implements `SyncBackendMediaInterface.backend_data_value()` - """ - return self.backend_uniqid - - @property - def backend_data_type(self): - """ - Implements `SyncBackendMediaInterface.backend_data_type()` - """ - return self.BACKEND_DATA_TYPE - - @property - def backend_uniqid(self): - """ - Implements `SyncBackendMediaInterface.backend_uniqid()` + Implements `SyncBackendMediaInterface.backend_media_id()` """ from kpi.urls.router_api_v2 import URL_NAMESPACE # avoid circular imports # noqa paired_data_url = reverse( @@ -110,31 +113,16 @@ def deleted_at(self): """ return None - @property - def filename(self): - """ - Implements: - - `OpenRosaManifestInterface.filename()` - - `SyncBackendMediaInterface.filename()` - """ - # Could be easier to just use a public attribute, but (IMHO) the - # `@property` makes the implementation of the interface more obvious - return self.__filename - - @filename.setter - def filename(self, f): - self.__filename = f - def generate_hash(self): # It generates the hash based on the related AssetFile content. # If the file does not exist yet, the hash is randomly generated with - # the current timestamp and `self.backend_uniqid`. A hash is needed to - # synchronize with KoBoCAt + # the current timestamp and `self.backend_media_id`. A hash is needed + # to synchronize with KoBoCAT try: asset_file = AssetFile.objects.get(uid=self.paired_data_uid) except AssetFile.DoesNotExist: self.__hash = get_hash( - f'{str(time.time())}.{self.backend_uniqid}', + f'{str(time.time())}.{self.backend_media_id}', prefix=True ) else: @@ -214,6 +202,8 @@ def objects(cls, asset: 'kpi.models.Asset') -> 'kpi.models.PairedData': def save(self, **kwargs): try: self.asset.paired_data[self.source_uid]['paired_data_uid'] + # self.paired_data_uid would have been set when `objects()` + # calls the constructor except KeyError: self.paired_data_uid = KpiUidField.generate_unique_id('pd') diff --git a/kpi/utils/query_parser/query_parser.py b/kpi/utils/query_parser/query_parser.py index 9dda82bab2..c094cd6a60 100644 --- a/kpi/utils/query_parser/query_parser.py +++ b/kpi/utils/query_parser/query_parser.py @@ -171,6 +171,7 @@ def name(text, a, b, elements): def get_parsed_parameters(parsed_query: Q) -> dict: """ + NOTE: this is a hack that does not respect boolean logic. Returns a dictionary of all parameters detected in the query and their values. Values are always returned as list even if there is only one value found.