Wrong URL insert in database for review usr change

Review Request #3771 — Created Jan. 19, 2013 and discarded

gabriellorin
Review Board
master
2592
reviewboard
Wrong URL insert in database for review usr change

The wrong URL was inserted in database for user change in reviews (added or deleted), when the root is not / but a subfolder.
The Django function that was used to retrieve the URL get_absolute_url() for a particular user was returning the wrong url. So we now have a new function that generates the right URL from the local_site_name and username.
Tested with a different SITE_ROOT parameter. (e.g: SITE_ROOT = '/reviewboard/' instead of SITE_ROOT = '/')
Description From Last Updated

Col: 80 E501 line too long (98 > 79 characters)

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 80 E501 line too long (87 > 79 characters)

reviewbotreviewbot

Col: 30 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 72 E231 missing whitespace after ','

reviewbotreviewbot

Col: 9 E301 expected 1 blank line, found 0

reviewbotreviewbot

Col: 80 E501 line too long (82 > 79 characters)

reviewbotreviewbot

Col: 35 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 80 E501 line too long (90 > 79 characters)

reviewbotreviewbot

Col: 67 W291 trailing whitespace

reviewbotreviewbot

Col: 30 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 30 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 50 W291 trailing whitespace

reviewbotreviewbot

Col: 21 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 68 W291 trailing whitespace

reviewbotreviewbot

Col: 29 E128 continuation line under-indented for visual indent

reviewbotreviewbot

Col: 25 E128 continuation line under-indented for visual indent

reviewbotreviewbot

According to PEP-8, imports should be broken up into three groups, each separated by a blank line (standard library, third-party ...

daviddavid

I think this will break for any model that doesn't have a "username" field (for example, if the target_groups field ...

daviddavid

Col: 30 E128 continuation line under-indented for visual indent

reviewbotreviewbot
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/changedescs/models.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
  2. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (98 > 79 characters)
    
  3. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (87 > 79 characters)
    
  5. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 30
     E127 continuation line over-indented for visual indent
    
  6. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 72
     E231 missing whitespace after ','
    
  7. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 9
     E301 expected 1 blank line, found 0
    
  8. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (82 > 79 characters)
    
  9. reviewboard/changedescs/models.py (Diff revision 1)
     
     
    Col: 35
     E128 continuation line under-indented for visual indent
    
  10. reviewboard/reviews/models.py (Diff revision 1)
     
     
    Col: 80
     E501 line too long (90 > 79 characters)
    
  11. 
      
GA
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/changedescs/models.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
  2. reviewboard/changedescs/models.py (Diff revision 2)
     
     
    Col: 67
     W291 trailing whitespace
    
  3. reviewboard/changedescs/models.py (Diff revision 2)
     
     
    Col: 30
     E127 continuation line over-indented for visual indent
    
  4. reviewboard/changedescs/models.py (Diff revision 2)
     
     
    Col: 30
     E128 continuation line under-indented for visual indent
    
  5. reviewboard/changedescs/models.py (Diff revision 2)
     
     
    Col: 50
     W291 trailing whitespace
    
  6. reviewboard/changedescs/models.py (Diff revision 2)
     
     
    Col: 21
     E128 continuation line under-indented for visual indent
    
  7. reviewboard/reviews/models.py (Diff revision 2)
     
     
    Col: 68
     W291 trailing whitespace
    
  8. 
      
GA
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/changedescs/models.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
  2. reviewboard/changedescs/models.py (Diff revision 3)
     
     
    Col: 29
     E128 continuation line under-indented for visual indent
    
  3. reviewboard/changedescs/models.py (Diff revision 3)
     
     
    Col: 25
     E128 continuation line under-indented for visual indent
    
  4. 
      
GA
reviewbot
  1. This is a review from Review Bot.
      Tool: PEP8 Style Checker
      Processed Files:
        reviewboard/changedescs/models.py
        reviewboard/reviews/models.py
      Ignored Files:
    
    
  2. reviewboard/changedescs/models.py (Diff revision 4)
     
     
    Col: 30
     E128 continuation line under-indented for visual indent
    
  3. 
      
david
  1. 
      
  2. reviewboard/changedescs/models.py (Diff revision 4)
     
     
     
    According to PEP-8, imports should be broken up into three groups, each separated by a blank line (standard library, third-party libraries, this package).
    
    From the "reviewboard" point of view, django/djblets are third-party, and reviewboard is "this package", so we should have a blank line between these.
  3. reviewboard/changedescs/models.py (Diff revision 4)
     
     
     
    I think this will break for any model that doesn't have a "username" field (for example, if the target_groups field changes). It's also kind of weird in that the code here is very generic, but you're adding specific knowledge of types that are elsewhere in the codebase (the idea is that "reviews" depends on "changedescs", not the other way around).
    
    I believe a better solution would be to pass in a callable instead of a boolean:
    
    def record_field_change(self, field, old_value, new_value,
                            name_field=None, get_item_url_func=None):
    
    ....
    
    
    if name_field:
        results = []
        for item in list(items):
            if get_item_url_func:
                url = get_item_url_func(item)
            else:
                url = item.get_absolute_url()
            results.append((getattr(item, name_field),
                            get_item_url_func(item),
                            item.id))
        return results
    
    ....
    
    That way we can encode knowledge of the "user" type in reviews/models.py where we know we're building a set of the changed users.
  4. 
      
GA
Review request changed

Status: Discarded

Loading...