From 97fcf094b8bca97eceeb4560c82b8dc80e9f6e5a Mon Sep 17 00:00:00 2001 From: "Hassan D. M. Sambo" Date: Fri, 23 Aug 2024 18:46:07 -0400 Subject: [PATCH] Removed logic to persist workbook upon download from dissemination UI --- .../views/pre_dissemination_download_view.py | 11 +- backend/dissemination/summary_reports.py | 53 ++--- backend/dissemination/test_summary_reports.py | 2 +- backend/dissemination/test_views.py | 220 ++++++++++++------ backend/dissemination/views.py | 31 ++- 5 files changed, 195 insertions(+), 122 deletions(-) diff --git a/backend/audit/views/pre_dissemination_download_view.py b/backend/audit/views/pre_dissemination_download_view.py index 950c718359..788095b959 100644 --- a/backend/audit/views/pre_dissemination_download_view.py +++ b/backend/audit/views/pre_dissemination_download_view.py @@ -1,3 +1,4 @@ +from django.http import HttpResponse from django.shortcuts import get_object_or_404, redirect from django.views.generic import View @@ -33,7 +34,11 @@ def get(self, request, report_id): ) i2d_data = intake_to_dissem.load_all() - filename = generate_presubmission_report(i2d_data) - download_url = get_download_url(filename) + filename, workbook_bytes = generate_presubmission_report(i2d_data) + response = HttpResponse( + workbook_bytes, + content_type="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + response["Content-Disposition"] = f"attachment; filename={filename}" - return redirect(download_url) + return response diff --git a/backend/dissemination/summary_reports.py b/backend/dissemination/summary_reports.py index 7921829134..48e32194fe 100644 --- a/backend/dissemination/summary_reports.py +++ b/backend/dissemination/summary_reports.py @@ -5,11 +5,7 @@ import time import openpyxl as pyxl -from boto3 import client as boto3_client -from botocore.client import ClientError, Config - from django.conf import settings -from fs.memoryfs import MemoryFS from openpyxl.workbook.defined_name import DefinedName from openpyxl.utils import quote_sheetname @@ -606,40 +602,19 @@ def create_workbook(data, protect_sheets=False): return (workbook, t1 - t0) -def persist_workbook(workbook): +def prepare_workbook_for_download(workbook): + t0 = time.time() - s3_client = boto3_client( - service_name="s3", - region_name=settings.AWS_S3_PRIVATE_REGION_NAME, - aws_access_key_id=settings.AWS_PRIVATE_ACCESS_KEY_ID, - aws_secret_access_key=settings.AWS_PRIVATE_SECRET_ACCESS_KEY, - endpoint_url=settings.AWS_S3_PRIVATE_INTERNAL_ENDPOINT, - config=Config(signature_version="s3v4"), - ) + now = datetime.utcnow().strftime("%Y%m%d%H%M%S") + filename = f"fac-summary-report-{now}.xlsx" + + # Save the workbook directly to a BytesIO object + workbook_bytes = io.BytesIO() + workbook.save(workbook_bytes) + workbook_bytes.seek(0) - with MemoryFS() as mem_fs: - now = datetime.utcnow().strftime("%Y%m%d%H%M%S") - filename = f"fac-summary-report-{now}.xlsx" - s3_dir = "temp" - - workbook_write_fp = mem_fs.openbin(filename, mode="w") - workbook.save(workbook_write_fp) - workbook_read_fp = mem_fs.openbin(filename, mode="r") - workbook_read_fp.seek(0) - content = workbook_read_fp.read() - workbook_bytes = io.BytesIO(content) - - try: - s3_client.put_object( - Body=workbook_bytes, - Bucket=settings.AWS_PRIVATE_STORAGE_BUCKET_NAME, - Key=f"{s3_dir}/{filename}", - ) - except ClientError: - logger.warn(f"Unable to put summary report file {filename} in S3!") - raise t1 = time.time() - return (f"temp/{filename}", t1 - t0) + return (filename, workbook_bytes, t1 - t0) def generate_summary_report(report_ids, include_private=False): @@ -651,12 +626,12 @@ def generate_summary_report(report_ids, include_private=False): data = separate_notes_single_fields_from_array_fields(data) (workbook, tcw) = create_workbook(data) insert_dissem_coversheet(workbook, bool(tribal_report_ids), include_private) - (filename, tpw) = persist_workbook(workbook) + (filename, workbook_bytes, tpw) = prepare_workbook_for_download(workbook) t1 = time.time() logger.info( f"SUMMARY_REPORTS generate_summary_report\n\ttotal: {t1-t0} ttri: {ttri} tgrdd: {tgrdd} tcw: {tcw} tpw: {tpw}" ) - return filename + return (filename, workbook_bytes) # Ignore performance profiling for the presub. @@ -667,9 +642,9 @@ def generate_presubmission_report(i2d_data): insert_precert_coversheet(workbook) workbook.security.workbookPassword = str(uuid.uuid4()) workbook.security.lockStructure = True - (filename, _) = persist_workbook(workbook) + (filename, workbook_bytes, _) = prepare_workbook_for_download(workbook) - return filename + return (filename, workbook_bytes) def separate_notes_single_fields_from_array_fields(data): diff --git a/backend/dissemination/test_summary_reports.py b/backend/dissemination/test_summary_reports.py index 00a62e26c9..ea29d6febe 100644 --- a/backend/dissemination/test_summary_reports.py +++ b/backend/dissemination/test_summary_reports.py @@ -25,7 +25,7 @@ def test_generate_summary_report_returns_filename(self): baker.make(FederalAward, report_id=g) self.refresh_materialized_view() - filename = generate_summary_report(report_ids) + filename, _ = generate_summary_report(report_ids) self.assertTrue(filename.startswith, "fac-summary-report-") self.assertTrue(filename.endswith, ".xlsx") diff --git a/backend/dissemination/test_views.py b/backend/dissemination/test_views.py index 22b049057d..252bb5846c 100644 --- a/backend/dissemination/test_views.py +++ b/backend/dissemination/test_views.py @@ -1,3 +1,4 @@ +import io from django.conf import settings from django.contrib.auth import get_user_model from django.test import Client, TestCase @@ -672,8 +673,8 @@ def _mock_filename(self): def _mock_download_url(self): return "http://example.com/gsa-fac-private-s3/temp/some-report-name.xlsx" - @patch("dissemination.summary_reports.persist_workbook") - def test_bad_search_returns_400(self, mock_persist_workbook): + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_bad_search_returns_400(self, mock_prepare_workbook_for_download): """ Submitting a form with bad parameters should throw a BadRequest. """ @@ -682,8 +683,8 @@ def test_bad_search_returns_400(self, mock_persist_workbook): ) self.assertEquals(response.status_code, 400) - @patch("dissemination.summary_reports.persist_workbook") - def test_empty_results_returns_404(self, mock_persist_workbook): + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_empty_results_returns_404(self, mock_prepare_workbook_for_download): """ Searches with no results should return a 404, not an empty excel file. """ @@ -695,89 +696,162 @@ def test_empty_results_returns_404(self, mock_persist_workbook): ) self.assertEquals(response.status_code, 404) - @patch("dissemination.views.get_download_url") - @patch("dissemination.summary_reports.persist_workbook") - def test_no_permissions_returns_404_on_private( - self, mock_persist_workbook, mock_get_download_url + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_authorized_user_with_private_data( + self, mock_prepare_workbook_for_download ): - """ - Non-permissioned users can access private audits through the summary report post. - """ - mock_persist_workbook.return_value = self._mock_filename() - mock_get_download_url.return_value = self._mock_download_url() + """Test that an authorized user can access private data.""" + mock_filename = "mocked-report.xlsx" + mock_workbook_bytes = io.BytesIO(b"fake file content") + mock_prepare_workbook_for_download.return_value = ( + mock_filename, + mock_workbook_bytes, + 1.0, + ) general = self._make_general(is_public=False) baker.make(FederalAward, report_id=general) + baker.make(FindingText, report_id=general) self.refresh_materialized_view() - response = self.anon_client.post(self._summary_report_url(), {}) - mock_persist_workbook.assert_called_once() - self.assertRedirects( - response, - self._mock_download_url(), - status_code=302, - target_status_code=200, - fetch_redirect_response=False, - ) - - @patch("dissemination.views.get_download_url") - @patch("dissemination.summary_reports.persist_workbook") - def test_permissions_returns_file_on_private( - self, mock_persist_workbook, mock_get_download_url + + response = self.perm_client.post(self._summary_report_url(), {}) + self.assertEqual(response.status_code, 200) + self.assertEqual( + response["Content-Type"], + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + self.assertIn( + f"attachment; filename={mock_filename}", response["Content-Disposition"] + ) + self.assertEqual(response.content, b"fake file content") + + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_unauthorized_user_with_private_data( + self, mock_prepare_workbook_for_download ): - """ - Permissioned users receive a file if there are private results. - """ - mock_persist_workbook.return_value = self._mock_filename() - mock_get_download_url.return_value = self._mock_download_url() + """Test that an unauthorized user can still receive a file, but without private data.""" + mock_filename = "mocked-report.xlsx" + mock_workbook_bytes = io.BytesIO(b"fake file content") + mock_prepare_workbook_for_download.return_value = ( + mock_filename, + mock_workbook_bytes, + 1.0, + ) general = self._make_general(is_public=False) baker.make(FederalAward, report_id=general) + baker.make(FindingText, report_id=general) self.refresh_materialized_view() + + response = self.anon_client.post(self._summary_report_url(), {}) + self.assertEqual(response.status_code, 200) + self.assertEqual( + response["Content-Type"], + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + self.assertIn( + f"attachment; filename={mock_filename}", response["Content-Disposition"] + ) + self.assertEqual(response.content, b"fake file content") + + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_authorized_user_with_public_data(self, mock_prepare_workbook_for_download): + """Test that an authorized user can access public data.""" + mock_filename = "mocked-report.xlsx" + mock_workbook_bytes = io.BytesIO(b"fake file content") + mock_prepare_workbook_for_download.return_value = ( + mock_filename, + mock_workbook_bytes, + 1.0, + ) + + general = self._make_general(is_public=True) + baker.make(FederalAward, report_id=general) + baker.make(FindingText, report_id=general) + self.refresh_materialized_view() + response = self.perm_client.post(self._summary_report_url(), {}) - mock_persist_workbook.assert_called_once() - self.assertRedirects( - response, - self._mock_download_url(), - status_code=302, - target_status_code=200, - fetch_redirect_response=False, - ) - - @patch("dissemination.views.get_download_url") - @patch("dissemination.summary_reports.persist_workbook") - def test_empty_search_params_returns_file( - self, mock_persist_workbook, mock_get_download_url + self.assertEqual(response.status_code, 200) + self.assertEqual( + response["Content-Type"], + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + self.assertIn( + f"attachment; filename={mock_filename}", response["Content-Disposition"] + ) + self.assertEqual(response.content, b"fake file content") + + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_unauthorized_user_with_public_data( + self, mock_prepare_workbook_for_download ): + """Test that an unauthorized user can access public data.""" + mock_filename = "mocked-report.xlsx" + mock_workbook_bytes = io.BytesIO(b"fake file content") + mock_prepare_workbook_for_download.return_value = ( + mock_filename, + mock_workbook_bytes, + 1.0, + ) + + general = self._make_general(is_public=True) + baker.make(FederalAward, report_id=general) + baker.make(FindingText, report_id=general) + self.refresh_materialized_view() + + response = self.anon_client.post(self._summary_report_url(), {}) + self.assertEqual(response.status_code, 200) + self.assertEqual( + response["Content-Type"], + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + self.assertIn( + f"attachment; filename={mock_filename}", response["Content-Disposition"] + ) + self.assertEqual(response.content, b"fake file content") + + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_empty_search_params_returns_file(self, mock_prepare_workbook_for_download): """ File should be generated on empty search parameters ("search all"). """ - mock_persist_workbook.return_value = self._mock_filename() - mock_get_download_url.return_value = self._mock_download_url() - + mock_filename = "mocked-report.xlsx" + mock_workbook_bytes = io.BytesIO(b"fake file content") + mock_prepare_workbook_for_download.return_value = ( + mock_filename, + mock_workbook_bytes, + 1.0, + ) general = self._make_general(is_public=True) baker.make(FederalAward, report_id=general) self.refresh_materialized_view() response = self.anon_client.post(self._summary_report_url(), {}) - mock_persist_workbook.assert_called_once() - self.assertRedirects( - response, - self._mock_download_url(), - status_code=302, - target_status_code=200, - fetch_redirect_response=False, - ) - - @patch("dissemination.views.get_download_url") - @patch("dissemination.summary_reports.persist_workbook") - def test_many_results_returns_file( - self, mock_persist_workbook, mock_get_download_url - ): + + mock_prepare_workbook_for_download.assert_called_once() + + self.assertEqual(response.status_code, 200) + self.assertEqual( + response["Content-Type"], + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + self.assertIn( + f"attachment; filename={mock_filename}", response["Content-Disposition"] + ) + self.assertEqual(response.content, b"fake file content") + + @patch("dissemination.summary_reports.prepare_workbook_for_download") + def test_many_results_returns_file(self, mock_prepare_workbook_for_download): """ File should still be generated if there are above SUMMARY_REPORT_DOWNLOAD_LIMIT total results. """ - mock_persist_workbook.return_value = self._mock_filename() - mock_get_download_url.return_value = self._mock_download_url() + mock_filename = "mocked-report.xlsx" + mock_workbook_bytes = io.BytesIO(b"fake file content") + mock_prepare_workbook_for_download.return_value = ( + mock_filename, + mock_workbook_bytes, + 1.0, + ) for i in range(4): general = self._make_general( @@ -789,11 +863,15 @@ def test_many_results_returns_file( with self.settings(SUMMARY_REPORT_DOWNLOAD_LIMIT=2): response = self.anon_client.post(self._summary_report_url(), {}) - mock_persist_workbook.assert_called_once() - self.assertRedirects( - response, - self._mock_download_url(), - status_code=302, - target_status_code=200, - fetch_redirect_response=False, + mock_prepare_workbook_for_download.assert_called_once() + + self.assertEqual(response.status_code, 200) + self.assertEqual( + response["Content-Type"], + "application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + self.assertIn( + f"attachment; filename={mock_filename}", response["Content-Disposition"] ) + + self.assertEqual(response.content, b"fake file content") diff --git a/backend/dissemination/views.py b/backend/dissemination/views.py index d74a596485..49be54a841 100644 --- a/backend/dissemination/views.py +++ b/backend/dissemination/views.py @@ -6,7 +6,7 @@ from django.conf import settings from django.core.exceptions import BadRequest, ValidationError from django.core.paginator import Paginator -from django.http import Http404 +from django.http import Http404, HttpResponse from django.shortcuts import get_object_or_404, redirect, render from django.utils import timezone from django.utils.decorators import method_decorator @@ -478,10 +478,18 @@ def get(self, request, report_id): """ sac = get_object_or_404(General, report_id=report_id) include_private = include_private_results(request) - filename = generate_summary_report([sac.report_id], include_private) - download_url = get_download_url(filename) + filename, workbook_bytes = generate_summary_report( + [sac.report_id], include_private + ) + + # Create an HTTP response with the workbook file for download + response = HttpResponse( + workbook_bytes, + content_type="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + response["Content-Disposition"] = f"attachment; filename={filename}" - return redirect(download_url) + return response class MultipleSummaryReportDownloadView(View): @@ -508,10 +516,17 @@ def post(self, request): if len(results) == 0: raise Http404("Cannot generate summary report. No results found.") report_ids = [result.report_id for result in results] - filename = generate_summary_report(report_ids, include_private) - download_url = get_download_url(filename) + filename, workbook_bytes = generate_summary_report( + report_ids, include_private + ) + # Create an HTTP response with the workbook file for download + response = HttpResponse( + workbook_bytes, + content_type="application/vnd.openxmlformats-officedocument.spreadsheetml.sheet", + ) + response["Content-Disposition"] = f"attachment; filename={filename}" - return redirect(download_url) + return response except Http404 as err: logger.info( @@ -520,6 +535,6 @@ def post(self, request): raise Http404 from err except Exception as err: logger.info( - "Enexpected error in MultipleSummaryReportDownloadView post:\n%s", err + "Unexpected error in MultipleSummaryReportDownloadView post:\n%s", err ) raise BadRequest(err)