code review feedback

app/aws/s3.py

@@ -94,7 +94,7 @@ def incr_jobs_cache_hits():
         redis_store.incr(JOBS_CACHE_HITS)
     hits = redis_store.get(JOBS_CACHE_HITS).decode("utf-8")
     misses = redis_store.get(JOBS_CACHE_MISSES).decode("utf-8")
-    current_app.logger.info(f"JOBS CACHE MISS hits {hits} misses {misses}")
+    current_app.logger.info(f"JOBS CACHE HIT hits {hits} misses {misses}")
 
 
 def extract_phones(job):
@@ -135,38 +135,32 @@ def get_phone_number_from_s3(service_id, job_id, job_row_number):
     else:
         incr_jobs_cache_hits()
 
+    # If the job is None after our attempt to retrieve it from s3, it
+    # probably means the job is old and has been deleted from s3, in
+    # which case there is nothing we can do.  It's unlikely to run into
+    # this, but it could theoretically happen, especially if we ever
+    # change the task schedules
     if job is None:
         current_app.logger.warning(
             "Couldnt find phone for job_id {job_id} row number {job_row_number} because job is missing"
         )
         return "Unknown Phone"
 
+    # If we look in the JOBS cache for the quick lookup dictionary of phones for a given job
+    # and that dictionary is not there, create it
     if JOBS.get(f"{job_id}_phones") is None:
         JOBS[f"{job_id}_phones"] = extract_phones(job)
 
+    # If we can find the quick dictionary, use it
     if JOBS.get(f"{job_id}_phones") is not None:
         phone_to_return = JOBS.get(f"{job_id}_phones").get(job_row_number)
         if phone_to_return:
             return phone_to_return
-
-    job = job.split("\r\n")
-    first_row = job[0]
-    job.pop(0)
-    first_row = first_row.split(",")
-    phone_index = 0
-    for item in first_row:
-        if item.lower() == "phone number":
-            break
-        phone_index = phone_index + 1
-
-    correct_row = job[job_row_number]
-    correct_row = correct_row.split(",")
-
-    my_phone = correct_row[phone_index]
-    my_phone = re.sub(r"[\+\s\(\)\-\.]*", "", my_phone)
-    if not my_phone:
-        return "Unknown Phone"
-    return my_phone
+        else:
+            current_app.logger.warning(
+                "Was unable to retrieve phone number from lookup dictionary for job {job_id}"
+            )
+            return "Unknown Phone"
 
 
 def get_job_metadata_from_s3(service_id, job_id):

app/service/rest.py

@@ -433,7 +433,6 @@ def get_all_notifications_for_service(service_id):
                 notification.job_id,
                 notification.job_row_number,
             )
-            print(f"RECIPIENTE IN service/rest {recipient}")
             notification.to = recipient
             notification.normalised_to = recipient
         else: