code review feedback

app/main/views/send.py

@@ -847,7 +847,7 @@ def send_notification(service_id, template_id):
     vals = ",".join(values)
     data = f"{data}\r\n{vals}"
 
-    filename = f"one-off:{current_user.name}:{uuid.uuid4()}.csv"
+    filename = f"one-off-{current_user.name}-{uuid.uuid4()}.csv"
     my_data = {"filename": filename, "template_id": template_id, "data": data}
     upload_id = s3upload(service_id, my_data)
     form = CsvUploadForm()
@@ -873,34 +873,18 @@ def send_notification(service_id, template_id):
 
     # We have to wait for the job to run and create the notification in the database
     time.sleep(0.1)
-    notis = notification_api_client.get_notifications_for_service(
+    notifications = notification_api_client.get_notifications_for_service(
         service_id, job_id=upload_id, include_one_off=True
     )
     attempts = 0
-    while notis["total"] == 0 and attempts < 5:
+    while notifications["total"] == 0 and attempts < 5:
-        notis = notification_api_client.get_notifications_for_service(
+        notifications = notification_api_client.get_notifications_for_service(
             service_id, job_id=upload_id, include_one_off=True
         )
         time.sleep(0.1)
         attempts = attempts + 1
-    # TODO we are replacing the original 'one-off send' functionality with a job that
+
-    # we create on the fly.  The purpose for this is to ultimately remove the phone numbers
+    if notifications["total"] == 0 and attempts == 5:
-    # from the db.  However, by running a job we no longer get error messages we used to get.
-    # If the user is in trial mode and trying to send to a phone number they are not allowed
-    # to send to, right now they will see that their job started and the only way they will
-    # know something went wrong, is to sit and watch the status sit as pending for 3 hours
-    # and ultimately switch to failed with no reason why.
-    #
-    # In future, we should block the user from sending to phone numbers they aren't allowed
-    # to send to.
-    # the way to do that would be to make this available to the front end:
-    #
-    # <api>/service/utils/service_allowed_to_send_to
-    #
-    # After that the UI should be making this call as part of the phone number validation
-    # A user in trial mode should not be able to 'send message' to a phone number they are not
-    # allowed to send to
-    if notis["total"] == 0 and attempts == 5:
         # This shows the job we auto-generated for the user
         return redirect(
             url_for(
@@ -915,7 +899,7 @@ def send_notification(service_id, template_id):
             ".view_notification",
             service_id=service_id,
             from_job=upload_id,
-            notification_id=notis["notifications"][0]["id"],
+            notification_id=notifications["notifications"][0]["id"],
             # used to show the final step of the tour (help=3) or not show
             # a back link on a just sent one off notification (help=0)
             help=request.args.get("help"),

app/notify_client/job_api_client.py

@@ -119,16 +119,16 @@ class JobApiClient(NotifyAdminAPIClient):
 
         if scheduled_for:
             scheduled_for = JobApiClient.convert_user_time_to_utc(scheduled_for)
-            data.update({"scheduled_for": scheduled_for})
+            data["scheduled_for"] = scheduled_for
 
         if template_id:
-            data.update({"template_id": template_id})
+            data["template_id"] = template_id
         if original_file_name:
-            data.update({"original_file_name": original_file_name})
+            data["original_file_name"] = original_file_name
         if notification_count:
-            data.update({"notification_count": notification_count})
+            data["notification_count"] = notification_count
         if valid:
-            data.update({"valid": valid})
+            data["valid"] = valid
 
         data = _attach_current_user(data)
         job = self.post(url="/service/{}/job".format(service_id), data=data)

app/templates/views/check/column-errors.html

@@ -11,7 +11,7 @@ Error
 
 {% block backLink %}
 <!--hide back link for one-off sends because the user never created a csv file-->
-{% if recipients.__len__() == 1 and not recipients.allowed_to_send_to and not recipients.missing_column_headers %}
+{% if recipients|length == 1 and not recipients.allowed_to_send_to and not recipients.missing_column_headers %}
     <!--do nothing-->
 {% else  %}
     {{ usaBackLink({ "href": back_link }) }}
@@ -136,7 +136,7 @@ Error
 </div>
 
 <!--hide the upload button and back to top link for one off sends-->
-{% if recipients.__len__() == 1 and not recipients.allowed_to_send_to and not recipients.missing_column_headers %}
+{% if recipients|length == 1 and not recipients.allowed_to_send_to and not recipients.missing_column_headers %}
     <!-- do nothing -->
 {% else %}
 <div class="js-stick-at-top-when-scrolling">

tests/app/main/views/test_send.py

@@ -2823,9 +2823,8 @@ def test_send_notification_shows_error_if_400(
         session["recipient"] = "2028675301"
         session["placeholders"] = {"name": "a" * 900}
 
-    # TODO This part of the test is commented out due to notify-api-679 which is
+
-    # replacing one-off sends with jobs.  The new workflow is not embedded error messages into
+    # This now redirects to the jobs results page
-    # the page properly when the user specifies an invalid phone number
     page = client_request.post(
         "main.send_notification",
         service_id=service_one["id"],
@@ -2833,11 +2832,6 @@ def test_send_notification_shows_error_if_400(
         _expected_status=302,
     )
 
-    # assert normalize_spaces(page.select(".banner-dangerous h1")[0].text) == expected_h1
-    # assert (
-    #    normalize_spaces(page.select(".banner-dangerous p")[0].text)
-    #    == expected_err_details
-    # )
     assert not page.find("input[type=submit]")
 
 
@@ -2867,24 +2861,14 @@ def test_send_notification_shows_email_error_in_trial_mode(
         session["recipient"] = "test@example.com"
         session["placeholders"] = {"date": "foo", "thing": "bar"}
 
-    # TODO This part of the test is commented out due to notify-api-679 which is
+    # Calling this means we successful ran a job so we will be redirect to the jobs page
-    # replacing one-off sends with jobs.  The new workflow is not embedded error messages into
-    # the page properly when the user specifies an invalid phone number
-    # page = client_request.post(
     client_request.post(
         "main.send_notification",
         service_id=SERVICE_ONE_ID,
         template_id=fake_uuid,
-        # _expected_status=302,
+        _expected_status=302,
     )
 
-    # assert normalize_spaces(page.select(".banner-dangerous h1")[0].text) == (
-    #    "You cannot send to this email address"
-    # )
-    # assert normalize_spaces(page.select(".banner-dangerous p")[0].text) == (
-    #    "In trial mode you can only send to yourself and members of your team"
-    # )
-
 
 @pytest.mark.parametrize(
     ("endpoint", "extra_args"),