Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

Add term table, accomodate multiple terms, fix course.sis_id bug (#113, #117) #115

Merged
merged 19 commits into from
Apr 28, 2020

Conversation

ssciolla
Copy link
Contributor

@ssciolla ssciolla commented Apr 27, 2020

This PR modifies the COURSE_INVENTORYjob and the database schema as to be able to collect and store data related to multiple academic terms. Some additional details are provided in the task list below. This PR aims to resolve issues #113 and #117.

  • Add migration with new term table and with a term_id column and foreign key constraint to the course table.
  • Make CANVAS_TERM_ID configuration variable plural, i.e. CANVAS_TERM_IDS.
  • Add function that pulls data for terms specified in the config.
  • Modify get_course_data_from_api function to accept multiple terms, nest fetching process insidefor loop, and then combine all results into one course_df (ensuring that future processes will consider courses from the multiple terms specified).
  • Write contents of term_df to database.
  • Add migration changing the data type of course.sis_id to VARCHAR(15) and type coercion to string of those values (see issue Change data type of course.sis_id to VARCHAR #117 for details).
  • Update online_meetings/canvas_zoom_meetings.py to handle multiple terms.
  • Update README.

@ssciolla ssciolla added 📈 enhancement New feature or request config change introduced changes to the configuration file labels Apr 27, 2020
course_inventory/inventory.py Show resolved Hide resolved
course_inventory/inventory.py Outdated Show resolved Hide resolved
course_inventory/inventory.py Show resolved Hide resolved
course_inventory/inventory.py Show resolved Hide resolved
@ssciolla ssciolla marked this pull request as ready for review April 27, 2020 21:47
course_inventory/inventory.py Outdated Show resolved Hide resolved
course_inventory/inventory.py Show resolved Hide resolved
@ssciolla ssciolla requested a review from zqian April 27, 2020 22:07
@ssciolla ssciolla changed the title Add term table, accomodate multiple terms (#113) Add term table, accomodate multiple terms, fix course.sis_id bug (#113, #117) Apr 28, 2020
@lsloan
Copy link
Member

lsloan commented Apr 28, 2020

@ssciolla, any idea why Codacy claims zoom_course_report is using "%s as argument"? (Which Codacy can't even display correctly!)

Can we use comments to tell Codacy (or in this case, PyLint specifically) to ignore this error for this section of code? (https://github.com/tl-its-umich-edu/course-inventory/pull/115/files#diff-9e1a06acd700cfb38555dc5980f1cdb0R163)

@ssciolla
Copy link
Contributor Author

@ssciolla, any idea why Codacy claims zoom_course_report is using "%s as argument"? (Which Codacy can't even display correctly!)

Can we use comments to tell Codacy (or in this case, PyLint specifically) to ignore this error for this section of code? (https://github.com/tl-its-umich-edu/course-inventory/pull/115/files#diff-9e1a06acd700cfb38555dc5980f1cdb0R163)

I think that's an issue that was there before; it doesn't like the default mutable argument of []. I'm not really sure I get why it's an issue; if we agree, maybe it's something we can configure in Codacy?

@ssciolla ssciolla added the 🐛 bug Something isn't working label Apr 28, 2020
Copy link
Member

@lsloan lsloan left a comment

Choose a reason for hiding this comment

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

After discussion, the things I commented on seem to be minor issues, so I'll give my approval of this PR.

:shipit:

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.

I think this seems fine and will work. The only problem is that there is no indication of the term for the canvas_zoom_meetings in the csv report but I don't think this is necessary right now and this will be changed anyway with #114. Just have the one fix that should probably be made.

online_meetings/canvas_zoom_meetings.py Outdated Show resolved Hide resolved
@ssciolla ssciolla merged commit a818e64 into tl-its-umich-edu:master Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
🐛 bug Something isn't working config change introduced changes to the configuration file 📈 enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change data type of course.sis_id to VARCHAR Accomodate multiple terms in database
4 participants