• 
      

    Make mimetype match scoring much stricter.

    Review Request #10226 — Created Oct. 12, 2018 and submitted

    Information

    Review Board
    release-4.0.x
    e8c7593...

    Reviewers

    When we initially created the mimetype matcher, it was okay to get some
    false positives, because all it was used for was choosing an icon to
    show for the file attachment. We now no longer have icons at all, but we
    do have review UIs which behave completely incorrectly when there's a
    bad file type. We recently added application/x-javascript to the
    TextReviewUI, but this was matching all sorts of incorrect files, like
    application/msdownload and application/x-pdf.

    This change tightens up the scoring system a lot more, making it so that
    if there aren't wildcards, both the type and subtype have to match
    exactly. The addition of a vendor tag will cause the match to score
    higher, but is not required.

    Ran unit tests.

    Description From Last Updated

    Don't we have to update reviews/ui/base to call this in the new way? It is still calling it with mimetype/test …

    brenniebrennie

    We use "mimetype" elsewhere in our documentation (and even within this docstring). Can we standardize on that?

    chipx86chipx86

    :py:func, since mimeparse isn't a class.

    chipx86chipx86

    Should be in alphabetical order.

    chipx86chipx86

    No trailing period. This (and others in this file) are missing the function name being tested.

    chipx86chipx86

    Does it fail with using assertEqual? If we're being impacted by the precision, then we should probably pass places=1 (I …

    chipx86chipx86
    brennie
    1. 
        
    2. reviewboard/attachments/mimetypes.py (Diff revision 1)
       
       
      Show all issues

      Don't we have to update reviews/ui/base to call this in the new way? It is still calling it with mimetype/test as a string.

      1. I see other docs that are wrong, but nowhere that the calls are wrong.

    3. 
        
    david
    brennie
    1. Ship It!
    2. 
        
    chipx86
    1. 
        
    2. reviewboard/attachments/mimetypes.py (Diff revision 2)
       
       
      Show all issues

      We use "mimetype" elsewhere in our documentation (and even within this docstring). Can we standardize on that?

    3. reviewboard/attachments/mimetypes.py (Diff revision 2)
       
       
      Show all issues

      :py:func, since mimeparse isn't a class.

    4. reviewboard/attachments/tests.py (Diff revision 2)
       
       
       
      Show all issues

      Should be in alphabetical order.

      1. Normally I'd say yes but it seemed better to keep register and unregister right next to each other...

      2. We have those types of imports all over the codebases and they're all alphabetical.

      3. It's also harder to keep other things in alphabetical order if there's an exception to the order in the import list.

      4. OK, done.

    5. reviewboard/attachments/tests.py (Diff revision 2)
       
       
      Show all issues

      No trailing period.

      This (and others in this file) are missing the function name being tested.

    6. 
        
    david
    david
    chipx86
    1. 
        
    2. reviewboard/attachments/tests.py (Diff revision 3)
       
       
      Show all issues

      Does it fail with using assertEqual? If we're being impacted by the precision, then we should probably pass places=1 (I think?) to this.

      1. Yeah, it's a like .00000000001 difference because floats. I think the default places=7 is fine here.

      2. Okay, cool. Haven't hit that before.

    3. 
        
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-4.0.x (20a62bb)