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

Adding date_start and date_end to Course model and making other backend adjustments #652

Merged
merged 4 commits into from
Aug 13, 2019

Conversation

ssciolla
Copy link
Contributor

@ssciolla ssciolla commented Aug 9, 2019

This PR makes modifications to models.py, views.py, and admin.py and creates a new migration, in an effort to resolve Issue 343. The changes are summarized below:

  1. Two new fields -- date_start and date_end -- were added to the Course model in order to allow admins to manually enter a date range for a course. A helper method get_course_date_range was also added to the model so that if course dates are not provided, the dates associated with the relatedAcademicTerm are accessed. Additionally, the term_id field was changed to term to reflect that the field holds the AcademicTerms object instance, not an integer or string. The id field was also made editable, possibly eliminating some unwanted behavior; see Issue 613.

  2. The objects field of the AcademicTerms model was removed, as were the Manager and QuerySet helper classes for the model, as their functionality had been replaced with the changes to Course and they were no longer being used. A helper method get_correct_date_end was also added to correct far-out end years (usually ten years into the future) to the same year as the start date.

  3. A new migration file, 0010_auto_20190812_0924.py, was added to reflect the changes outlined in 1. and 2. (The original PR included another migration file, 0010_auto_20190809_1625.py.)

  4. Changes were made to views.py to rely on the get_course_date_range method for determining the course's beginning and end dates. These modifications mean that course dates provided in the admin interface will affect the number of weeks that show up in the front end, for instance, through the Resources Accessed view.

  5. Some additional fields were provided in the list_display field of the CourseAdmin class in admin.py to make the list appearance of courses in the admin interface more informative.

@ssciolla ssciolla requested review from jonespm and zqian August 9, 2019 21:29
dashboard/migrations/0010_auto_20190809_1625.py Outdated Show resolved Hide resolved
dashboard/models.py Outdated Show resolved Hide resolved
dashboard/models.py Outdated Show resolved Hide resolved
dashboard/models.py Outdated Show resolved Hide resolved
dashboard/views.py Outdated Show resolved Hide resolved
@ssciolla
Copy link
Contributor Author

FYI, in addition to the changes @zqian requested, I simplified the verbose names for dates on the AcademicTerms model, and I modified the name of a method in views.py -- get_course_date_start -- to be in line with the name of the Course model field.

@tl-its-umich-edu tl-its-umich-edu deleted a comment Aug 12, 2019
@zqian zqian self-requested a review August 12, 2019 18:31
Copy link
Member

@zqian zqian left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

end = self.date_end
else:
end = self.term.get_correct_date_end()
return start, end
Copy link
Member

Choose a reason for hiding this comment

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

A namedtuple would make this easier to read and work with. It's an 'anti-pattern' to use an ambiguous index.

https://docs.quantifiedcode.com/python-anti-patterns/readability/not_using_named_tuples_when_returning_more_than_one_value.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense to me. See latest commit.

return df['date_start'].iloc[0]
def get_course_date_start(course_id):
logger.info(get_course_date_start.__name__)
course_date_start = Course.objects.get(id=course_id).get_course_date_range()[0]
Copy link
Member

Choose a reason for hiding this comment

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

Using namedtuple (above) would make this get_course_date_range().date_start

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See latest commit.

Copy link
Member

@jonespm jonespm left a comment

Choose a reason for hiding this comment

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

This looks good @zqian's comments looks good. I'd fix the one issue I mentioned. I'm not sure if we've been using this practice in the past but this one is better.

@jonespm
Copy link
Member

jonespm commented Aug 12, 2019

@ssciolla Looks good, this was a really good Pycon session where I first saw namedtuple and how cool it was if you ever have time to watch! https://www.youtube.com/watch?v=wf-BqAjZb8M

@tl-its-umich-edu tl-its-umich-edu deleted a comment Aug 12, 2019
@zqian
Copy link
Member

zqian commented Aug 13, 2019

@ssciolla Looks good, this was a really good Pycon session where I first saw namedtuple and how cool it was if you ever have time to watch! https://www.youtube.com/watch?v=wf-BqAjZb8M

Maybe dev should have a list of "favorite videos to watch when you got time" somewhere...

@ssciolla ssciolla merged commit c964a30 into tl-its-umich-edu:master Aug 13, 2019
justin0022 added a commit that referenced this pull request Aug 14, 2019
* "the welcome page" snuck in again somehow (#627)

* Extracted the snackbar component so it can be used anywhere where there is user setting (#626)

* Fixes #619 resource_type is marked as primary key but doesn't need to be (#620)

* #579 assignments now scrolls to current week row (#632)

* #579 assignments now scrolls to current week row

* #579 getting the local time from the timezone object instead

* #631 adding default resources property (#633)

* #631 adding default resources property

* Update URL

* Fixes #533 Files accessed view keeps showing loading spinner (#636)

* Fixes #634 Fix spelling of variable roundToOneDecimcal (#635)

* Resolving issue 628: checkbox color, grade toggling, admin handling (#640)

* Fix Resources view issues (#638)

* Fix typo (#649)

Renamed `show_files_accessed` to `show_resources_accessed`  in a couple places

* Changes to README and new CONTRIBUTING doc (#606)

* README intro written

* Added contributing guide

* Reorganized readme

* Added a note about technology used

* Updated headings

* Moved contributing guide to bottom

* Small change to CONTRIBUTING

* Added link to google drive

* Small changes to contributing

* expanded technology overview for front end

* Email field required for creating user via command line

* Adding Backend tech stack

* Fixes #644 - Name for courses in urls.py wrong (#645)

* xaxis is reflects based on % from data than always showing  0-100 (#654)

* Adding date_start and date_end to Course model and making other backend adjustments (#652)

* Modifying Course, AcademicTerms models; updating views to use course dates

* Re-adding db_constraint to term; adding far-out year handling; fleshing out Course appearance in admin

* Modifying verbose names; simplifying query; adjusting logger

* Implementing namedtuple for DateRange

* #643 adding a check for no students in a particular grade range (#655)

* Fixes #650 Update support for CSP headers (#656)

* Fixes #650 Update CSP headers

* Adding XFrameOptions as alternative

* Fixes #653 map a secrets directory for easier addition of secrets (#657)

* Fixes #653 map a secrets directory for easier addition of secrets

* Updating documentation

* Fixes based on Andrew's code review

* Fix LTI (#659)

Noticed that `rules.permissions.ObjectPermissionBackend` was interfering with the LTI login (ObjectPermissionBackend doesn't have a get_user function) since it is the first auth method listed after the permissions update. Switching to explicitly using the `django.contrib.auth.backends.ModelBackend` backend (and making sure its a backend available if lti is enabled)
andrew-gardener added a commit that referenced this pull request Aug 14, 2019
* master:
  Fix LTI (#659)
  Fixes #653 map a secrets directory for easier addition of secrets (#657)
  Fixes #650 Update support for CSP headers (#656)
  #643 adding a check for no students in a particular grade range (#655)
  Adding date_start and date_end to Course model and making other backend adjustments (#652)
  xaxis is reflects based on % from data than always showing  0-100 (#654)
  Fixes #644 - Name for courses in urls.py wrong (#645)
  Changes to README and new CONTRIBUTING doc (#606)
  Fix typo (#649)
  Fix Resources view issues (#638)
  Resolving issue 628: checkbox color, grade toggling, admin handling (#640)

# Conflicts:
#	README.md
#	dashboard/models.py
@ssciolla ssciolla mentioned this pull request Sep 19, 2019
@ssciolla ssciolla deleted the issue-343-course-dates branch December 16, 2020 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
MyLA-Fall-2019.02.01
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Allow admins to set manual start and end dates for the term.
3 participants