[checklist] Added warning when user publishes review without checking off all items

Review Request #8053 — Created March 10, 2016 and discarded

Information

rb-extension-pack
master

Reviewers

Added a warning when a user tries to publish a review without first checking off
all the items on their checklist.

This makes use of the new extension hook that I added in #8052.

Ensured that the alert showed for multiple variations of checklist items
being checked or left unchecked.

Ensured empty checklists are always valid.

Ensured that the cancel and force publish options worked as expected.


Description From Last Updated

Nit - two newlines between top-level items in a py file.

mike_conleymike_conley

I think we tend to avoid one-liners like this. I wonder if it's simpler to use countBy return this.collection.countBy(function(item) { …

mike_conleymike_conley

If you're going to break it over two lines like this, probably best to indent 510 only 4-spaces in from …

mike_conleymike_conley

This is equivalent to just saying: if (unchecked) { Since 0 is false-y, and anything not 0 is truth-y.

mike_conleymike_conley

Again, best to avoid the one-liner if you can. Try: } else { setValidation(true); }

mike_conleymike_conley
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        checklist/checklist/extension.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/extension.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
    
    
  2. 
      
imadueme
imadueme
imadueme
mike_conley
  1. 
      
  2. checklist/checklist/extension.py (Diff revision 1)
     
     
    Show all issues

    Nit - two newlines between top-level items in a py file.

  3. Show all issues

    I think we tend to avoid one-liners like this. I wonder if it's simpler to use countBy

    return this.collection.countBy(function(item) {
      return item.get('checked');
    });
    
  4. Show all issues

    If you're going to break it over two lines like this, probably best to indent 510 only 4-spaces in from 509.

  5. Show all issues

    This is equivalent to just saying:

    if (unchecked) {

    Since 0 is false-y, and anything not 0 is truth-y.

  6. Show all issues

    Again, best to avoid the one-liner if you can. Try:

    } else {
      setValidation(true);
    }
    
  7. 
      
imadueme
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        checklist/checklist/extension.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        checklist/checklist/extension.py
    
    Ignored Files:
        checklist/checklist/static/js/views/checklistView.js
    
    
  2. 
      
david
Review request changed
Status:
Discarded