Conditions for adding DefaultReviewers to reviewrequest

Review Request #9579 — Created Feb. 3, 2018 and updated

rodgerwa
Review Board
master
a8dbfd7...
reviewboard, students

Replaces file_match field with a user set conditions which applied
will be tested on all posted review requests for the corresponding
repositories.

Changes:

  • Removed file_match field from form

  • Added Conditions widget to form

  • Added description field to form and as a list column

  • Added diffed file options (corresponding to old regex functionality)

  • Added evolution for database

  • Refactored code for adding default reviewers to a ReviewRequest

  • Added lazy migration for default reviewers

    • Migrated when tested against review requests

    • Migrated when before being edited by an admin

  • Created a test repository with a test struture to try various
    conditions including string matching and files diffed.
  • Created commit to trigger migrations and be applied to review
    request.
  • Test commit as review request to verify expected behavior
    before applying evolution
  • Applied evolution
  • Posted review request to verify same behavior was exhibited
  • Test other string operations
    • ends with
    • starts with
    • contains
  • 63
  • 0
  • 4
  • 38
  • 105
Description From Last Updated
E126 continuation line over-indented for hanging indent reviewbot reviewbot
E126 continuation line over-indented for hanging indent reviewbot reviewbot
Please undo this change. david david
The comma isn't necessary here. We probably also don't need to say it replaces X fields here in the UI, ... david david
django before djblets (alphabetical order) david david
Having each of the models in their own file is an implementation detail. We still want to import from reviewboard.reviews.models. david david
Please undo this change. david david
Please add a trailing comma to this line. david david
Can you add a module docstring to this? david david
Swap these two lines. david david
No blank line here. david david
No blank lines here. david david
Shouldn't have the blank line above this, but this should be sorted with the other reviewboard.reviews imports. david david
E126 continuation line over-indented for hanging indent reviewbot reviewbot
Can you use single-quotes here? david david
Condition -> Conditions? david david
Blank line between these two please. david david
Blank line between these two please. david david
What was the case that was causing this? david david
Blank line between these two please. david david
No blank line here. david david
c before d david david
I know the other fields use double-quotes and lower case, but we should have new stuff use single-quotes and uppercase. ... david david
Review Bot complained about this but indentation is wacky. How about: conditions = JSONField(default={ 'mode': ConditionSet.DEFAULT_MODE, 'conditions': [], }) david david
E126 continuation line over-indented for hanging indent reviewbot reviewbot
E126 continuation line over-indented for hanging indent reviewbot reviewbot
review_request should probably be called draft because it's operating on a draft. david david
Docstring summaries should be in the imperative mood (Return X) rather than passive (Returns X). There's also a typo (wether) ... david david
This should be prefixed with "Args:" and indented another 4 spaces. We're missing docs for the review_request argument (well, if ... david david
These two conditions can be combined together: if (check_repo and review_request.repository not in self.repository.all()): david david
Blank line between these two. david david
Blank line between these two. david david
This looks like it probably will fit on one line. david david
Needs a docstring. david david
Needs a docstring. david david
Needs a docstring. david david
Needs a docstring. david david
u prefixes on strings aren't necessary (we're importing unicode_literals at the top of the file). david david
Trailing comma. david david
Trailing comma. david david
Trailing comma. david david
E261 at least two spaces before inline comment reviewbot reviewbot
E262 inline comment should start with '# ' reviewbot reviewbot
E265 block comment should start with '# ' reviewbot reviewbot
E265 block comment should start with '# ' reviewbot reviewbot
F401 'djblets.conditions.ConditionSet' imported but unused reviewbot reviewbot
F401 'nose.tools.set_trace' imported but unused reviewbot reviewbot
F401 'pdb' imported but unused reviewbot reviewbot
F401 'json' imported but unused reviewbot reviewbot
E126 continuation line over-indented for hanging indent reviewbot reviewbot
F401 'nose.tools.set_trace' imported but unused reviewbot reviewbot
E126 continuation line over-indented for hanging indent reviewbot reviewbot
E126 continuation line over-indented for hanging indent reviewbot reviewbot
E126 continuation line over-indented for hanging indent reviewbot reviewbot
F401 'djblets.webapi.fields.ListFieldType' imported but unused reviewbot reviewbot
F401 'nose.tools.set_trace' imported but unused reviewbot reviewbot
E226 missing whitespace around arithmetic operator reviewbot reviewbot
F841 local variable 'ke' is assigned to but never used reviewbot reviewbot
E303 too many blank lines (2) reviewbot reviewbot
E501 line too long (97 > 79 characters) reviewbot reviewbot
E501 line too long (97 > 79 characters) reviewbot reviewbot
E501 line too long (97 > 79 characters) reviewbot reviewbot
E501 line too long (97 > 79 characters) reviewbot reviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Change Summary:

progress from Fed 9th added.

Summary:

-WIP CODE DROP 2: Getting closer but still not working
+WIP CODE DROP: ConditionField for DefaultReviewers

Description:

~  

CODE DROP 2: Getting closer but still not working

  ~

Replaces 'file_match' field and the select repository field with user a

  + set conditions which applied will be tested on all posted review
  + requests.

   
~  

Didn't get as much time as I would've liked to flesh this out. But I

~   got a really good idea where I need to go since I've tried a fair
~   amount of things. The big issue is getting the condition options
~   to display. Most of the other examples I found with conditions are
~   integrations which build forms differently than anything part of
~   reviewboard as far as I can tell.

  ~

This update provides an evolution to add a column for a json entry

  ~ that stores the condition set. Now, settings selected in the add
  ~ default reviewers for the condition field will be saved and reloaded
  ~ if a default reviewer is viewed from the admin panel.
  ~ This also changes the labels displayed when the list view for all the
  ~ default reviewers is displayed. Now, there is only the name. A
  + description could be a possible solution for describing this
  + condition.

Testing Done:

  +

Created a new default reviewer, viewed that the count of reviewers

  + changed. Edited the default reviwer, reloaded it to verify that the
  + settings changed.

Diff:

Revision 3 (+99 -10)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Change Summary:

Now adds options for diffed file regex and readds the specified repository field.

Description:

~  

Replaces 'file_match' field and the select repository field with user a

  ~

Replaces file_match field with user a

    set conditions which applied will be tested on all posted review
~   requests.

  ~ requests for a specified repository.

   
~  

This update provides an evolution to add a column for a json entry

~   that stores the condition set. Now, settings selected in the add
~   default reviewers for the condition field will be saved and reloaded
~   if a default reviewer is viewed from the admin panel.
~   This also changes the labels displayed when the list view for all the
~   default reviewers is displayed. Now, there is only the name. A
~   description could be a possible solution for describing this
~   condition.

  ~

Progress to far:

  ~ Add Conditions widget to form
  ~ Remove file_match field from form
  ~ Add evoluion for database
  ~ Add diffed file option
  ~ * Add code to execute conditions on review request post

  ~
  ~

Work to be done:

  + Add migration
  + Tweak regex options
  + * Add description field

Diff:

Revision 4 (+85 -5)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Change Summary:

Now adds migrations from file_regex to conditions, automatic application of conditions to new review request (still buggy),

Description:

   

Replaces file_match field with user a

    set conditions which applied will be tested on all posted review
    requests for a specified repository.

   
   

Progress to far:

   
   
  • Add Conditions widget to form

   
  • Remove file_match field from form

   
  • Add evolution for database

   
  • Add diffed file option

   
  • Add code to execute conditions on review request post

  +
  • Added migrations
   
   

Work to be done:

   
~  
  • Add migration

~  
  • Tweak regex options

  ~
  • Add description field
  ~
  • Create Test cases
-  
  • Add description field

Testing Done:

   

Created a new default reviewer, viewed that the count of reviewers

    changed. Edited the default reviwer, reloaded it to verify that the
    settings changed.

  +
  +

Fair amount, will update one current issue is fixed.

Commit:

-fd1ed14ead060f0b38cdd0827b27d5321077660c
+c072e9936a24c25f60fd6d5e5fd061fcd70ab51b

Diff:

Revision 5 (+570 -30)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Review request changed

Change Summary:

Finished task so far. Now requires reviewing

Summary:

-WIP CODE DROP: ConditionField for DefaultReviewers
+Conditions for adding DefaultReviewers to reviewrequest

Description:

~  

Replaces file_match field with user a

~   set conditions which applied will be tested on all posted review
~   requests for a specified repository.

  ~

Replaces file_match field with a user set conditions which applied

  ~ will be tested on all posted review requests for the corresponding
  ~ repositories.

   
~  

Progress to far:

  ~

Changes:

   
~  
  • Add Conditions widget to form

~  
  • Remove file_match field from form

~  
  • Add evolution for database

~  
  • Add diffed file option

~  
  • Add code to execute conditions on review request post

~  
  • Added migrations
~  
  ~
  • Removed file_match field from form

  ~
  • Added Conditions widget to form

  ~
  • Added description field to form and as a list column

  ~
  • Added diffed file options (corresponding to old regex functionality)

  ~
  • Added evolution for database

  ~
  • Refactored code for adding default reviewers to a ReviewRequest

  ~
  • Added lazy migration for default reviewers

    • Migrated when tested against review requests

    • Migrated when before being edited by an admin

-  

Work to be done:

-  
-  
  • Add description field
-  
  • Create Test cases

Testing Done:

~  

Created a new default reviewer, viewed that the count of reviewers

~   changed. Edited the default reviwer, reloaded it to verify that the
~   settings changed.

~  
~  

Fair amount, will update one current issue is fixed.

  ~
  • Created a test repository with a test struture to try various
    conditions including string matching and files diffed.
  ~
  • Created commit to trigger migrations and be applied to review
    request.
  ~
  • Test commit as review request to verify expected behavior
    before applying evolution
  ~
  • Applied evolution
  ~
  • Posted review request to verify same behavior was exhibited
  +
  • Test other string operations
    • ends with
    • starts with
    • contains

Commit:

-c072e9936a24c25f60fd6d5e5fd061fcd70ab51b
+d6e14f4f738e41e6ae6e11b852567d110c4305fb

Diff:

Revision 6 (+128 -56)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. reviewboard/admin/widgets.py (Diff revision 6)
     
     

    Please undo this change.

  3. reviewboard/reviews/admin.py (Diff revision 6)
     
     

    The comma isn't necessary here. We probably also don't need to say it replaces X fields here in the UI, since those fields are no longer present (and it would be confusing to users who are looking at this for the first time). We'll talk about the change in the release notes.

  4. reviewboard/reviews/conditions.py (Diff revision 6)
     
     
     

    django before djblets (alphabetical order)

  5. reviewboard/reviews/conditions.py (Diff revision 6)
     
     

    Having each of the models in their own file is an implementation detail. We still want to import from reviewboard.reviews.models.

  6. reviewboard/reviews/conditions.py (Diff revision 6)
     
     

    Please undo this change.

  7. reviewboard/reviews/conditions.py (Diff revision 6)
     
     

    Please add a trailing comma to this line.

  8. Can you add a module docstring to this?

  9. Swap these two lines.

  10. reviewboard/reviews/forms.py (Diff revision 6)
     
     

    No blank line here.

  11. reviewboard/reviews/forms.py (Diff revision 6)
     
     
     

    No blank lines here.

  12. reviewboard/reviews/forms.py (Diff revision 6)
     
     
     

    Shouldn't have the blank line above this, but this should be sorted with the other reviewboard.reviews imports.

  13. reviewboard/reviews/forms.py (Diff revision 6)
     
     

    Can you use single-quotes here?

    1. How do you determine the type of quote to use (single vs double)?

  14. reviewboard/reviews/forms.py (Diff revision 6)
     
     

    Condition -> Conditions?

  15. reviewboard/reviews/forms.py (Diff revision 6)
     
     
     

    Blank line between these two please.

  16. reviewboard/reviews/forms.py (Diff revision 6)
     
     
     

    Blank line between these two please.

  17. reviewboard/reviews/forms.py (Diff revision 6)
     
     

    What was the case that was causing this?

  18. reviewboard/reviews/forms.py (Diff revision 6)
     
     
     

    Blank line between these two please.

  19. No blank line here.

  20. c before d

  21. reviewboard/reviews/models/default_reviewer.py (Diff revision 6)
     
     
     
     

    I know the other fields use double-quotes and lower case, but we should have new stuff use single-quotes and uppercase.

    (the names all get title-cased by the UI display anyway--we'll fix up the older code at some point in the future)

  22. reviewboard/reviews/models/default_reviewer.py (Diff revision 6)
     
     
     
     

    Review Bot complained about this but indentation is wacky. How about:

    conditions = JSONField(default={
        'mode': ConditionSet.DEFAULT_MODE,
        'conditions': [],
    })
    
  23. review_request should probably be called draft because it's operating on a draft.

  24. Docstring summaries should be in the imperative mood (Return X) rather than passive (Returns X).

    There's also a typo (wether) and some weird grammar (this requests). How about:

    Return whether this default reviewer rule matches the draft.

  25. reviewboard/reviews/models/default_reviewer.py (Diff revision 6)
     
     
     
     

    This should be prefixed with "Args:" and indented another 4 spaces. We're missing docs for the review_request argument (well, if you make the above change, draft).

    We also need a "Returns:" section explaining the type and meaning of the return value.

  26. These two conditions can be combined together:

    if (check_repo and
        review_request.repository not in self.repository.all()):
    
  27. Blank line between these two.

  28. Blank line between these two.

  29. This looks like it probably will fit on one line.

  30. Needs a docstring.

  31. Needs a docstring.

  32. Needs a docstring.

  33. Needs a docstring.

  34. reviewboard/reviews/models/default_reviewer.py (Diff revision 6)
     
     
     
     
     
     
     
     
     

    u prefixes on strings aren't necessary (we're importing unicode_literals at the top of the file).

  35. Trailing comma.

  36. Trailing comma.

  37. Trailing comma.

  38. 
      
  1. 
      
  2. I'm guessing this is a Python convention or something but why do some methods start with a letter and some start with an '_'?

    1. Python doesn't have access control (like private/public/protected), so a leading underscore is a convention to say "this is internal and if you rely on it your code may break at any point".

  3. 
      
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Loading...