From e0cc6260d060ad54b2b0f63cb5447fa8144f4203 Mon Sep 17 00:00:00 2001 From: ncongthang Date: Mon, 8 Jun 2026 17:05:29 +0700 Subject: [PATCH 1/6] =?UTF-8?q?Ref=202.3.=E6=A9=9F=E9=96=A2=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=83=AC=E3=83=BC=E3=82=B8=E3=82=AF=E3=82=AA=E3=83=BC?= =?UTF-8?q?=E3=82=BF=E5=88=9D=E6=9C=9F=E5=80=A4=E8=A8=AD=E5=AE=9A=E6=A9=9F?= =?UTF-8?q?=E8=83=BD=E3=81=AE=E6=94=B9=E4=BF=AE:=20Add=20enhanced=20defaul?= =?UTF-8?q?t=20max=20quota=20logic=20for=20Institution=20Storage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../views.py | 18 ++++++- admin/quota_recalc/views.py | 8 ++- .../user_list_by_institute.html | 2 +- osf/migrations/0267_auto_20260608_0901.py | 36 +++++++++++++ osf/models/__init__.py | 1 + osf/models/institution_default_max_quota.py | 54 +++++++++++++++++++ website/util/quota.py | 35 +++++++++--- 7 files changed, 144 insertions(+), 10 deletions(-) create mode 100644 osf/migrations/0267_auto_20260608_0901.py create mode 100644 osf/models/institution_default_max_quota.py diff --git a/admin/institutional_storage_quota_control/views.py b/admin/institutional_storage_quota_control/views.py index 7b028da5986..851f5f120f9 100644 --- a/admin/institutional_storage_quota_control/views.py +++ b/admin/institutional_storage_quota_control/views.py @@ -4,13 +4,14 @@ from django.http import Http404 from admin.institutions.views import QuotaUserList -from osf.models import Institution, OSFUser, UserQuota +from osf.models import Institution, OSFUser, UserQuota, InstitutionDefaultMaxQuota from admin.base import settings from addons.osfstorage.models import Region from django.views.generic import ListView, View from django.shortcuts import redirect from admin.rdm.utils import RdmPermissionMixin from django.core.urlresolvers import reverse +from api.base import settings as api_settings class InstitutionStorageList(RdmPermissionMixin, UserPassesTestMixin, ListView): @@ -122,6 +123,17 @@ def get_institution(self): raise Http404 return institution + def get_default_max_quota(self): + """ Get default max quota for the institution """ + default_max_quota = InstitutionDefaultMaxQuota.get_quota_by_institution(self.institution_id) + return default_max_quota if default_max_quota is not None else api_settings.DEFAULT_MAX_QUOTA + + def get_context_data(self, **kwargs): + """ Add default_max_quota to template context """ + context = super().get_context_data(**kwargs) + context['default_max_quota'] = self.get_default_max_quota() + return context + class UpdateQuotaUserListByInstitutionStorageID(RdmPermissionMixin, UserPassesTestMixin, View): """ Change max quota for an institution's users if that institution is not using NII Storage. """ @@ -163,6 +175,10 @@ def post(self, request, *args, **kwargs): min_value, max_value = connection.ops.integer_field_range('PositiveIntegerField') if min_value <= max_quota <= max_value: # If max quota value is between 0 and 2147483647, update or create used quota for each user in the institution + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution_id, + defaults={'default_max_quota': max_quota} + ) for user in OSFUser.objects.filter( affiliated_institutions=self.institution_id): try: diff --git a/admin/quota_recalc/views.py b/admin/quota_recalc/views.py index 29484c3d559..c40a17315b0 100644 --- a/admin/quota_recalc/views.py +++ b/admin/quota_recalc/views.py @@ -4,6 +4,7 @@ from addons.osfstorage.models import Region from api.base import settings as api_settings from osf.models import OSFUser, UserQuota, Node +from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota from osf.models.node import set_project_storage_type from osf.utils.requests import check_select_for_update from website.util.quota import used_quota @@ -37,12 +38,17 @@ def calculate_quota(user): user_quota.used = used user_quota.save() except UserQuota.DoesNotExist: + max_quota = api_settings.DEFAULT_MAX_QUOTA + if storage_type == UserQuota.CUSTOM_STORAGE: + max_default_quota = InstitutionDefaultMaxQuota.get_quota_by_user(user.id) + if max_default_quota: + max_quota = max_default_quota try: with transaction.atomic(): UserQuota.objects.create( user=user, storage_type=storage_type, - max_quota=api_settings.DEFAULT_MAX_QUOTA, + max_quota=max_quota, used=used, ) except IntegrityError: diff --git a/admin/templates/institutional_storage_quota_control/user_list_by_institute.html b/admin/templates/institutional_storage_quota_control/user_list_by_institute.html index 2e2050514b0..b5b25db61b6 100644 --- a/admin/templates/institutional_storage_quota_control/user_list_by_institute.html +++ b/admin/templates/institutional_storage_quota_control/user_list_by_institute.html @@ -121,7 +121,7 @@

{% trans "Institutional Storage" %} > {{ institution_name }}

Date: Fri, 12 Jun 2026 08:28:10 +0700 Subject: [PATCH 2/6] =?UTF-8?q?Ref=202.3.=E6=A9=9F=E9=96=A2=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=83=AC=E3=83=BC=E3=82=B8=E3=82=AF=E3=82=AA=E3=83=BC?= =?UTF-8?q?=E3=82=BF=E5=88=9D=E6=9C=9F=E5=80=A4=E8=A8=AD=E5=AE=9A=E6=A9=9F?= =?UTF-8?q?=E8=83=BD=E3=81=AE=E6=94=B9=E4=BF=AE:=20Rework=20the=20review?= =?UTF-8?q?=20comment=20+=20Fix=20institution=20default=20max=20quota=20is?= =?UTF-8?q?sues=20(owner=20lookup,=20zero=20quota,=20quota=20retrieval,=20?= =?UTF-8?q?transaction=20handling)=20+=20Refactor=20quota=20handling=20log?= =?UTF-8?q?ic=20+=20Update=20migration=20(use=20OneToOneField=20and=20remo?= =?UTF-8?q?ve=20db=5Findex)=20+=20Update=20default=20value=20in=20"Institu?= =?UTF-8?q?tional=20Storage=20Quota=20Control"=20screen?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../views.py | 31 ++++++----- admin/quota_recalc/views.py | 10 +--- .../user_list_by_institute.html | 2 +- osf/migrations/0267_auto_20260608_0901.py | 8 +-- osf/models/institution_default_max_quota.py | 39 +++----------- website/util/quota.py | 51 ++++++++++--------- 6 files changed, 58 insertions(+), 83 deletions(-) diff --git a/admin/institutional_storage_quota_control/views.py b/admin/institutional_storage_quota_control/views.py index 851f5f120f9..d9b01b94d6f 100644 --- a/admin/institutional_storage_quota_control/views.py +++ b/admin/institutional_storage_quota_control/views.py @@ -124,9 +124,14 @@ def get_institution(self): return institution def get_default_max_quota(self): - """ Get default max quota for the institution """ - default_max_quota = InstitutionDefaultMaxQuota.get_quota_by_institution(self.institution_id) - return default_max_quota if default_max_quota is not None else api_settings.DEFAULT_MAX_QUOTA + """Get default max quota for the institution, fallback to DEFAULT_MAX_QUOTA when not found.""" + try: + institution_default_max_quota = InstitutionDefaultMaxQuota.objects.get( + institution_id=self.institution_id + ) + return institution_default_max_quota.default_max_quota + except InstitutionDefaultMaxQuota.DoesNotExist: + return api_settings.DEFAULT_MAX_QUOTA def get_context_data(self, **kwargs): """ Add default_max_quota to template context """ @@ -175,21 +180,21 @@ def post(self, request, *args, **kwargs): min_value, max_value = connection.ops.integer_field_range('PositiveIntegerField') if min_value <= max_quota <= max_value: # If max quota value is between 0 and 2147483647, update or create used quota for each user in the institution - InstitutionDefaultMaxQuota.objects.update_or_create( - institution_id=self.institution_id, - defaults={'default_max_quota': max_quota} - ) - for user in OSFUser.objects.filter( - affiliated_institutions=self.institution_id): - try: - with transaction.atomic(): + with transaction.atomic(): + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution_id, + defaults={'default_max_quota': max_quota} + ) + for user in OSFUser.objects.filter( + affiliated_institutions=self.institution_id): + try: UserQuota.objects.update_or_create( user=user, storage_type=UserQuota.CUSTOM_STORAGE, defaults={'max_quota': max_quota} ) - except IntegrityError: - UserQuota.objects.filter(user=user, storage_type=UserQuota.CUSTOM_STORAGE).update(max_quota=max_quota) + except IntegrityError: + UserQuota.objects.filter(user=user, storage_type=UserQuota.CUSTOM_STORAGE).update(max_quota=max_quota) return redirect( 'institutional_storage_quota_control:institution_user_list', institution_id=self.institution_id diff --git a/admin/quota_recalc/views.py b/admin/quota_recalc/views.py index c40a17315b0..4c79508d039 100644 --- a/admin/quota_recalc/views.py +++ b/admin/quota_recalc/views.py @@ -2,12 +2,10 @@ from django.db import transaction, IntegrityError from addons.osfstorage.models import Region -from api.base import settings as api_settings from osf.models import OSFUser, UserQuota, Node -from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota from osf.models.node import set_project_storage_type from osf.utils.requests import check_select_for_update -from website.util.quota import used_quota +from website.util.quota import get_default_max_quota, used_quota def calculate_quota(user): @@ -24,6 +22,7 @@ def calculate_quota(user): with transaction.atomic(): for storage_type in storage_type_list: used = used_quota(user._id, storage_type) + max_quota = get_default_max_quota(user, storage_type) try: if check_select_for_update(): user_quota = UserQuota.objects.filter( @@ -38,11 +37,6 @@ def calculate_quota(user): user_quota.used = used user_quota.save() except UserQuota.DoesNotExist: - max_quota = api_settings.DEFAULT_MAX_QUOTA - if storage_type == UserQuota.CUSTOM_STORAGE: - max_default_quota = InstitutionDefaultMaxQuota.get_quota_by_user(user.id) - if max_default_quota: - max_quota = max_default_quota try: with transaction.atomic(): UserQuota.objects.create( diff --git a/admin/templates/institutional_storage_quota_control/user_list_by_institute.html b/admin/templates/institutional_storage_quota_control/user_list_by_institute.html index b5b25db61b6..3d747299e1c 100644 --- a/admin/templates/institutional_storage_quota_control/user_list_by_institute.html +++ b/admin/templates/institutional_storage_quota_control/user_list_by_institute.html @@ -121,7 +121,7 @@

{% trans "Institutional Storage" %} > {{ institution_name }}

Date: Fri, 12 Jun 2026 17:11:42 +0700 Subject: [PATCH 3/6] =?UTF-8?q?Ref=202.3.=E6=A9=9F=E9=96=A2=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=83=AC=E3=83=BC=E3=82=B8=E3=82=AF=E3=82=AA=E3=83=BC?= =?UTF-8?q?=E3=82=BF=E5=88=9D=E6=9C=9F=E5=80=A4=E8=A8=AD=E5=AE=9A=E6=A9=9F?= =?UTF-8?q?=E8=83=BD=E3=81=AE=E6=94=B9=E4=BF=AE:=20Add=20UT=20for=20enhanc?= =?UTF-8?q?ed=20default=20max=20quota=20logic=20in=20Institution=20Storage?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../test_views.py | 202 ++++- admin_tests/quota_recalc/test_views.py | 42 +- tests/test_quota.py | 720 +++++++++++++++++- tests/tests_instutition_default_max_quota.py | 54 ++ 4 files changed, 1004 insertions(+), 14 deletions(-) create mode 100644 tests/tests_instutition_default_max_quota.py diff --git a/admin_tests/institutional_storage_quota_control/test_views.py b/admin_tests/institutional_storage_quota_control/test_views.py index e5996209d36..3b8b610bbfc 100644 --- a/admin_tests/institutional_storage_quota_control/test_views.py +++ b/admin_tests/institutional_storage_quota_control/test_views.py @@ -253,6 +253,108 @@ def test_post__max_quota_too_large(self): nt.assert_is_not_none(new_user_quota) nt.assert_equal(new_user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + def test_post__institution_default_max_quota_created(self): + """Test that InstitutionDefaultMaxQuota is created/updated on POST""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + + new_max_quota = 200 + region = RegionFactory(_id=self.institution01._id) + region.waterbutler_settings['storage']['type'] = Region.INSTITUTIONS + region.save() + + request = RequestFactory().post( + reverse(self.view_name, + kwargs={'institution_id': self.institution01.id}), + {'maxQuota': new_max_quota}) + request.user = self.superuser + response = self.view(request, institution_id=self.institution01.id) + + nt.assert_equal(response.status_code, 302) + + # Verify InstitutionDefaultMaxQuota record was created + quota = InstitutionDefaultMaxQuota.objects.filter( + institution_id=self.institution01.id + ).first() + nt.assert_is_not_none(quota) + nt.assert_equal(quota.default_max_quota, new_max_quota) + + def test_post__institution_default_max_quota_updated(self): + """Test that InstitutionDefaultMaxQuota is updated on subsequent POST""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + + # Create initial record + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution01.id, + defaults={'default_max_quota': 100} + ) + + # Update via POST + new_max_quota = 250 + region = RegionFactory(_id=self.institution01._id) + region.waterbutler_settings['storage']['type'] = Region.INSTITUTIONS + region.save() + + request = RequestFactory().post( + reverse(self.view_name, + kwargs={'institution_id': self.institution01.id}), + {'maxQuota': new_max_quota}) + request.user = self.superuser + response = self.view(request, institution_id=self.institution01.id) + + nt.assert_equal(response.status_code, 302) + + # Verify InstitutionDefaultMaxQuota record was updated + quota = InstitutionDefaultMaxQuota.objects.filter( + institution_id=self.institution01.id + ).first() + nt.assert_is_not_none(quota) + nt.assert_equal(quota.default_max_quota, new_max_quota) + + # Verify only one record exists for this institution + count = InstitutionDefaultMaxQuota.objects.filter( + institution_id=self.institution01.id + ).count() + nt.assert_equal(count, 1) + + def test_post__integrity_error_handles_fallback_update(self): + """Test that IntegrityError in update_or_create triggers fallback update logic""" + from unittest import mock + + new_max_quota = 300 + region = RegionFactory(_id=self.institution01.guid) + region.waterbutler_settings['storage']['type'] = Region.INSTITUTIONS + region.save() + + # Create initial UserQuota + initial_quota = UserQuota.objects.create( + user=self.institution01_admin, + storage_type=UserQuota.CUSTOM_STORAGE, + max_quota=100 + ) + + # Mock update_or_create to raise IntegrityError, simulating race condition + with mock.patch.object(UserQuota.objects, 'update_or_create') as mock_update: + from django.db import IntegrityError + mock_update.side_effect = IntegrityError("Simulated race condition") + + request = RequestFactory().post( + reverse(self.view_name, + kwargs={'institution_id': self.institution01.id}), + {'maxQuota': new_max_quota}) + request.user = self.institution01_admin + response = self.view(request, institution_id=self.institution01.id) + + # Verify response is redirect + nt.assert_equal(response.status_code, 302) + + # Verify max_quota was updated via fallback logic + updated_quota = UserQuota.objects.filter( + user=self.institution01_admin, storage_type=UserQuota.CUSTOM_STORAGE + ).first() + nt.assert_is_not_none(updated_quota) + nt.assert_equal(updated_quota.max_quota, new_max_quota) + nt.assert_equal(updated_quota.id, initial_quota.id) + class TestUserListByInstitutionStorageID(AdminTestCase): def setUp(self): @@ -261,7 +363,11 @@ def setUp(self): self.institution02 = InstitutionFactory(name='inst02') self.region01 = RegionFactory(_id=self.institution01._id, name='Storage 01') + self.region01.waterbutler_settings['storage']['type'] = Region.INSTITUTIONS + self.region01.save() self.region02 = RegionFactory(_id=self.institution02._id, name='Storage 02') + self.region02.waterbutler_settings['storage']['type'] = Region.INSTITUTIONS + self.region02.save() self.anon = AnonymousUser() self.user = AuthUserFactory(fullname='user') @@ -441,18 +547,17 @@ def test_get_institution__not_found(self): view.get_institution() # Institution use NII Storage - self.institution01._id = '' - self.institution01.save() + nii_institution = InstitutionFactory(_id='', name='nii_inst') request = RequestFactory().get( reverse( self.view_name, - kwargs={'institution_id': self.institution01.id} + kwargs={'institution_id': nii_institution.id} ) ) request.user = self.superuser view = setup_view(self.view_instance, request, - institution_id=self.institution01.id) + institution_id=nii_institution.id) with nt.assert_raises(Http404): view.get_institution() @@ -476,6 +581,95 @@ def test__institution_id_not_valid(self): ) ) + def test_get_default_max_quota__institution_has_default(self): + """Test get_default_max_quota returns value from DB when record exists""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + + # Create default max quota for institution + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution01.id, + defaults={'default_max_quota': 500} + ) + + request = RequestFactory().get( + reverse( + self.view_name, + kwargs={'institution_id': self.institution01.id} + ) + ) + request.user = self.institution01_admin + + view = setup_view(self.view_instance, request, + institution_id=self.institution01.id) + view.institution_id = self.institution01.id + default_max_quota = view.get_default_max_quota() + + nt.assert_equal(default_max_quota, 500) + + def test_get_default_max_quota__institution_no_default(self): + """Test get_default_max_quota returns DEFAULT_MAX_QUOTA when record doesn't exist""" + request = RequestFactory().get( + reverse( + self.view_name, + kwargs={'institution_id': self.institution01.id} + ) + ) + request.user = self.institution01_admin + + view = setup_view(self.view_instance, request, + institution_id=self.institution01.id) + view.institution_id = self.institution01.id + default_max_quota = view.get_default_max_quota() + + nt.assert_equal(default_max_quota, api_settings.DEFAULT_MAX_QUOTA) + + def test_get_context_data__includes_default_max_quota(self): + """Test get_context_data includes default_max_quota in context""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + + # Create default max quota for institution + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution01.id, + defaults={'default_max_quota': 300} + ) + + request = RequestFactory().get( + reverse( + self.view_name, + kwargs={'institution_id': self.institution01.id} + ) + ) + request.user = self.institution01_admin + + view = setup_view(self.view_instance, request, + institution_id=self.institution01.id) + view.institution_id = self.institution01.id + view.object_list = view.get_queryset() + context = view.get_context_data() + + nt.assert_in('default_max_quota', context) + nt.assert_equal(context['default_max_quota'], 300) + + def test_get_context_data__default_max_quota_fallback(self): + """Test get_context_data uses DEFAULT_MAX_QUOTA as fallback""" + request = RequestFactory().get( + reverse( + self.view_name, + kwargs={'institution_id': self.institution01.id} + ) + ) + request.user = self.institution01_admin + + view = setup_view(self.view_instance, request, + institution_id=self.institution01.id) + view.institution_id = self.institution01.id + view.object_list = view.get_queryset() + context = view.get_context_data() + + nt.assert_in('default_max_quota', context) + nt.assert_equal(context['default_max_quota'], api_settings.DEFAULT_MAX_QUOTA) + + class TestAccessInstitutionStorageList(AdminTestCase): def setUp(self): super(TestAccessInstitutionStorageList, self).setUp() diff --git a/admin_tests/quota_recalc/test_views.py b/admin_tests/quota_recalc/test_views.py index cbaa876e015..03c1dc9e862 100644 --- a/admin_tests/quota_recalc/test_views.py +++ b/admin_tests/quota_recalc/test_views.py @@ -16,9 +16,11 @@ class TestQuotaRecalcView(AdminTestCase): def get_request(view, **kwargs): return view(RequestFactory().get('/fake_path'), **kwargs) + @mock.patch('admin.quota_recalc.views.get_default_max_quota') @mock.patch('admin.quota_recalc.views.used_quota') - def test_user_create_userquota_record(self, mock_usedquota): + def test_user_create_userquota_record(self, mock_usedquota, mock_get_default_max_quota): mock_usedquota.return_value = 1500 + mock_get_default_max_quota.return_value = api_settings.DEFAULT_MAX_QUOTA user = AuthUserFactory() UserQuota.objects.filter(user=user).delete() @@ -31,9 +33,11 @@ def test_user_create_userquota_record(self, mock_usedquota): nt.assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) nt.assert_equal(user_quota.used, 1500) + @mock.patch('admin.quota_recalc.views.get_default_max_quota') @mock.patch('admin.quota_recalc.views.used_quota') - def test_user_update_userquota_record(self, mock_usedquota): + def test_user_update_userquota_record(self, mock_usedquota, mock_get_default_max_quota): mock_usedquota.return_value = 7000 + mock_get_default_max_quota.return_value = 200 user = AuthUserFactory() UserQuota.objects.create( @@ -51,9 +55,11 @@ def test_user_update_userquota_record(self, mock_usedquota): nt.assert_equal(user_quota.max_quota, 200) nt.assert_equal(user_quota.used, 7000) + @mock.patch('admin.quota_recalc.views.get_default_max_quota') @mock.patch('admin.quota_recalc.views.used_quota') - def test_user_invalid_guid(self, mock_usedquota): + def test_user_invalid_guid(self, mock_usedquota, mock_get_default_max_quota): mock_usedquota.return_value = 3000 + mock_get_default_max_quota.return_value = api_settings.DEFAULT_MAX_QUOTA response = self.get_request(views.user, guid='cuzidontcare') res_json = json.loads(response.content) @@ -61,9 +67,11 @@ def test_user_invalid_guid(self, mock_usedquota): nt.assert_equal(res_json['status'], 'failed') nt.assert_equal(res_json['message'], 'User not found.') + @mock.patch('admin.quota_recalc.views.get_default_max_quota') @mock.patch('admin.quota_recalc.views.used_quota') - def test_users_create_userquota_record(self, mock_usedquota): + def test_users_create_userquota_record(self, mock_usedquota, mock_get_default_max_quota): mock_usedquota.return_value = 1500 + mock_get_default_max_quota.return_value = api_settings.DEFAULT_MAX_QUOTA user = AuthUserFactory() user2 = AuthUserFactory() UserQuota.objects.filter(user=user).delete() @@ -94,19 +102,24 @@ def setUp(self): super(TestCalculateQuota, self).setUp() self.user = AuthUserFactory() + @mock.patch('admin.quota_recalc.views.get_default_max_quota') @mock.patch('admin.quota_recalc.views.used_quota') - def test_user_without_institution(self, mock_usedquota): + def test_user_without_institution(self, mock_usedquota, mock_get_default_max_quota): mock_usedquota.return_value = 5000 + mock_get_default_max_quota.return_value = 1000 views.calculate_quota(self.user) user_quota = UserQuota.objects.filter(user=self.user).all() nt.assert_equal(len(user_quota), 1) nt.assert_equal(user_quota[0].used, 5000) + nt.assert_equal(user_quota[0].max_quota, 1000) + @mock.patch('admin.quota_recalc.views.get_default_max_quota') @mock.patch('admin.quota_recalc.views.used_quota') - def test_user_institution_without_custom_storage(self, mock_usedquota): + def test_user_institution_without_custom_storage(self, mock_usedquota, mock_get_default_max_quota): mock_usedquota.return_value = 6000 + mock_get_default_max_quota.return_value = 2000 institution = InstitutionFactory() self.user.affiliated_institutions.add(institution) @@ -116,12 +129,17 @@ def test_user_institution_without_custom_storage(self, mock_usedquota): user_quota = UserQuota.objects.filter(user=self.user).all() nt.assert_equal(len(user_quota), 1) nt.assert_equal(user_quota[0].used, 6000) + nt.assert_equal(user_quota[0].max_quota, 2000) + @mock.patch('admin.quota_recalc.views.get_default_max_quota') @mock.patch('admin.quota_recalc.views.used_quota') - def test_user_institution_with_custom_storage(self, mock_usedquota): + def test_user_institution_with_custom_storage(self, mock_usedquota, mock_get_default_max_quota): + mock_usedquota.side_effect = \ lambda uid, storage_type: 300 if storage_type == UserQuota.NII_STORAGE else 7000 + mock_get_default_max_quota.side_effect = \ + lambda _, storage_type: 1000 if storage_type == UserQuota.NII_STORAGE else 2000 institution = InstitutionFactory() self.user.affiliated_institutions.add(institution) RegionFactory(_id=institution._id) @@ -136,5 +154,11 @@ def test_user_institution_with_custom_storage(self, mock_usedquota): UserQuota.CUSTOM_STORAGE: 7000, } - nt.assert_equal(user_quota[0].used, expected[user_quota[0].storage_type]) - nt.assert_equal(user_quota[1].used, expected[user_quota[1].storage_type]) + expected_max = { + UserQuota.NII_STORAGE: 1000, + UserQuota.CUSTOM_STORAGE: 2000, + } + + for uq in user_quota: + nt.assert_equal(uq.used, expected[uq.storage_type]) + nt.assert_equal(uq.max_quota, expected_max[uq.storage_type]) diff --git a/tests/test_quota.py b/tests/test_quota.py index 2fc183213d0..69af0f1022b 100644 --- a/tests/test_quota.py +++ b/tests/test_quota.py @@ -4,7 +4,7 @@ from nose.tools import * # noqa (PEP8 asserts) import pytest -from addons.osfstorage.models import OsfStorageFileNode +from addons.osfstorage.models import OsfStorageFileNode, Region from api.base import settings as api_settings from framework.auth import signing from tests.base import OsfTestCase @@ -16,6 +16,7 @@ ) from osf.utils.requests import check_select_for_update from website.util import web_url_for, quota +from django.db import IntegrityError @pytest.mark.enable_implicit_clean @@ -1532,6 +1533,36 @@ def test_update_user_used_quota_method_for_recalculate_quota_process__nii_custom user_quota = user_quota.first() assert_equal(user_quota.used, 1000) + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + @mock.patch('website.util.quota.used_quota') + def test_update_user_used_quota_without_select_for_update_existing(self, mock_used, _mock_check): + """update_user_used_quota should work without select_for_update when UserQuota exists""" + mock_used.return_value = 750 + + quota.update_user_used_quota( + user=self.user, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota = UserQuota.objects.get(user=self.user, storage_type=UserQuota.NII_STORAGE) + assert_equal(user_quota.used, 750) + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + @mock.patch('website.util.quota.used_quota') + def test_update_user_used_quota_without_select_for_update_create(self, mock_used, _mock_check): + """update_user_used_quota should work without select_for_update when creating new UserQuota""" + another_user = UserFactory() + mock_used.return_value = 600 + + quota.update_user_used_quota( + user=another_user, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota = UserQuota.objects.get(user=another_user, storage_type=UserQuota.NII_STORAGE) + assert_equal(user_quota.used, 600) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + class TestQuotaApiWaterbutler(OsfTestCase): def setUp(self): @@ -1661,3 +1692,690 @@ def test_used_half_custom_institution_quota(self): assert_equal(response.status_code, 200) assert_equal(response.json['max'], 200 * api_settings.SIZE_UNIT_GB) assert_equal(response.json['used'], 100 * api_settings.SIZE_UNIT_GB) + + +@pytest.mark.enable_implicit_clean +class TestGetQuotaInfoWithInstitutionDefaultMaxQuota(OsfTestCase): + """Test get_quota_info() with InstitutionDefaultMaxQuota logic""" + + def setUp(self): + super(TestGetQuotaInfoWithInstitutionDefaultMaxQuota, self).setUp() + self.user = AuthUserFactory() + self.institution = InstitutionFactory(name='Test Institution') + self.user.affiliated_institutions.add(self.institution) + + @mock.patch('website.util.quota.used_quota') + def test_get_quota_info_nii_storage_uses_default(self, mock_used_quota): + """NII_STORAGE should always use DEFAULT_MAX_QUOTA, ignoring InstitutionDefaultMaxQuota""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + mock_used_quota.return_value = 50 + + # Set institution quota + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution.id, + defaults={'default_max_quota': 500} + ) + + # NII_STORAGE should ignore institution quota + max_quota, used = quota.get_quota_info(self.user, UserQuota.NII_STORAGE) + + assert_equal(max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(used, 50) + + @mock.patch('website.util.quota.used_quota') + def test_get_quota_info_custom_storage_with_institution_quota(self, mock_used_quota): + """CUSTOM_STORAGE should use InstitutionDefaultMaxQuota when available""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + mock_used_quota.return_value = 100 + + # Set institution quota + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution.id, + defaults={'default_max_quota': 350} + ) + + # CUSTOM_STORAGE should use institution quota + max_quota, used = quota.get_quota_info(self.user, UserQuota.CUSTOM_STORAGE) + + assert_equal(max_quota, 350) + assert_equal(used, 100) + + @mock.patch('website.util.quota.used_quota') + def test_get_quota_info_custom_storage_fallback_to_default(self, mock_used_quota): + """CUSTOM_STORAGE should fallback to DEFAULT_MAX_QUOTA when no InstitutionDefaultMaxQuota""" + mock_used_quota.return_value = 75 + + # No InstitutionDefaultMaxQuota set + max_quota, used = quota.get_quota_info(self.user, UserQuota.CUSTOM_STORAGE) + + assert_equal(max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(used, 75) + + @mock.patch('website.util.quota.used_quota') + def test_get_quota_info_with_existing_userquota(self, mock_used_quota): + """get_quota_info should return existing UserQuota, not use InstitutionDefaultMaxQuota""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + mock_used_quota.return_value = 0 + + # Create UserQuota with custom max_quota + UserQuota.objects.create( + user=self.user, + storage_type=UserQuota.CUSTOM_STORAGE, + max_quota=600, + used=200 + ) + + # Set institution quota (should be ignored) + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution.id, + defaults={'default_max_quota': 350} + ) + + # Should return existing UserQuota, not institution quota + max_quota, used = quota.get_quota_info(self.user, UserQuota.CUSTOM_STORAGE) + + assert_equal(max_quota, 600) + assert_equal(used, 200) + + +@pytest.mark.enable_implicit_clean +class TestFileAddedWithInstitutionDefaultMaxQuota(OsfTestCase): + """Test file_added() with InstitutionDefaultMaxQuota logic""" + + def setUp(self): + super(TestFileAddedWithInstitutionDefaultMaxQuota, self).setUp() + self.user = AuthUserFactory() + self.institution = InstitutionFactory(name='Test Institution') + self.region = RegionFactory(_id=self.institution._id) + self.region.waterbutler_settings['storage']['type'] = Region.INSTITUTIONS + self.region.save() + self.user.affiliated_institutions.add(self.institution) + self.project = ProjectFactory(creator=self.user, is_deleted=False) + self.file_node = mock.MagicMock() + + def tearDown(self): + super(TestFileAddedWithInstitutionDefaultMaxQuota, self).tearDown() + if hasattr(self, 'project') and self.project: + self.project.delete() + + @mock.patch('osf.models.FileInfo.objects.create') + @mock.patch('website.util.quota.used_quota') + def test_file_added_custom_storage_with_institution_quota(self, mock_used_quota, mock_create): + """file_added should create UserQuota with InstitutionDefaultMaxQuota value""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + mock_used_quota.return_value = 0 + + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution.id, + defaults={'default_max_quota': 400} + ) + + payload = {'metadata': {'size': '100'}} + quota.file_added(self.project, payload, self.file_node, UserQuota.CUSTOM_STORAGE) + + user_quota = UserQuota.objects.get( + user=self.project.creator, + storage_type=UserQuota.CUSTOM_STORAGE + ) + assert_equal(user_quota.max_quota, 400) + assert_equal(user_quota.used, 100) + + @mock.patch('osf.models.FileInfo.objects.create') + @mock.patch('website.util.quota.used_quota') + def test_file_added_custom_storage_without_institution_quota(self, mock_used_quota, mock_create): + """file_added should fallback to DEFAULT_MAX_QUOTA when no InstitutionDefaultMaxQuota""" + mock_used_quota.return_value = 0 + + # No InstitutionDefaultMaxQuota + payload = {'metadata': {'size': '150'}} + quota.file_added(self.project, payload, self.file_node, UserQuota.CUSTOM_STORAGE) + + user_quota = UserQuota.objects.get( + user=self.project.creator, + storage_type=UserQuota.CUSTOM_STORAGE + ) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 150) + + @mock.patch('osf.models.FileInfo.objects.create') + @mock.patch('website.util.quota.used_quota') + def test_file_added_nii_storage_ignores_institution_quota(self, mock_used_quota, mock_create): + """file_added should use DEFAULT_MAX_QUOTA for NII_STORAGE, ignoring InstitutionDefaultMaxQuota""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + mock_used_quota.return_value = 0 + + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution.id, + defaults={'default_max_quota': 400} + ) + + payload = {'metadata': {'size': '75'}} + quota.file_added(self.project, payload, self.file_node, UserQuota.NII_STORAGE) + + user_quota = UserQuota.objects.get( + user=self.project.creator, + storage_type=UserQuota.NII_STORAGE + ) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 75) + + +@pytest.mark.enable_implicit_clean +class TestUpdateUserUsedQuotaWithInstitutionDefaultMaxQuota(OsfTestCase): + """Test update_user_used_quota() with InstitutionDefaultMaxQuota logic""" + + def setUp(self): + super(TestUpdateUserUsedQuotaWithInstitutionDefaultMaxQuota, self).setUp() + self.user = UserFactory() + self.user.save() + self.institution = InstitutionFactory(name='Test Institution') + self.user.affiliated_institutions.add(self.institution) + + @mock.patch('website.util.quota.used_quota') + def test_update_user_used_quota_create_custom_storage_with_institution_quota(self, mock_used_quota): + """update_user_used_quota should create UserQuota with InstitutionDefaultMaxQuota value""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + mock_used_quota.return_value = 300 + + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution.id, + defaults={'default_max_quota': 500} + ) + + quota.update_user_used_quota(self.user, UserQuota.CUSTOM_STORAGE) + + user_quota = UserQuota.objects.get( + user=self.user, + storage_type=UserQuota.CUSTOM_STORAGE + ) + assert_equal(user_quota.max_quota, 500) + assert_equal(user_quota.used, 300) + + @mock.patch('website.util.quota.used_quota') + def test_update_user_used_quota_create_custom_storage_without_institution_quota(self, mock_used_quota): + """update_user_used_quota should fallback to DEFAULT_MAX_QUOTA when no InstitutionDefaultMaxQuota""" + mock_used_quota.return_value = 250 + + # No InstitutionDefaultMaxQuota + quota.update_user_used_quota(self.user, UserQuota.CUSTOM_STORAGE) + + user_quota = UserQuota.objects.get( + user=self.user, + storage_type=UserQuota.CUSTOM_STORAGE + ) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 250) + + @mock.patch('website.util.quota.used_quota') + def test_update_user_used_quota_nii_storage_ignores_institution_quota(self, mock_used_quota): + """update_user_used_quota should use DEFAULT_MAX_QUOTA for NII_STORAGE""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + mock_used_quota.return_value = 200 + + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=self.institution.id, + defaults={'default_max_quota': 500} + ) + + quota.update_user_used_quota(self.user, UserQuota.NII_STORAGE) + + user_quota = UserQuota.objects.get( + user=self.user, + storage_type=UserQuota.NII_STORAGE + ) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 200) + + +@pytest.mark.enable_implicit_clean +class TestFileAddedWithoutSelectForUpdate(OsfTestCase): + """Test file_added() without select_for_update branch""" + + def setUp(self): + super(TestFileAddedWithoutSelectForUpdate, self).setUp() + self.user = UserFactory() + self.project_creator = UserFactory() + self.node = ProjectFactory(creator=self.project_creator) + self.file = OsfStorageFileNode.create( + target=self.node, + path='/testfile', + _id='testfile', + name='testfile', + materialized_path='/testfile' + ) + self.file.save() + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + def test_file_added_without_select_for_update_new_userquota(self, _mock_check): + """file_added should work without select_for_update when creating new UserQuota""" + assert_false(UserQuota.objects.filter(user=self.project_creator).exists()) + + payload = {'metadata': {'size': 1000}} + quota.file_added(self.node, payload, self.file, UserQuota.NII_STORAGE) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(user_quota.used, 1000) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + def test_file_added_without_select_for_update_existing_userquota(self, _mock_check): + """file_added should work without select_for_update when updating existing UserQuota""" + UserQuota.objects.create( + user=self.project_creator, + storage_type=UserQuota.NII_STORAGE, + max_quota=500, + used=2000 + ) + + payload = {'metadata': {'size': 500}} + quota.file_added(self.node, payload, self.file, UserQuota.NII_STORAGE) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(user_quota.used, 2500) + assert_equal(user_quota.max_quota, 500) + + +@pytest.mark.enable_implicit_clean +class TestNodeRemovedWithoutSelectForUpdate(OsfTestCase): + """Test node_removed() without select_for_update branch""" + + def setUp(self): + super(TestNodeRemovedWithoutSelectForUpdate, self).setUp() + self.user = UserFactory() + self.project_creator = UserFactory() + self.node = ProjectFactory(creator=self.project_creator) + self.file = OsfStorageFileNode.create( + target=self.node, + path='/testfile', + _id='testfile', + name='testfile', + materialized_path='/testfile' + ) + self.file.save() + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + def test_node_removed_without_select_for_update(self, _mock_check): + """node_removed should work without select_for_update when deleting file""" + UserQuota.objects.create( + user=self.project_creator, + storage_type=UserQuota.NII_STORAGE, + max_quota=api_settings.DEFAULT_MAX_QUOTA, + used=1500 + ) + FileInfo.objects.create(file=self.file, file_size=1000) + + self.file.deleted_on = datetime.datetime.now() + self.file.deleted_by = self.user + self.file.type = 'osf.trashedfile' + self.file.save() + + quota.node_removed(self.node, self.user, {}, self.file, UserQuota.NII_STORAGE) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(user_quota.used, 500) + + +@pytest.mark.enable_implicit_clean +class TestUserQuotaGetOrCreate(OsfTestCase): + """Test get_or_create logic in file_modified() function""" + + def setUp(self): + super(TestUserQuotaGetOrCreate, self).setUp() + self.user = UserFactory() + self.project_creator = UserFactory() + self.node = ProjectFactory(creator=self.project_creator) + self.file = OsfStorageFileNode.create( + target=self.node, + path='/testfile', + _id='testfile', + name='testfile', + materialized_path='/testfile' + ) + self.file.save() + + def test_get_or_create_creates_new_userquota(self): + """get_or_create should create new UserQuota if it doesn't exist""" + assert_false(UserQuota.objects.filter(user=self.project_creator).exists()) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 5000 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota = UserQuota.objects.filter(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(len(user_quota), 1) + user_quota = user_quota[0] + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 5000) + + def test_get_or_create_retrieves_existing_userquota(self): + """get_or_create should retrieve existing UserQuota without creating a new one""" + UserQuota.objects.create( + user=self.project_creator, + storage_type=UserQuota.NII_STORAGE, + max_quota=200, + used=100 + ) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 200 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota_list = UserQuota.objects.filter(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(len(user_quota_list), 1) + user_quota = user_quota_list[0] + assert_equal(user_quota.max_quota, 200) + assert_equal(user_quota.used, 300) + + def test_get_or_create_with_custom_storage_new_userquota(self): + """get_or_create should work correctly with CUSTOM_STORAGE""" + institution = InstitutionFactory() + self.project_creator.affiliated_institutions.add(institution) + RegionFactory(_id=institution._id) + ProjectStorageType.objects.filter(node=self.node).update( + storage_type=ProjectStorageType.CUSTOM_STORAGE + ) + + assert_false(UserQuota.objects.filter(user=self.project_creator, storage_type=UserQuota.CUSTOM_STORAGE).exists()) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 3000 + } + }, + file_node=self.file, + storage_type=UserQuota.CUSTOM_STORAGE + ) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.CUSTOM_STORAGE) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 3000) + + def test_get_or_create_with_default_max_quota_logic(self): + """get_or_create should use get_default_max_quota() for max_quota value""" + from osf.models.institution_default_max_quota import InstitutionDefaultMaxQuota + + institution = InstitutionFactory() + self.project_creator.affiliated_institutions.add(institution) + RegionFactory(_id=institution._id) + ProjectStorageType.objects.filter(node=self.node).update( + storage_type=ProjectStorageType.CUSTOM_STORAGE + ) + + custom_max = 450 + InstitutionDefaultMaxQuota.objects.update_or_create( + institution_id=institution.id, + defaults={'default_max_quota': custom_max} + ) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 2000 + } + }, + file_node=self.file, + storage_type=UserQuota.CUSTOM_STORAGE + ) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.CUSTOM_STORAGE) + assert_equal(user_quota.max_quota, custom_max) + assert_equal(user_quota.used, 2000) + + def test_get_or_create_multiple_storage_types(self): + """get_or_create should handle multiple storage types for same user""" + institution = InstitutionFactory() + self.project_creator.affiliated_institutions.add(institution) + RegionFactory(_id=institution._id) + + # Create separate files for each storage type + file_nii = OsfStorageFileNode.create( + target=self.node, + path='/testfile_nii', + _id='testfile_nii', + name='testfile_nii', + materialized_path='/testfile_nii' + ) + file_nii.save() + + file_custom = OsfStorageFileNode.create( + target=self.node, + path='/testfile_custom', + _id='testfile_custom', + name='testfile_custom', + materialized_path='/testfile_custom' + ) + file_custom.save() + + # Create NII_STORAGE first + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 1000 + } + }, + file_node=file_nii, + storage_type=UserQuota.NII_STORAGE + ) + + # Create CUSTOM_STORAGE + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 2000 + } + }, + file_node=file_custom, + storage_type=UserQuota.CUSTOM_STORAGE + ) + + nii_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + custom_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.CUSTOM_STORAGE) + + assert_equal(nii_quota.used, 1000) + assert_equal(custom_quota.used, 2000) + + def test_get_or_create_preserves_existing_max_quota(self): + """get_or_create should not overwrite existing max_quota when record exists""" + custom_max = 300 + UserQuota.objects.create( + user=self.project_creator, + storage_type=UserQuota.NII_STORAGE, + max_quota=custom_max, + used=0 + ) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 1500 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(user_quota.max_quota, custom_max) + assert_equal(user_quota.used, 1500) + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + def test_get_or_create_without_select_for_update_new_userquota(self, _mock_check_select): + """get_or_create without select_for_update should create new UserQuota""" + assert_false(UserQuota.objects.filter(user=self.project_creator).exists()) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 5000 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota = UserQuota.objects.filter(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(len(user_quota), 1) + user_quota = user_quota[0] + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 5000) + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + def test_get_or_create_without_select_for_update_existing_userquota(self, _mock_check_select): + """get_or_create without select_for_update should retrieve existing UserQuota""" + UserQuota.objects.create( + user=self.project_creator, + storage_type=UserQuota.NII_STORAGE, + max_quota=200, + used=100 + ) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 200 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota_list = UserQuota.objects.filter(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(len(user_quota_list), 1) + user_quota = user_quota_list[0] + assert_equal(user_quota.max_quota, 200) + assert_equal(user_quota.used, 300) + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + def test_get_or_create_without_select_for_update_custom_storage(self, mock_check_select): + """get_or_create without select_for_update should work with CUSTOM_STORAGE""" + institution = InstitutionFactory() + self.project_creator.affiliated_institutions.add(institution) + RegionFactory(_id=institution._id) + ProjectStorageType.objects.filter(node=self.node).update( + storage_type=ProjectStorageType.CUSTOM_STORAGE + ) + + assert_false(UserQuota.objects.filter(user=self.project_creator, storage_type=UserQuota.CUSTOM_STORAGE).exists()) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 3000 + } + }, + file_node=self.file, + storage_type=UserQuota.CUSTOM_STORAGE + ) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.CUSTOM_STORAGE) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + assert_equal(user_quota.used, 3000) + + @mock.patch('website.util.quota.check_select_for_update') + def test_file_modified_else_branch_fileinfo_get(self, mock_check): + """Test the else branch at line 330 for FileInfo.get without select_for_update""" + mock_check.return_value = False + + # Create initial FileInfo + FileInfo.objects.create(file=self.file, file_size=500) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 1200 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + # used = 1200 - 500 = 700 (retrieved existing FileInfo via else branch) + assert_equal(user_quota.used, 700) + + # Verify check_select_for_update was called multiple times (for UserQuota and FileInfo) + assert_true(mock_check.called) + + @mock.patch('website.util.quota.check_select_for_update', return_value=False) + def test_file_modified_without_select_for_update_fileinfo_not_exists(self, mock_check_select): + """file_modified should handle FileInfo.DoesNotExist without select_for_update""" + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 2000 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + # used = 2000 - 0 = 2000 (no FileInfo exists, so old size is 0) + assert_equal(user_quota.used, 2000) + + file_info = FileInfo.objects.get(file=self.file) + assert_equal(file_info.file_size, 2000) + + @mock.patch('website.util.quota.check_select_for_update') + def test_file_modified_else_branch_get_or_create(self, mock_check): + """Test the else branch at line 315 for get_or_create without select_for_update""" + mock_check.return_value = False + + assert_false(UserQuota.objects.filter(user=self.project_creator, storage_type=UserQuota.NII_STORAGE).exists()) + + quota.file_modified( + target=self.node, + user=self.user, + payload={ + 'metadata': { + 'size': 1500 + } + }, + file_node=self.file, + storage_type=UserQuota.NII_STORAGE + ) + + # Verify that the else branch (line 315) was executed successfully + user_quota = UserQuota.objects.get(user=self.project_creator, storage_type=UserQuota.NII_STORAGE) + assert_equal(user_quota.used, 1500) + assert_equal(user_quota.max_quota, api_settings.DEFAULT_MAX_QUOTA) + + # Verify check_select_for_update was called + assert_true(mock_check.called) diff --git a/tests/tests_instutition_default_max_quota.py b/tests/tests_instutition_default_max_quota.py new file mode 100644 index 00000000000..0e9a0fa961e --- /dev/null +++ b/tests/tests_instutition_default_max_quota.py @@ -0,0 +1,54 @@ +# -*- coding: utf-8 -*- +from nose import tools as nt + +from osf.models import InstitutionDefaultMaxQuota +from osf_tests.factories import AuthUserFactory, InstitutionFactory +from tests.base import OsfTestCase + + +class TestInstitutionDefaultMaxQuota(OsfTestCase): + + def test_get_quota_by_user_has_institution(self): + user = AuthUserFactory() + institution = InstitutionFactory() + + user.affiliated_institutions.add(institution) + + InstitutionDefaultMaxQuota.objects.create( + institution=institution, + default_max_quota=500 + ) + + result = InstitutionDefaultMaxQuota.get_quota_by_user(user.id) + + nt.assert_equal(result, 500) + + def test_get_quota_by_user_no_institution(self): + user = AuthUserFactory() + + result = InstitutionDefaultMaxQuota.get_quota_by_user(user.id) + + nt.assert_equal(result, None) + + def test_get_quota_by_user_multiple_institutions(self): + user = AuthUserFactory() + + inst1 = InstitutionFactory() + inst2 = InstitutionFactory() + + user.affiliated_institutions.add(inst1) + user.affiliated_institutions.add(inst2) + + InstitutionDefaultMaxQuota.objects.create( + institution=inst1, + default_max_quota=100 + ) + + InstitutionDefaultMaxQuota.objects.create( + institution=inst2, + default_max_quota=200 + ) + + result = InstitutionDefaultMaxQuota.get_quota_by_user(user.id) + + nt.assert_true(result in [100, 200]) From 44a5f64ecfa0c1d91af26ef2d84c4313c7197a4b Mon Sep 17 00:00:00 2001 From: ncongthang Date: Mon, 15 Jun 2026 13:38:50 +0700 Subject: [PATCH 4/6] =?UTF-8?q?Ref=202.3.=E6=A9=9F=E9=96=A2=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=83=AC=E3=83=BC=E3=82=B8=E3=82=AF=E3=82=AA=E3=83=BC?= =?UTF-8?q?=E3=82=BF=E5=88=9D=E6=9C=9F=E5=80=A4=E8=A8=AD=E5=AE=9A=E6=A9=9F?= =?UTF-8?q?=E8=83=BD=E3=81=AE=E6=94=B9=E4=BF=AE:=20Rework=20the=20review?= =?UTF-8?q?=20comment=20+=20Fix=20user=20quota=20update=20rollback=20behav?= =?UTF-8?q?ior=20+=20Align=20related=5Fname=20between=20model=20and=20migr?= =?UTF-8?q?ation=20+=20Remove=20unused=20user=20parameter=20from=20file=5F?= =?UTF-8?q?modified=20+=20Update=20UT=20after=20change=20logic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../views.py | 11 +++---- .../test_views.py | 29 ++++++++++++++----- osf/migrations/0267_auto_20260608_0901.py | 2 +- ... => test_institution_default_max_quota.py} | 2 +- tests/test_quota.py | 13 --------- website/util/quota.py | 4 +-- 6 files changed, 31 insertions(+), 30 deletions(-) rename tests/{tests_instutition_default_max_quota.py => test_institution_default_max_quota.py} (97%) diff --git a/admin/institutional_storage_quota_control/views.py b/admin/institutional_storage_quota_control/views.py index d9b01b94d6f..4cb75c81647 100644 --- a/admin/institutional_storage_quota_control/views.py +++ b/admin/institutional_storage_quota_control/views.py @@ -188,11 +188,12 @@ def post(self, request, *args, **kwargs): for user in OSFUser.objects.filter( affiliated_institutions=self.institution_id): try: - UserQuota.objects.update_or_create( - user=user, - storage_type=UserQuota.CUSTOM_STORAGE, - defaults={'max_quota': max_quota} - ) + with transaction.atomic(): + UserQuota.objects.update_or_create( + user=user, + storage_type=UserQuota.CUSTOM_STORAGE, + defaults={'max_quota': max_quota} + ) except IntegrityError: UserQuota.objects.filter(user=user, storage_type=UserQuota.CUSTOM_STORAGE).update(max_quota=max_quota) return redirect( diff --git a/admin_tests/institutional_storage_quota_control/test_views.py b/admin_tests/institutional_storage_quota_control/test_views.py index 3b8b610bbfc..9f55b0ce523 100644 --- a/admin_tests/institutional_storage_quota_control/test_views.py +++ b/admin_tests/institutional_storage_quota_control/test_views.py @@ -319,6 +319,7 @@ def test_post__institution_default_max_quota_updated(self): def test_post__integrity_error_handles_fallback_update(self): """Test that IntegrityError in update_or_create triggers fallback update logic""" from unittest import mock + from django.db import IntegrityError, transaction new_max_quota = 300 region = RegionFactory(_id=self.institution01.guid) @@ -332,25 +333,37 @@ def test_post__integrity_error_handles_fallback_update(self): max_quota=100 ) - # Mock update_or_create to raise IntegrityError, simulating race condition + def raise_integrity_error(*args, **kwargs): + transaction.set_rollback(True) + raise IntegrityError('Simulated race condition') + + # Mock update_or_create to raise IntegrityError and mark transaction for rollback with mock.patch.object(UserQuota.objects, 'update_or_create') as mock_update: - from django.db import IntegrityError - mock_update.side_effect = IntegrityError("Simulated race condition") + mock_update.side_effect = raise_integrity_error request = RequestFactory().post( - reverse(self.view_name, - kwargs={'institution_id': self.institution01.id}), - {'maxQuota': new_max_quota}) + reverse( + self.view_name, + kwargs={'institution_id': self.institution01.id} + ), + {'maxQuota': new_max_quota} + ) request.user = self.institution01_admin - response = self.view(request, institution_id=self.institution01.id) + + response = self.view( + request, + institution_id=self.institution01.id + ) # Verify response is redirect nt.assert_equal(response.status_code, 302) # Verify max_quota was updated via fallback logic updated_quota = UserQuota.objects.filter( - user=self.institution01_admin, storage_type=UserQuota.CUSTOM_STORAGE + user=self.institution01_admin, + storage_type=UserQuota.CUSTOM_STORAGE ).first() + nt.assert_is_not_none(updated_quota) nt.assert_equal(updated_quota.max_quota, new_max_quota) nt.assert_equal(updated_quota.id, initial_quota.id) diff --git a/osf/migrations/0267_auto_20260608_0901.py b/osf/migrations/0267_auto_20260608_0901.py index 7078ba2a3a5..c30ca53aaf1 100644 --- a/osf/migrations/0267_auto_20260608_0901.py +++ b/osf/migrations/0267_auto_20260608_0901.py @@ -22,7 +22,7 @@ class Migration(migrations.Migration): ('created', django_extensions.db.fields.CreationDateTimeField(auto_now_add=True, verbose_name='created')), ('modified', django_extensions.db.fields.ModificationDateTimeField(auto_now=True, verbose_name='modified')), ('default_max_quota', models.IntegerField(default=100)), - ('institution', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='institution_default_max_quota', to='osf.Institution')), + ('institution', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='default_max_quota', to='osf.Institution')), ], options={ 'db_table': 'osf_institution_default_max_quota', diff --git a/tests/tests_instutition_default_max_quota.py b/tests/test_institution_default_max_quota.py similarity index 97% rename from tests/tests_instutition_default_max_quota.py rename to tests/test_institution_default_max_quota.py index 0e9a0fa961e..af02b5a0d63 100644 --- a/tests/tests_instutition_default_max_quota.py +++ b/tests/test_institution_default_max_quota.py @@ -51,4 +51,4 @@ def test_get_quota_by_user_multiple_institutions(self): result = InstitutionDefaultMaxQuota.get_quota_by_user(user.id) - nt.assert_true(result in [100, 200]) + nt.assert_equal(result, 100) diff --git a/tests/test_quota.py b/tests/test_quota.py index 69af0f1022b..7f473bd581f 100644 --- a/tests/test_quota.py +++ b/tests/test_quota.py @@ -2039,7 +2039,6 @@ def test_get_or_create_creates_new_userquota(self): quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 5000 @@ -2066,7 +2065,6 @@ def test_get_or_create_retrieves_existing_userquota(self): quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 200 @@ -2095,7 +2093,6 @@ def test_get_or_create_with_custom_storage_new_userquota(self): quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 3000 @@ -2128,7 +2125,6 @@ def test_get_or_create_with_default_max_quota_logic(self): quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 2000 @@ -2170,7 +2166,6 @@ def test_get_or_create_multiple_storage_types(self): # Create NII_STORAGE first quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 1000 @@ -2183,7 +2178,6 @@ def test_get_or_create_multiple_storage_types(self): # Create CUSTOM_STORAGE quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 2000 @@ -2211,7 +2205,6 @@ def test_get_or_create_preserves_existing_max_quota(self): quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 1500 @@ -2232,7 +2225,6 @@ def test_get_or_create_without_select_for_update_new_userquota(self, _mock_check quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 5000 @@ -2260,7 +2252,6 @@ def test_get_or_create_without_select_for_update_existing_userquota(self, _mock_ quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 200 @@ -2290,7 +2281,6 @@ def test_get_or_create_without_select_for_update_custom_storage(self, mock_check quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 3000 @@ -2314,7 +2304,6 @@ def test_file_modified_else_branch_fileinfo_get(self, mock_check): quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 1200 @@ -2336,7 +2325,6 @@ def test_file_modified_without_select_for_update_fileinfo_not_exists(self, mock_ """file_modified should handle FileInfo.DoesNotExist without select_for_update""" quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 2000 @@ -2362,7 +2350,6 @@ def test_file_modified_else_branch_get_or_create(self, mock_check): quota.file_modified( target=self.node, - user=self.user, payload={ 'metadata': { 'size': 1500 diff --git a/website/util/quota.py b/website/util/quota.py index b421263a4a5..062b9e76f52 100644 --- a/website/util/quota.py +++ b/website/util/quota.py @@ -217,7 +217,7 @@ def update_used_quota(self, target, user, event_type, payload): else: node_removed(target, user, payload, file_node, storage_type) elif event_type == FileLog.FILE_UPDATED: - file_modified(target, user, payload, file_node, storage_type) + file_modified(target, payload, file_node, storage_type) else: return @@ -298,7 +298,7 @@ def node_removed(target, user, payload, file_node, storage_type): file_info.save() user_quota.save() -def file_modified(target, user, payload, file_node, storage_type): +def file_modified(target, payload, file_node, storage_type): file_size = int(payload['metadata']['size']) if file_size < 0: return From 75d0bfb1bcf28f1ffbc7ee5980da0ec42568dac2 Mon Sep 17 00:00:00 2001 From: ncongthang Date: Tue, 16 Jun 2026 18:26:04 +0700 Subject: [PATCH 5/6] =?UTF-8?q?Ref=202.3.=E6=A9=9F=E9=96=A2=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=83=AC=E3=83=BC=E3=82=B8=E3=82=AF=E3=82=AA=E3=83=BC?= =?UTF-8?q?=E3=82=BF=E5=88=9D=E6=9C=9F=E5=80=A4=E8=A8=AD=E5=AE=9A=E6=A9=9F?= =?UTF-8?q?=E8=83=BD=E3=81=AE=E6=94=B9=E4=BF=AE:=20Resolve=20migration=20c?= =?UTF-8?q?onflicts?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...to_20260608_0901.py => 0270_institutiondefaultmaxquota.py} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename osf/migrations/{0267_auto_20260608_0901.py => 0270_institutiondefaultmaxquota.py} (92%) diff --git a/osf/migrations/0267_auto_20260608_0901.py b/osf/migrations/0270_institutiondefaultmaxquota.py similarity index 92% rename from osf/migrations/0267_auto_20260608_0901.py rename to osf/migrations/0270_institutiondefaultmaxquota.py index c30ca53aaf1..06c5686ac9d 100644 --- a/osf/migrations/0267_auto_20260608_0901.py +++ b/osf/migrations/0270_institutiondefaultmaxquota.py @@ -1,5 +1,5 @@ # -*- coding: utf-8 -*- -# Generated by Django 1.11.28 on 2026-06-08 09:01 +# Generated by Django 1.11.28 on 2026-06-16 11:23 from __future__ import unicode_literals from django.db import migrations, models @@ -11,7 +11,7 @@ class Migration(migrations.Migration): dependencies = [ - ('osf', '0266_merge_20260424_1200'), + ('osf', '0269_merge_20260525_0425'), ] operations = [ From 8dbf693f89e34497d496dc1f3ba6737e3c9d22e4 Mon Sep 17 00:00:00 2001 From: ncongthang Date: Thu, 25 Jun 2026 17:51:54 +0700 Subject: [PATCH 6/6] =?UTF-8?q?Ref=202.3.=E6=A9=9F=E9=96=A2=E3=82=B9?= =?UTF-8?q?=E3=83=88=E3=83=AC=E3=83=BC=E3=82=B8=E3=82=AF=E3=82=AA=E3=83=BC?= =?UTF-8?q?=E3=82=BF=E5=88=9D=E6=9C=9F=E5=80=A4=E8=A8=AD=E5=AE=9A=E6=A9=9F?= =?UTF-8?q?=E8=83=BD=E3=81=AE=E6=94=B9=E4=BF=AE:=20Add=20warning=20logs=20?= =?UTF-8?q?for=20IntegrityError=20in=20quota=20management?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- admin/institutional_storage_quota_control/views.py | 6 +++++- admin/quota_recalc/views.py | 6 +++++- website/util/quota.py | 9 ++++++--- 3 files changed, 16 insertions(+), 5 deletions(-) diff --git a/admin/institutional_storage_quota_control/views.py b/admin/institutional_storage_quota_control/views.py index 4cb75c81647..ce8e844a4c7 100644 --- a/admin/institutional_storage_quota_control/views.py +++ b/admin/institutional_storage_quota_control/views.py @@ -1,8 +1,11 @@ +import logging from django.contrib.auth.mixins import UserPassesTestMixin from django.db import connection, transaction, IntegrityError from django.db.models import Subquery, OuterRef from django.http import Http404 +logger = logging.getLogger(__name__) + from admin.institutions.views import QuotaUserList from osf.models import Institution, OSFUser, UserQuota, InstitutionDefaultMaxQuota from admin.base import settings @@ -194,7 +197,8 @@ def post(self, request, *args, **kwargs): storage_type=UserQuota.CUSTOM_STORAGE, defaults={'max_quota': max_quota} ) - except IntegrityError: + except IntegrityError as e: + logger.warning(u'IntegrityError while updating UserQuota: user={}, storage_type={}, max_quota={}: {}.'.format(user.id, UserQuota.CUSTOM_STORAGE, max_quota, str(e))) UserQuota.objects.filter(user=user, storage_type=UserQuota.CUSTOM_STORAGE).update(max_quota=max_quota) return redirect( 'institutional_storage_quota_control:institution_user_list', diff --git a/admin/quota_recalc/views.py b/admin/quota_recalc/views.py index 4c79508d039..abeb1bf6537 100644 --- a/admin/quota_recalc/views.py +++ b/admin/quota_recalc/views.py @@ -1,3 +1,4 @@ +import logging from django.http import JsonResponse from django.db import transaction, IntegrityError @@ -7,6 +8,8 @@ from osf.utils.requests import check_select_for_update from website.util.quota import get_default_max_quota, used_quota +logger = logging.getLogger(__name__) + def calculate_quota(user): storage_type_list = [UserQuota.NII_STORAGE] @@ -45,7 +48,8 @@ def calculate_quota(user): max_quota=max_quota, used=used, ) - except IntegrityError: + except IntegrityError as e: + logger.warning(u'IntegrityError while creating UserQuota in calculate_quota: user={}, storage_type={}: {}.'.format(user.id, storage_type, str(e))) used = used_quota(user._id, storage_type) if check_select_for_update(): user_quota = UserQuota.objects.filter( diff --git a/website/util/quota.py b/website/util/quota.py index 062b9e76f52..93ea2e64f3b 100644 --- a/website/util/quota.py +++ b/website/util/quota.py @@ -96,7 +96,8 @@ def update_user_used_quota(user, storage_type=UserQuota.NII_STORAGE, is_recalcul max_quota=max_quota, used=used, ) - except IntegrityError: + except IntegrityError as e: + logger.warning(u'IntegrityError while creating UserQuota: user={}, storage_type={}: {}'.format(user.id, storage_type, str(e))) if is_recalculating_quota and storage_type == UserQuota.CUSTOM_STORAGE: used_quota_for_nii_default_storage = used_quota(user._id, UserQuota.NII_STORAGE) used_quota_for_nii_custom_storage = used_quota(user._id, UserQuota.CUSTOM_STORAGE) @@ -249,7 +250,8 @@ def file_added(target, payload, file_node, storage_type): max_quota=max_quota, used=file_size ) - except IntegrityError: + except IntegrityError as e: + logger.warning(u'IntegrityError while creating UserQuota in file_added: user={}, storage_type={}: {}'.format(target.creator.id, storage_type, str(e))) if check_select_for_update(): user_quota = UserQuota.objects.filter( user=target.creator, @@ -317,7 +319,8 @@ def file_modified(target, payload, file_node, storage_type): storage_type=storage_type, defaults={'max_quota': max_quota} ) - except IntegrityError: + except IntegrityError as e: + logger.warning(u'IntegrityError while creating UserQuota in file_modified: user={}, storage_type={}: {}'.format(target.creator.id, storage_type, str(e))) if check_select_for_update(): user_quota = UserQuota.objects.filter(user=target.creator, storage_type=storage_type).select_for_update().get() else: