Fish Trophy

totoroliu got a fish trophy!

Fish Trophy

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

Review Request #8558 — Created Nov. 30, 2016 and discarded

Information

Review Board
master

Reviewers

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

N/A

Description From Last Updated

'formataddr' imported but unused

reviewbotreviewbot

This is not an acceptable change.

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/notifications/email.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/notifications/email.py
    
    
  2. reviewboard/notifications/email.py (Diff revision 1)
     
     
    Show all issues
     'formataddr' imported but unused
    
  3. 
      
brennie
  1. 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.

    1. 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.

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

  2. reviewboard/notifications/email.py (Diff revision 1)
     
     
     
    Show all issues

    This is not an acceptable change.

  3. 
      
chipx86
  1. 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.

    1. 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?

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

  2. 
      
david
Review request changed
Status:
Discarded