Skip to content
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

E0d/refine oep 26 #405

Merged
merged 16 commits into from
Nov 4, 2022
Merged

E0d/refine oep 26 #405

merged 16 commits into from
Nov 4, 2022

Conversation

e0d
Copy link
Contributor

@e0d e0d commented Nov 2, 2022

@bmtcril here's my initial crack at fixing obviously outdated parts of this OEP.

Copy link

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

Just the one comment about clarifying the LRS decision

@@ -26,7 +26,7 @@ xAPI includes a specification for a Learning Record Store (LRS), which encapsula
.. image:: ./adaptive_learning_lrs_basic.png
:alt: The diagram above is enhanced with a new LRS component that receives events from the Open edX "Eventing" component and is accessed by adaptive engines for training purposes, using xAPI for both transactions.

In the short term, however, we will not implement our own LRS. We will look into integration efforts with third party LRS services.
We will not implement our own LRS. We will look into integration efforts with third party LRS services.
Copy link

Choose a reason for hiding this comment

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

This change seems to be at odds with oeps/architectural-decisions/oep-0026-arch-realtime-events.rst line 242

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is this better?

Copy link

@bmtcril bmtcril left a comment

Choose a reason for hiding this comment

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

I believe this aligns with where we're at 👍

@@ -138,7 +139,10 @@ For details on integrating with Caliper, please see the :ref:`caliper_realtime_e
Anonymized User ID
==================

The *LMS user_id* will be used to uniquely identify a user in the Open edX system. This decision is detailed in :ref:`oep-32`.
Users will be identified to external systems using a UUID that is associated uniquely with a single user and the external system type with which the UUID can be shared. This decision overrides :ref:`oep-32`. and is captured in `ADR 0001-externalid.rst`_
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. The period is in the wrong place in the final sentence.
  2. Do you want to create an issue in https://github.com/openedx/open-edx-proposals/issues about OEP-32 and what you think needs to happen? Or a PR that adds a warning to OEP-32 about its questionable state? (I admit that I think something should be done, but am unsure of exactly what to do and am not thinking in this area right now, so this will probably just sit if you decide to do nothing.)

Copy link
Contributor Author

@e0d e0d Nov 4, 2022

Choose a reason for hiding this comment

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

I've created a basic ticket to capture the work. I think that OEP-32 should be marked as obsolete.

I'm happy to do that if you are happy to act at the arbiter for that small change. I'm not planning to take on the substantial rewrite at this time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we could point to the issue as the Superseded link, and clarify that the link should be updated once the issue is complete?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean in OEP-32 or here? I think that as an initial useful step we would update OEP-32 to indicate that it was obsolete and link to the ADR for reference.

@e0d e0d merged commit 5bdd9ac into openedx:master Nov 4, 2022
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