Conversation
bradley-erickson
left a comment
bradley-erickson
left a comment
There was a problem hiding this comment.
I left quite a few comments. Many of them apply to more places in the code than where I mentioned. For each piece of feedback, try to apply it to like items (i.e. anything I state about Canvas apply to Schoology).
It might be easier to focus on a single LMS. Get that down and then the next one should be a little easier/quicker to manage.
| build/ | ||
| dist/ | ||
| node_modules | ||
| learning_observer/learning_observer/learning_tools/config.ini |
There was a problem hiding this comment.
All settings should be defined using PMSS. I would assume this code currently don't have PMSS anywhere in it. ArgLab is a bit behind and ought to catch up.
| debug_log("Error handling:", google_id) | ||
| raise | ||
|
|
||
| def canvas_id_to_user_id(google_id): |
There was a problem hiding this comment.
This is exactly the same as the google_id_to_user_id method above it,. Why do we need it?
Most of the student events should be getting recorded with a Google ID. After signing into either LMS, we need to be able to fetch to Google ID somehow.
| register_incoming_event_views(app) | ||
| register_debug_routes(app) | ||
| learning_observer.google.initialize_and_register_routes(app) | ||
| learning_observer.canvas.initialize_and_register_routes(app) |
There was a problem hiding this comment.
We ought to specify which items we want initialized and used in the settings. Some installs we may only want one or two of these methods.
| if settings.settings['roster_data']['source'] in ["google_api"]: | ||
| runtime = learning_observer.runtime.Runtime(request) | ||
| return await learning_observer.google.courses(runtime) | ||
| elif settings.settings['roster_data']['source'] in ["canvas"]: |
There was a problem hiding this comment.
This code and the following code look really similar. Eventually we want to query the system like this
learning_observer.rosters.courselist(school="appleton")
The courselist function should find the appropriate source to use for school Appleton and then call it. PMSS takes care a bit of this for us.
rosters = {'google': learning_observer.google.courses, 'canvas': ...}
roster_source = learning_observer.settings.pmss_settings.roster_source(school="appleton")
runtime = learning_observer.runtime.Runtime(request)
await rosters[roster_source](params)| elif settings.settings['roster_data']['source'] in ["all"]: | ||
| ajax = all_ajax | ||
| elif settings.settings['roster_data']['source'] in ["canvas", "schoology"]: | ||
| ajax = google_ajax |
There was a problem hiding this comment.
Why does this use google_ajax? If its intentional that it uses the same requests as Google, we ought to rename the function to be more general and descriptive about its use.
| >>> ("hello {hi} my {bye}")] | ||
| ['hi', 'bye'] | ||
| ''' | ||
| # The parse returns a lot of context, which we discard. In particular, the |
There was a problem hiding this comment.
If this is copied straight from the google.py document, please make sure the comments are updated to reflect the new workflows.
| script_dir = os.path.dirname(os.path.abspath(__file__)) | ||
| config_path = os.path.join(script_dir, config_path) | ||
|
|
||
| self.config = configparser.ConfigParser() |
| # `None` | ||
| return [f[1] for f in string.Formatter().parse(format_string) if f[1] is not None] | ||
|
|
||
| class Canvas: |
There was a problem hiding this comment.
Is a whole class needed here? Google gets by without it.
If the user logs in with Canvas the request object should contain their auth information to query the Canvas APIs.
|
|
||
| return await response.json() | ||
|
|
||
| async def refresh_tokens(self): |
There was a problem hiding this comment.
When does this come up? I understand that we are refreshing the user's token, but I'd like to know when this occurs (docstring).
|
|
||
| def initialize_and_register_routes(app): | ||
| ''' | ||
| This is a big 'ol function which might be broken into smaller ones at some |
There was a problem hiding this comment.
This is a big 'ol function that was copied from google.py. We ought to abstract this out.
#112) - Added timestamp that keeps track of when documents are accessed - Added source function to communication protocol - Added filter to each dashboard to select docs accessed at a specific date and time
2 participants
No description provided.