Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comma escape #1191

Draft
wants to merge 23 commits into
base: main
Choose a base branch
from
Draft

Comma escape #1191

wants to merge 23 commits into from

Conversation

A-Shumway42
Copy link
Member

@A-Shumway42 A-Shumway42 commented Jul 17, 2024

Description

We are trying to find a way to figure out the comma issue for csv's/one-offs when a user has extra commas inside of fields. Added debug statements to help see what's happening in extract_personalisation.

I've added more debug statements to try and figure out where the issue is coming from in the api. The value sent from the front-end during a one-off send is wrapped in quotes like so "July 23, 2024" and everything seems to be correct in the debug statements except for the "To" number. I think the phone number is still being swapped for one of the other values somewhere in the create_job process.

TODO (optional)

  • TODO item 1
  • TODO item 2
  • TODO item ...

Security Considerations

@@ -86,6 +86,7 @@
notification.job_id,
notification.job_row_number,
)
print(hilite(f"recipient: {recipient}"))

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (private)
as clear text.
@@ -108,6 +109,7 @@
"sender": notification.reply_to_text,
"international": notification.international,
}
print(hilite(f"send_sms_kwargs: {send_sms_kwargs}"))

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information High

This expression logs
sensitive data (private)
as clear text.
Copy link
Member Author

Choose a reason for hiding this comment

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

I believe this is okay to keep here for the moment for debugging purposes.

@A-Shumway42
Copy link
Member Author

Stack trace with the current error I am stuck on.

13:29:54 worker.1    |     message_id = send_to_providers.send_sms_to_provider(notification)
13:29:54 worker.1    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13:29:54 worker.1    |   File "/Users/andrewshumway/work/notify-api-local/notifications-api/app/delivery/send_to_providers.py", line 124, in send_sms_to_provider
13:29:54 worker.1    |     raise e
13:29:54 worker.1    |   File "/Users/andrewshumway/work/notify-api-local/notifications-api/app/delivery/send_to_providers.py", line 115, in send_sms_to_provider
13:29:54 worker.1    |     message_id = provider.send_sms(**send_sms_kwargs)
13:29:54 worker.1    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13:29:54 worker.1    |   File "/Users/andrewshumway/work/notify-api-local/notifications-api/app/clients/sms/aws_sns.py", line 99, in send_sms
13:29:54 worker.1    |     raise ValueError("No valid numbers found for SMS delivery")
13:29:54 worker.1    | ValueError: No valid numbers found for SMS delivery
13:29:54 worker.1    | [2024-07-23 13:29:54,719: ERROR/ForkPoolWorker-4] SMS notification delivery for id: 44c39de1-4a02-44eb-ab4e-fd3fc62b3345 failed
13:29:54 worker.1    | Traceback (most recent call last):
13:29:54 worker.1    |   File "/Users/andrewshumway/work/notify-api-local/notifications-api/app/celery/provider_tasks.py", line 114, in deliver_sms
13:29:54 worker.1    |     message_id = send_to_providers.send_sms_to_provider(notification)
13:29:54 worker.1    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13:29:54 worker.1    |   File "/Users/andrewshumway/work/notify-api-local/notifications-api/app/delivery/send_to_providers.py", line 124, in send_sms_to_provider
13:29:54 worker.1    |     raise e
13:29:54 worker.1    |   File "/Users/andrewshumway/work/notify-api-local/notifications-api/app/delivery/send_to_providers.py", line 115, in send_sms_to_provider
13:29:54 worker.1    |     message_id = provider.send_sms(**send_sms_kwargs)
13:29:54 worker.1    |                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
13:29:54 worker.1    |   File "/Users/andrewshumway/work/notify-api-local/notifications-api/app/clients/sms/aws_sns.py", line 99, in send_sms
13:29:54 worker.1    |     raise ValueError("No valid numbers found for SMS delivery")
13:29:54 worker.1    | ValueError: No valid numbers found for SMS delivery```

Copy link
Contributor

@xlorepdarkhelm xlorepdarkhelm left a comment

Choose a reason for hiding this comment

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

Looks like a job for isort to make this clear up in the tests. Otherwise, LGTM

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.

2 participants