Skip to content

Conversation

@JamesW1-NHS
Copy link
Contributor

Summary

  • Routine Change
  • ❗ Breaking Change
  • 🤖 Operational or Infrastructure Change
  • ✨ New Feature
  • ⚠️ Potential issues that might be caused by this change

Add any other relevant notes or explanations here. Remove this line if you have nothing to add.

Reviews Required

  • Dev
  • Test
  • Tech Author
  • Product Owner

Review Checklist

ℹ️ This section is to be filled in by the reviewer.

  • I have reviewed the changes in this PR and they fill all or part of the acceptance criteria of the ticket, and the code is in a mergeable state.
  • If there were infrastructure, operational, or build changes, I have made sure there is sufficient evidence that the changes will work.
  • I have ensured the changelog has been updated by the submitter, if necessary.

)


def move_file(bucket_name: str, source_file_key: str, destination_file_key: str) -> None:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo: this function and others like it are prime candidates to be refactored into a utils file e.g. shared/utils.py

I won't do this now, but when we refactor one of the other lambdas which has the identical function (e.g. filenameprocessor). There's a unit test for it (existing only in ack_backend!) which should move as well, with appropriate mocking

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Errors hadn't been covered with their own unit tests at all - just by tests in other modules. This was an omission in code coverage which Sonar didn't pick up (basically because the changes hadn't been large enough!)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are some missing unit tests added in passing

dlzhry2nhs
dlzhry2nhs previously approved these changes Sep 29, 2025
Copy link
Contributor

@dlzhry2nhs dlzhry2nhs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor suggestions with regards to optimising the Dockerfiles we have been creating in the Lambdas directory.

@sonarqubecloud
Copy link

@JamesW1-NHS JamesW1-NHS merged commit 0aa9012 into master Sep 29, 2025
7 checks passed
@JamesW1-NHS JamesW1-NHS deleted the VED-788-refactor-ack-backend branch September 29, 2025 14:22
mfjarvis pushed a commit that referenced this pull request Oct 22, 2025
* init

* lint

* smells

* typo

* imports

* cleanup

* sonarcloud

* sonarcloud II

* local Makefile

* tests for get_s3_client

* tests for get_s3_client II

* delete .coverage

* use_ms_precision

* lint

* shared helper functions

* prefix

* missing cache tests

* test_errors

* terraform fixes per VED-810

* review fixes

* review fixes II

* review fixes III
mfjarvis pushed a commit that referenced this pull request Oct 27, 2025
* init

* lint

* smells

* typo

* imports

* cleanup

* sonarcloud

* sonarcloud II

* local Makefile

* tests for get_s3_client

* tests for get_s3_client II

* delete .coverage

* use_ms_precision

* lint

* shared helper functions

* prefix

* missing cache tests

* test_errors

* terraform fixes per VED-810

* review fixes

* review fixes II

* review fixes III
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants