Skip to content

Commit

Permalink
fix(organization): Block profile updates to org-related fields in Mul…
Browse files Browse the repository at this point in the history
…ti-Member Organization context (#5310)

### 📣 Summary
Blocked updates to profile fields related to the organization when the
user belongs to a Multi-Member Organization (MMO). Ensured other profile
updates remain unaffected.


### 📖 Description
This PR fixes an issue where users in a Multi-Member Organization (MMO)
could update fields in their profile that are directly related to the
organization. Such updates are now blocked to maintain data integrity
within MMO contexts.
These organization-related fields are synchronized with the
`Organization` model to ensure the information remains up-to-date,
especially when a user purchases a plan.
  • Loading branch information
noliveleger authored Dec 2, 2024
1 parent 0711df4 commit b5d51b3
Show file tree
Hide file tree
Showing 10 changed files with 205 additions and 88 deletions.
45 changes: 33 additions & 12 deletions hub/models/extra_user_detail.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ def save(
self.standardize_json_field('data', 'organization', str)
self.standardize_json_field('data', 'name', str)
if not created:
self._sync_org_name()
self._sync_org_details()

super().save(
force_insert=force_insert,
Expand All @@ -62,22 +62,43 @@ def save(
self.validated_password,
)

def _sync_org_name(self):
def _sync_org_details(self):
"""
Synchronizes the `name` field of the Organization model with the
"organization" attribute found in the `data` field of ExtraUserDetail,
but only if the user is the owner.
Synchronizes the `name`, `organization_type`, and `organization_website` fields
of the Organization model with the corresponding attributes in the `data` field
of ExtraUserDetail. This is performed only if the user is the owner, and their
organization is **not** a multi-member organization.
This ensures that any updates in the metadata are accurately reflected
in the organization's name.
This ensures that metadata updates are accurately reflected before the
organization potentially transitions to a multi-member state.
"""
user_organization = self.user.organization
if user_organization.is_owner(self.user):
if user_organization.is_owner(self.user) and not user_organization.is_mmo:
fields_to_update = []
try:
organization_name = self.data['organization'].strip()
except (KeyError, AttributeError):
organization_name = None
pass
else:
if organization_name:
user_organization.name = organization_name
fields_to_update.append('name')

if organization_name:
user_organization.name = organization_name
user_organization.save(update_fields=['name'])
try:
organization_type = self.data['organization_type'].strip()
except (KeyError, AttributeError):
pass
else:
user_organization.organization_type = organization_type
fields_to_update.append('organization_type')

try:
organization_website = self.data['organization_website'].strip()
except (KeyError, AttributeError):
pass
else:
user_organization.website = organization_website
fields_to_update.append('website')

if fields_to_update:
user_organization.save(update_fields=fields_to_update)
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# Generated by Django 4.2.15 on 2024-11-28 19:58
from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('organizations', '0005_add_mmo_override_field_to_organization'),
]

operations = [
migrations.AddField(
model_name='organization',
name='organization_type',
field=models.CharField(
choices=[
('non-profit', 'Non-profit organization'),
('government', 'Government institution'),
('educational', 'Educational organization'),
('commercial', 'A commercial/for-profit company'),
('none', 'I am not associated with any organization'),
],
default='none',
max_length=20,
),
),
migrations.AddField(
model_name='organization',
name='website',
field=models.CharField(default='', max_length=255),
),
]
Original file line number Diff line number Diff line change
Expand Up @@ -26,17 +26,32 @@ def update_organization_names(apps, schema_editor):
and organization.name.strip() != ''
and organization.name.startswith(user.username)
):
try:
organization_name = user.extra_details.data['organization'].strip()
except (KeyError, AttributeError):
continue

if organization_name:
update = False
if organization_name := user.extra_details.data.get(
'organization', ''
).strip():
update = True
organization.name = organization_name

if organization_website := user.extra_details.data.get(
'organization_website', ''
).strip():
update = True
organization.website = organization_website

if organization_type := user.extra_details.data.get(
'organization_type', ''
).strip():
update = True
organization.organization_type = organization_type

if update:
organizations.append(organization)

if organizations:
Organization.objects.bulk_update(organizations, ['name'])
Organization.objects.bulk_update(
organizations, ['name', 'organization_type', 'website']
)


def noop(apps, schema_editor):
Expand All @@ -46,7 +61,7 @@ def noop(apps, schema_editor):
class Migration(migrations.Migration):

dependencies = [
('organizations', '0005_add_mmo_override_field_to_organization'),
('organizations', '0006_add_organization_type_and_website'),
]

operations = [migrations.RunPython(update_organization_names, noop)]
44 changes: 30 additions & 14 deletions kobo/apps/organizations/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from django.db import models
from django.db.models import F
from django_request_cache import cache_for_request
from django.utils.translation import gettext_lazy as t

if settings.STRIPE_ENABLED:
from djstripe.models import Customer, Subscription
Expand Down Expand Up @@ -37,11 +38,25 @@
]


class OrganizationType(models.TextChoices):
NON_PROFIT = 'non-profit', t('Non-profit organization')
GOVERNMENT = 'government', t('Government institution')
EDUCATIONAL = 'educational', t('Educational organization')
COMMERCIAL = 'commercial', t('A commercial/for-profit company')
NONE = 'none', t('I am not associated with any organization')


class Organization(AbstractOrganization):
id = KpiUidField(uid_prefix='org', primary_key=True)
mmo_override = models.BooleanField(
default=False, verbose_name='Multi-members override'
)
website = models.CharField(default='', max_length=255)
organization_type = models.CharField(
default=OrganizationType.NONE,
max_length=20,
choices=OrganizationType.choices,
)

def add_user(self, user, is_admin=False):
if not self.is_mmo and self.users.all().count():
Expand Down Expand Up @@ -117,8 +132,20 @@ def canceled_subscription_billing_cycle_anchor(self):

return None

@property
def email(self):
"""
As organization is our customer model for Stripe, Stripe requires that
it has an email address attribute.
"""
try:
return self.owner_user_object.email
except AttributeError:
return

@classmethod
def get_from_user_id(cls, user_id: int):
@cache_for_request
def get_from_user_id(cls, user_id: int) -> 'Organization':
"""
Get organization that this user is a member of.
"""
Expand All @@ -133,17 +160,6 @@ def get_from_user_id(cls, user_id: int):

return org

@property
def email(self):
"""
As organization is our customer model for Stripe, Stripe requires that
it has an email address attribute.
"""
try:
return self.owner_user_object.email
except AttributeError:
return

@cache_for_request
def get_user_role(self, user: 'User') -> OrganizationRole:

Expand Down Expand Up @@ -244,8 +260,8 @@ def active_subscription_statuses(self):
@property
def active_subscription_status(self):
"""
Return a comma-separated string of active subscriptions for the organization
user.
Return a comma-separated string of active subscriptions for the
organization user.
"""
return ', '.join(self.active_subscription_statuses)

Expand Down
6 changes: 3 additions & 3 deletions kobo/apps/organizations/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,15 +92,15 @@ class Meta:
fields = [
'id',
'name',
'is_active',
'website',
'organization_type',
'created',
'modified',
'slug',
'is_owner',
'is_mmo',
'request_user_role',
]
read_only_fields = ['id', 'slug']
read_only_fields = ['id']

def create(self, validated_data):
user = self.context['request'].user
Expand Down
65 changes: 52 additions & 13 deletions kobo/apps/organizations/tests/test_organizations.py
Original file line number Diff line number Diff line change
Expand Up @@ -19,26 +19,35 @@ def setUp(self):
# someuser is the only member their organization, and the owner as well.
self.organization = self.someuser.organization
self.organization.mmo_override = True
self.organization.save(update_fields=['mmo_override'])
self.organization.website = 'https://someuser.org/'
self.organization.organization_type = 'non-profit'
self.organization.save()

self.anotheruser = User.objects.get(username='anotheruser')

def test_owner_user_object_property(self):
anotheruser = User.objects.get(username='anotheruser')
self.organization.add_user(anotheruser)
self.organization.add_user(self.anotheruser)
assert self.organization.owner_user_object == self.someuser

def test_get_user_role(self):
anotheruser = User.objects.get(username='anotheruser')
alice = User.objects.create(username='alice', email='alice@alice.com')
external = User.objects.create(
username='external', email='external@external.com'
)
self.organization.add_user(anotheruser, is_admin=True)
self.organization.add_user(self.anotheruser, is_admin=True)
self.organization.add_user(alice)
assert self.organization.get_user_role(self.someuser) == ORG_OWNER_ROLE
assert self.organization.get_user_role(anotheruser) == ORG_ADMIN_ROLE
assert self.organization.get_user_role(self.anotheruser) == ORG_ADMIN_ROLE
assert self.organization.get_user_role(alice) == ORG_MEMBER_ROLE
assert self.organization.get_user_role(external) == ORG_EXTERNAL_ROLE

def test_get_from_user_id(self):
org = Organization.get_from_user_id(self.someuser.pk)
assert org.pk == self.organization.pk

org = Organization.get_from_user_id(self.anotheruser.pk)
assert org.pk != self.organization.pk

def test_create_organization_on_user_creation(self):
assert not Organization.objects.filter(name__startswith='alice').exists()
organization_count = Organization.objects.all().count()
Expand All @@ -48,7 +57,7 @@ def test_create_organization_on_user_creation(self):
assert Organization.objects.filter(name__startswith='alice').exists()
assert Organization.objects.all().count() == organization_count + 1

def test_sync_org_name_on_save(self):
def test_org_attributes_not_synced_with_mmo(self):
"""
Tests the synchronization of the organization name with the metadata in
ExtraUserDetail upon saving.
Expand All @@ -57,25 +66,55 @@ def test_sync_org_name_on_save(self):
organization.
"""
# someuser is the owner

# Empty the name
assert self.organization.name == 'someuser’s organization'
someuser_extra_details = self.someuser.extra_details
someuser_extra_details.data['organization'] = ''
someuser_extra_details.save()
self.organization.refresh_from_db()
assert self.organization.name == 'someuser’s organization'

# Update org settings
someuser_extra_details = self.someuser.extra_details
someuser_extra_details.data['organization'] = 'SomeUser Technologies'
someuser_extra_details.data['organization_website'] = 'https://someuser.com/'
someuser_extra_details.data['organization_type'] = 'commercial'
someuser_extra_details.save()
self.organization.refresh_from_db()
assert self.organization.name == 'SomeUser Technologies'
assert self.organization.name == 'someuser’s organization'
assert self.organization.website == 'https://someuser.org/'
assert self.organization.organization_type == 'non-profit'

# another is an admin
anotheruser = User.objects.get(username='anotheruser')

self.organization.add_user(anotheruser, is_admin=True)
anotheruser_extra_details = anotheruser.extra_details
self.organization.add_user(self.anotheruser, is_admin=True)
anotheruser_extra_details = self.anotheruser.extra_details
anotheruser_extra_details.data['organization'] = 'AnotherUser Enterprises'
someuser_extra_details.data['organization_website'] = 'https://anotheruser.com/'
someuser_extra_details.data['organization_type'] = 'none'
anotheruser_extra_details.save()
self.organization.refresh_from_db()
assert self.organization.name == 'SomeUser Technologies'
assert self.organization.name == 'someuser’s organization'
assert self.organization.website == 'https://someuser.org/'
assert self.organization.organization_type == 'non-profit'

def test_org_attributes_synced_without_mmo(self):

# anotheruser's organization is not mmo
organization = self.anotheruser.organization
assert organization.is_mmo is False
assert organization.website == ''
assert organization.organization_type == 'none'
assert organization.name == 'anotheruser’s organization'

anotheruser_extra_details = self.anotheruser.extra_details
anotheruser_extra_details.data['organization'] = 'AnotherUser Enterprises'
anotheruser_extra_details.data['organization_website'] = (
'https://anotheruser.org/'
)
anotheruser_extra_details.data['organization_type'] = 'commercial'
anotheruser_extra_details.save()
organization.refresh_from_db()
assert organization.name == 'AnotherUser Enterprises'
assert organization.website == 'https://anotheruser.org/'
assert organization.organization_type == 'commercial'
3 changes: 1 addition & 2 deletions kobo/apps/organizations/tests/test_organizations_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ def test_list(self):
def test_api_returns_org_data(self):
self._insert_data()
response = self.client.get(self.url_detail)
self.assertContains(response, self.organization.slug)
self.assertContains(response, self.organization.id)
self.assertContains(response, self.organization.name)

Expand Down Expand Up @@ -539,7 +538,7 @@ def test_can_delete_asset(
('bob', False, False, status.HTTP_200_OK),
)
@unpack
def test_can_archive_or_unarchive(
def test_can_archive_or_unarchive_project(
self,
username: str,
owned_by_org: bool,
Expand Down
Loading

0 comments on commit b5d51b3

Please sign in to comment.