Initial code for tracking user changes

Review Request #2891 — Created Feb. 18, 2012 and discarded

weilul
Review Board
reviewboard
Added one field "change_user" to changedesc model.
Modify evolution files related.
Unsucessful. Evolution did not happen to the database.
Description From Last Updated

This must stay. This particular evolution is only for mysql. The sequences code should look like: SEQUENCE = [] if ...

chipx86chipx86

This stuff that got axed here - I think we might want to put it back. The "fields_changed_longtext" evolution should ...

mike_conleymike_conley

This line is longer than 80 characters.

ME medanat

This looks like its longer than 80 characters.

ME medanat

Both these lines are longer than 80 characters.

ME medanat

This line is longer than 80 characters.

ME medanat

whitespace

mike_conleymike_conley

I'm not sure if 'change_user' is appropriately evocative of the purpose of this field. Perhaps 'last_modified_by'?

AM ammok

Should this be 'draft_creator' instead of 'user'?

AM ammok

This is a little nit-picky as it doesn't usually affect the rendering, but there are extraneous spaces before "Review" and ...

AM ammok

This line is a bit too long.

AM ammok

This line is a bit too long.

AM ammok
chipx86
  1. 
      
  2. This must stay. This particular evolution is only for mysql.
    
    The sequences code should look like:
    
        SEQUENCE = []
    
        if settings.....mysql:
            .... fields_changed_longtext
    
        SEQUENCE += [
            'change_desc_user',
        ]
  3. 
      
mike_conley
  1. When I made the below changes, this evolution applied cleanly for me.
  2. This stuff that got axed here - I think we might want to put it back.  The "fields_changed_longtext" evolution should *only* run when using a MySQL database.
    
    So what we want is:
    
    from django.conf import settings
    
    SEQUENCE = []
    
    if settings.DATABASES['default']['ENGINE'].endswith('mysql'):
        SEQUENCE.append('fields_changed_longtext')
    
    SEQUENCE.append('change_desc_user')
  3. 
      
WE
WE
ME
  1. I'm not familiar with django evolutions, but I can point out some style issues.
    
    -Yazan
  2. This line is longer than 80 characters.
  3. reviewboard/changedescs/models.py (Diff revision 3)
     
     
    This looks like its longer than 80 characters.
  4. Both these lines are longer than 80 characters.
    1. I recently rolled back to a previous revision so this issue is no longer present. Thanks anyway.
  5. reviewboard/reviews/models.py (Diff revision 3)
     
     
    This line is longer than 80 characters.
    1. The same as above.
  6. 
      
WE
WE
WE
david
  1. This looks like it will record whoever hit "publish", but won't
    track anything up to that point. In the initial change, I believe
    Waleed had started to add the ability for there to be multiple
    concurrent drafts, owned by separate users.
    1. I will add multiple user support soon.
      
      But we also have race/override problem (R1R2W1W2 -> 2 wins).
      Christian/Mike suggests: 1) make an explicit alert to prevent overriding, or, 2) using transaction control; they prefer 1)
      
      How do you think?
      
    2. I still don't think we want that.
      
      What I really just want to see is a banner above the review request when there's a draft owned by someone other than you saying "<User> [, <user>, ...] is making changes to this review request".
      
      Don't block submission, or display an alert, or do anything fancy. Just inform the user and let them decide whether to hold off or whatever.
  2. 
      
WE
mike_conley
  1. Hey Willer, I think we should get an update going on this project.
    
    1)  It looks like it's been bitrotted by other changes in master.  Can you update the patch?
    2)  What about ChipX86's request, "What I really just want to see is a banner above the review request when there's a draft owned by someone other than you saying "<User> [, <user>, ...] is making changes to this review request"." <-- has that been done?
    3)  In your "testing done" section, you write that the evolutions never successfully applied...is this still true?
    
    -Mike
    1. Hey Mike,
      
      1) Ok, will do.
      2) Not yet.
      3) That sentance is out of date. Evolution applied successfully. I am just struggling with testing.
      
      -Willer
  2. reviewboard/reviews/models.py (Diff revision 6)
     
     
    whitespace
  3. 
      
WE
AM
  1. 
      
  2. reviewboard/changedescs/models.py (Diff revision 7)
     
     
    I'm not sure if 'change_user' is appropriately evocative of the purpose of this field. Perhaps 'last_modified_by'?
  3. reviewboard/reviews/models.py (Diff revision 7)
     
     
    Should this be 'draft_creator' instead of 'user'?
  4. This is a little nit-picky as it doesn't usually affect the rendering, but there are extraneous spaces before "Review" and after the period.
  5. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    This line is a bit too long.
  6. reviewboard/webapi/resources.py (Diff revision 7)
     
     
    This line is a bit too long.
  7. 
      
WE
Review request changed

Status: Discarded

Loading...