Skip to content

Commit

Permalink
Move warnings to top and use clearer message
Browse files Browse the repository at this point in the history
  • Loading branch information
sarayourfriend committed Apr 8, 2024
1 parent caddd9e commit d6f14eb
Show file tree
Hide file tree
Showing 5 changed files with 21 additions and 16 deletions.
1 change: 0 additions & 1 deletion api/api/examples/audio_responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
"results": [
base_audio | {"fields_matched": ["title"]},
],
"warnings": [],
},
}

Expand Down
1 change: 0 additions & 1 deletion api/api/examples/image_responses.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,6 @@
"page_size": 20,
"page": 1,
"results": [base_image | {"fields_matched": ["title"]}],
"warnings": [],
},
}

Expand Down
13 changes: 8 additions & 5 deletions api/api/serializers/media_serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -423,15 +423,18 @@ def validate_source(self, value):
f"Refer to the source list for valid options: {sources_list}."
)
elif invalid_sources := (sources - valid_sources):
available_sources_uri = self.context["request"].build_absolute_uri(
reverse(f"{self.media_type}-stats")
)
self.context["warnings"].append(
{
"code": "partially invalid source parameter",
"message": "The source parameter was partially invalid.",
"invalid_sources": invalid_sources,
"referenced_sources": valid_sources,
"available_sources": self.context["request"].build_absolute_uri(
reverse(f"{self.media_type}-stats")
"message": (
"The source parameter included non-existent sources. "
f"For a list of available sources, see {available_sources_uri}"
),
"invalid_sources": invalid_sources,
"valid_sources": valid_sources,
}
)

Expand Down
20 changes: 12 additions & 8 deletions api/api/utils/pagination.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,18 @@ def __init__(self, *args, **kwargs):
self.warnings = [] # populated later as needed

def get_paginated_response(self, data):
response = {
"result_count": self.result_count,
"page_count": min(settings.MAX_PAGINATION_DEPTH, self.page_count),
"page_size": self.page_size,
"page": self.page,
"results": data,
}
return Response(
{
"result_count": self.result_count,
"page_count": min(settings.MAX_PAGINATION_DEPTH, self.page_count),
"page_size": self.page_size,
"page": self.page,
"results": data,
({
# Put ``warnings`` first so it is as visible as possible.
"warnings": list(self.warnings),
}
} if self.warnings else {}) | response
)

def get_paginated_response_schema(self, schema):
Expand Down Expand Up @@ -63,7 +66,7 @@ def get_paginated_response_schema(self, schema):
},
"description": (
"Warnings pertinent to the request. "
"If there are no warnings, this list will be empty. "
"If there are no warnings, this property will not be present on the response. "
"Warnings are non-critical problems with the request. "
"Responses with warnings should be treated as unstable. "
"Warning descriptions must not be treated as machine readable "
Expand All @@ -86,4 +89,5 @@ def get_paginated_response_schema(self, schema):
return {
"type": "object",
"properties": properties,
"required": list(set(properties.keys()) - {"warnings"}),
}
2 changes: 1 addition & 1 deletion api/test/integration/test_media_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ def test_search_with_only_valid_sources_produces_no_warning(media_type, api_clie
{"source": ",".join(media_type.providers)},
)
assert search.status_code == 200
assert search.json()["warnings"] == []
assert "warnings" not in search.json()


def test_search_with_partially_invalid_sources_produces_warning_but_still_succeeds(
Expand Down

0 comments on commit d6f14eb

Please sign in to comment.