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
Change Summary:

JS changes

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.

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

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

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

RO
david
  1. 
      
  2. reviewboard/admin/widgets.py (Diff revision 6)
     
     
    Show all issues

    Please undo this change.

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

    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)
     
     
     
    Show all issues

    django before djblets (alphabetical order)

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

    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)
     
     
    Show all issues

    Please undo this change.

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

    Please add a trailing comma to this line.

  8. Show all issues

    Can you add a module docstring to this?

  9. Show all issues

    Swap these two lines.

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

    No blank line here.

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

    No blank lines here.

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

    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)
     
     
    Show all issues

    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)
     
     
    Show all issues

    Condition -> Conditions?

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

    Blank line between these two please.

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

    Blank line between these two please.

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

    What was the case that was causing this?

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

    Blank line between these two please.

  19. Show all issues

    No blank line here.

  20. Show all issues

    c before d

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

    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)
     
     
     
     
    Show all issues

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

    conditions = JSONField(default={
        'mode': ConditionSet.DEFAULT_MODE,
        'conditions': [],
    })
    
  23. Show all issues

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

  24. Show all issues

    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)
     
     
     
     
    Show all issues

    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. Show all issues

    These two conditions can be combined together:

    if (check_repo and
        review_request.repository not in self.repository.all()):
    
  27. Show all issues

    Blank line between these two.

  28. Show all issues

    Blank line between these two.

  29. Show all issues

    This looks like it probably will fit on one line.

  30. Show all issues

    Needs a docstring.

  31. Show all issues

    Needs a docstring.

  32. Show all issues

    Needs a docstring.

  33. Show all issues

    Needs a docstring.

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

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

  35. Show all issues

    Trailing comma.

  36. Show all issues

    Trailing comma.

  37. Show all issues

    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
Change Summary:

Current progress on moving repositories to conditions.

Commit:
3fd535ff50c16c2e2e499b29af9c53b9fe41f968
a8dbfd785d5005411962ed5cee5976f55cb4703a

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed
Status:
Discarded