Skip to content

[BD-03] [TNL-2624] [BB-3168] Piazza Plugin for discussions - #291

Closed
xitij2000 wants to merge 3 commits into
kshitij/discussion-plugin-apifrom
kshitij/piazza-plugin
Closed

[BD-03] [TNL-2624] [BB-3168] Piazza Plugin for discussions#291
xitij2000 wants to merge 3 commits into
kshitij/discussion-plugin-apifrom
kshitij/piazza-plugin

Conversation

@xitij2000

@xitij2000 xitij2000 commented Nov 10, 2020

Copy link
Copy Markdown
Member

Adds a plugin to enable Piazza discussions in the discussions tab.
Build on https://github.com/edx/edx-platform/pull/24584

Dependencies: https://github.com/edx/edx-platform/pull/24584

Screenshots: Always include screenshots if there is any change to the UI.

Sandbox URL: TBD - sandbox is being provisioned.

Reviewers

  • (OpenCraft internal reviewer's GitHub username goes here)
  • edX reviewer[s] TBD

Settings

EDXAPP_FEATURES:
  ENABLE_COMBINED_LOGIN_REGISTRATION: true

@xitij2000 xitij2000 changed the title Kshitij/piazza plugin [BD-03] [TNL-2624] [BB-3168] Piazza Plugin for discussions Nov 10, 2020
}
"""
)
return fragment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@giovannicimolin

I'd like to create some base code that all Discussion plugins to use for embedding via LTI.
I think the most flexible approach is to have a base LTI Launcher view that automates most important parameters, but can be extended with additional ones as needed.

Do you think what I've done here roughly makes sense for that goal?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm sort of wondering if we can fold all the LTI-specific code into a general purpose LTI fragment, owned by xblock_lti_consumer, and store any custom parameters in our LearningContextDiscussionConfig table or on LtiConfiguration itself. If we're able to lean more heavily on our existing LTI classes, it feels like the need for these provider-specific plugins almost completely disappears.

  1. LtiCourseLaunchMixin seems totally generic and gets folded into xblock_lti_consumer.
  2. PiazzaCourseTabView is no longer necessary, since the LTI fragment would know how to get the oauth credentials out of where they're stored by LtiConfiguration.
  3. The only thing left, I think, is the friendly_name and any future information we need to store in capabilities on PiazzaDiscussionApp. Both of those seem solely related to rendering the Pages and Resources view discussion plugin tiles and capabilities table, and actually have nothing (I think) to do with actually rendering or configuring the plugin itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I agree about extracting common code, and that is why I split the mixin out this way. Since @giovannicimolin has worked a lot with LTI, I meant to take his input about how this code should be structured to allow the flexibility we might need.

  1. This is definitely the mid-term intention, but I want to do some discovery about where the boundaries should lie, i.e. where should the discussion plugin be able to reach out to the plugin. Also how do you render this in an MFE instead of the current UI. That said, it's easier to test a single PR with all the changes and then split it out.
  2. and 3. In the case of Piazza, yes. A pure LTI integration might not need any additional config. However, CS comments service, cannot plug in as an LTI fragment, and other discussion plugins might not use LTI. So they will need a different mechanism to declare their views. The metadata here can just come from a settings dict, so mainly the plugin class is needed to provide actual code paths.

@davidjoy davidjoy Nov 17, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Regarding 2: In more general terms, I think what we're talking about is having edx-platform render views supplied from the plugin. I don't know of a use case for this, given that we're moving all our view code out into MFEs anyway. Certainly today we need to render the discussion tab via django templates, but for our current use cases I think that's purely LTI anyway. It can be owned by xblock_lti_consumer in the medium term until we fully cut over to MFEs.

What I'm advocating is effectively that these discussion apps become purely frontend plugins that interact with the platform solely via LTI, or if a more rich integration is necessary that the LTI protocol doesn't support, other REST APIs that we then document as part of the platform's discussion API.

Third-party providers would then have to write some code if they wanted to take advantage of those rich capabilities; that said, I don't know that we have any expectation anyone will do that in the foreseeable future.

Anything else that the plugin needs could be supplied as part of its configuration and stored in the DB, I hope. There are likely to be snippets of data we want to display outside an iframe. I.e., if we wanted the discussion tab to say "Piazza Discussions" for some reason, then the name of the discussion app, "Piazza", would be supplied as part of the configuration, rather than needing to be rendered by a server-side plugin.

Separately, none of this precludes us from using server-side plugins in conjunction with frontend plugins in the future for complex, rich interfaces that are tightly coupled to the internals of edx-platform, should the need arise. But it does set up the expectation that we'll still aim for a frontend-backend split, even in our plugins.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@xitij2000 Getting back to your first comment:

I think the most flexible approach is to have a base LTI Launcher view that automates most important parameters, but can be extended with additional ones as needed.

This is being slowly implemented for the LTI 1.3 integration. The LTI-specific configuration lives (or will live) in the LtiConfiguration model, and a LTI consumer can instanced using LtiConfiguration.objects.get().get_lti_consumer() which returns a class that does all LTI logic.

Launch views are still being handled inside the XBlock (see https://github.com/edx/xblock-lti-consumer/blob/master/lti_consumer/lti_xblock.py#L1013) to avoid having to deal with access and visibility settings directly.


Contrary to LTI 1.3, LTI 1.1 launches are pretty simple: there's the obligatory parameters and then the extra custom fields, and the entire launch was extracted to a single function (lti_embed). You can use it directly instead of relying in the LtiConfiguration class, but handling access permissions is up to you since this just does the launch.

LtiCredentials = namedtuple('LtiCredentials', ('oauth_key', 'oauth_secret'))


class LtiCourseLaunchMixin:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Once we create a yellodig plugin we'll have a better idea of what all LTI code can be kept in common, and come up with an generic interface that multiple plugins can reuse.

Essentially, what we need is code that helps in building a LTI launch that automatically derives as many correct settings as possible from the current context.

Here it will only support the course context, but for LTI embeds inside a unit or elsewhere this should be able to support other contexts as well. This can potentially be done by overriding attributes or methods.

Comment on lines +59 to +64
def get_resource_link_id(course_key: CourseKey, request: HttpRequest) -> str:
site = get_current_site(request)
return quote('{}-{}'.format(
site.domain,
str(course_key.make_usage_key('course', course_key.run)),
))

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This needs to be different for every placement, so perhaps instead of using the course key and converting it to a usage key this can take a usage key directly, and another method can build that.

Comment on lines +159 to +170
class PiazzaCourseTabView(LtiCourseLaunchMixin, EdxFragmentView):
"""
Course tab view for Piazza discusion provider.
"""
LTI_LAUNCH_URL = "https://piazza.com/connect"

@staticmethod
def get_oauth_credentials(config):
return LtiCredentials(
config.config["consumer_key"],
config.config["consumer_secret"],
)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Eventually this will be all there is in the plugin, the rest will move to openedx.core ...

@davidjoy

Copy link
Copy Markdown

Initial thought - I'm wondering about whether or not the plugin should actually be in edx-platform, or whether it should be in a separate repo. Since it's technically an extension and not really a part of the core, does it belong here? Or should we just link it in through requirements?

@xitij2000 xitij2000 closed this Dec 25, 2020
@xitij2000
xitij2000 deleted the kshitij/piazza-plugin branch December 25, 2020 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants