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

fix: mail notifier subject length issue #65

Merged
merged 4 commits into from
Oct 28, 2021

Conversation

rafaeliga
Copy link
Contributor

Hello,

When sending an email, the reason is added to the subject, but there are cases that the reason has too much information.

When that happens, Bamboo returns this error:

** (Bamboo.ApiError) {:http_error, 400, %{code: "InvalidParameterValue", detail: "", message: "Header too long: 'Subject'.", request_id: "fb0db6bd-3130-4597-8ed8-313aaee84caf", type: "Sender"}} (bamboo 2.2.0) lib/bamboo/strategies/task_supervisor_strategy.ex:25: anonymous fn/3 in Bamboo.TaskSupervisorStrategy.deliver_later/3 (elixir 1.12.2) lib/task/supervised.ex:90: Task.Supervised.invoke_mfa/2 (stdlib 3.14.1) proc_lib.erl:226: :proc_lib.init_p_do_apply/3 Function: #Function<0.6361412/0 in Bamboo.TaskSupervisorStrategy.deliver_later/3>.

My suggestion is that we have a config to handle that but at the same time always truncate the subject so it should work even without the new option.

I've added the full reason to the body so we don't lose that information.

@iaguirre88
Copy link
Member

Do you have an example of the subject that generated that error? Maybe besides truncating the length (that I'm favor of doing) we can also improve how we generate the subject

@rafaeliga
Copy link
Contributor Author

@iaguirre88 If you try to render a html template that does not exists:

render(conn, "index-does-not-exists.html", items: items)

You will see that the reason will include all phoenix data and assigns.

Copy link
Member

@iaguirre88 iaguirre88 left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for the PR 🙌
I left some comments so let me know what do you think

lib/boom_notifier/mail_notifier.ex Outdated Show resolved Hide resolved
test/mailer_notifier_test.exs Outdated Show resolved Hide resolved
test/mailer_notifier_test.exs Outdated Show resolved Hide resolved
iaguirre88
iaguirre88 previously approved these changes Oct 28, 2021
Copy link
Member

@iaguirre88 iaguirre88 left a comment

Choose a reason for hiding this comment

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

Great work 💪
Thanks for your contribution

@iaguirre88
Copy link
Member

@rafaeliga The CI is failing because of the linter. Would you mind you run mix format and push the changes so I can merge it? Thanks ❤️

@rafaeliga
Copy link
Contributor Author

@rafaeliga The CI is failing because of the linter. Would you mind you run mix format and push the changes so I can merge it? Thanks ❤️

Oops! done 👍

@iaguirre88 iaguirre88 merged commit 6f833f4 into wyeworks:master Oct 28, 2021
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