From d4b8273f04965f450b49caf794e682a09ba4da31 Mon Sep 17 00:00:00 2001 From: Aaron Kanzer Date: Mon, 3 Jun 2024 19:09:04 -0400 Subject: [PATCH 1/9] Only include APPROVED users for stats on homepage --- dandiapi/api/tests/test_stats.py | 21 ++++++++++++++++----- dandiapi/api/views/stats.py | 4 ++-- 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index 546d36a70..4ffd548c5 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -1,8 +1,9 @@ from __future__ import annotations +from dandiapi.api.models import UserMetadata +from django.contrib.auth.models import User import pytest - @pytest.mark.django_db() def test_stats_baseline(api_client): assert api_client.get('/api/stats/').data == { @@ -33,10 +34,20 @@ def test_stats_published(api_client, published_version_factory): @pytest.mark.django_db() def test_stats_user(api_client, user): + + # Create users with different statuses + approved_user_count = 0 + for status in UserMetadata.Status.choices: + user = User.objects.create(username=f'{status.lower()}_user') + UserMetadata.objects.create(user=user, status=status) + if status == UserMetadata.Status.APPROVED: + approved_user_count += 1 + stats = api_client.get('/api/stats/').data - # django-guardian automatically creates an AnonymousUser - assert stats['user_count'] == 2 + # Assert that the user count only includes users with APPROVED status + assert stats['user_count'] == 1 + assert stats['user_count'] == approved_user_count @pytest.mark.django_db() @@ -50,7 +61,7 @@ def test_stats_asset(api_client, version, asset): @pytest.mark.django_db() def test_stats_embargoed_asset(api_client, version, asset_factory, embargoed_asset_blob_factory): embargoed_asset = asset_factory() - embargoed_asset.blob = embargoed_asset_blob_factory() + embargoed_asset.embargoed_blob = embargoed_asset_blob_factory() version.assets.add(embargoed_asset) stats = api_client.get('/api/stats/').data @@ -66,7 +77,7 @@ def test_stats_embargoed_and_regular_blobs( version.assets.add(asset) embargoed_asset = asset_factory() - embargoed_asset.blob = embargoed_asset_blob_factory() + embargoed_asset.embargoed_blob = embargoed_asset_blob_factory() version.assets.add(embargoed_asset) stats = api_client.get('/api/stats/').data diff --git a/dandiapi/api/views/stats.py b/dandiapi/api/views/stats.py index 7175dd00d..cec31a5a2 100644 --- a/dandiapi/api/views/stats.py +++ b/dandiapi/api/views/stats.py @@ -5,7 +5,7 @@ from rest_framework.decorators import api_view from rest_framework.response import Response -from dandiapi.api.models import Asset, Dandiset +from dandiapi.api.models import Asset, Dandiset, UserMetadata # Cache this response for 12 hours @@ -14,7 +14,7 @@ def stats_view(self): dandiset_count = Dandiset.objects.count() published_dandiset_count = Dandiset.published_count() - user_count = User.objects.count() + user_count = User.objects.filter(metadata__status=UserMetadata.Status.APPROVED).count() size = Asset.total_size() return Response( { From a602ae9e6246058da18f89ad6dd2fab9544990b2 Mon Sep 17 00:00:00 2001 From: Aaron Kanzer Date: Mon, 3 Jun 2024 19:12:15 -0400 Subject: [PATCH 2/9] bump unit test --- dandiapi/api/tests/test_stats.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index 4ffd548c5..4808842b7 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -38,9 +38,10 @@ def test_stats_user(api_client, user): # Create users with different statuses approved_user_count = 0 for status in UserMetadata.Status.choices: - user = User.objects.create(username=f'{status.lower()}_user') - UserMetadata.objects.create(user=user, status=status) - if status == UserMetadata.Status.APPROVED: + status_value = status[0] + user = User.objects.create(username=f'{status_value.lower()}_user') + UserMetadata.objects.create(user=user, status=status_value) + if status_value == UserMetadata.Status.APPROVED: approved_user_count += 1 stats = api_client.get('/api/stats/').data @@ -61,7 +62,7 @@ def test_stats_asset(api_client, version, asset): @pytest.mark.django_db() def test_stats_embargoed_asset(api_client, version, asset_factory, embargoed_asset_blob_factory): embargoed_asset = asset_factory() - embargoed_asset.embargoed_blob = embargoed_asset_blob_factory() + embargoed_asset.blob = embargoed_asset_blob_factory() version.assets.add(embargoed_asset) stats = api_client.get('/api/stats/').data @@ -77,7 +78,7 @@ def test_stats_embargoed_and_regular_blobs( version.assets.add(asset) embargoed_asset = asset_factory() - embargoed_asset.embargoed_blob = embargoed_asset_blob_factory() + embargoed_asset.blob = embargoed_asset_blob_factory() version.assets.add(embargoed_asset) stats = api_client.get('/api/stats/').data From 60514f41655480e930e459b1d41e6df0ba7faeab Mon Sep 17 00:00:00 2001 From: Aaron Kanzer Date: Mon, 1 Jul 2024 17:09:50 -0700 Subject: [PATCH 3/9] Resolve failing stats unit tests and linting --- dandiapi/api/tests/test_stats.py | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index 4808842b7..37240dcd8 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -1,16 +1,18 @@ from __future__ import annotations -from dandiapi.api.models import UserMetadata -from django.contrib.auth.models import User +from django.contrib.auth.models import User import pytest +from dandiapi.api.models import UserMetadata + + @pytest.mark.django_db() def test_stats_baseline(api_client): assert api_client.get('/api/stats/').data == { 'dandiset_count': 0, 'published_dandiset_count': 0, - # django-guardian automatically creates an AnonymousUser - 'user_count': 1, + # django-guardian automatically creates an AnonymousUser, but user is not approved + 'user_count': 0, 'size': 0, } @@ -34,6 +36,8 @@ def test_stats_published(api_client, published_version_factory): @pytest.mark.django_db() def test_stats_user(api_client, user): + # Reset the User table for test + User.objects.all().delete() # Create users with different statuses approved_user_count = 0 From ea8cd0011c04448be7ce886dec3094c3c4d21326 Mon Sep 17 00:00:00 2001 From: Aaron Kanzer Date: Mon, 1 Jul 2024 17:11:58 -0700 Subject: [PATCH 4/9] resolve linting error again --- dandiapi/api/models/asset.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/dandiapi/api/models/asset.py b/dandiapi/api/models/asset.py index cc2bed6ad..e2158560b 100644 --- a/dandiapi/api/models/asset.py +++ b/dandiapi/api/models/asset.py @@ -218,10 +218,7 @@ def is_different_from( if self.path != path: return True - if self.metadata != metadata: - return True - - return False + return self.metadata != metadata @staticmethod def dandi_asset_id(asset_id: str | uuid.UUID): From 0899ba88b6c0b2882b57fb481ff7eebaad2961da Mon Sep 17 00:00:00 2001 From: Aaron Kanzer Date: Mon, 16 Sep 2024 13:27:29 -0400 Subject: [PATCH 5/9] Revise PR based on Roni's comments --- dandiapi/api/tests/test_stats.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index 37240dcd8..6af947f6f 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -11,7 +11,6 @@ def test_stats_baseline(api_client): assert api_client.get('/api/stats/').data == { 'dandiset_count': 0, 'published_dandiset_count': 0, - # django-guardian automatically creates an AnonymousUser, but user is not approved 'user_count': 0, 'size': 0, } @@ -35,23 +34,27 @@ def test_stats_published(api_client, published_version_factory): @pytest.mark.django_db() -def test_stats_user(api_client, user): - # Reset the User table for test +def test_stats_user(api_client, user_factory): User.objects.all().delete() - # Create users with different statuses + # Create multiple users with different statuses approved_user_count = 0 + users_per_status = 3 + user_index = 0 + for status in UserMetadata.Status.choices: status_value = status[0] - user = User.objects.create(username=f'{status_value.lower()}_user') - UserMetadata.objects.create(user=user, status=status_value) - if status_value == UserMetadata.Status.APPROVED: - approved_user_count += 1 + for _ in range(users_per_status): + username = f'{status_value.lower()}_user_{user_index}' + user = user_factory(username=username) + UserMetadata.objects.create(user=user, status=status_value) + if status_value == UserMetadata.Status.APPROVED: + approved_user_count += 1 + user_index += 1 stats = api_client.get('/api/stats/').data # Assert that the user count only includes users with APPROVED status - assert stats['user_count'] == 1 assert stats['user_count'] == approved_user_count From 9d388d01c8b8f81284a1b7119bcfb1b9bb55f2c8 Mon Sep 17 00:00:00 2001 From: Aaron Kanzer Date: Mon, 16 Sep 2024 13:30:50 -0400 Subject: [PATCH 6/9] linting --- dandiapi/api/tests/test_stats.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index 6af947f6f..2d11d8fd9 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -33,7 +33,7 @@ def test_stats_published(api_client, published_version_factory): assert stats['published_dandiset_count'] == 1 -@pytest.mark.django_db() +@pytest.mark.django_db def test_stats_user(api_client, user_factory): User.objects.all().delete() From ee11e9e44f56be605d79ac97c4b106b70d17760c Mon Sep 17 00:00:00 2001 From: Aaron Kanzer Date: Mon, 16 Sep 2024 13:39:58 -0400 Subject: [PATCH 7/9] Resolve UserMetadata issue --- dandiapi/api/tests/test_stats.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index 2d11d8fd9..946f95102 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -47,7 +47,16 @@ def test_stats_user(api_client, user_factory): for _ in range(users_per_status): username = f'{status_value.lower()}_user_{user_index}' user = user_factory(username=username) - UserMetadata.objects.create(user=user, status=status_value) + user_metadata, created = UserMetadata.objects.get_or_create( + user=user, + defaults={ + 'status': status_value, + 'questionnaire_form': None, + 'rejection_reason': '' + } + ) + if not created: + pass if status_value == UserMetadata.Status.APPROVED: approved_user_count += 1 user_index += 1 From f42558a5e134a665557ee923aff5b23b57973aa9 Mon Sep 17 00:00:00 2001 From: Roni Choudhury <2903332+waxlamp@users.noreply.github.com> Date: Thu, 19 Sep 2024 20:20:39 -0400 Subject: [PATCH 8/9] Avoid deleting user objects explicitly This is not needed due to the testing framework creating a fresh database for each test run. Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com> --- dandiapi/api/tests/test_stats.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index d1579fab6..176307dbe 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -35,7 +35,6 @@ def test_stats_published(api_client, published_version_factory): @pytest.mark.django_db def test_stats_user(api_client, user_factory): - User.objects.all().delete() # Create multiple users with different statuses approved_user_count = 0 From a8b5bd66956e6ba3e9e3de91a58000990f5aed45 Mon Sep 17 00:00:00 2001 From: Roni Choudhury <2903332+waxlamp@users.noreply.github.com> Date: Thu, 19 Sep 2024 20:25:02 -0400 Subject: [PATCH 9/9] Simplify test code Co-authored-by: Mike VanDenburgh <37340715+mvandenburgh@users.noreply.github.com> --- dandiapi/api/tests/test_stats.py | 24 ++---------------------- 1 file changed, 2 insertions(+), 22 deletions(-) diff --git a/dandiapi/api/tests/test_stats.py b/dandiapi/api/tests/test_stats.py index 176307dbe..3304f43f7 100644 --- a/dandiapi/api/tests/test_stats.py +++ b/dandiapi/api/tests/test_stats.py @@ -1,6 +1,5 @@ from __future__ import annotations -from django.contrib.auth.models import User import pytest from dandiapi.api.models import UserMetadata @@ -35,30 +34,11 @@ def test_stats_published(api_client, published_version_factory): @pytest.mark.django_db def test_stats_user(api_client, user_factory): - # Create multiple users with different statuses - approved_user_count = 0 - users_per_status = 3 - user_index = 0 + users_per_status = approved_user_count = 3 for status in UserMetadata.Status.choices: - status_value = status[0] - for _ in range(users_per_status): - username = f'{status_value.lower()}_user_{user_index}' - user = user_factory(username=username) - user_metadata, created = UserMetadata.objects.get_or_create( - user=user, - defaults={ - 'status': status_value, - 'questionnaire_form': None, - 'rejection_reason': '' - } - ) - if not created: - pass - if status_value == UserMetadata.Status.APPROVED: - approved_user_count += 1 - user_index += 1 + [user_factory(metadata__status=status[0]) for _ in range(users_per_status)] stats = api_client.get('/api/stats/').data