-
Notifications
You must be signed in to change notification settings - Fork 7
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
DRAFT: BB-2798 LTI plugin for piazza discussions #232
DRAFT: BB-2798 LTI plugin for piazza discussions #232
Conversation
Add Discussion model Added discussions app Added model to link course and discusion Added API to access configuration options for a course Added API to access active configuration for a course
Add new discussions app to test settings
Added tests for apis and fix existing tests Fix bugs Fix docs
name should never be blank provider should be indexed and never blank configs should never be blank and should default to an empty dict
Add admin models
3132844
to
98e9d1a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is just for testing so not approving it yet. I've set up a sandbox based on this.
HTML( | ||
lti_embed( | ||
html_element_id='piazza-discussion-lti-embed', | ||
lti_launch_url=self.LTI_LAUNCH_URL, | ||
oauth_key=discussion_config.private_config["oauth_key"], | ||
oauth_secret=discussion_config.private_config["oauth_secret"], | ||
resource_link_id=resource_link_id, | ||
user_id=user_id, | ||
roles='Student', # TODO Use API for this when available | ||
context_id=course_id, # TODO Use API for this when available | ||
context_title=course.display_name_with_default, | ||
context_label=course.display_org_with_default, | ||
result_sourcedid=result_sourcedid | ||
) | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice if we could be put this in an iframe. But it's OK for testing.
# IMPORTANT: Do not import anything in this file in the top level of any | ||
# file that is required by classes that are themselves DiscussionApps, as it | ||
# will cause a circular import issue. Instead, import it only at the last moment | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out. I'll try to reduce these interdependencies.
from openedx.core.djangoapps.discussions.discussions_apps import DiscussionApp | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid unnecessary diffs, in this case it's OK since it's just to test.
ff81a85
to
e22d5b4
Compare
e22d5b4
to
fa8e131
Compare
40f8a53
to
303662a
Compare
80be2d4
to
604d51e
Compare
0cc328f
to
c5e85c7
Compare
Please note that this PR is NEVER intended to be merged. It has been created to facilitate discussion on the code while https://github.com/edx/edx-platform/pull/24584/ waits to be merged upstream. This PR builds on (and is compared directly against) https://github.com/open-craft/edx-platform/tree/kshitij/discussion-plugin-api.
** Testing Instructions **:
pip install -e .
make lms-restart