Feedback from code review

microsetta_private_api/api/_survey.py

@@ -537,12 +537,12 @@ def get_skin_scoring_app_credentials(account_id, source_id, token_info):
 
     with Transaction() as t:
         st_repo = SurveyTemplateRepo(t)
-        ssa_u, ssa_p = st_repo.get_skin_scoring_app_credentials_if_exists(
+        ssa_u, ssa_s = st_repo.get_skin_scoring_app_credentials_if_exists(
             account_id, source_id
         )
         response_obj = {
             "app_username": ssa_u,
-            "app_password": ssa_p
+            "app_studycode": ssa_s
         }
         return jsonify(response_obj), 200
 
@@ -567,7 +567,7 @@ def post_skin_scoring_app_credentials(account_id, source_id, token_info):
 
         # Now, try to allocate credentials and create an entry in the skin
         # scoring app registry table
-        ssa_u, ssa_p = st_repo.create_skin_scoring_app_entry(
+        ssa_u, ssa_s = st_repo.create_skin_scoring_app_entry(
             account_id, source_id
         )
         t.commit()
@@ -583,6 +583,6 @@ def post_skin_scoring_app_credentials(account_id, source_id, token_info):
             return jsonify(
                 {
                     "app_username": ssa_u,
-                    "app_password": ssa_p
+                    "app_studycode": ssa_s
                 }
             ), 201

microsetta_private_api/api/microsetta_private_api.yaml

@@ -1045,7 +1045,7 @@ paths:
                   app_username:
                     type: string
                     nullable: true
-                  app_password:
+                  app_studycode:
                     type: string
                     nullable: true
         '401':
@@ -1071,7 +1071,7 @@ paths:
                 properties:
                   app_username:
                     type: string
-                  app_password:
+                  app_studycode:
                     type: string
         '400':
           description: 'Credentials already exist for source'

microsetta_private_api/api/tests/test_api.py

@@ -181,7 +181,8 @@
     'sample_projects': ['American Gut Project'],
     'account_id': None,
     'source_id': None,
-    'sample_site': None}
+    'sample_site': None,
+    'sample_project_ids': [1]}
 
 DUMMY_FILLED_SAMPLE_INFO = {
     'sample_barcode': BARCODE,
@@ -195,7 +196,8 @@
     'sample_projects': ['American Gut Project'],
     'account_id': 'foobar',
     'source_id': 'foobarbaz',
-    'sample_site': 'Saliva'}
+    'sample_site': 'Saliva',
+    'sample_project_ids': [1]}
 
 ACCT_ID_KEY = "account_id"
 ACCT_TYPE_KEY = "account_type"
@@ -594,7 +596,8 @@ def create_dummy_sample_objects(filled=False):
                     info_dict['account_id'],
                     None,
                     info_dict["sample_projects"],
-                    None)
+                    None,
+                    sample_project_ids=info_dict["sample_project_ids"])
 
     return sample_info, sample
 # endregion help methods
@@ -2244,6 +2247,10 @@ def test_associate_sample_to_source_success(self):
         exp['account_id'] = ACCT_ID_1
         exp['kit_id'] = None
 
+        # Remove the sample_project_ids element since we don't expect that
+        # to come out of the API
+        exp.pop("sample_project_ids")
+
         self.assertEqual(get_resp_obj, [exp])
 
         # TODO: We should also have tests of associating a sample to a source

microsetta_private_api/db/migration_support.py

@@ -798,7 +798,7 @@ def migrate_133(TRN):
     @staticmethod
     def migrate_144(TRN):
         # We need to load the credentials that the vendor provided in CSV form
-        # Format is username, password and includes a header row
+        # Format is username, studycode and includes a header row
         skin_app_credentials_path = SERVER_CONFIG["skin_app_credentials_path"]
         if not os.path.exists(skin_app_credentials_path):
             print(
@@ -814,13 +814,13 @@ def migrate_144(TRN):
                 if header:
                     header = False
                     continue
-                app_username, app_password = csv_row
+                app_username, app_studycode = csv_row
 
                 TRN.add(
                     "INSERT INTO ag.skin_scoring_app_credentials "
-                    "(app_username, app_password) "
+                    "(app_username, app_studycode) "
                     "VALUES (%s, %s)",
-                    (app_username, app_password)
+                    (app_username, app_studycode)
                 )
         TRN.execute()
 

microsetta_private_api/db/patches/0144.sql

@@ -1,11 +1,11 @@
 -- The organization that hosts the skin-scoring app provides us with username
--- and password pairings for participants to access the app. We need to store
+-- and studycode pairings for participants to access the app. We need to store
 -- these pairings, as well as a flag for whether the pairing has been
 -- allocated to a participant. We explicitly store this flag to avoid reuse
 -- if sources (and their related survey databsase records) were to be deleted.
 CREATE TABLE ag.skin_scoring_app_credentials (
     app_username VARCHAR PRIMARY KEY,
-    app_password VARCHAR NOT NULL,
+    app_studycode VARCHAR NOT NULL,
     credentials_allocated BOOLEAN NOT NULL DEFAULT FALSE
 );
 

microsetta_private_api/model/sample.py

@@ -6,7 +6,7 @@ class Sample(ModelBase):
     def __init__(self, sample_id, datetime_collected, site, notes, barcode,
                  latest_scan_timestamp, source_id, account_id,
                  latest_sample_information_update, sample_projects,
-                 latest_scan_status, kit_id=None):
+                 latest_scan_status, kit_id=None, sample_project_ids=None):
         self.id = sample_id
         # NB: datetime_collected may be None if sample not yet used
         self.datetime_collected = datetime_collected
@@ -28,9 +28,14 @@ def __init__(self, sample_id, datetime_collected, site, notes, barcode,
         self.accession_urls = []
         self.kit_id = kit_id
 
+        self._sample_project_ids = sample_project_ids
+
     def set_accession_urls(self, accession_urls):
         self.accession_urls = accession_urls
 
+    def get_project_ids(self):
+        return self._sample_project_ids
+
     @property
     def edit_locked(self):
         # If a sample has been scanned and is valid, it is locked.
@@ -47,7 +52,8 @@ def remove_locked(self):
     def from_db(cls, sample_id, date_collected, time_collected,
                 site, notes, barcode, latest_scan_timestamp,
                 latest_sample_information_update, source_id,
-                account_id, sample_projects, latest_scan_status):
+                account_id, sample_projects, latest_scan_status,
+                sample_project_ids):
         datetime_collected = None
         # NB a sample may NOT have date and time collected if it has been sent
         # out but not yet used
@@ -56,7 +62,8 @@ def from_db(cls, sample_id, date_collected, time_collected,
                                                   time_collected)
         return cls(sample_id, datetime_collected, site, notes, barcode,
                    latest_scan_timestamp, latest_sample_information_update,
-                   source_id, account_id, sample_projects, latest_scan_status)
+                   source_id, account_id, sample_projects, latest_scan_status,
+                   sample_project_ids=sample_project_ids)
 
     def to_api(self):
         return {

microsetta_private_api/repo/sample_repo.py

@@ -61,24 +61,36 @@ class SampleRepo(BaseRepo):
     def __init__(self, transaction):
         super().__init__(transaction)
 
-    def _retrieve_projects(self, sample_barcode):
+    def _retrieve_projects(self, sample_barcode, return_ids=False):
         with self._transaction.cursor() as cur:
-            # If there is a sample, we can look for the projects associated
-            #  with it.  We do this as a secondary query:
-            cur.execute("SELECT barcodes.project.project FROM "
-                        "barcodes.barcode "
-                        "LEFT JOIN "
-                        "barcodes.project_barcode "
-                        "ON "
-                        "barcodes.barcode.barcode = "
-                        "barcodes.project_barcode.barcode "
-                        "LEFT JOIN barcodes.project "
-                        "ON "
-                        "barcodes.project_barcode.project_id = "
-                        "barcodes.project.project_id "
-                        "WHERE "
-                        "barcodes.barcode.barcode = %s",
-                        (sample_barcode,))
+            if return_ids is True:
+                # If the caller wants the project IDs, we can directly
+                # query the project_barcode table
+                cur.execute(
+                    "SELECT project_id "
+                    "FROM barcodes.project_barcode "
+                    "WHERE barcode = %s",
+                    (sample_barcode,)
+                )
+            else:
+                # Otherwise, we defualt to the existing behavior of returning
+                # the project names.
+                # If there is a sample, we look for the projects associated
+                # with it.  We do this as a secondary query:
+                cur.execute("SELECT barcodes.project.project FROM "
+                            "barcodes.barcode "
+                            "LEFT JOIN "
+                            "barcodes.project_barcode "
+                            "ON "
+                            "barcodes.barcode.barcode = "
+                            "barcodes.project_barcode.barcode "
+                            "LEFT JOIN barcodes.project "
+                            "ON "
+                            "barcodes.project_barcode.project_id = "
+                            "barcodes.project.project_id "
+                            "WHERE "
+                            "barcodes.barcode.barcode = %s",
+                            (sample_barcode,))
 
             project_rows = cur.fetchall()
             sample_projects = [project[0] for project in project_rows]
@@ -91,9 +103,13 @@ def _create_sample_obj(self, sample_row):
         sample_barcode = sample_row[5]
         scan_timestamp = sample_row[6]
         sample_projects = self._retrieve_projects(sample_barcode)
+        sample_project_ids = self._retrieve_projects(
+            sample_barcode, return_ids=True
+        )
         sample_status = self.get_sample_status(sample_barcode, scan_timestamp)
 
-        return Sample.from_db(*sample_row, sample_projects, sample_status)
+        return Sample.from_db(*sample_row, sample_projects, sample_status,
+                              sample_project_ids=sample_project_ids)
 
     # TODO: I'm still not entirely happy with the linking between samples and
     #  sources.  The new source_id is direct (and required for environmental
@@ -261,9 +277,6 @@ def get_samples_by_source(self, account_id, source_id,
                 sample.kit_id = self._get_supplied_kit_id_by_sample(
                     sample.barcode
                 )
-                sample.project_id = self._get_project_ids_by_sample(
-                    sample.barcode
-                )
                 samples.append(sample)
             return samples
 
@@ -410,19 +423,6 @@ def _get_supplied_kit_id_by_sample(self, sample_barcode):
             row = cur.fetchone()
             return row[0]
 
-    def _get_project_ids_by_sample(self, sample_barcode):
-        with self._transaction.cursor() as cur:
-            cur.execute(
-                "SELECT project_id "
-                "FROM barcodes.project_barcode "
-                "WHERE barcode = %s",
-                (sample_barcode, )
-            )
-            rows = cur.fetchall()
-
-            project_ids = [row[0] for row in rows]
-            return project_ids
-
     def scrub(self, account_id, source_id, sample_id):
         """Wipe out free text information for a sample
 

microsetta_private_api/repo/survey_template_repo.py

@@ -782,7 +782,7 @@ def create_skin_scoring_app_entry(self,
         str or None
             The username allocated to the source, or None if process fails
         str or None
-            The password allocated to the source, or None if process fails
+            The studycode allocated to the source, or None if process fails
         """
 
         # We need to lock both tables relevant to app credentials
@@ -791,7 +791,7 @@ def create_skin_scoring_app_entry(self,
 
         with self._transaction.cursor() as cur:
             cur.execute(
-                "SELECT app_username, app_password "
+                "SELECT app_username, app_studycode "
                 "FROM ag.skin_scoring_app_credentials "
                 "WHERE credentials_allocated = FALSE "
                 "LIMIT 1"
@@ -802,7 +802,7 @@ def create_skin_scoring_app_entry(self,
                 return None, None
             else:
                 app_username = row[0]
-                app_password = row[1]
+                app_studycode = row[1]
 
                 # Mark the credentials as allocated
                 cur.execute(
@@ -830,12 +830,12 @@ def create_skin_scoring_app_entry(self,
                      source_id, SurveyTemplateRepo.SKIN_SCORING_APP_ID)
                 )
 
-                return app_username, app_password
+                return app_username, app_studycode
 
     def get_skin_scoring_app_credentials_if_exists(self,
                                                    account_id,
                                                    source_id):
-        """Returns a Skin Scoring App username/password set if they exist
+        """Returns a Skin Scoring App username/studycode set if it exists
 
         Parameters
         ----------
@@ -846,15 +846,15 @@ def get_skin_scoring_app_credentials_if_exists(self,
 
         Returns
         -------
-        (str) or (None)
+        str or None
             The associated Skin Scoring App username
-        (str) or (None)
-            The associated Skin Scoring App password
+        str or None
+            The associated Skin Scoring App studycode
         """
         with self._transaction.cursor() as cur:
             cur.execute(
                 """
-                SELECT ssac.app_username, ssac.app_password
+                SELECT ssac.app_username, ssac.app_studycode
                 FROM ag.skin_scoring_app_credentials ssac
                 INNER JOIN ag.skin_scoring_app_registry ssar
                 ON ssac.app_username = ssar.app_username
@@ -1519,7 +1519,7 @@ def check_prompt_survey_update(
         return True
 
     def check_skin_scoring_app_credentials_available(self):
-        """ Checks whether any username/password pairings in the
+        """ Checks whether any username/studycode pairings in the
             ag.skin_scoring_app_credentials table are available to allocate
 
         Returns
@@ -1571,7 +1571,7 @@ def check_display_skin_scoring_app(self, account_id, source_id):
             if samples:
                 has_skin_sample = any(
                     self.SBI_COHORT_PROJECT_ID
-                    in s.project_id for s in samples
+                    in s.get_project_ids() for s in samples
                 )
             else:
                 has_skin_sample = False

microsetta_private_api/repo/tests/test_sample.py

@@ -168,30 +168,6 @@ def test_get_supplied_kit_id_by_sample(self):
             )
             self.assertEqual(kit_id, supplied_kit_id)
 
-    def test_get_project_ids_by_sample(self):
-        with Transaction() as t:
-            # First we'll create a kit so that we have a kit_id/barcode combo
-            # to test with
-            admin_repo = AdminRepo(t)
-            res = admin_repo.create_kits(
-                1,
-                1,
-                "UNITTEST",
-                [1, 2]
-            )
-
-            # Extract the info from results. We know we created 1 kit with 1
-            # sample so we don't need to iterate
-            kit_info = res['created'][0]
-            sample_barcode = kit_info['sample_barcodes'][0]
-
-            # Verify that the function returns the correct project_id
-            sample_repo = SampleRepo(t)
-            project_ids = sample_repo._get_project_ids_by_sample(
-                sample_barcode
-            )
-            self.assertEqual([1, 2], project_ids)
-
 
 if __name__ == '__main__':
     unittest.main()