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 single quote quotation #8

Merged
merged 1 commit into from
May 4, 2021
Merged

Conversation

GerardRodes
Copy link
Contributor

No description provided.

@GerardRodes GerardRodes requested a review from a team May 4, 2021 13:54
Copy link

@gustavohmsilva gustavohmsilva left a comment

Choose a reason for hiding this comment

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

LGTM. Although it is weird to have one case with two conditional tests.

@GerardRodes GerardRodes requested a review from a team May 4, 2021 13:57
@arp242
Copy link
Contributor

arp242 commented May 4, 2021

According to RFC5322 only " is a valid quotation character. A ' is just a literal.

@GerardRodes
Copy link
Contributor Author

According to RFC5322 only " is a valid quotation character. A ' is just a literal.

We have the following on a customer email

To: 'Foo Bar' <foo@bar.com>; var@var.com

and also

>To: 'var@var.com'

This email client is quoting with single quotes


We have to treat ' as a quotation character on this package or remove ' characters up on the desk pkg making use of mailaddress since we don't want the name of the customer to appear quoted by '

@arp242
Copy link
Contributor

arp242 commented May 4, 2021

I figured as much, but that's basically wrong. Practicality beats purity, but I'd be real careful just changing this based on a single customer/example, as some email clients may use a literal ' and it may end up breaking more than it fixes.

You can do what you want, I haven't even worked at Teamwork for years, but personally I'd be really careful with this. For example I just tested from FastMail, and it just sends:

To: a'b <martin@arp242.net>

A ' is somewhat common-ish in names, so it's not inconceivable it's set.

A better way might be to treat ' as a quotation character only if it starts and ends with it. That is, leave the loop as-is and after it do something like if strings.HasPrefix(addr.Name, ') && strings.HasSuffix(addr.Name, "'") { strings.Trim(addr.Name, "'") }. The case 'Martin' foo 'Tournoij' probably doesn't need to be supported(?)

@GerardRodes
Copy link
Contributor Author

GerardRodes commented May 4, 2021

I figured as much, but that's basically wrong. Practicality beats purity, but I'd be real careful just changing this based on a single customer/example, as some email clients may use a literal ' and it may end up breaking more than it fixes.

You can do what you want, I haven't even worked at Teamwork for years, but personally I'd be really careful with this. For example I just tested from FastMail, and it just sends:

To: a'b <martin@arp242.net>

A ' is somewhat common-ish in names, so it's not inconceivable it's set.

A better way might be to treat ' as a quotation character only if it starts and ends with it. That is, leave the loop as-is and after it do something like if strings.HasPrefix(addr.Name, ') && strings.HasSuffix(addr.Name, "'") { strings.Trim(addr.Name, "'") }. The case 'Martin' foo 'Tournoij' probably doesn't need to be supported(?)

You are right, good find

@GerardRodes
Copy link
Contributor Author

Updated it, now only will remove wrapping ' if there are no other ' in the middle of the name

@GerardRodes GerardRodes merged commit aad6afe into master May 4, 2021
@GerardRodes GerardRodes deleted the fix-single-quote-quotation branch May 4, 2021 16:08
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.

None yet

3 participants