Add status reports to sonar.

Review Request #7574 — Created Aug. 8, 2015 and submitted

Information

student-sonar
master
4c04f3e...

Reviewers

This is a very large change which adds a big new status reports feature to
sonar. This contains a lot of individual pieces, along with a number of
refactors to clean things up.

At a high level, the new features are:

  • A "My Status Reports" page which is aimed towards students. This shows a list
    of the due dates that apply to them. These are color coded/labeled as either
    "Submitted", "Past Due", "Due Soon", or nothing. Clicking on a due date will
    open up the editor view.
  • An "All Status Reports" view, which is only seen by admins. This shows all of
    the due dates for all groups. Users are sorted into two columns: "Submitted"
    and "Not Submitted". Any dates which are in the past are color-coded either
    red or green depending on whether any status reports are missing.
  • A status report editor view. This is pre-populated with the example text from
    our welcome packet. The editor can be flipped to a "preview" mode, and has a
    neato fullscreen mode that presents a side-by-side view.
  • A status report reading view, which just shows the rendered markdown text.
  • Links to the new status reports from the user detail view.

Various things have been refactored, mostly related to the "ready" state of the
application. Things seem to be migrating towards a steady state here so I'll
probably follow up later with a new view class that handles all of this. There
was also some very confusing issues with the parsing of the "me" user, so I've
switched that out to just pre-populate the userID for the current user and then
match that up once all the users are fetched from the API.

Exercised all of the functionality a ton.


Description From Last Updated

Leftover from debugging?

brenniebrennie

You're inconsistent with spacing between top-level definitions.

brenniebrennie

You can use submitted.forEach here.

brenniebrennie

Leftover from debugging?

brenniebrennie

Use an arrow function.

brenniebrennie

Use an arrow function.

brenniebrennie

Use an arrow function.

brenniebrennie

Do we care about line width here?

brenniebrennie

This is indented strangely. I would expect the body to be indented relative to the function keyword.

brenniebrennie

Same here.

brenniebrennie

Shouldn't this be this.get('users').get(window.userID) ?

brenniebrennie

Blank line between these.

brenniebrennie

Shouldn't this use <%- -> ?

brenniebrennie

Should 200 be a constant somewhere?

brenniebrennie

If this is listening for resize events, maybe we want to _.debounce instead of _.throttle ?

brenniebrennie

Use an arrow function.

brenniebrennie

Should this use <%- ?

brenniebrennie

Assuming dueDates is an array, you can use dueDates.each((dueDate) => { // ... });

brenniebrennie

I'm not very familiar with mongo/mongoose, but shouldn't this be a Date ? Or is this a foreign key-esque pointer …

brenniebrennie

Blank line between these.

brenniebrennie
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        package.json
        lib/frontend/config.js
        lib/frontend/application.js
        lib/frontend/user-detail-view.js
        lib/frontend/util.js
        lib/api.js
        views/student-list.handlebars
        lib/frontend/status-report-editor-view.js
        lib/frontend/all-status-reports-view.js
        lib/frontend/sidebar-view.js
        lib/frontend/calendar-view.js
        lib/frontend/application-view.js
        lib/frontend/status-report-checker-view.js
        lib/schema.js
        lib/frontend/my-status-reports-view.js
        css/style.less
        lib/frontend/status-report-reading-view.js
        lib/routes.js
        lib/frontend/models.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        package.json
        lib/frontend/config.js
        lib/frontend/application.js
        lib/frontend/user-detail-view.js
        lib/frontend/util.js
        lib/api.js
        views/student-list.handlebars
        lib/frontend/status-report-editor-view.js
        lib/frontend/all-status-reports-view.js
        lib/frontend/sidebar-view.js
        lib/frontend/calendar-view.js
        lib/frontend/application-view.js
        lib/frontend/status-report-checker-view.js
        lib/schema.js
        lib/frontend/my-status-reports-view.js
        css/style.less
        lib/frontend/status-report-reading-view.js
        lib/routes.js
        lib/frontend/models.js
    
    
  2. 
      
brennie
  1. This looks awesome :D

  2. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
    Show all issues

    Leftover from debugging?

  3. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    You're inconsistent with spacing between top-level definitions.

  4. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    You can use submitted.forEach here.

  5. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
    Show all issues

    Leftover from debugging?

  6. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
     
     
    Show all issues

    Use an arrow function.

  7. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
     
     
    Show all issues

    Use an arrow function.

  8. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
     
     
     
     
    Show all issues

    Use an arrow function.

  9. lib/frontend/all-status-reports-view.js (Diff revision 1)
     
     
    Show all issues

    Do we care about line width here?

    1. I think I'm going to make an executive decision for this codebase and say "no".

  10. lib/frontend/application-view.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    This is indented strangely. I would expect the body to be indented relative to the function keyword.

  11. lib/frontend/application-view.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    Same here.

  12. lib/frontend/application.js (Diff revision 1)
     
     
    Show all issues

    Shouldn't this be this.get('users').get(window.userID) ?

  13. lib/frontend/my-status-reports-view.js (Diff revision 1)
     
     

    Missing a period :)

  14. lib/frontend/my-status-reports-view.js (Diff revision 1)
     
     
     
    Show all issues

    Blank line between these.

  15. Show all issues

    Shouldn't this use <%- -> ?

    1. Arr, I always get those backwards.

  16. lib/frontend/status-report-editor-view.js (Diff revision 1)
     
     
     
     
     
    Show all issues

    Should 200 be a constant somewhere?

  17. Show all issues

    If this is listening for resize events, maybe we want to _.debounce instead of _.throttle ?

  18. lib/frontend/status-report-editor-view.js (Diff revision 1)
     
     
     
     
     
     
     
     
    Show all issues

    Use an arrow function.

    1. This is multiple statements so an arrow function isn't appropriate.

    2. I still feel this should be a multistatement arrow function.

  19. Show all issues

    Should this use <%- ?

  20. lib/frontend/user-detail-view.js (Diff revision 1)
     
     
    Show all issues

    Assuming dueDates is an array, you can use

    dueDates.each((dueDate) => {
        // ...
    });
    
  21. lib/schema.js (Diff revision 1)
     
     
    Show all issues

    I'm not very familiar with mongo/mongoose, but shouldn't this be a Date ? Or is this a foreign key-esque pointer to a CalendarEntry. If so, can we document it as such?

  22. 
      
david
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        package.json
        lib/frontend/config.js
        lib/frontend/application.js
        lib/frontend/user-detail-view.js
        lib/frontend/util.js
        lib/api.js
        views/student-list.handlebars
        lib/frontend/status-report-editor-view.js
        lib/frontend/all-status-reports-view.js
        lib/frontend/sidebar-view.js
        lib/frontend/calendar-view.js
        lib/frontend/application-view.js
        lib/frontend/confirm.js
        lib/frontend/status-report-checker-view.js
        lib/schema.js
        lib/frontend/my-status-reports-view.js
        css/style.less
        lib/frontend/status-report-reading-view.js
        lib/routes.js
        lib/frontend/models.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        package.json
        lib/frontend/config.js
        lib/frontend/application.js
        lib/frontend/user-detail-view.js
        lib/frontend/util.js
        lib/api.js
        views/student-list.handlebars
        lib/frontend/status-report-editor-view.js
        lib/frontend/all-status-reports-view.js
        lib/frontend/sidebar-view.js
        lib/frontend/calendar-view.js
        lib/frontend/application-view.js
        lib/frontend/confirm.js
        lib/frontend/status-report-checker-view.js
        lib/schema.js
        lib/frontend/my-status-reports-view.js
        css/style.less
        lib/frontend/status-report-reading-view.js
        lib/routes.js
        lib/frontend/models.js
    
    
  2. 
      
david
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        lib/frontend/user-detail-view.js
        lib/frontend/confirm.js
        lib/frontend/config.js
        lib/frontend/application-view.js
        css/style.less
        lib/routes.js
        lib/frontend/models.js
        package.json
        lib/frontend/my-status-reports-view.js
        lib/frontend/sidebar-view.js
        lib/schema.js
        lib/frontend/util.js
        lib/frontend/collection-view.js
        lib/frontend/status-report-editor-view.js
        lib/frontend/application.js
        lib/frontend/header-view.js
        lib/frontend/calendar-view.js
        lib/api.js
        views/student-list.handlebars
        lib/frontend/all-status-reports-view.js
        lib/frontend/status-report-checker-view.js
        lib/frontend/status-report-reading-view.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        lib/frontend/user-detail-view.js
        lib/frontend/confirm.js
        lib/frontend/config.js
        lib/frontend/application-view.js
        css/style.less
        lib/routes.js
        lib/frontend/models.js
        package.json
        lib/frontend/my-status-reports-view.js
        lib/frontend/sidebar-view.js
        lib/schema.js
        lib/frontend/util.js
        lib/frontend/collection-view.js
        lib/frontend/status-report-editor-view.js
        lib/frontend/application.js
        lib/frontend/header-view.js
        lib/frontend/calendar-view.js
        lib/api.js
        views/student-list.handlebars
        lib/frontend/all-status-reports-view.js
        lib/frontend/status-report-checker-view.js
        lib/frontend/status-report-reading-view.js
    
    
  2. 
      
brennie
  1. 
      
  2. lib/frontend/all-status-reports-view.js (Diff revision 3)
     
     
     
    Show all issues

    Blank line between these.

  3. 
      
david
Review request changed
Status:
Completed
Change Summary:
Pushed to master (ee02373)