totoroliu got a fish trophy!

Summary

Fix for ticket-4438: Email header is too large when too many changed files

Review Request #8558 — Created Nov. 30, 2016 and discarded May 26, 2017, 2:14 p.m.

Information

Owner

totoroliu

Repository

Review Board

Branch

master

Bugs

4438

Depends On

Reviewers

Groups

reviewboard

People

Description

Fix for ticket-4438: Email header is too large when too many changed files

Testing Done

N/A

Issues

Description	From	Last Updated
'formataddr' imported but unused	reviewbot	Nov. 30, 2016, 11:12 a.m.
This is not an acceptable change.	brennie	Nov. 30, 2016, 1:45 p.m.

Fix it!

Tool: Pyflakes
Processed Files:
    reviewboard/notifications/email.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/notifications/email.py

reviewboard/notifications/email.py (Diff revision 1)
An issue was opened. Show all issues
```
 'formataddr' imported but unused
```

Fix it!

This will not land as is. A toggle could be added in the siteconfig to enable/disable this header, but we do not want to outright remove it.

totoroliu Dec. 13, 2016, 10:03 a.m.

Hi Barret,

I'm new to ReviewBoard.
Is there an example from the source code using siteconfig toggle?
I could try to apply the same code.

brennie Dec. 13, 2016, 11:06 a.m.

SiteConfig defaults are set up in admin/siteconfig.py. You will also want to add a setting in EMailSettingsForm to turn it on/off.

reviewboard/notifications/email.py (Diff revision 1)
An issue was opened. Show all issues
```
This is not an acceptable change.
```

Hi Rick,

Thanks for the contribution! However, this is a feature people do depend on, so we can't simply shut it off.

I know we have some issues with mail header lengths, so I think the right way to address this is to keep a running count on the length as we add the files to the header, making sure we don't exceed the header length (and we need to factor in UTF-8 encodings for the file list values here).

I also considered a maximum number of files to cap, but since each file is allowed technically to be 1024 characters in length, we'd only be able to fit under 32 files in the list. This might not be enough. So I think looping is the best option. In the majority of cases, we'll be able to fit the entire file list. In these kinds of corner cases with hundreds of files (which is generally not reviewable by a person anyway), we'll simply not list all the files, and those depending exclusively on this header to sort will find their e-mail in the Inbox instead of the desired folder.

totoroliu Dec. 13, 2016, 10:01 a.m.

Hi Christian,

I could add a loop to check the total mail header lengths.
Do you know what's the default max header lengths for common mail servers?

totoroliu Dec. 13, 2016, 10:02 a.m.

and do you know what features of reviewboard is using this header?

Status:: Discarded