Delete SEFT collection instrument (#282)

* Delete SEFT collection instrument

_infra/helm/collection-instrument/Chart.yaml

@@ -14,8 +14,8 @@ type: application
 
 # This is the chart version. This version number should be incremented each time you make changes
 # to the chart and its templates, including the app version.
-version: 3.0.0
+version: 3.0.1
 
 # This is the version number of the application being deployed. This version number should be
 # incremented each time you make changes to the application.
-appVersion: 3.0.0
+appVersion: 3.0.1

application/controllers/collection_instrument.py

@@ -3,6 +3,7 @@
 
 import structlog
 from flask import current_app
+from sqlalchemy.orm import Session
 
 from application.controllers.helper import validate_uuid
 from application.controllers.service_helper import (
@@ -275,17 +276,43 @@ def unlink_instrument_from_exercise(self, instrument_id, exercise_id, session=No
             bound_logger.info("Failed to unlink, unable to find instrument or exercise")
             raise RasError("Unable to find instrument or exercise", 404)
 
-        instrument.exercises.remove(exercise)
-        for business in instrument.businesses:
-            bound_logger.info("Removing business/exercise link", business_id=business.id, ru_ref=business.ru_ref)
-            business.instruments.remove(instrument)
+        if instrument.type == "SEFT":
+            # SEFT instruments need to be deleted from both GCP and the db, removing just the link will leave orphaned
+            # data. When deleting SEFT instruments, the link will automatically be removed without this function
+            raise RasError(
+                f"{instrument_id} is of type SEFT which should be deleted and not unlinked",
+                405,
+            )
 
+        instrument.exercises.remove(exercise)
         response = self.publish_remove_collection_instrument(exercise_id, instrument.instrument_id)
         if response.status_code != 200:
             raise RasError("Failed to publish upload message", 500)
         bound_logger.info("Successfully unlinked instrument to exercise")
         return True
 
+    @with_db_session
+    def delete_seft_collection_instrument(self, instrument_id: str, session: Session = None) -> None:
+        """
+        Deletes a seft collection instrument from the database and gcs
+        :param instrument_id: A collection instrument id (UUID)
+        :param session: database session
+        """
+        instrument = self.get_instrument_by_id(instrument_id, session)
+
+        if not instrument:
+            raise RasError(f"Collection instrument {instrument_id} not found", 404)
+        if instrument.type != "SEFT":
+            raise RasError(
+                f"Only SEFT collection instruments can be deleted {instrument_id} has type {instrument.type}",
+                405,
+            )
+
+        session.delete(instrument)
+        gcs_seft_bucket = GoogleCloudSEFTCIBucket(current_app.config)
+        file_path = self._build_seft_file_path(instrument)
+        gcs_seft_bucket.delete_file_from_bucket(file_path)
+
     @staticmethod
     def publish_remove_collection_instrument(exercise_id, instrument_id):
         """
@@ -418,9 +445,8 @@ def _update_seft_file(seft_model, file, survey_ref, exercise_id):
         seft_ci_bucket.upload_file_to_bucket(file=file)
         return seft_model
 
-    @staticmethod
     @with_db_session
-    def get_instruments_by_exercise_id_csv(exercise_id, session=None):
+    def get_instruments_by_exercise_id_csv(self, exercise_id, session=None):
         """
         Finds all collection instruments associated with an exercise and returns them in csv format
 
@@ -441,9 +467,7 @@ def get_instruments_by_exercise_id_csv(exercise_id, session=None):
 
         for instrument in exercise.instruments:
             try:
-                survey_ref = get_survey_ref(instrument.survey.survey_id)
-                exercise_id = str(instrument.exids[0])
-                file_path = survey_ref + "/" + exercise_id + "/" + instrument.seft_file.file_name
+                file_path = self._build_seft_file_path(instrument)
                 seft_ci_bucket = GoogleCloudSEFTCIBucket(current_app.config)
                 file = seft_ci_bucket.download_file_from_bucket(file_path)
                 csv += csv_format.format(
@@ -471,9 +495,8 @@ def get_instrument_json(instrument_id, session):
         instrument_json = instrument.json if instrument else None
         return instrument_json
 
-    @staticmethod
     @with_db_session
-    def get_instrument_data(instrument_id, session):
+    def get_instrument_data(self, instrument_id, session):
         """
         Get the instrument data from the db or bucket using the id
 
@@ -489,9 +512,7 @@ def get_instrument_data(instrument_id, session):
 
         if instrument:
             try:
-                survey_ref = get_survey_ref(instrument.survey.survey_id)
-                exercise_id = str(instrument.exids[0])
-                file_path = survey_ref + "/" + exercise_id + "/" + instrument.seft_file.file_name
+                file_path = self._build_seft_file_path(instrument)
                 seft_ci_bucket = GoogleCloudSEFTCIBucket(current_app.config)
                 file = seft_ci_bucket.download_file_from_bucket(file_path)
                 return file, instrument.seft_file.file_name
@@ -566,3 +587,9 @@ def _build_model_joins(json_search_parameters, session):
                 query = query.join(SurveyModel, InstrumentModel.survey)
                 already_joined.append(SurveyModel)
         return query
+
+    @staticmethod
+    def _build_seft_file_path(instrument) -> str:
+        survey_ref = get_survey_ref(instrument.survey.survey_id)
+        exercise_id = str(instrument.exids[0])
+        return f"{survey_ref}/{exercise_id}/{instrument.seft_file.file_name}"

application/models/google_cloud_bucket.py

@@ -51,10 +51,7 @@ def download_file_from_bucket(self, file_location: str):
         return file
 
     def delete_file_from_bucket(self, file_location: str):
-        if self.prefix != "":
-            path = self.prefix + "/" + file_location
-        else:
-            path = file_location
+        path = self.prefix + "/" + file_location if self.prefix != "" else file_location
         log.info("Deleting SEFT CI from GCP bucket: " + path)
         try:
             self.bucket.delete_blob(path)

application/models/models.py

@@ -40,10 +40,16 @@ class InstrumentModel(Base):
     survey_id = Column(Integer, ForeignKey("survey.id"))
     classifiers = Column(JSONB)
     survey = relationship("SurveyModel", back_populates="instruments")
-    seft_file = relationship("SEFTModel", uselist=False, back_populates="instrument")
+    seft_file = relationship("SEFTModel", uselist=False, back_populates="instrument", cascade="all, delete-orphan")
 
     exercises = relationship("ExerciseModel", secondary=instrument_exercise_table, back_populates="instruments")
-    businesses = relationship("BusinessModel", secondary=instrument_business_table, back_populates="instruments")
+    businesses = relationship(
+        "BusinessModel",
+        secondary=instrument_business_table,
+        back_populates="instruments",
+        single_parent=True,
+        cascade="all, delete-orphan",
+    )
 
     def __init__(self, classifiers=None, ci_type=None):
         """Initialise the class with optionally supplied defaults"""

application/views/collection_instrument_view.py

@@ -13,6 +13,7 @@
 collection_instrument_view = Blueprint("collection_instrument_view", __name__)
 
 COLLECTION_INSTRUMENT_NOT_FOUND = "Collection instrument not found"
+SEFT_COLLECTION_INSTRUMENT_DELETED_SUCCESSFUL = "SEFT collection instrument deleted successfully"
 NO_INSTRUMENT_FOR_EXERCISE = "There are no collection instruments for that exercise id"
 UPLOAD_SUCCESSFUL = "The upload was successful"
 PATCH_SUCCESSFUL = "The patch of the instrument was successful"
@@ -79,6 +80,12 @@ def unlink_collection_instrument(instrument_id, exercise_id):
     return make_response(UNLINK_SUCCESSFUL, 200)
 
 
+@collection_instrument_view.route("/delete/<instrument_id>", methods=["DELETE"])
+def delete_seft_collection_instrument(instrument_id):
+    CollectionInstrument().delete_seft_collection_instrument(instrument_id)
+    return make_response(SEFT_COLLECTION_INSTRUMENT_DELETED_SUCCESSFUL, 200)
+
+
 @collection_instrument_view.route("/download_csv/<exercise_id>", methods=["GET"])
 def download_csv(exercise_id):
     csv = CollectionInstrument().get_instruments_by_exercise_id_csv(exercise_id)

tests/controllers/test_collection_instrument.py

@@ -20,6 +20,7 @@
 from tests.test_client import TestClient
 
 TEST_FILE_LOCATION = "tests/files/test.xlsx"
+COLLECTION_EXERCISE_ID = "db0711c3-0ac8-41d3-ae0e-567e5ea1ef87"
 url_collection_instrument_link_url = "http://localhost:8145/collection-instrument/link"
 
 
@@ -121,10 +122,59 @@ def test_get_instrument_by_search_string_invalid_search_value(self):
         with self.assertRaises(RasDatabaseError):
             self.collection_instrument.get_instrument_by_search_string('{"COLLECTION_EXERCISE": "invalid_uuid"}')
 
+    @patch("application.controllers.collection_instrument.GoogleCloudSEFTCIBucket")
+    @requests_mock.mock()
+    def test_delete_seft_collection_instrument(self, mock_bucket, mock_request):
+        mock_bucket.delete_file_from_bucket.return_value = True
+        mock_request.get(
+            "http://localhost:8080/surveys/cb0711c3-0ac8-41d3-ae0e-567e5ea1ef87",
+            status_code=200,
+            json={"surveyId": "cb0711c3-0ac8-41d3-ae0e-567e5ea1ef87", "surveyRef": "139"},
+        )
+        seft_instrument_id = str(self.instrument_id)
+        self.collection_instrument.delete_seft_collection_instrument(seft_instrument_id)
+        instrument = self.collection_instrument.get_instrument_json(seft_instrument_id)
+
+        self.assertEqual(instrument, None)
+
+    def test_delete_seft_collection_instrument_not_found(self):
+        with self.assertRaises(RasError) as error:
+            self.collection_instrument.delete_seft_collection_instrument("8b4a214b-466b-4882-90a1-fe90ad59e2fc")
+
+        self.assertEqual(404, error.exception.status_code)
+        self.assertEqual(
+            ["Collection instrument 8b4a214b-466b-4882-90a1-fe90ad59e2fc not found"],
+            error.exception.errors,
+        )
+
+    def test_delete_eq_collection_instrument(self):
+        eq_collection_instrument_id = self.add_instrument_data(ci_type="EQ")
+        with self.assertRaises(RasError) as error:
+            self.collection_instrument.delete_seft_collection_instrument(str(eq_collection_instrument_id))
+
+        self.assertEqual(405, error.exception.status_code)
+        self.assertEqual(
+            [f"Only SEFT collection instruments can be deleted {eq_collection_instrument_id} has type EQ"],
+            error.exception.errors,
+        )
+
+    def test_unlink_instrument_from_exercise_seft(self):
+        eq_collection_instrument = self.add_instrument_data()
+        with self.assertRaises(RasError) as error:
+            self.collection_instrument.unlink_instrument_from_exercise(
+                str(eq_collection_instrument), COLLECTION_EXERCISE_ID
+            )
+
+        self.assertEqual(405, error.exception.status_code)
+        self.assertEqual(
+            [f"{eq_collection_instrument} is of type SEFT which should be deleted and not unlinked"],
+            error.exception.errors,
+        )
+
     def test_get_instrument_by_incorrect_id(self):
         # Given there is an instrument in the db
         # When an incorrect instrument id is used to find that instrument
-        instrument = self.collection_instrument.get_instrument_json("db0711c3-0ac8-41d3-ae0e-567e5ea1ef87")
+        instrument = self.collection_instrument.get_instrument_json(COLLECTION_EXERCISE_ID)
 
         # Then that instrument is not found
         self.assertEqual(instrument, None)
@@ -133,8 +183,7 @@ def test_get_instrument_by_incorrect_id(self):
     def test_publish_uploaded_collection_instrument(self, mock_request):
         mock_request.post(url_collection_instrument_link_url, status_code=200)
         # Given there is an instrument in the db
-        c_id = "db0711c3-0ac8-41d3-ae0e-567e5ea1ef87"
-        result = publish_uploaded_collection_instrument(c_id, self.instrument_id)
+        result = publish_uploaded_collection_instrument(COLLECTION_EXERCISE_ID, self.instrument_id)
 
         # Then the message is successfully published
         self.assertEqual(result.status_code, 200)
@@ -143,20 +192,20 @@ def test_publish_uploaded_collection_instrument(self, mock_request):
     def test_publish_uploaded_collection_instrument_fails(self, mock_request):
         mock_request.post(url_collection_instrument_link_url, status_code=500)
         # Given there is an instrument in the db
-        c_id = "db0711c3-0ac8-41d3-ae0e-567e5ea1ef87"
         with self.assertRaises(Exception):
-            publish_uploaded_collection_instrument(c_id, self.instrument_id)
+            publish_uploaded_collection_instrument(COLLECTION_EXERCISE_ID, self.instrument_id)
 
     @staticmethod
     @with_db_session
-    def add_instrument_data(session=None):
-        instrument = InstrumentModel(ci_type="SEFT")
-        seft_file = SEFTModel(instrument_id=instrument.instrument_id, file_name="test_file")
-        exercise = ExerciseModel(exercise_id="db0711c3-0ac8-41d3-ae0e-567e5ea1ef87")
-        business = BusinessModel(ru_ref="test_ru_ref")
-        instrument.seft_file = seft_file
+    def add_instrument_data(session=None, ci_type="SEFT"):
+        instrument = InstrumentModel(ci_type=ci_type)
+        exercise = ExerciseModel(exercise_id=COLLECTION_EXERCISE_ID)
         instrument.exercises.append(exercise)
-        instrument.businesses.append(business)
+        if ci_type == "SEFT":
+            seft_file = SEFTModel(instrument_id=instrument.instrument_id, file_name="test_file")
+            business = BusinessModel(ru_ref="test_ru_ref")
+            instrument.seft_file = seft_file
+            instrument.businesses.append(business)
         survey = SurveyModel(survey_id="cb0711c3-0ac8-41d3-ae0e-567e5ea1ef87")
         instrument.survey = survey
         session.add(instrument)