#M27. Add "Status" model to replace "Last LMS module" field with a Foreignkey. #236
Conversation
TravelTimN
left a comment
TravelTimN
left a comment
There was a problem hiding this comment.
LGTM - a few comments and suggestions, but otherwise, nicely done to convert into statuses instead of LMS level. 👍🏻
| "model": "accounts.status", | ||
| "pk": 6, | ||
| "fields": { | ||
| "display_name": "Studying for a (between Project 4 & 5)", |
There was a problem hiding this comment.
Why is this the only fixture that states: Studying for a whereas the others all just say Studying?
| "model": "accounts.status", | ||
| "pk": 8, | ||
| "fields": { | ||
| "display_name": "Working as Developer 0-3 yrs", |
There was a problem hiding this comment.
I would make this 0-2 years, and then pk9 can stay 3-5 years so the 3rd year doesn't overlap.
accounts/fixtures/statuses.json
Outdated
| "display_order": 13 | ||
| } | ||
| } | ||
| ] No newline at end of file |
There was a problem hiding this comment.
EOF blank line missing
| {% endfor %} | ||
| </table> | ||
| </div> | ||
|
|
There was a problem hiding this comment.
Outer div with class of table-responsive seems to have gone missing... was this intentional, and is it still responsive?
| <button class="btn btn-primary btn-sm float-right downloadTable" data-tableid="judgesTable">Export Judges Data to CSV</button> | ||
| </div> | ||
|
|
||
| <table class="table table-sm border-top-0" id="judgesTable"> |
There was a problem hiding this comment.
Same here... outer div with table-responsive is now missing... assuming this was intentional, and does it still respond well?
stefdworschak
force-pushed
the
new_lms_modules
branch
from
8dc853c to
1b26604
Compare
2 participants
Description
Pull request type
Related Issue
#230
Configuration instructions
Testing
All tests passing successfully.