Bug: uploaded at set in wrong places.

[ChrOertlin]

Describe the bug

In cg/cli/deliver/utils.py:deliver_raw_data_for_analyses we will set the uploaded_at date to datetime.now() even if there are no files to deliver. This is the current behaviour, (so not a consequence of the code in this PR) but I still think it should not be that way. This happens because when there are no files the function deliver_files_for_case exits successfully and returns None. The same happens for all the pipeline's UploadAPI method upload.

To Reproduce
Steps to reproduce the behavior:

1. Run command in environment
2. ...

Expected behavior
A clear and concise description of what you expected to happen.

Additional context
Add any other context about the problem here.

Clarification

Description

• The uploading of files from a case with delivery options analysis and/or fastq relies on the function deliver_files_for_case from the DeliverFilesService (cg/services/deliver_files/deliver_files_service/deliver_files_service.py).
• This function is decorated with @handle_no_delivery_files_error, which catches NoDeliveryFilesError and returns None instead -> If no files are found to deliver, no error is raised.
• This deliver_files_for_case function is called in the uploading of files of all pipelines and in the delivery of raw data.
  • In the case of the pipeline uploading, the uploaded_at date is updated with datetime.now() after the function is called and whether or not the files to deliver have been found
  • In the case of raw data, the uploaded started at date is updated after the function is called and whether or not there were files to deliver

Conclusion: If no files are found to be delivered, the program exits successfully and the updated date is updated as if the files were successfully delivered (a message saying that the delivery was successful is printed in the logs).

To reproduce

See first comment

Expected behaviour

Asumptions: If there are no files to be delivered

• Neither updated_at nor uploaded_started_at date should be updated with datetime.now()
• An error should be raised

[diitaz93]

I @diitaz93 will expand this issue with concrete examples and specific modules where this was discovered

[diitaz93]

To reproduce the error (MIP-RNA upload):

1. Find a MIP-RNA case with delivery type == analysis and customer cust000 in stage (e.g. newbug)
2. Delete the case from the case bundle that should be delivered (See the tags in cg/constants/delivery.py)
3. Find the analysis for the case and delete uploaded_at and uploaded_started_at date
4. run cg upload -c newbug

Result:

Initialising Process with binary: /bin/dsmc
Use base call ['/bin/dsmc']
Instantiating delivery service factory
Instantiating delivery rsync service
[FETCH SERVICE] Fetching analysis files for case: newbug
Fetch latest version from bundle newbug
Fetching files from version 45276
Fetch latest version from bundle newbug
Fetching files from version 45276
No files to deliver for case newbug in ticket: 876669

Setting analysis uploaded at for {'case_id': 'newbug', 'uploaded_at': '2024-10-23 14:42:38'}
newbug - uploaded at set to 2024-10-23 14:42:38
REQUEST HEADER {'...'}
RESPONSE STATUS CODE 201
RESPONSE BODY "Success! Uploaded at request sent"

newbug analysis has been successfully uploaded

[diitaz93]

Technical refinement

• Remove the handle_no_delivery_files_error decorator from deliver_files_from_cases
• Add a try-except when this function is called in deliver_ticket (in the for loop)

[islean]

To refine

@Clinical-Genomics/sysdev The deliver_files_for_case method is used extensively throughout the code; perhaps most importantly in the automatic uploads. The refinement seems to have only considered its involvement in deliver ticket. Have we decided on how the error handling should be done for the remaining uses?

[ChrOertlin]

Technical refinement

• Remove the handle_no_delivery_files_error decorator from deliver_files_from_cases
• Add a try-except when this function is called in deliver_ticket (in the for loop)

Why do we remove the error handling here? It should raise an error if there is nothing to deliver.

[islean]

Technical refinement

• Remove the handle_no_delivery_files_error decorator from deliver_files_from_cases
• Add a try-except when this function is called in deliver_ticket (in the for loop)

Why do we remove the error handling here? It should raise an error if there is nothing to deliver.

The error handling in question only serves to hide the NoDeliveryFilesError so if we remove it, an error will be raised:

I guess that some other functionality hinges on these exceptions not being raised though

[ChrOertlin]

Why not raise the error here then?

[islean]

Why not raise the error here then?

That is probably what should happen, I just wanted a clarification if the refinement took into consideration the other places this functionality is used as well? Currently there is only one place in which it is specified that we should add a try/except. But there may be more places where the code will crash if an error is raised.