diff --git a/admin/templates/download_events/download_events.html b/admin/templates/download_events/download_events.html index bb1563a39ec..0149733d2a8 100644 --- a/admin/templates/download_events/download_events.html +++ b/admin/templates/download_events/download_events.html @@ -92,7 +92,7 @@

Download telemetry dashboard

{{ download_events_dashboard.split.file.gb|floatformat:2 }} / {{ download_events_dashboard.split.zip.gb|floatformat:2 }}
-
Failed zip downloads (server error)
+
Failed zip downloads (server or provider error)
{{ download_events_dashboard.summary.failed_zips }}
diff --git a/osf/admin.py b/osf/admin.py index 284b8695f66..ad63d6bd03d 100644 --- a/osf/admin.py +++ b/osf/admin.py @@ -408,10 +408,12 @@ def user(self, obj): user.short_description = 'User' -# A zip that WaterButler ended on a 5xx failed through no fault of the user (the server -# buckled) -- distinct from a cancel, which ends completed=False at 200 because the headers -# already went out. This is the threshold that separates the two. -DOWNLOAD_FAILURE_MIN_STATUS = 500 +# A zip that didn't complete failed through no fault of the user whenever it ended on an +# error status -- a 4xx (e.g. an add-on provider returning 404 for the resource) or a 5xx +# (the server buckled). A cancel is the other case: completed=False at a success status +# (200/302), because the headers already went out and the client aborted mid-stream. This +# threshold is what separates a real failure from a cancel. +DOWNLOAD_FAILURE_MIN_STATUS = 400 class DownloadOutcomeFilter(SimpleListFilter): @@ -428,7 +430,7 @@ def lookups(self, request, model_admin): return [ (self.COMPLETED, 'Completed'), (self.CANCELLED, 'Cancelled mid-download'), - (self.FAILED, 'Failed (server error)'), + (self.FAILED, 'Failed (server or provider error)'), ] def queryset(self, request, queryset): diff --git a/tests/test_download_events_dashboard.py b/tests/test_download_events_dashboard.py index 5cdfefabcad..2d54c1f6edb 100644 --- a/tests/test_download_events_dashboard.py +++ b/tests/test_download_events_dashboard.py @@ -136,7 +136,7 @@ def test_totals_and_split(self): assert data['split']['zip']['count_percent'] == 50 def test_zip_outcomes_split_completed_cancelled_failed(self): - # a completed zip, a user cancel (False at 200), and a server failure (False at 5xx) + # a completed zip, a user cancel (False at 200), and a failure (False at an error status) make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=True, status_code=200) make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=200) make_event(download_type=DownloadEvent.PROJECT, zip_completed=False, status_code=502) @@ -147,15 +147,19 @@ def test_zip_outcomes_split_completed_cancelled_failed(self): assert outcomes == {'completed': 1, 'cancelled': 1, 'failed': 1} - def test_failed_zips_summary_counts_only_server_errors(self): + def test_failed_zips_summary_counts_error_statuses(self): + # any 4xx or 5xx that didn't complete is a failure; a 404 from an add-on provider + # is just as much a failure as a 500 from our server + make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=404) make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=500) make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=503) + # a cancel (ended on a success status) must NOT count as failed make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=200) make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=True, status_code=200) data = self.admin.get_dashboard_data(DownloadEvent.objects.all()) - assert data['summary']['failed_zips'] == 2 + assert data['summary']['failed_zips'] == 3 def test_incomplete_zip_without_a_status_counts_as_cancelled(self): """Callbacks from a WaterButler build predating status_code have status None — treat @@ -169,12 +173,15 @@ def test_incomplete_zip_without_a_status_counts_as_cancelled(self): def test_outcome_column_labels(self): completed = make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=True) cancelled = make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=200) - failed = make_event(download_type=DownloadEvent.PROJECT, zip_completed=False, status_code=500) + failed_5xx = make_event(download_type=DownloadEvent.PROJECT, zip_completed=False, status_code=500) + # an add-on provider 404 is a failure, not a cancel + failed_404 = make_event(download_type=DownloadEvent.FOLDER_ZIP, zip_completed=False, status_code=404) single = make_event(download_type=DownloadEvent.FILE) assert self.admin.outcome(completed) == 'Completed' assert self.admin.outcome(cancelled) == 'Cancelled' - assert self.admin.outcome(failed) == 'Failed' + assert self.admin.outcome(failed_5xx) == 'Failed' + assert self.admin.outcome(failed_404) == 'Failed' assert self.admin.outcome(single) == '—' def test_storage_provider_breakdown(self):