Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#M27. Add "Status" model to replace "Last LMS module" field with a Foreignkey. #236

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

stefdworschak
Copy link
Member

Description

  • Adding new model called "Status"
  • Adding fixtures to seed and updating existing fixtures
  • Replacing all existing references to Last LMS module to the new Status model
  • Updating label from "Last LMS module" to "Programming Experience"

Pull request type

Related Issue

#230

Configuration instructions

  • Need to download the current statuses and update with new model afterwards
  • Communicate the change in the hackathon channel

Testing

All tests passing successfully.

Copy link
Collaborator

@TravelTimN TravelTimN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make this 0-2 years, and then pk9 can stay 3-5 years so the 3rd year doesn't overlap.

"display_order": 13
}
}
]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

EOF blank line missing

{% endfor %}
</table>
</div>

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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">
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here... outer div with table-responsive is now missing... assuming this was intentional, and does it still respond well?

@stefdworschak stefdworschak added the hacktoberfest-accepted Accepted PR during Hacktoberfest label Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accepted PR during Hacktoberfest
Projects
None yet
Development

Successfully merging this pull request may close these issues.

#M27. Add "Status" model to replace "Last LMS module" field with a Foreignkey.
2 participants