docs: add ADR for runtime architecture components #12

mariajgrimaldi · 2025-06-25T12:22:43Z

Description

Add a follow-up ADR with the decisions regarding the architecture surrounding the functioning of the unified model as a consolidated system. This means that ADR 0002 describes what we need for the unified model, while ADR 0003 describes how we're making it work as a system.

This and the upcoming ADRs result from team conversations during a Spike phase, which produced several architectural documents. You can find them here: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4901404678/User+Groups

For detailed implementation information for this ADR, see: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4917657604/Unified+User+Group+Model+Technical+Approach

Here is also a proof of concept (POC) designed to test these decisions, along with the other ADRs: #7

⚠️ I use copilot and cursor with Claude Sonnet 4 for writing and structure improvements.

Merge checklist:
Check off if complete or not applicable:

Version bumped
Changelog record added
Documentation updated (not only docstrings)
Fixup commits are squashed away
Unit tests added/updated
Manual testing instructions provided
Noted any: Concerns, dependencies, migration issues, deadlines, tickets

openedx-webhooks · 2025-06-25T12:22:48Z

Thanks for the pull request, @mariajgrimaldi!

This repository is currently maintained by @openedx/committers-user-groups.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Update the status of your PR

Your PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

bmtcril

Also looking good, just a few more questions and comments!

bmtcril · 2025-07-10T16:43:49Z

docs/decisions/0003-runtime-architecture.rst

+* **Scope Integration**: Backend clients provide scope-aware methods that handle different contexts (course-level, organization-level, instance-level), using scopes to determine the appropriate data boundaries for queries.
+* **Dependency Injection Model**: The evaluation engine injects the appropriate backend clients into criterion types during evaluation, matching backends to criterion configuration requirements.
+* **Interface Abstraction**: All backend clients inherit from a common ``BackendClient`` base class and provide a consistent interface for data retrieval, allowing criterion types to remain agnostic of the underlying data source implementation.
+* **Data Format Standardization**: All backends with Django ORM access return QuerySet objects rather than materialized lists to enable lazy evaluation, query composition, and efficient optimization through Django's Q objects and database-level operations.


I think that more specifically it should return QuerySet objects of the configured User type, does that sound right to you?

Yes! I'll update this right away: c29e7a8.

Should we enforce this somehow?

I think type hinting might be enough?

bmtcril · 2025-07-10T16:49:00Z

docs/decisions/0003-runtime-architecture.rst

+
+To establish a consistent runtime interface for all criterion type templates, we will define a base criterion class that includes:
+
+* **Name**: How to identify the criterion type.


I'm a little unclear whether we want to represent version here as a first class property or just in the name. I think there was also a description on this model that I was hoping we could use for help text around usage of the criterion and offering upgrade guidance.

Now that I'm coming back to this I kind of find this a bit confusing. There are two concepts at play here:

The class name itself: where I proposed we added the versioning for the criterion. Now I'm questioning this, and wonder if we should be more explicit and add the version to the definition of the class alongside the configuration etc. something like:

class CourseEnrollmentCriterion(BaseCriterionType): """A criterion that is used to evaluate the membership of a user group based on the course enrollment mode of the user.""" updated_by_events = [COURSE_ENROLLMENT_CREATED, COURSE_ENROLLMENT_CHANGED] criterion_type: str = "course_enrollment" description: str = ( "A criterion that is used to evaluate the membership of a user group based on the course enrollment mode of the user." ) VERSION = "1.0.0"

We'd have to store this information in the model as we stated before, but not as part of the class name but as an additional field, I think.

The type, which is actually what identifies the criterion. This could be anything unique, this is used to load the class to actually run the evaluation, here: https://github.com/openedx/openedx-user-groups/pull/7/files#diff-51326ad6ae622049633d29835e91eb13381068aaccbad7188b010ec8fa3f775bR33-R41

That sounds great to me!

bmtcril · 2025-07-10T16:52:49Z

docs/decisions/0003-runtime-architecture.rst

+  * **Data access**: Handle retrieval of user data needed for evaluation
+  * **Evaluation logic**: Implement the core logic that determines which users match the criterion
+  * **Schema definition**: Expose machine-readable configuration requirements for UI generation
+  * **Error handling**: Provide clear error messages for invalid configurations


I suspect we will want/need some kind of management command to run through all saved criteria and validate them / report on failing criteria for active groups to help site admins troubleshoot upgrades or find configuration issues. Not sure if it belongs in this doc or is an implementation detail, but wanted to raise it.

What I think we could include is that any criteria should be able to dry run to raise any issues and not necessarily at runtime. I think this should also be included in the functional requirements for operators, what do you think?

Sounds great!

bmtcril · 2025-07-10T17:00:21Z

docs/decisions/0003-runtime-architecture.rst

+
+* Use an evaluation service that iterates over a group's configured ``Criterion`` entries.
+* Load the appropriate criteria type class via the registry for each rule, associating criterion type strings with their runtime classes.
+* Inject the appropriate backend client into each criterion type for data access.


Is this a thing we need to do (Criteria are independent of Backends), or would it be inherent to the Criterion class (Backends have explicit Criteria)? I'm not sure how we would support the former.

Instead of injecting the backends like I've been doing so far, we could consider making them available as utilities. I think that would be the easier approach.

bmtcril · 2025-07-10T17:02:07Z

docs/decisions/0003-runtime-architecture.rst

+* Use an evaluation service that iterates over a group's configured ``Criterion`` entries.
+* Load the appropriate criteria type class via the registry for each rule, associating criterion type strings with their runtime classes.
+* Inject the appropriate backend client into each criterion type for data access.
+* Invoke the logic defined in each criteria type class (the evaluator method) to return a list of matching user IDs.


This wouldn't be true in the case of ORM based criteria, right?

True! This would be true only for non-ORM based criteria. Do you think we should also return QS in those cases?

I'm conflicted on this point! QS is a no brainer for ORM criteria. I'd love to talk this through with you, but my current thinking is:

QS objects don't add a whole lot of overhead to pass around, and are vastly better for ORM based criteria (at least until we hit the 16MB MySQL default packet limit on query size...)

Calls to other services I though should prefer integer user ids when available (usernames change on retirement, emails could change on either side and cause issues)

... but can fall back to some other identifier that the plugin can look up to a user

Either way, what a criteria communicates back when it does a check is a QS

This is probably different from things I've said in other places, but I think it makes sense. Definitely open to more conversation about it.

I totally agree with that! I was thinking about a more straightforward way of combining this type of criterion with ORM-based ones. However, I think it wouldn't be too difficult with well-formatted identifiers.

bmtcril · 2025-07-10T17:20:06Z

docs/decisions/0003-runtime-architecture.rst

+  * Validate user input against criterion type schemas before submission.
+
+* Ensure schema definitions include sufficient metadata for generating user-friendly form interfaces through UI slots specific for criterion types.
+* Allow operators to extend or customize UI generation by providing additional metadata in the schema.


I'm not sure how feasible this is, we may need to limit criteria to certain known operators and values that the criteria can choose from. The UI will somehow need to know how to represent each type and I'm not sure we want to reinvent a markup language, allow arbitrary HTML (which would be brittle and possibly break on sites with custom CSS), or something similar to indicate every type of possible comparison in a way that we can translate to UI elements.

You're right. I agree! If a new operator needs to be considered, then it should be appropriately supported in, for example, here: https://github.com/openedx/openedx-user-groups/pull/7/files#diff-5976d9e5dbff7cb74f6c71a30db9f74661609f9a3a50dd1dbf1113cd58af38f2R28-R51

saraburns1 · 2025-07-16T14:51:06Z

docs/decisions/0003-runtime-architecture.rst

+To implement the materialized membership storage established in ADR 0002, we will:
+
+* Treat dynamic group membership as derived data, computed by evaluating the group's criteria against the available user data.
+* Store the evaluation result in the ``UserGroupMembership`` table, replacing any previous members for that group.


if the previous members are replaced, will we still be able to log somewhere that a specific user was removed from the group at that date and time?

Would it be possible we'd want to know why the user was removed (like, "criteria 1 not met, last visit was < 10 days ago"), since it's possible a member may be dynamically removed and added multiple times?

In #11 there is an open question about whether a tracking log exists. From reading the ADR I think that's the intention but still awaiting an answer.

I think it's sorted that auditing will happen in the tracking logs, but I would say that we should keep in mind that there could be huge groups at the instance level and churning out millions of tracking log statements when they evaluate may be prohibitive performance-wise.

To avoid overloading the model with membership data like this one, we logged any changes in user membership. Maybe a superset dashboard with this kind of information could be created, not sure about the performance implications though.

Add a follow-up ADR with the decisions regarding the architecture surrounding the functioning of the unified model as a consolidated system. This means that ADR 0002 describes what we need for the unified model, while ADR 0003 describes how we're making it work as a system. This and the upcoming ADRs result from team conversations during a Spike phase, which produced several architectural documents. You can find them here: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4901404678/User+Groups For detailed implementation information, see: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/4917657604/Unified+User+Group+Model+Technical+Approach

…h section

mphilbrick211 · 2025-09-16T20:25:13Z

Friendly ping on this, @saraburns1 @bmtcril

openedx-webhooks added open-source-contribution PR author is not from Axim or 2U core contributor PR author is a Core Contributor (who may or may not have write access to this repo). labels Jun 25, 2025

openedx-webhooks added this to Contributions Jun 25, 2025

github-project-automation bot moved this to Needs Triage in Contributions Jun 25, 2025

mariajgrimaldi mentioned this pull request Jun 25, 2025

docs: add arch decision record for runtime architecture components #6

Merged

mariajgrimaldi changed the title ~~docs: add arch decision record for runtime architecture components~~ docs: add ADR for runtime architecture components Jun 25, 2025

mariajgrimaldi force-pushed the MJG/runtime-architecture-for-evaluation-and-extension-adr branch 2 times, most recently from 8ca6e86 to 080c904 Compare June 27, 2025 12:50

mariajgrimaldi requested review from BryanttV, bmtcril and saraburns1 June 27, 2025 13:00

mariajgrimaldi force-pushed the MJG/runtime-architecture-for-evaluation-and-extension-adr branch 2 times, most recently from f8775da to 4abee11 Compare June 27, 2025 16:53

bmtcril reviewed Jul 10, 2025

View reviewed changes

mphilbrick211 moved this from Needs Triage to In Eng Review in Contributions Jul 15, 2025

saraburns1 reviewed Jul 16, 2025

View reviewed changes

mariajgrimaldi added 5 commits July 24, 2025 14:57

fix: reference correctly previous foundational ADR

6a87ab8

docs: add model-based approach in rejected alternative

5c15f22

docs: reorganize decisions for better logical order, add intro to eac…

2b72950

…h section

docs: specify type of format returned but criteria

c29e7a8

mariajgrimaldi force-pushed the MJG/runtime-architecture-for-evaluation-and-extension-adr branch from 4abee11 to c29e7a8 Compare July 24, 2025 13:01

mariajgrimaldi requested review from bmtcril, saraburns1 and sarina July 30, 2025 10:39

sarina added the FC Relates to an Axim Funded Contribution project label Sep 16, 2025

bmtcril marked this pull request as draft September 22, 2025 14:14

mphilbrick211 moved this from In Eng Review to Waiting on Author in Contributions Oct 15, 2025


		To establish a consistent runtime interface for all criterion type templates, we will define a base criterion class that includes:

		* Name: How to identify the criterion type.

docs: add ADR for runtime architecture components #12

Are you sure you want to change the base?

docs: add ADR for runtime architecture components #12

Uh oh!

Conversation

mariajgrimaldi commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

openedx-webhooks commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bmtcril left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mariajgrimaldi Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mphilbrick211 commented Sep 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mariajgrimaldi commented Jun 25, 2025 •

edited

Loading

openedx-webhooks commented Jun 25, 2025 •

edited

Loading

mariajgrimaldi Jul 24, 2025 •

edited

Loading