• 
      

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

    Review Request #8053 — Created March 11, 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