Skip to content

[BD-24][TNL-7919] LTI Advantage Names and Roles provisioning service implementation direction - #142

Merged
nedbat merged 12 commits into
openedx:masterfrom
open-craft:giovanni/BB-3623-nrps-adr
Mar 3, 2021
Merged

[BD-24][TNL-7919] LTI Advantage Names and Roles provisioning service implementation direction#142
nedbat merged 12 commits into
openedx:masterfrom
open-craft:giovanni/BB-3623-nrps-adr

Conversation

@giovannicimolin

Copy link
Copy Markdown
Contributor

This PR adds an ADR to discuss the possible implementations of the LTI NRPS service.

Related PRs:
#124
https://github.com/edx/edx-platform/pull/25843
https://github.com/edx/edx-platform/pull/25844

@ormsbee Since you raised the scalability issue of the API.
@nedbat @shimulch

@openedx-webhooks openedx-webhooks added blended PR is managed through 2U's blended developmnt program needs triage labels Feb 17, 2021
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @giovannicimolin! I've created BLENDED-769 to keep track of it in Jira. More details are on the BD-24 project page.

When this pull request is ready, tag your edX technical lead.

@coveralls

coveralls commented Feb 17, 2021

Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 0.0% when pulling 168faa4 on open-craft:giovanni/BB-3623-nrps-adr into 8d18bc9 on edx:master.

Placeholder.

Consequences
============

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Another option is to pre-calculate the enrollment data (say on a nightly basis) and make a giant file that we push up to S3 and redirect to with an authorized link. That would allow us to serve the data quickly, though it would mean that the data was stale. Also, there's a good chance that getting a single file with a million enrollments would choke any potential consumer... but it is an option.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can actually pre-calculate multiple files in a scheduled task. And use those files as paginations. That will not decrease the number of requests, but requests will not do any db queries.

~~~~~~~~~~~~~~~~~~~~
Placeholder.

Implement NRPS gated by course

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This will be applicable for any implementation if we create xblock level flag if NRPS is enabled or not.

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Placeholder.

Implement NRPS limiting the context of the data retrieved

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure what context limiting means and how that helps. Is it reducing the number of fields per record?

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Placeholder.

Implement NRPS for courses up to a predefined number of students

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems a good option to me. One suggestion I have is to use the NRPS spec (pagination) based API for courses up to a fixed number of students. And Implement the pre-calculation approach suggested by ormsbee for other courses.

Placeholder.

Consequences
============

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We can actually pre-calculate multiple files in a scheduled task. And use those files as paginations. That will not decrease the number of requests, but requests will not do any db queries.

@ormsbee

ormsbee commented Feb 18, 2021

Copy link
Copy Markdown
Contributor

So before we dive too deeply on the merits of particular approaches, I'd really like this ADR to articulate the use cases we're solving for in implementing this part of the spec for Open edX courses. There are certain use cases I could imagine (e.g. check to see if this user is really allowed to use some service), for which having one-day-stale data may be unacceptable. If it's mostly used by proctoring and other premium services, maybe limiting it to a subset of enrollments makes sense. Those are completely unvalidated examples I made up on the spot, but my point is that I really don't know how different categories of clients are going to use this information, so it's hard to frame any discussion around technical tradeoffs.

Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Consequences
============

Depends on choosen approach. No newline at end of file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
Depends on choosen approach.
Depends on chosen approach.

giovannicimolin and others added 4 commits March 1, 2021 16:21
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>

@nedbat nedbat left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some tweaks of English to clarify who is acting.

Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
Comment thread docs/decisions/0004-lti-advantage-nrps.rst Outdated
giovannicimolin and others added 6 commits March 2, 2021 20:36
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
Co-authored-by: Ned Batchelder <ned@nedbatchelder.com>
@giovannicimolin

Copy link
Copy Markdown
Contributor Author

@nedbat I addressed your comments in the last commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

blended PR is managed through 2U's blended developmnt program merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants