Skip to content

Commit

Permalink
feat(organizations): add owner_label field to AssetSerializer TAS…
Browse files Browse the repository at this point in the history
…K-1181 (#5312)

### 📣 Summary
Added `owner_label` field to the Asset API to clarify ownership
information, dynamically reflecting either the owner's username or the
organization's name.



### 📖 Description
The `owner_label` field in the Asset API now dynamically displays:

- The username of the asset owner if the organization is not a
multi-member organization (MMO).
- The name of the organization if the organization is a multi-member
organization (MMO) and the user is the organization owner.
This enhancement improves clarity by providing contextual ownership
details directly in the API response.



### 👷 Description for instance maintainers
This update introduces a new owner_label field in the Asset serializer:

- The field logic dynamically evaluates the organization's multi-member
status (is_mmo) and ownership (is_owner).
- Efficient database queries and optimizations ensure minimal impact on
performance.



### 👀 Preview steps
1. ℹ️ Have an account and at least one asset in a project.
2. Ensure the asset belongs to a user who is also an organization owner.
3. 🟢 Verify that the API response for the asset includes owner_label
reflecting the username of the owner.
4. Change the organization's status to a multi-member organization
(enable is_mmo).
5. 🟢 Verify that the owner_label in the API response now reflects the
organization's name.
6. Transfer ownership of the organization to a different user.
7. 🟢 Verify that the owner_label reverts to the original owner's
username.


### 💭 Notes
- Testing: Unit tests were added to validate the owner_label behavior
under various conditions, including ownership changes and multi-member
organization scenarios.
- Performance: The implementation minimizes additional queries by
prefetching related data and leveraging efficient annotations.
- Scalability: The logic is designed to handle a large number of assets
and organizations without introducing significant overhead.

---------

Co-authored-by: Olivier Leger <olivierleger@gmail.com>
  • Loading branch information
rajpatel24 and noliveleger authored Dec 4, 2024
1 parent be25956 commit f5e9183
Show file tree
Hide file tree
Showing 5 changed files with 106 additions and 1 deletion.
1 change: 1 addition & 0 deletions kobo/apps/organizations/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,7 @@ def is_admin(self, user: 'User') -> bool:
return super().is_admin(user)

@property
@cache_for_request
def is_mmo(self):
"""
Determines if the multi-members feature is active for the organization
Expand Down
18 changes: 17 additions & 1 deletion kpi/serializers/v2/asset.py
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ class AssetSerializer(serializers.HyperlinkedModelSerializer):
owner = RelativePrefixHyperlinkedRelatedField(
view_name='user-kpi-detail', lookup_field='username', read_only=True)
owner__username = serializers.ReadOnlyField(source='owner.username')
owner_label = serializers.SerializerMethodField()
url = HyperlinkedIdentityField(
lookup_field='uid', view_name='asset-detail')
asset_type = serializers.ChoiceField(choices=ASSET_TYPES)
Expand Down Expand Up @@ -407,6 +408,7 @@ class Meta:
'data_sharing',
'paired_data',
'project_ownership',
'owner_label',
)
extra_kwargs = {
'parent': {
Expand Down Expand Up @@ -827,6 +829,19 @@ def get_access_types(self, obj):

return access_types

def get_owner_label(self, asset):
try:
organization = self.context['organization_by_asset'].get(asset.id)
except KeyError:
organization = asset.owner.organization
if (
organization
and organization.is_owner(asset.owner)
and organization.is_mmo
):
return organization.name
return asset.owner.username

def validate_data_sharing(self, data_sharing: dict) -> dict:
"""
Validates `data_sharing`. It is really basic.
Expand Down Expand Up @@ -1010,7 +1025,8 @@ class Meta(AssetSerializer.Meta):
'status',
'access_types',
'children',
'data_sharing'
'data_sharing',
'owner_label',
)

def get_permissions(self, asset):
Expand Down
4 changes: 4 additions & 0 deletions kpi/tests/api/v1/test_api_assets.py
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ def test_asset_list_matches_detail(self):
self.assertIsNotNone(list_result_detail)
self.assertDictEqual(expected_list_data, dict(list_result_detail))

@unittest.skip(reason='`owner_label` field only exists in v2 endpoint')
def test_asset_owner_label(self):
pass


class AssetVersionApiTests(test_api_assets.AssetVersionApiTests):
URL_NAMESPACE = None
Expand Down
73 changes: 73 additions & 0 deletions kpi/tests/api/v2/test_api_assets.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,10 @@
import os

import dateutil.parser
from django.conf import settings
from django.urls import reverse
from django.utils import timezone
from model_bakery import baker
from rest_framework import status

from kobo.apps.kobo_auth.shortcuts import User
Expand Down Expand Up @@ -85,6 +87,77 @@ def test_asset_list_matches_detail(self):
self.assertIsNotNone(list_result_detail)
self.assertDictEqual(expected_list_data, dict(list_result_detail))

def test_asset_owner_label(self):
"""
Test the behavior of the owner_label field in the Asset API.
- If the user is an organization owner and the organization is MMO,
then the `owner_label` should be the organization's name.
- If the user is not an organization owner and the organization is MMO,
then the `owner_label` should be the owner's username.
- If the user is an organization owner but the organization is not MMO,
then the `owner_label` should be the owner's username.
"""
detail_response = self.create_asset()
someuser_username = detail_response.data.get('owner__username')
someuser = User.objects.get(username=someuser_username)

# Check the user is an owner of an organization
self.assertTrue(someuser.is_org_owner)

# Fetch the assets list and verify the initial owner_label
list_response = self.client.get(self.list_url)
self.assertEqual(
list_response.data['results'][0]['owner_label'],
someuser_username
)

# Make the organization a MMO and verify the owner_label
self.organization = someuser.organization
self.organization.mmo_override = True
self.organization.save(update_fields=['mmo_override'])

list_response = self.client.get(self.list_url)
self.assertEqual(
list_response.data['results'][0]['owner_label'],
self.organization.name
)

# Add another user to the organization and share the asset
anotheruser = User.objects.get(username='anotheruser')
self.organization.add_user(anotheruser)
someuser_asset = someuser.assets.last()
someuser_asset.assign_perm(anotheruser, PERM_VIEW_ASSET)

# Create an external user's asset and share it with anotheruser
external_user = baker.make(settings.AUTH_USER_MODEL)
self.client.force_login(external_user)
detail_response = self.create_asset()
external_user_asset = external_user.assets.last()
external_user_asset.assign_perm(anotheruser, PERM_VIEW_ASSET)

# Fetch the external user's asset and verify the owner_label
list_response = self.client.get(self.list_url)
self.assertEqual(
list_response.data['results'][0]['owner_label'],
external_user.username
)

# Verify owner_label for both assets when logged in as anotheruser
self.client.force_login(anotheruser)
list_response = self.client.get(self.list_url)

anotheruser_assets = {
item['uid']: item['owner_label']
for item in list_response.data['results']
}
self.assertEqual(
anotheruser_assets[external_user_asset.uid], external_user.username
)
self.assertEqual(
anotheruser_assets[someuser_asset.uid], self.organization.name
)

def test_assets_hash(self):
another_user = User.objects.get(username='anotheruser')
user_asset = Asset.objects.get(pk=1)
Expand Down
11 changes: 11 additions & 0 deletions kpi/views/v2/asset.py
Original file line number Diff line number Diff line change
Expand Up @@ -722,6 +722,17 @@ def get_serializer_context(self):

context_['children_count_per_asset'] = children_count_per_asset

# 5) Get organization per asset
assets = (
Asset.objects.filter(id__in=asset_ids)
.prefetch_related('owner__organizations_organization')
)
organization_by_asset = defaultdict(dict)
for asset in assets:
organization = getattr(asset.owner, 'organization', None)
organization_by_asset[asset.id] = organization
context_['organization_by_asset'] = organization_by_asset

return context_

def list(self, request, *args, **kwargs):
Expand Down

0 comments on commit f5e9183

Please sign in to comment.