From 47f3dc069478384951146ccb7127596ffd2e2557 Mon Sep 17 00:00:00 2001 From: Tim <6887938+digitalmnt@users.noreply.github.com> Date: Mon, 16 Oct 2023 16:44:29 -0600 Subject: [PATCH] 417 score normalization (#432) * fix(api): normalize score naming * fix(api): use schema resolver instead of manual format * fix(api): fix default values in test for score response * fix(api): update historical endpoint to return DetailedScoreResponse and allow filtering by only address --- .../submit_passport/submit_passport.py | 2 +- api/registry/api/common.py | 89 +++++++++++++++++-- api/registry/api/schema.py | 8 +- api/registry/api/v1.py | 15 +--- api/registry/atasks.py | 2 +- .../management/commands/recalculate_scores.py | 4 +- .../0025_rename_points_score_stamp_scores.py | 18 ++++ api/registry/models.py | 21 +++-- .../test/test_command_recalculate_scores.py | 54 +++++------ api/registry/test/test_passport_submission.py | 6 +- .../test/test_v2_passport_get_score.py | 58 +++++++----- api/scorer/test/test_submit_passport.py | 10 ++- api/scorer_weighted/models.py | 2 +- 13 files changed, 198 insertions(+), 91 deletions(-) create mode 100644 api/registry/migrations/0025_rename_points_score_stamp_scores.py diff --git a/api/aws_lambdas/submit_passport/submit_passport.py b/api/aws_lambdas/submit_passport/submit_passport.py index c50c42b38..4bb18bb52 100644 --- a/api/aws_lambdas/submit_passport/submit_passport.py +++ b/api/aws_lambdas/submit_passport/submit_passport.py @@ -87,7 +87,7 @@ def handler(_event, _context): # TODO: preferably we would have a 1:1 mapping of the fields for DetailedScoreResponse # or if not, then specify a resolver for stamp_scores ret = DetailedScoreResponse.from_orm(score) - ret.stamp_scores = score.points + ret.stamp_scores = score.stamp_scores return { "statusCode": 200, diff --git a/api/registry/api/common.py b/api/registry/api/common.py index 08affe0bc..f21653579 100644 --- a/api/registry/api/common.py +++ b/api/registry/api/common.py @@ -5,13 +5,14 @@ # --- Deduplication Modules from account.models import Community -from registry.api.utils import ApiKey, check_rate_limit, with_read_db from registry.api.schema import ( CursorPaginatedHistoricalScoreResponse, + DetailedScoreResponse, ErrorMessageResponse, ) +from registry.api.utils import ApiKey, check_rate_limit, with_read_db from registry.exceptions import InvalidLimitException, api_get_object_or_404 -from registry.models import Event +from registry.models import Event, Score from registry.utils import ( decode_cursor, get_cursor_query_condition, @@ -52,7 +53,33 @@ def get_score_history( else: created_at = None - # Scenario 1 - Snapshot for 1 addresses + # Scenario 2 - Snapshot for 1 addresses + # the user has passed in the address, but no created_at + # In this case only 1 result will be returned + if address and not created_at: + scores = base_query.filter(address=address).order_by("-created_at") + + score_response = [] + for score in scores: + score_data = DetailedScoreResponse( + address=score.address, + score=score.data["score"], + status=Score.Status.DONE, + last_score_timestamp=score.created_at.isoformat(), + evidence=score.data["evidence"], + # below aren't currently stored in the events table, but can be + error=None, + stamp_scores=None, + ) + + score_response.append(score_data) + + response = CursorPaginatedHistoricalScoreResponse( + next=None, prev=None, items=score_response + ) + return response + + # Scenario 2 - Snapshot for 1 addresses and timestamp # the user has passed in the created_at and address # In this case only 1 result will be returned if address and created_at: @@ -62,14 +89,30 @@ def get_score_history( .first() ) + if not score: + return CursorPaginatedHistoricalScoreResponse( + next=None, prev=None, items=[] + ) + score.created_at = score.created_at.isoformat() + score_data = DetailedScoreResponse( + address=address, + score=score.data["score"], + status=Score.Status.DONE, + last_score_timestamp=score.created_at, + evidence=score.data["evidence"], + # below aren't currently stored in the events table, but can be + error=None, + stamp_scores=None, + ) + response = CursorPaginatedHistoricalScoreResponse( - next=None, prev=None, items=[score] + next=None, prev=None, items=[score_data] ) return response - # Scenario 2 - Snapshot for all addresses + # Scenario 3 - Snapshot for all addresses # the user has passed in the created_at, but no address elif created_at: pagination_sort_fields = ["address"] @@ -103,12 +146,27 @@ def get_score_history( endpoint, ) + score_response = [] + for score in scores: + score_data = DetailedScoreResponse( + address=score.address, + score=score.data["score"], + status=Score.Status.DONE, + last_score_timestamp=score.created_at, + evidence=score.data["evidence"], + # below aren't currently stored in the events table, but can be + error=None, + stamp_scores=None, + ) + + score_response.append(score_data) + response = CursorPaginatedHistoricalScoreResponse( - next=page_links["next"], prev=page_links["prev"], items=scores + next=page_links["next"], prev=page_links["prev"], items=score_response ) return response - # Scenario 3 - Just return history ... + # # Scenario 4 - Just return history ... else: pagination_sort_fields = ["id"] filter_condition, field_ordering = get_cursor_query_condition( @@ -141,8 +199,23 @@ def get_score_history( endpoint, ) + score_response = [] + for score in scores: + score_data = DetailedScoreResponse( + address=score.address, + score=score.data["score"], + status=Score.Status.DONE, + last_score_timestamp=score.created_at, + evidence=score.data["evidence"], + # below aren't currently stored in the events table, but can be + error=None, + stamp_scores=None, + ) + + score_response.append(score_data) + response = CursorPaginatedHistoricalScoreResponse( - next=page_links["next"], prev=page_links["prev"], items=scores + next=page_links["next"], prev=page_links["prev"], items=score_response ) return response diff --git a/api/registry/api/schema.py b/api/registry/api/schema.py index f3107277d..301354166 100644 --- a/api/registry/api/schema.py +++ b/api/registry/api/schema.py @@ -92,6 +92,12 @@ def resolve_last_score_timestamp(obj): def resolve_address(obj): return obj.passport.address + @staticmethod + def resolve_stamp_scores(obj): + if obj.stamp_scores is None or obj.stamp_scores == "": + return {} + return obj.stamp_scores + class HistoricalScoreData(Schema): score: float @@ -116,7 +122,7 @@ class CursorPaginatedScoreResponse(Schema): class CursorPaginatedHistoricalScoreResponse(Schema): next: Optional[str] prev: Optional[str] - items: List[DetailedHistoricalScoreResponse] + items: List[DetailedScoreResponse] class SimpleScoreResponse(Schema): diff --git a/api/registry/api/v1.py b/api/registry/api/v1.py index 5598b4eac..4efd266f0 100644 --- a/api/registry/api/v1.py +++ b/api/registry/api/v1.py @@ -385,20 +385,7 @@ def handle_get_score( score = Score.objects.get( passport__address=lower_address, passport__community=user_community ) - - response_data = { - "address": score.passport.address, - "score": score.score, - "status": score.status, - "last_score_timestamp": score.last_score_timestamp.isoformat() - if score.last_score_timestamp - else None, - "evidence": score.evidence, - "error": score.error, - "stamp_scores": score.points, - } - - return DetailedScoreResponse(**response_data) + return score except NotFoundApiException as e: raise e except Exception as e: diff --git a/api/registry/atasks.py b/api/registry/atasks.py index 473a4eb09..997d2bffa 100644 --- a/api/registry/atasks.py +++ b/api/registry/atasks.py @@ -63,7 +63,7 @@ async def acalculate_score(passport: Passport, community_id: int, score: Score): score.last_score_timestamp = get_utc_time() score.evidence = scoreData.evidence[0].as_dict() if scoreData.evidence else None score.error = None - score.points = scoreData.points + score.stamp_scores = scoreData.stamp_scores log.info("Calculated score: %s", score) diff --git a/api/registry/management/commands/recalculate_scores.py b/api/registry/management/commands/recalculate_scores.py index 1f2214f15..41e0ebd10 100644 --- a/api/registry/management/commands/recalculate_scores.py +++ b/api/registry/management/commands/recalculate_scores.py @@ -113,7 +113,7 @@ def handle(self, *args, **kwargs): else None ) score.error = None - score.points = scoreData.points + score.stamp_scores = scoreData.stamp_scores if scores_to_create: Score.objects.bulk_create(scores_to_create) @@ -127,7 +127,7 @@ def handle(self, *args, **kwargs): "last_score_timestamp", "evidence", "error", - "points", + "stamp_scores", ], ) diff --git a/api/registry/migrations/0025_rename_points_score_stamp_scores.py b/api/registry/migrations/0025_rename_points_score_stamp_scores.py new file mode 100644 index 000000000..fe7ad2685 --- /dev/null +++ b/api/registry/migrations/0025_rename_points_score_stamp_scores.py @@ -0,0 +1,18 @@ +# Generated by Django 4.2.4 on 2023-10-13 20:01 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("registry", "0022_stamp_points_squashed_0024_alter_score_points"), + ] + + operations = [ + migrations.RenameField( + model_name="score", + old_name="points", + new_name="stamp_scores", + ), + ] diff --git a/api/registry/models.py b/api/registry/models.py index 998fa0b57..fc43c7669 100644 --- a/api/registry/models.py +++ b/api/registry/models.py @@ -70,7 +70,7 @@ class Status: ) error = models.TextField(null=True, blank=True) evidence = models.JSONField(null=True, blank=True) - points = models.JSONField(null=True, blank=True) + stamp_scores = models.JSONField(null=True, blank=True) def __str__(self): return f"Score #{self.id}, score={self.score}, last_score_timestamp={self.last_score_timestamp}, status={self.status}, error={self.error}, evidence={self.evidence}, passport_id={self.passport_id}" @@ -81,16 +81,15 @@ def score_updated(sender, instance, **kwargs): if instance.status != Score.Status.DONE: return instance - if not Score.objects.filter(pk=instance.pk, score=instance.score).exists(): - Event.objects.create( - action=Event.Action.SCORE_UPDATE, - address=instance.passport.address, - community=instance.passport.community, - data={ - "score": float(instance.score) if instance.score != None else 0, - "evidence": instance.evidence, - }, - ) + Event.objects.create( + action=Event.Action.SCORE_UPDATE, + address=instance.passport.address, + community=instance.passport.community, + data={ + "score": float(instance.score) if instance.score != None else 0, + "evidence": instance.evidence, + }, + ) return instance diff --git a/api/registry/test/test_command_recalculate_scores.py b/api/registry/test/test_command_recalculate_scores.py index 4428bfd1a..a0fb8fbd0 100644 --- a/api/registry/test/test_command_recalculate_scores.py +++ b/api/registry/test/test_command_recalculate_scores.py @@ -178,21 +178,21 @@ def test_rescoring_binary_scorer( s1 = Score.objects.get(passport=binary_weighted_scorer_passports[0]) assert s1.evidence["rawScore"] == "75" - assert len(s1.points) == 1 - assert "Facebook" in s1.points + assert len(s1.stamp_scores) == 1 + assert "Facebook" in s1.stamp_scores s2 = Score.objects.get(passport=binary_weighted_scorer_passports[1]) assert s2.evidence["rawScore"] == "76" - assert len(s2.points) == 2 - assert "Facebook" in s2.points - assert "Google" in s2.points + assert len(s2.stamp_scores) == 2 + assert "Facebook" in s2.stamp_scores + assert "Google" in s2.stamp_scores s3 = Score.objects.get(passport=binary_weighted_scorer_passports[2]) assert s3.evidence["rawScore"] == "77" - assert len(s3.points) == 3 - assert "Facebook" in s3.points - assert "Google" in s3.points - assert "Ens" in s3.points + assert len(s3.stamp_scores) == 3 + assert "Facebook" in s3.stamp_scores + assert "Google" in s3.stamp_scores + assert "Ens" in s3.stamp_scores def test_rescoring_weighted_scorer( self, @@ -224,21 +224,21 @@ def test_rescoring_weighted_scorer( s1 = Score.objects.get(passport=weighted_scorer_passports[0]) assert s1.score == 1 - assert len(s1.points) == 1 - assert "Facebook" in s1.points + assert len(s1.stamp_scores) == 1 + assert "Facebook" in s1.stamp_scores s2 = Score.objects.get(passport=weighted_scorer_passports[1]) assert s2.score == 2 - assert len(s2.points) == 2 - assert "Facebook" in s2.points - assert "Google" in s2.points + assert len(s2.stamp_scores) == 2 + assert "Facebook" in s2.stamp_scores + assert "Google" in s2.stamp_scores s3 = Score.objects.get(passport=weighted_scorer_passports[2]) assert s3.score == 3 - assert len(s3.points) == 3 - assert "Facebook" in s3.points - assert "Google" in s3.points - assert "Ens" in s3.points + assert len(s3.stamp_scores) == 3 + assert "Facebook" in s3.stamp_scores + assert "Google" in s3.stamp_scores + assert "Ens" in s3.stamp_scores ######################################################### # Change weights and rescore ... @@ -259,21 +259,21 @@ def test_rescoring_weighted_scorer( s1 = Score.objects.get(passport=weighted_scorer_passports[0]) assert s1.score == 75 - assert len(s1.points) == 1 - assert "Facebook" in s1.points + assert len(s1.stamp_scores) == 1 + assert "Facebook" in s1.stamp_scores s2 = Score.objects.get(passport=weighted_scorer_passports[1]) assert s2.score == 76 - assert len(s2.points) == 2 - assert "Facebook" in s2.points - assert "Google" in s2.points + assert len(s2.stamp_scores) == 2 + assert "Facebook" in s2.stamp_scores + assert "Google" in s2.stamp_scores s3 = Score.objects.get(passport=weighted_scorer_passports[2]) assert s3.score == 77 - assert len(s3.points) == 3 - assert "Facebook" in s3.points - assert "Google" in s3.points - assert "Ens" in s3.points + assert len(s3.stamp_scores) == 3 + assert "Facebook" in s3.stamp_scores + assert "Google" in s3.stamp_scores + assert "Ens" in s3.stamp_scores def test_rescoring_include_filter( self, diff --git a/api/registry/test/test_passport_submission.py b/api/registry/test/test_passport_submission.py index c08f59a22..6d2f2ffd8 100644 --- a/api/registry/test/test_passport_submission.py +++ b/api/registry/test/test_passport_submission.py @@ -542,7 +542,7 @@ def test_submitting_without_passport(self, aget_passport, validate_credential): "last_score_timestamp": None, "status": "ERROR", "error": "No Passport found for this address.", - "stamp_scores": None, + "stamp_scores": {}, } @patch("registry.atasks.validate_credential", side_effect=[[], [], [], []]) @@ -576,7 +576,7 @@ def test_submit_passport_multiple_times( "score": Decimal("2.000000000"), "status": "DONE", "error": None, - "stamp_scores": None, + "stamp_scores": {"Ens": 1.0, "Google": 1.0}, } expected2ndResponse = { @@ -586,7 +586,7 @@ def test_submit_passport_multiple_times( "score": Decimal("2.000000000"), "status": "DONE", "error": None, - "stamp_scores": None, + "stamp_scores": {"Ens": 1.0, "Google": 1.0}, } # First submission diff --git a/api/registry/test/test_v2_passport_get_score.py b/api/registry/test/test_v2_passport_get_score.py index 1ca3ff667..9ca6373a2 100644 --- a/api/registry/test/test_v2_passport_get_score.py +++ b/api/registry/test/test_v2_passport_get_score.py @@ -287,7 +287,7 @@ def test_v2_get_scores_filter_by_last_score_timestamp__gte( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores ] @@ -313,7 +313,7 @@ def test_v2_get_scores_filter_by_last_score_timestamp__gte( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores ] @@ -373,7 +373,7 @@ def test_v2_get_scores_pagination_when_having_multiple_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores ] @@ -399,7 +399,7 @@ def test_v2_get_scores_pagination_when_having_multiple_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in older_scores[:expected_length] ] @@ -423,7 +423,7 @@ def test_v2_get_scores_pagination_when_having_multiple_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores ] @@ -471,7 +471,7 @@ def test_v2_get_scores_pagination_when_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores[:2] ] @@ -496,7 +496,7 @@ def test_v2_get_scores_pagination_when_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores[2:4] ] @@ -522,7 +522,7 @@ def test_v2_get_scores_pagination_when_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores[4:5] ] @@ -547,7 +547,7 @@ def test_v2_get_scores_pagination_when_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores[2:4] ] @@ -572,7 +572,7 @@ def test_v2_get_scores_pagination_when_identical_timestamps( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores[:2] ] @@ -616,7 +616,7 @@ def test_v2_get_scores_filter_by_last_score_timestamp__gt( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores[1:] ] @@ -642,7 +642,7 @@ def test_v2_get_scores_filter_by_last_score_timestamp__gt( "last_score_timestamp": s.last_score_timestamp.isoformat(), "evidence": s.evidence, "error": s.error, - "stamp_scores": None, + "stamp_scores": {}, } for s in newer_scores ] @@ -767,6 +767,29 @@ def to_datetime(string_timestamp): assert is_sorted, "The scores are not in order" + def test_get_historical_score_filter_by_address( + self, + scorer_api_key, + passport_holder_addresses, + scorer_community, + paginated_scores, + ): + address = passport_holder_addresses[0]["address"] + + event_object = list(Event.objects.all()) + + client = Client() + response = client.get( + f"{self.base_url}/score/{scorer_community.id}/history", + HTTP_AUTHORIZATION="Token " + scorer_api_key, + data={"address": address}, + ) + + response_data = response.json() + assert response.status_code == 200 + assert len(response_data["items"]) == 1 + assert response_data["items"][0]["address"] == address.lower() + def test_get_historical_score_filter_by_address_and_timestamp( self, scorer_api_key, @@ -787,14 +810,9 @@ def test_get_historical_score_filter_by_address_and_timestamp( ) response_data = response.json() - assert response.status_code == 200 assert len(response_data["items"]) == 1 - event_address = None - for event in event_object: - if event.address == response_data["items"][0]["address"]: - event_address = event.address - assert response_data["items"][0]["address"] == event_address + assert response_data["items"][0]["address"] == address def test_get_historical_scores_filter_by_timestamp( self, @@ -818,9 +836,9 @@ def test_get_historical_scores_filter_by_timestamp( items = response_data["items"] for item in items: - item_created_at = item["created_at"] + last_score_timestamp = item["last_score_timestamp"] assert ( - datetime.datetime.fromisoformat(item_created_at) <= created_at + datetime.datetime.fromisoformat(last_score_timestamp) <= created_at ), f"Item with address {item['address']} has a created_at greater than the given timestamp!" def test_get_historical_scores_filter_by_timestamp_sorted( diff --git a/api/scorer/test/test_submit_passport.py b/api/scorer/test/test_submit_passport.py index 7d3a7d03c..8d7073cf7 100644 --- a/api/scorer/test/test_submit_passport.py +++ b/api/scorer/test/test_submit_passport.py @@ -105,7 +105,10 @@ def _(scorer_community_with_gitcoin_default, submit_passport_response): "last_score_timestamp": mock_utc_timestamp.isoformat(), "evidence": None, "error": None, - "stamp_scores": None, + "stamp_scores": { + "Ens": 1000000.0, + "Google": 1234.0, + }, } @@ -224,7 +227,10 @@ def _(scorer_community_with_gitcoin_default, scoring_failed_score_response): "last_score_timestamp": None, "evidence": None, "error": "something bad", - "stamp_scores": None, + "stamp_scores": { + "Ens": 1000000.0, + "Google": 1234.0, + }, } diff --git a/api/scorer_weighted/models.py b/api/scorer_weighted/models.py index 050989ea3..20d3b46f1 100644 --- a/api/scorer_weighted/models.py +++ b/api/scorer_weighted/models.py @@ -45,7 +45,7 @@ def __init__( ): self.score = score self.evidence = evidence - self.points = points + self.stamp_scores = points def __repr__(self): return f"ScoreData(score={self.score}, evidence={self.evidence})"