[PRMP-1063] blank bulk upload reports don't create a file

[AndyFlintNHS]

No description provided.

[NogaNHS]

Is it possible to take this out to a new function?

[AndyFlintNHS]

It feels like scope creep to me as the only functionality that has changed properly is line 173, though I agree that the code would be cleaner this way. @abbas-khan10 what do you reckon?

[abbas-khan10]

I have no preference, I had considered this when first implementing as there is repetition. What were you thinking @NogaNHS? Something like:

def write_and_upload_additional_reports(file_name, headers, data_rows) -> bool:
    if data_rows:
            self.write_additional_report_items_to_csv(
                file_name=file_name, headers=headers, rows_to_write=data_rows
            )

            logger.info("Uploading daily success report file to S3")
            self.s3_service.upload_file(
                s3_bucket_name=self.reports_bucket,
                file_key=f"{self.s3_key_prefix}/{file_name}",
                file_name=f"/tmp/{file_name}",
            )
            return true
       return false

[abbas-khan10]

The bool would serve as a way of customising the log message for each report type as we want to be specific with which report wasn't created (open to discussion obviously) e.g. for the code above:

report_uploaded = write_and_upload_additional_reports(file_name, headers, data_rows)

if not report_uploaded:
    logger.info("No data to report for daily success report file")

[AndyFlintNHS]

As opposed to doing a true/false return, I was going to suggest something more like this, so that it only calls 'write and upload' if it actually needs to:

        if data_rows:
            logger.info("Uploading daily success report file to S3")
            self.write_and_upload_additional_reports(file_name, headers, data_rows)
        else:
            logger.info("No data to report for daily success report file")

    def write_and_upload_additional_reports(self, file_name, headers, data_rows):
        self.write_additional_report_items_to_csv(
            file_name=file_name, headers=headers, rows_to_write=data_rows
        )

        self.s3_service.upload_file(
            s3_bucket_name=self.reports_bucket,
            file_key=f"{self.s3_key_prefix}/{file_name}",
            file_name=f"/tmp/{file_name}",
        )

[AndyFlintNHS]

Logic has been broken out into a new function

[abbas-khan10]

daily restricted*

[AndyFlintNHS]

Corrected

[abbas-khan10]

could we add param types here please? See above method params for examples

[AndyFlintNHS]

Added

[NogaNHS]

Can you add a small unit test for this?

[AndyFlintNHS]

Discussed with Abbas - this logic is being tested by each of the daily report unit tests.