Conditions for adding DefaultReviewers to reviewrequest

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

Information

Review Board
master

Reviewers

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
Description From Last Updated

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

F401 'djblets.conditions.choices.BaseConditionChoice' imported but unused

reviewbotreviewbot

F401 'reviewboard.scmtools.conditions.RepositoriesChoice' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.conditions.ReviewRequestConditionChoiceMixin' imported but unused

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestRepositoryTypeChoice' from line 19

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestRepositoriesChoice' from line 19

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestConditionChoices' from line 19

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

F401 'djblets.conditions.choices.BaseConditionChoice' imported but unused

reviewbotreviewbot

F401 'reviewboard.scmtools.conditions.RepositoriesChoice' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.conditions.ReviewRequestConditionChoiceMixin' imported but unused

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestRepositoryTypeChoice' from line 19

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestRepositoriesChoice' from line 19

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestConditionChoices' from line 19

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (85 > 79 characters)

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

F401 'djblets.conditions.choices.BaseConditionChoice' imported but unused

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestRepositoryTypeChoice' from line 19

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestRepositoriesChoice' from line 19

reviewbotreviewbot

F811 redefinition of unused 'ReviewRequestConditionChoices' from line 19

reviewbotreviewbot

E127 continuation line over-indented for visual indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

F401 'djblets.conditions.Condition' imported but unused

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

F401 'djblets.conditions.Condition' imported but unused

reviewbotreviewbot

F401 'djblets.conditions.ConditionSet' imported but unused

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Please undo this change.

daviddavid

The comma isn't necessary here. We probably also don't need to say it replaces X fields here in the UI, …

daviddavid

django before djblets (alphabetical order)

daviddavid

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

daviddavid

Please undo this change.

daviddavid

Please add a trailing comma to this line.

daviddavid

Can you add a module docstring to this?

daviddavid

Swap these two lines.

daviddavid

No blank line here.

daviddavid

No blank lines here.

daviddavid

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

daviddavid

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

Can you use single-quotes here?

daviddavid

Condition -> Conditions?

daviddavid

Blank line between these two please.

daviddavid

Blank line between these two please.

daviddavid

What was the case that was causing this?

daviddavid

Blank line between these two please.

daviddavid

No blank line here.

daviddavid

c before d

daviddavid

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

daviddavid

Review Bot complained about this but indentation is wacky. How about: conditions = JSONField(default={ 'mode': ConditionSet.DEFAULT_MODE, 'conditions': [], })

daviddavid

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

review_request should probably be called draft because it's operating on a draft.

daviddavid

Docstring summaries should be in the imperative mood (Return X) rather than passive (Returns X). There's also a typo (wether) …

daviddavid

This should be prefixed with "Args:" and indented another 4 spaces. We're missing docs for the review_request argument (well, if …

daviddavid

These two conditions can be combined together: if (check_repo and review_request.repository not in self.repository.all()):

daviddavid

Blank line between these two.

daviddavid

Blank line between these two.

daviddavid

This looks like it probably will fit on one line.

daviddavid

Needs a docstring.

daviddavid

Needs a docstring.

daviddavid

Needs a docstring.

daviddavid

Needs a docstring.

daviddavid

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

daviddavid

Trailing comma.

daviddavid

Trailing comma.

daviddavid

Trailing comma.

daviddavid

E261 at least two spaces before inline comment

reviewbotreviewbot

E262 inline comment should start with '# '

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

E265 block comment should start with '# '

reviewbotreviewbot

F401 'djblets.conditions.ConditionSet' imported but unused

reviewbotreviewbot

F401 'nose.tools.set_trace' imported but unused

reviewbotreviewbot

F401 'pdb' imported but unused

reviewbotreviewbot

F401 'json' imported but unused

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

F401 'nose.tools.set_trace' imported but unused

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

E126 continuation line over-indented for hanging indent

reviewbotreviewbot

F401 'djblets.webapi.fields.ListFieldType' imported but unused

reviewbotreviewbot

F401 'nose.tools.set_trace' imported but unused

reviewbotreviewbot

E226 missing whitespace around arithmetic operator

reviewbotreviewbot

F841 local variable 'ke' is assigned to but never used

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot

E501 line too long (97 > 79 characters)

reviewbotreviewbot
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

RO
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

RO
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

RO
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

RO
RO
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

RO
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

RO
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. 
      
JC
  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. 
      
RO
RO
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed

Status: Discarded

Loading...