[WIP] Publishing possible without required fields

Review Request #7886 — Created Jan. 16, 2016 and discarded

nfredette
Review Board
master
4057, 4064
reviewboard, students

-Added checks and improved error message from can_publish() and publishing failures.- returned can_publish() to its former glory.

A list of errors will be presented to the user when they run 'rbt publish ##'


validate_can_publish(self, errors=False) has been added. passing true will generate an exception whereas false or default will only return a boolean if it is a valid review request having a reviewer, summary, and description.

--Merged Review Request with request 7882

TODO: create automated test
test publish with user + summary fails
test publish with user + description fails
test publish with group + summary fails
test publish with group + description fails



Description From Last Updated

The summary should be in the form of "Return <...>", and should have a trailing period. How about: "Return whether ...

chipx86chipx86

Blank line between code blocks (if, for, etc.) and other code. Same with the if statements below.

chipx86chipx86

Col: 51 W291 trailing whitespace

reviewbotreviewbot

This function has always returned a boolean, and must continue to do so. Otherwise, we break other parts of the ...

chipx86chipx86

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

Col: 34 E225 missing whitespace around operator

reviewbotreviewbot

Col: 36 W291 trailing whitespace

reviewbotreviewbot

undefined name 'validate_can_publish'

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

local variable 'errors' is assigned to but never used

reviewbotreviewbot

undefined name 'validate_can_publish'

reviewbotreviewbot

Col: 34 E225 missing whitespace around operator

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

local variable 'errors' is assigned to but never used

reviewbotreviewbot

Col: 38 W291 trailing whitespace

reviewbotreviewbot

Col: 13 E113 unexpected indentation

reviewbotreviewbot

This should be self.validate_can_publish().

brenniebrennie

undefined name 'validate_can_publish'

reviewbotreviewbot

This needs translation. Also, favour .append over += with lists.

brenniebrennie

This should really use .count instead of len((...).all()) e.g. if draft.target_people.count() or draft.target_groups.count()

brenniebrennie

Same here.

brenniebrennie

It is more pythonic to do if draft.summary. However, if the summary is all whitespace, we probably don't want to ...

brenniebrennie

Likewise, this should be if draft.description.trim().

brenniebrennie

You can just do if errors.

brenniebrennie

We already have a mechanism for reporting errors to the user. This checking should go in the API. Have a ...

brenniebrennie
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2. Col: 51
     W291 trailing whitespace
    
  3. Col: 34
     E127 continuation line over-indented for visual indent
    
  4. Col: 17
     E128 continuation line under-indented for visual indent
    
  5. 
      
NF
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2. Col: 21
     E128 continuation line under-indented for visual indent
    
  3. Col: 25
     E128 continuation line under-indented for visual indent
    
  4. Col: 29
     E128 continuation line under-indented for visual indent
    
  5. 
      
chipx86
  1. 
      
  2. reviewboard/reviews/models/review_request.py (Diff revision 1)
     
     
     
     
     

    The summary should be in the form of "Return <...>", and should have a trailing period. How about:

    "Return whether the draft can be published."

    We then also add some other indicators for our newly generated codebase docs. This docstring should have the form of:

    """Return [...]
    
    [...]
    
    Returns:
        bool:
        Whether the draft can be published.
    """
    

    Always make sure all sentences are explanatory to those not familiar with the code and are coming into this as a new user, and always use correct grammar and punctuation.

  3. Blank line between code blocks (if, for, etc.) and other code.

    Same with the if statements below.

  4. This function has always returned a boolean, and must continue to do so. Otherwise, we break other parts of the codebase, as well as third-party extensions.

    We may also have various ways that a consumer would want to represent any error states here. A single string isn't going to really work.

    Forms in Django have a concept of validation, where you call full_clean() on a form of model and it either returns or it raises a ValidationError for what it finds. I'd suggest a similar thing.

    Let's define a validate_can_publish() method that does this work instead. Instead of building a list of strings, it can raise a django.core.exceptions.ValidationError when encountering things.

    can_publish() can then call into this, and return a boolean depending on whether it raised an error.

    That way, can_publish() can be a simple call, but anything needing more complex validation with error results can call validate_can_publish() instead.

  5. 
      
NF
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2. 
      
NF
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2. Col: 34
     E225 missing whitespace around operator
    
  3. Col: 36
     W291 trailing whitespace
    
  4.  undefined name 'validate_can_publish'
    
  5. Col: 1
     W293 blank line contains whitespace
    
  6. Col: 1
     W293 blank line contains whitespace
    
  7.  local variable 'errors' is assigned to but never used
    
  8. 
      
NF
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2.  undefined name 'validate_can_publish'
    
  3. Col: 34
     E225 missing whitespace around operator
    
  4. Col: 1
     W293 blank line contains whitespace
    
  5.  local variable 'errors' is assigned to but never used
    
  6. 
      
NF
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2. Col: 38
     W291 trailing whitespace
    
  3. Col: 13
     E113 unexpected indentation
    
  4. 
      
NF
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2.  undefined name 'validate_can_publish'
    
  3. 
      
NF
brennie
  1. Hey Nathan, this is a solid start. However, you'll be wanting to make the changes to the Web API, specifically the ReviewRequestDraftResource (in webapi/resources/review_request_draft.py).

    In that file, you'll want to look at update() which is responsible for setting values on a review request. The value taht ends up triggering this is public=1.

    W

  2. This should be self.validate_can_publish().

  3. This needs translation.

    Also, favour .append over += with lists.

  4. This should really use .count instead of len((...).all())

    e.g.

    if draft.target_people.count() or draft.target_groups.count()

  5. It is more pythonic to do if draft.summary. However, if the summary is all whitespace, we probably don't want to allow it. Hence we should check draft.summary.strip().

  6. Likewise, this should be if draft.description.trim().

  7. You can just do if errors.

    1. do we always want an exception thrown even if there are no errors?

    2. Sorry I just saw this comment. A list with length 0 will evaluate to False in an if statement.

      So the errors will only be raised when there are errors.

  8. reviewboard/reviews/models/review_request.py (Diff revision 7)
     
     
     
     
     

    We already have a mechanism for reporting errors to the user. This checking should go in the API.

    Have a look at lines 471--489 of reviewboard/webapi/resources/review_request.py for an example of data validation for review request drafts. Your logic will go there.

    1. I actually meant review_request_draft.py here.

  9. Feel free to ask my any questions about this review.

NF
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/models/review_request.py
    
    
  2. 
      
NF
david
Review request changed

Status: Discarded

Loading...