Repository navigation
feat(pathways): implement CatalogPathway - #819
Agrendalath wants to merge 8 commits into
Conversation
|
Thanks for the pull request, @Agrendalath! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
643644c to
fa1d75b
Compare
bradenmacdonald
left a comment
There was a problem hiding this comment.
Looks good! I read the code but did not test it.
We should probably wait until #818 is finalized and merged before merging this, unless that's blocking you.
| name = case_insensitive_char_field( | ||
| max_length=255, | ||
| blank=False, | ||
| help_text=_('The learner-facing name of this category, e.g. "Master\'s Degree". Operators may change this.'), | ||
| ) |
There was a problem hiding this comment.
Do you think this will need to be translatable in the future?
Individual courses/pathways usually have titles and content in one specific language, but I can imagine that Pathway Categories like "Master's Degree" may contain pathways of various languages and operators may want to be able to translate the name like "Master's Degree" into the user's language.
There was a problem hiding this comment.
@bradenmacdonald, yes. We could use something simple, like:
class PathwayCategoryTranslation(models.Model):
pathway_category = models.ForeignKey(PathwayCategory, on_delete=models.CASCADE)
language_code = models.CharField(max_length=10)
name = models.CharField(max_length=255)Then, we could add an optional language_code kwarg to get_catalog_pathway.
Should we add this now?
There was a problem hiding this comment.
@Agrendalath Not sure. That approach makes sense. My main question is if there's any other existing pattern we can use, as I haven't seen this exact solution before within the platform.
If you don't want to add it now, we could at least plan for it by requiring the "get pathway category" API to require a language or request parameter, so that translating the name will be possible in the future.
There was a problem hiding this comment.
@bradenmacdonald, I haven't found any existing solutions for this within Open edX.
I pushed c0047cd, which implements the translations. It uses django.utils.translation.get_language, which should technically retrieve it from the request, but given the number of translation layers in Open edX, we may end up using PathwayCategory.get_localized_name directly in the REST APIs anyway. In any case, I wanted to keep both options available.
|
Update: I added an API to list all Pathways in 4e19584. There is no such API for CourseRuns yet, but if we want to move forward with the Pathways dashboard listing work, we will need a way to list them. |
9af7940 to
c0047cd
Compare
|
Hi @Agrendalath: Was an LLM used in the creation of this PR? If so, please make of note of which LLM was used. Thank you. |
| def get_queryset(self, request) -> QuerySet[PathwayCategoryWithPathwayCount]: | ||
| """Add the 'pathway_count' to the list_display queryset""" | ||
| qs = super().get_queryset(request) | ||
| qs = qs.annotate(pathway_count=Count("pathways")) |
There was a problem hiding this comment.
| qs = qs.annotate(pathway_count=Count("pathways")) | |
| qs = qs.annotate(pathway_count=Count("pathways", distinct=True)) |
This gets inflated when the changelist search joins translations (search_fields includes translations__name). Django 5.2 dedupes search results with .distinct(), which doesn't fix the aggregate.
This only happens if you do a search for category type, which admittedly is a rare operation, but still.
Repro: a category with 2 pathways and 2 translations whose names match ?q=Pathway shows 4.
| PathwayCategory, | ||
| on_delete=models.PROTECT, | ||
| null=False, | ||
| default=get_default_pathway_category_id, |
There was a problem hiding this comment.
A couple of questions:
- Do we need an explicit default? It's already a required field, so can't we just make the creator pick one?
- If we do need a default, please see the note below...
Django evaluates callable field defaults in Model.__init__, so every CatalogPathway() runs a SELECT (and an INSERT if the row is missing), including rendering the admin add form. It also means a future data migration that instantiates the historical model via apps.get_model would call the live PathwayCategory model.
Could this be resolved at save time instead, next to the title fallback?
def clean(self) -> None:
if not self.title:
self.title = self.pathway_code
if self.category_id is None:
self.category_id = get_default_pathway_category_id()Then create_catalog_pathway can pass category=category unconditionally. If you want the admin add form to still preselect the default, CatalogPathwayAdmin.get_changeform_initial_data can return {"category": get_default_pathway_category_id()}.
There was a problem hiding this comment.
+1 to having no explicit default directly on the model.
Why do we even need a default set within clean()? We have the api.py layer for pathways, in which a default can be calculated and used, so there is no need to add moving parts to models.py, right?
There was a problem hiding this comment.
I moved this to api_impl.py. I still wanted to keep the default selection in the Django admin to give authors a more intuitive out-of-the-box experience if they don't need to customize this name.
| pathway_id = catalog_pathway.id if isinstance(catalog_pathway, CatalogPathway) else catalog_pathway | ||
| with transaction.atomic(): | ||
| # Lock the row so a concurrent unenroll can't slip in between reading `is_active` and writing it back. | ||
| enrollment, created = PathwayEnrollment.objects.select_for_update().get_or_create( |
There was a problem hiding this comment.
I don't understand the value of having a select_for_update() call here, and it seems like it would be bad for performance purposes. Also, the atomic() here wouldn't actually do anything, right? Since get_or_create is doing at most one insert?
Claude flagged the following as well: a nonexistent pathway ID surfaces here as an IntegrityError on the INSERT rather than CatalogPathway.DoesNotExist like the other functions. Either resolve the ID with a .get() first or say so in the docstring.
There was a problem hiding this comment.
Good point. I changed it to get.
| category, _created = PathwayCategory.objects.get_or_create( | ||
| category_code=DEFAULT_PATHWAY_CATEGORY_CODE, | ||
| defaults={"name": DEFAULT_PATHWAY_CATEGORY_NAME}, | ||
| ) |
There was a problem hiding this comment.
Why would we ever need to create it? Isn't it always bootstrapped as part of the migration? Is the idea that we might delete the default, and want to auto-generate a new one?
| return format_html('<a href="{}">{}</a>', url, obj.pathway_count) | ||
|
|
||
|
|
||
| admin.site.register(PathwayCategory, PathwayCategoryAdmin) |
There was a problem hiding this comment.
Nit: I think the decorator syntax for this is nicer, but that's just a quibble.
| PathwayCategoryTranslationID = NewType("PathwayCategoryTranslationID", int) | ||
| type ID = PathwayCategoryTranslationID | ||
|
|
||
| class IDField(TypedBigAutoField[ID]): # Boilerplate for fully-typed ID field. |
There was a problem hiding this comment.
We don't need a BigAutoField for what amounts to a category type that will have a small handful of entries.
| ] | ||
|
|
||
|
|
||
| class PathwayCategoryTranslation(models.Model): |
There was a problem hiding this comment.
In the future, please make something as big as translations into a separate PR. It's not as critical as the rest of this, and it could have been added/reviewed later.
| See the openedx_learning ADR 0007. | ||
| """ | ||
| category, _created = PathwayCategory.objects.get_or_create( | ||
| category_code=DEFAULT_PATHWAY_CATEGORY_CODE, |
There was a problem hiding this comment.
Wait, doesn't this mean that if I change my default (e.g. my institution calls them "programs"), this one auto-creates a "pathways" one again, forcing "pathways" to always exist?
There was a problem hiding this comment.
No - Django admin allows changing only the title - the part visible to the learners. The category_code will stay the same. The main reason for get_default_pathway_category() is to provide an intuitive out-of-the-box experience for authors who don't need different category names. I updated this to raise PathwayCategory.DoesNotExist instead of recreating the category.
| ) | ||
| catalog_pathway = models.ForeignKey( | ||
| CatalogPathway, | ||
| on_delete=models.CASCADE, |
There was a problem hiding this comment.
This is not a blocker to merging, but I'm following up on the question of whether we really want cascade behavior here. I worry that someone will accidentally delete the wrong thing and permanently wipe out all its enrollments.
There was a problem hiding this comment.
I forgot to update this one - I changed it to PROTECT now.
| As with `CatalogCourse.key_str`, this may become based on an editable `SlugField` or an opaque key in the | ||
| future, so don't assume it never changes. | ||
| """ | ||
| return f"catalog-pathway:{self.org_code}:{self.pathway_code}" |
There was a problem hiding this comment.
Question: Can you remind me again, is the intent for this to eventually be a key in the OpaqueKey sense?
FYI @kdmccormick
There was a problem hiding this comment.
This here follows the pattern we set for catalog-course::
Basically, we couldn't decide if it should be an opaque key or not, so we decided to make it a string which could become an opaque key.
Having stewed on this since then, I think we should just catalog-pathway: an opaque key, and also go back and do the same for catalog-course:. And I think we should do this going forward whenever we invent a new key type. Yes, the implementation of OpaqueKeys is overly complex, but it's so much better than having serialization/deserialization logic and untyped ad-hoc key variants flying around all over the place.
There was a problem hiding this comment.
@kdmccormick, do we have an ADR showing how this would work in practice? Do we want to store these as OpaqueKeyField subclasses in the DB, use opaque keys for (de)serialization within the Python API, or something else?
There was a problem hiding this comment.
it's a good question. I think we'll want OpaqueKeyField subclasses eventually, but I forget how hard those are to create, and we don't need them now. On these models, we can just build the opaque key instances on-the-fly in a .key property.
As to not hold up this PR, I made a followup ticket for it: openedx/opaque-keys#478
do you mind adding that link to the docstring here so we remember to come back to it?
| 'by a hyphen and a country/locale code, e.g. "fr", "pt-br", "zh-cn".' | ||
| ), | ||
| ) | ||
| title = case_insensitive_char_field( |
There was a problem hiding this comment.
Hi @Agrendalath!
I'm not familiar with our translation layer, so how hard would it be to include the title plural here?
We use it while grouping pathways in the frontend
There was a problem hiding this comment.
I appreciate you cc-ing me in on these @rpenido. Let me know if we need to find a middle ground for MVP if this is a challenge.
There was a problem hiding this comment.
I left a comment related to this on the decision log: https://openedx.atlassian.net/wiki/spaces/OEPM/pages/6630834191/Willow+Pathways+Decision+Log?focusedCommentId=6831538177
tl;dr-- I recommend leaving category translations out of Willow, and having a simple single-language title and title_plural for now.
But we should defer to Cassie & Chelsea's decision on that. Regardless, would you be able to remove category translations from this PR @Agrendalath ? Even if we decide to keep translations in Willow, I think that should be reviewed on its own.
There was a problem hiding this comment.
@kdmccormick, sure thing. Please let me know if you're still reviewing this - I'd like to address all comments at once to avoid partial review rounds (as tracking them is confusing from both ends).
There was a problem hiding this comment.
thanks! sorry for the sporadic comments. i'm still reviewing, i'll let you know once i've finished my review pass.
There was a problem hiding this comment.
I replaced translations with title_plural.
There was a problem hiding this comment.
thanks for your patience @Agrendalath -- I'm done with this pass.
| def get_default_pathway_category() -> PathwayCategory: | ||
| """ | ||
| Get the default `PathwayCategory`: the one `create_catalog_pathway` uses when it isn't given a category. | ||
|
|
||
| Rather than falling back to the word "Pathway" in code, the initial migration ships a database row with that title, | ||
| so that operators can rename it or add categories of their own without a code change (see the openedx_learning | ||
| ADR 0007). Renaming it keeps it the default, because only its ``title`` changes; its ``category_code`` can't. | ||
|
|
||
| Nothing ever creates this category except that migration. If an operator has deleted it, this raises | ||
| `PathwayCategory.DoesNotExist`, and catalog pathways have to be given a category explicitly. | ||
| """ | ||
| try: | ||
| return PathwayCategory.objects.get(category_code=DEFAULT_PATHWAY_CATEGORY_CODE) | ||
| except PathwayCategory.DoesNotExist as exc: | ||
| raise PathwayCategory.DoesNotExist( | ||
| f'The default pathway category ("{DEFAULT_PATHWAY_CATEGORY_CODE}") has been deleted, so catalog pathways ' | ||
| "must be given a category explicitly." | ||
| ) from exc |
There was a problem hiding this comment.
I think that the DEFAULT_PATHWAY_CATEGORY_* settings are conflating two different cases, and I don't think we need to handle either of them.
Firstly, DEFAULT_PATHWAY_CATEGORY_ is the default PathwayCategory that gets provisioned by the initial migration. In my experience, migrations that rely on configuration are tricky. As soon as someone configures their site away from the default, such migrations become non-idempotent, and that stresses me out. Has anyone asked for this initial row to be configurable? If not, then let's not make it configurable--just hard-code ("pathway", "Pathway") as the initial code/title for pathways.
Secondly, DEFAULT_PATHWAY_CATEGORY_ is the default PathwayCategory when none is specified. Why do we need this? A pathway category is required to create a pathway--the decision log says so. The Python API should never need to handle the case when pathway_category=None, and trying to handle it just adds a bunch of unnecessary fallback code.
Am I missing anything, or can we just drop these two settings entirely?
There was a problem hiding this comment.
It's not a configuration - it's just a constant created as a helper. I can see why someone might try moving it to settings in the future, though. How about we remove DEFAULT_PATHWAY_CATEGORY_TITLE and make "Pathway" a literal in the migration, while keeping DEFAULT_PATHWAY_CATEGORY_CODE intact (and maybe add a scary warning above it to leave this value alone), assuming we don't want to change the behavior I described below?
Secondly, DEFAULT_PATHWAY_CATEGORY_ is the default PathwayCategory when none is specified. Why do we need this? A pathway category is required to create a pathway--the decision log says so. The Python API should never need to handle the case when pathway_category=None, and trying to handle it just adds a bunch of unnecessary fallback code.
I thought that we want to make the default experience as author-friendly as possible, and not force them to enter the category each time if they want to use only a single category on their instance (i.e., whether they stick to the "Pathway", rename to "Class", or anything else - as long as they use only one name). When one is new to the platform (or simply this feature), they may wonder what to call this Item grouping - which is confusing. They set some random code, then (a month later) go through each pathway and move them all to the new category non-atomically because they're not happy with the previous code.
I was a bit conflicted about whether to use "pathway" as a DEFAULT_PATHWAY_CATEGORY_CODE (defaulting to a custom name seems like a gimmick). As an alternative, I considered using "default" as the default code for pathways. However, this may be confusing for analytics that query the DB and need to look for the default category code. On the other hand, would anybody care about actually specifying this code if they use only one category?
TL;DR: I just wanted to make as few required fields as possible, so we don't end up with another course-discovery or ecommerce that had frontend-app-publisher created partially to set all required values. I'm also okay with removing the defaults altogether (maybe I'm just overthinking this UX). What do you think?
There was a problem hiding this comment.
It's not a configuration - it's just a constant created as a helper. I can see why someone might try moving it to settings in the future, though. How about we remove DEFAULT_PATHWAY_CATEGORY_TITLE and make "Pathway" a literal in the migration,
ah, I see, my mistake! but yes, moving it to the migration file, either as a hard-coded string or as a private _-prefixed constant, would make it less likely for someone to come along in the future and try to make this configurable.
I thought that we want to make the default experience as author-friendly as possible, and not force them to enter the category each time if they want to use only a single category on their instance
...
TL;DR: I just wanted to make as few required fields as possible, so we don't end up with another course-discovery or ecommerce that had frontend-app-publisher created partially to set all required values.
from a UX perspective, I totally agree on both these counts! I just don't think this needs to be managed by the backend APIs. To break it down, I'm envisioning various possible future Studio pathways creation UIs...
-
Authors have various buttons to click:
Create Degree,Create Bootcamp, etc., and clicking them brings them into a creation UI for a pathway of the selected category.- In this case, there's no need for a default.
-
There is a single pathway creation UI, with a drop-down selector for
Choose a Category: [Degree | Bootcamp | etc.]. That drop-down selector could work in various ways:- A: It has no default. Authors must pick from the drop down or they get a validation error ("Please specify a category")
- B: It defaults to a category on some platform-defined heuristic. e.g., first-created category (
pathway), most-recently-created category, most-recently-modified category, most-used category, most-recently-used category, etc. - C: We have a new database table or django setting which stores the default category, and the drop-down defaults to that.
- D: The author can leave the drop-down blank, and then the backend decides on a default category to use.
Of all those possibilities, only 2D would actually use the defaulting-behavior that this PR is building into the backend APIs. And in the cases of 2B or 2C, those effective defaults would duplicate or, even worse, actually conflict with the backend API's defaults. (I am particularly sensitive to overlapping/conflicting defaults lately as I have been fighting to simplify openedx-platform's configuration, and this has been one of the hardest parts :P )
I'm also okay with removing the defaults altogether (maybe I'm just overthinking this UX). What do you think?
I would prefer they be removed. FWIW I don't think you're overthinking the UX, but I think you might be preemptively building a UX into the data model before we know what it looks like. (when it comes time to build the authoring UX, I would love to return to your mindset of make-the-happy-path-as-easy-as-posssible!)
There was a problem hiding this comment.
@kdmccormick, done in fabcf57.
Since the categories became more important without a default one, I added an API for them in 3e12f5f.
After adding this API, I noticed that api_impl.py became harder to read, so I split CatalogCourse and CatalogPathway in d28832d.
| id = IDField( | ||
| primary_key=True, | ||
| verbose_name=_("Primary Key"), | ||
| help_text=_("The internal database ID for this catalog pathway. Should not be exposed to users nor in APIs."), |
There was a problem hiding this comment.
| help_text=_("The internal database ID for this catalog pathway. Should not be exposed to users nor in APIs."), | |
| help_text=_("The internal database ID for this catalog pathway."), |
this is true of all PKs, seems weird to note it here.
There was a problem hiding this comment.
I just copy-pasted this from the CatalogCourse model. Should I remove it?
There was a problem hiding this comment.
your call, not a blocker for me
Agrendalath
left a comment
There was a problem hiding this comment.
@kdmccormick, I addressed your comments.
| 'by a hyphen and a country/locale code, e.g. "fr", "pt-br", "zh-cn".' | ||
| ), | ||
| ) | ||
| title = case_insensitive_char_field( |
There was a problem hiding this comment.
I replaced translations with title_plural.
| id = IDField( | ||
| primary_key=True, | ||
| verbose_name=_("Primary Key"), | ||
| help_text=_("The internal database ID for this catalog pathway. Should not be exposed to users nor in APIs."), |
There was a problem hiding this comment.
I just copy-pasted this from the CatalogCourse model. Should I remove it?
| def get_default_pathway_category() -> PathwayCategory: | ||
| """ | ||
| Get the default `PathwayCategory`: the one `create_catalog_pathway` uses when it isn't given a category. | ||
|
|
||
| Rather than falling back to the word "Pathway" in code, the initial migration ships a database row with that title, | ||
| so that operators can rename it or add categories of their own without a code change (see the openedx_learning | ||
| ADR 0007). Renaming it keeps it the default, because only its ``title`` changes; its ``category_code`` can't. | ||
|
|
||
| Nothing ever creates this category except that migration. If an operator has deleted it, this raises | ||
| `PathwayCategory.DoesNotExist`, and catalog pathways have to be given a category explicitly. | ||
| """ | ||
| try: | ||
| return PathwayCategory.objects.get(category_code=DEFAULT_PATHWAY_CATEGORY_CODE) | ||
| except PathwayCategory.DoesNotExist as exc: | ||
| raise PathwayCategory.DoesNotExist( | ||
| f'The default pathway category ("{DEFAULT_PATHWAY_CATEGORY_CODE}") has been deleted, so catalog pathways ' | ||
| "must be given a category explicitly." | ||
| ) from exc |
There was a problem hiding this comment.
It's not a configuration - it's just a constant created as a helper. I can see why someone might try moving it to settings in the future, though. How about we remove DEFAULT_PATHWAY_CATEGORY_TITLE and make "Pathway" a literal in the migration, while keeping DEFAULT_PATHWAY_CATEGORY_CODE intact (and maybe add a scary warning above it to leave this value alone), assuming we don't want to change the behavior I described below?
Secondly, DEFAULT_PATHWAY_CATEGORY_ is the default PathwayCategory when none is specified. Why do we need this? A pathway category is required to create a pathway--the decision log says so. The Python API should never need to handle the case when pathway_category=None, and trying to handle it just adds a bunch of unnecessary fallback code.
I thought that we want to make the default experience as author-friendly as possible, and not force them to enter the category each time if they want to use only a single category on their instance (i.e., whether they stick to the "Pathway", rename to "Class", or anything else - as long as they use only one name). When one is new to the platform (or simply this feature), they may wonder what to call this Item grouping - which is confusing. They set some random code, then (a month later) go through each pathway and move them all to the new category non-atomically because they're not happy with the previous code.
I was a bit conflicted about whether to use "pathway" as a DEFAULT_PATHWAY_CATEGORY_CODE (defaulting to a custom name seems like a gimmick). As an alternative, I considered using "default" as the default code for pathways. However, this may be confusing for analytics that query the DB and need to look for the default category code. On the other hand, would anybody care about actually specifying this code if they use only one category?
TL;DR: I just wanted to make as few required fields as possible, so we don't end up with another course-discovery or ecommerce that had frontend-app-publisher created partially to set all required values. I'm also okay with removing the defaults altogether (maybe I'm just overthinking this UX). What do you think?
|
BTW, please don't block on me, and feel free to merge whenever @kdmccormick is good with it. Thank you. |
This introduces:
TheCatalogPathway - Pathwayrelations are based on #818, so it makes sense to merge that PR first.I reverted the direction change, as decided in #818 (comment).
I used Claude for proofreading and the initial reviews before opening the PR.