Repository navigation
ADR for shared Assets and how course files are referenced/served from openedx_content [FC-0138] - #822
bradenmacdonald wants to merge 36 commits into
Conversation
openedx-webhooks
added
open-source-contribution
|
Thanks for the pull request, @bradenmacdonald! 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. |
6fe58fa to
ee4a005
Compare
bradenmacdonald
changed the title
08aa7b9 to
0bbe03b
Compare
|
For those following this so far, I have made major revisions to everything, primarily renaming it from "Upload" to "File component" and adding a "Consequences" section. |
0ebd33f to
d5a8ae0
Compare
| Naturally, the ``FileComponentMetadata`` table will enforce that ``legacy_path`` is unique per learning package. | ||
|
|
||
| 2. File components can be referenced using new ``oex-asset:`` URL | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
I just pushed this new ADR 14.
Decision 1 is the only part that is relevant to our short-term project of "move static assets into openedx_content and serve them using the contentstore API". The rest is trying to paint a picture of the long-term functionality we'll get when we also migrate courseware itself to openedx_content.
The reason I'm sketching this all out now is so we can decide if we want all this, or if we'll instead go with the dumb, simple approach that does the bare minimum for contentstore compatibility but drops versioning, tracking, linking, etc.
|
OK, I still have some TODOs and things to clean up, but I'd like to get more input before focusing on the fine details. This is ready for review, @ChrisChV @ormsbee @kdmccormick. CC @xitij2000 and @Kelketek on ADR 14 in case you have any thoughts on this after working on the XBlock Asset Picker UI. |
| When copying a Component with references to shared File components to a new Learning Package, each referenced File component is handled as follows: | ||
|
|
||
| - First, if the destination Learning Package already has a File component that is a downstream copy of the same upstream File component (see decision 6), that existing copy is reused, even if its ``component_code`` or content differs. The copy is not updated as a side effect of pasting, since that would change every other component that uses it; if the source uses a newer upstream version, the author can sync the File component as usual. Matching on upstream prevents repeated pastes or imports of the same library content from creating a new copy of the File component each time. | ||
| - Otherwise, if the destination Learning Package already has a File component with identical ``component_code`` and media asset file hash(es), it is reused and no File components need to be copied. Identical bytes don't prove that it's the same asset, but reusing a File component with the same code and content is harmless. |
There was a problem hiding this comment.
I'm a bit worried about this one. If this occurs, the File Component in the destination Learning Package might already be in use by another component. If an author updates the asset, it will be updated in both the existing component and the copied component. However, the author is unaware of this; as far as they are concerned, they are only changing the asset in the existing component.
There was a problem hiding this comment.
True, but we will presumably have something in the UI that shows authors where each File component is being used, so they would see a list of which changes will happen when they update it.
There is actually something like that in the "Files" page already, but it's currently not 100% accurate.
| 11. Image metadata will be in separate models | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| As mentioned in the :class:`Media` docstring, we can use a separate model called ``ImageMedia`` to capture metadata like image dimensions (assuming it can be purely derived from the image byte data itself). We can also use a separate model like ``ImageComponentVersion`` to store *editable* metadata about an image, such as its default alt text and whether or not the image is purely decorative; saving edits of such metadata would create a new version of the File component. |
There was a problem hiding this comment.
Does this mean that a File component that bundles an image with its thumbnail will have separate ImageMedia entries for each thumbnail size and the original asset?
There was a problem hiding this comment.
No, I don't think thumbnails should be considered part of the asset, nor bundled with the image. But if you did upload a pair of images with a regular and "retina" @2x resolution versions, each of those would have its own ImageMedia entry. The ImageMedia is 1:1 with each actual image file that we store (de-duplicated by hash), so it doesn't matter how they are logically organized. (Two different-resolution versions in one component or two separate components each with a different resolution.)
As for how thumbnails are stored, see the draft proposal from @ChrisChV at #837 which is more or less a separate system for storing thumbnails, not storing them as versioned file components.
|
|
||
| This is an appealing alternative, especially when you consider that the number of use cases that require multiple files per File component is likely very small. Videos require multiple files but are rarely if ever stored in Course Files (instead hosted on video-specific services like YouTube or edX.org's video platform). This is rejected primarily to support the use case of HTML "interactive" files that in turn reference image, JavaScript, and/or CSS files. Examples of this can be seen in the `Studio Advanced course <https://github.com/HarvardX/studio-advanced/tree/5cf820c29e3439c9f45cb88e5cf22fd4ab269739/course/static>`_. | ||
|
|
||
| TODO: can we quantify how much this feature would be used? |
There was a problem hiding this comment.
I think images are probably a big user of this and I expect would be very common as assets. I think embracing this would allow uploading the original asset and using a web-friendly resized version in a course and potential a mobile version on mobile.
There was a problem hiding this comment.
Also, for the PDF XBlock I'd worked on a feature to convert a .docx or other file type to pdf. It would be great that could be represented via this mechanism.
There was a problem hiding this comment.
@bradenmacdonald I think we need to identify which assets are authored and which are derived/generated. This is because adding an asset to a File Component means a new version of that File Component will be created. That is correct for authored assets, but creating a new version for derived assets is not ideal. I’ve explored this further in the thumbnails ADR (#837). Thumbnails are derived/generated and disposable; it doesn't make much sense to version-control them.
There was a problem hiding this comment.
Good points guys. If we are auto-converting .docx to .pdf or something, I think that it should be treated differently than if an author uploads both a .docx and a .pdf.
Derived files, like thumbnails and auto-converted docs:
- Are safe to delete
- Can be (re-)created on demand
- Are versioned differently: based on when the original file or the code that produces them changes (e.g. if we upgrade "pdflib" to a new version, we may want to re-create all the .pdf auto-conversions)
I think it makes sense to store such things in completely different models, not re-using the PublishableEntity machinery that we're using for the original file components themselves.
There was a problem hiding this comment.
There is another use-case for images, which is to allow editing while retaining the original. I think the versioning ability would probably be a better fit for that though. i.e. if a the authoring MFE or a plugin allows editing image it could save that as v2 of the asset or as another file in the same component.
Just pointing out a potential use case, not sure what would be the correct approach.
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
thanks for the writeups! this is making the whole problem space much more concrete.
i'm still chewing on all three ADRs. here are just a couple specific concerns on ADR 14 I wanted to capture, but don't feel like you need to address these in depth right now as I'm still not sure about ADR 13
| - First, if the destination Learning Package already has a File component that is a downstream copy of the same upstream File component (see decision 6), that existing copy is reused, even if its ``component_code`` or content differs. The copy is not updated as a side effect of pasting, since that would change every other component that uses it; if the source uses a newer upstream version, the author can sync the File component as usual. Matching on upstream prevents repeated pastes or imports of the same library content from creating a new copy of the File component each time. | ||
| - Otherwise, if the destination Learning Package already has a File component with identical ``component_code`` and media asset file hash(es), it is reused and no File components need to be copied. Identical bytes don't prove that it's the same asset, but reusing a File component with the same code and content is harmless. | ||
| - In both of the above cases, only the main Component needs to be copied, but we still have to create ``FileComponentLink`` and :class:`PublishableEntityVersionDependency` objects to track the relationship, using the original ``component_code`` as the ``alias`` if the reused File component's code differs. | ||
| - Otherwise, the File component is copied into the destination Learning Package, and becomes a downstream of the original if the original is in a library. If the destination Learning Package already has a conflicting File component, with identical ``component_code`` but different media asset file hash(es), the copy is given a different ``component_code``, and the ``FileComponentLink`` created for it will use the ``alias`` column to alias this new code to the old code, so that no changes to the OLX are required. |
There was a problem hiding this comment.
how does the FileComponentLink's alias get factored in when the OLX is moved out of the target LP? notably:
- copy
- export/import
would each of those systems now need to read FileComponentLink in order to make sense of the OLX? is that tradeoff worth the benefit of not rewriting media paths upon OLX paste?
There was a problem hiding this comment.
Export/import would presumably be at the LP level, and would need to include some list of all the FileComponentLinks in the package in any case.
Copy and paste into another LP would have to follow the same algorithm described above for handling references in the new learning package, creating an alias if needed.
|
|
||
| This scheme: | ||
|
|
||
| - **Is easy to detect.** ``oex-asset:…`` does not occur in content for any other reason, so a single pattern finds every reference in any field type (HTML, problem XML, JSON, CSS ``url()``, etc.) with no false positives from platform or theme ``/static/`` URLs. (The only exception is that course content *about* OLX authoring itself would need to escape ``oex-asset:`` if it occurs in text.) |
There was a problem hiding this comment.
I imagine we'll need to support rewriting of /static/, basically forever. So, adding rewrite-detection for oex-asset: would not simplify the system, right?
Beyond that, it seems like all the other listed benefits of oex-asset: also apply to /static.
The net-new thing I see oex-asset: doing is that it provides for both the component-local format (oex-asset:/{path}) and the cross-component format (oex-asset:{component_alias}/{path}).
There was a problem hiding this comment.
I mentioned this in the meeting, but to recap here:
The problem with /static/ is that it can occur in many unrelated contexts, such as a course referencing https://www.python.org/static/img/python-logo.png or https://en.wikipedia.org/static/images/icons/wikipedia.png (although they shouldn't be hotlinking).
So we use this regular expression to try to only match quoted references that begin with /static/, like <img src="/static/..." .... But this doesn't match every use case; e.g. "/static/..." would not be matched, but could be a valid reference to a file.
Because oex-asset: doesn't occur in any other contexts (I'm pretty sure), we can detect it reliably and robustly.
a3edb09 to
9f11319
Compare
|
I've made some major revisions and simplifications. In summary: shared files are now a new entity type, ADR 13: Shared Static Assets (was "Shared Static Assets as File Components")
ADR 14: Referencing Assets
ADR 15: Shadow Components (new)
ADR 5 and ADR 12
|
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
0005 and 0012 both look good to me ✅
I let some comments on 0013 💬
Still reviewing 0014 and 0015... 👀
|
|
||
| For example, an HTML Asset served at ``.../assets/published/page.html`` that references ``image.png`` causes the browser to request ``.../assets/published/image.png``, which is the ``image.png`` Asset's own URL. Filenames containing ``/`` work the same way: ``images/diagram.html`` referencing ``../logo.png`` resolves to ``.../assets/published/logo.png``. No server-side fallback is needed for references between Assets. The URL does not contain the Asset's ``entity_ref``. | ||
|
|
||
| A ``v{num}`` version segment refers to that version of the requested Asset only. Since version numbers of different Assets are unrelated, a relative reference from a historical version (which would resolve to ``v{num}`` of some other Asset) is not meaningful. Historical versions are therefore always served with ``Content-Disposition: attachment``. They can be downloaded (e.g. from a version history UI) but are never rendered as documents. In other words, relative references are unpinned, consistent with :ref:`openedx-content-adr-0014` (decision 7). |
There was a problem hiding this comment.
.../{context}/assets/v{num}/{filename} strikes me as a non-sequitur, for the reasons you point out in this paragraph.
Would this work?
.../{context}/assets/published/{filename}/-- published, relative links work.../{context}/assets/draft/{filename}/-- latest draft, relative links work.../{context}/assets/draft/{filename}/v{num}-- specific draft, relative links don't work
I think this would remove the "gotcha" of "requesting v3 of this asset will have references to v3 of another asset, even if those v3s have nothing to done with another." Instead, the relative references will just 404.
There was a problem hiding this comment.
It would have to be ?v=N or something, because otherwise it's hard to tell if the filename just happens to end in /v4 which seems weird but is allowed. (Or we could disallow it.)
The ?v=N format is also nice because it doesn't break relative references and the relative references will already be dropping the version.
But it's weird that draft is still in the URL if requesting a specific version?
There was a problem hiding this comment.
But it's weird that draft is still in the URL if requesting a specific version?
that's fair. I guess both schemes are a bit weird.
my thinking is that ?v=N is only weird URL-semantics-wise, whereas /vN/ is weird both URL-semtics-wise and behavior-wise (assets are randomly missing or outdated).
There was a problem hiding this comment.
I guess it could make sense that only the "draft" endpoint allows different versions, and the published endpoint always shows only the current version. But yes, the ?v=N has much clearer behavior.
There was a problem hiding this comment.
Changed how the URLs/endpoints work in 7f1c409. Let me know if that makes sense.
| * **Thumbnails** generated by contentstore are dropped and not migrated. Thumbnails will be handled separately, as described in an upcoming ADR. | ||
| * **Video transcripts** stored in contentstore (e.g. ``subs_*.srt.sjson``) become Assets like any other file. Video blocks reference transcripts by name rather than via ``/static/`` URLs, and during this migration they are still in modulestore with no :class:`Component`, so no dependency tracking is set up for transcript Assets at this stage. Dependencies between video components and their transcript Assets can be added when course content itself is migrated to ``openedx_content``. | ||
| * **python_lib.zip** automatically gets ``private=True`` (decision 8), so that it remains inaccessible to learners as it is today. | ||
| * **HTML files** get ``allow_scripts=True`` (decision 10) if the course currently relies on them being rendered with scripts enabled, so that they keep working. |
There was a problem hiding this comment.
I've found another category of files: ones that referenced from CourseBlock settings fields rather than from content. the instances Claude found are:
| Setting | Notes |
|---|---|
course.course_image |
Bare filename, not a path (default "images_course_image.jpg"). Converted to an asset-v1: key at read time by StaticContent.compute_location in openedx/core/lib/courses.py:36. Denormalized into CourseOverview.course_image_url. Confirmed present-but-unused on the Files page of a live course. |
course.banner_image |
Same mechanism, same default. Read via course_image_url(course, 'banner_image'); cached as CourseOverview.banner_image_url. |
course.video_thumbnail_image |
Same mechanism, same default. Cached as CourseOverview.video_thumbnail_image_url. |
course.certificates['certificates'][i]['signatories'][j]['signature_image_path'] |
Full asset reference stored as a string (/asset-v1:… or legacy /c4x/…). Rendered by lms/templates/certificates/_accomplishment-rendering.html:51. The only one of these with a direct contentstore().delete() write path, via certificate_manager._delete_asset() (cms/djangoapps/contentstore/views/certificate_manager.py:39). |
course.pdf_textbooks[i]['url'] and course.pdf_textbooks[i]['chapters'][j]['url'] |
Passed through replace_static_urls by lms/djangoapps/staticbook/views.py:51-62, so either form works: /static/foo.pdf gets rewritten, a raw asset-v1: URL passes through unchanged. Studio's upload modal produces the latter. |
course.html_textbooks[i]['url'] / [...]['chapters'][j]['url'] |
Same shape and same remap_static_url treatment as pdf_textbooks. |
course.instructor_info['instructors'][i]['image'] |
Documented in the field's own docstring (xmodule/course_block.py:1010-1020). Set via Studio Schedule & Details; consumed by openedx/core/djangoapps/programs/utils.py:954 and the course-details REST API. Not traced to a rendering template, so whether it is live in the LMS UI is unconfirmed. |
Because these aren't reachable from outline content, they show up as Unused in the Files page, which seems like a bug. Maybe there's an easy fix for that, or maybe it needs to be outside the scope of the FC. Either way, could you note it here, or say that it's currently undecided and to-be-addressed in a future ADR?
Other than usage-tracking, I don't think these need special handling.
There was a problem hiding this comment.
Good find. I've mentioned them in f3a8764.
| Context | ||
| ------- | ||
|
|
||
| Today, a course's authored "Files" (previously known as "Files & Uploads") live in the legacy MongoDB contentstore, as a semi-flat, course-wide namespace of paths that OLX references as ``/static/{path}``. Semi-flat means that the UI only supports a flat list of files, but by editing course tarballs, it's possible to nest assets within subfolders. The current Course Files system tends to get very disorganized in large courses, as the UI lacks the ability to organize assets into folders, and the reporting of which assets are in use in the course is not reliable. |
There was a problem hiding this comment.
Claude highlighted to me that the CourseBlock.static_asset_path setting exists and theoretically can be used to tell the platform to look on the filesystem rather than in contentstore. I think this is an XMLModuleStore holdover, since using it would actually mean putting course assets under STATIC_ROOT.
I don't think we should worry about that all in these ADRs. I am drafting a DEPR for it now, hopefully we can just rip it out before Willow: openedx/openedx-platform#39236 . (Even if it turns out we need to keep this setting , I don't think it would actually clash at all with your contentstore migration plan)
There was a problem hiding this comment.
Sounds good. Thanks!
| 5. Shadow Components follow the XBlock's lifecycle in modulestore | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Studio code that changes modulestore must keep the Shadow Components consistent with it: |
There was a problem hiding this comment.
Since we're already handling all these cases, how much harder do you think it'd be to make these "real" components, in that they'd also sync title and OLX? Are there other things that'd need to be synced, and/or would it be hard/expensive to serialize a faithful block.xml?
I just feel like this ADR brings us so close to having modulestore Components mirrored over in openedx-core, so if it's only 10% additional effort to go the full way, then maybe that's worth it.
(Totally sensitive to the fact that you have finite budget for the discovery portion of this FC if you'd rather not address this and move on.)
There was a problem hiding this comment.
Good question, but I think there's still a lot more work involved. Modulestore doesn't deal with XML, so there's no block.xml to serialize. We could add the serialization layer, but it's not 100% the same thing.
And Shadow Components don't deal with hierarchy at all (yet), so all the open questions around how to best represent the course outline in openedx_content still apply.
From the list below:
- "Edit" doesn't change any existing APIs - it's only a new API for "files attached to Components" which modulestore doesn't support yet.
- Publish/Discard/Delete/Duplicate are really very minimal actions that hook into these lifecycle events and then mirror the action to the shadow component. In principle this is just a few lines of code for each.
- Import/Export is a moderate lift to get working, and is also something we'd have to do as part of moving modulestore courses to openedx_content, but it's pretty independent from how the XBlock fields/OLX is stored.
|
|
||
| **Authors can attach assets to course components before the content migration is done**, and the content migration later becomes a smaller job, because those assets are already in place under their final keys. |
There was a problem hiding this comment.
| **Authors can attach assets to course components before the content migration is done**, and the content migration later becomes a smaller job, because those assets are already in place under their final keys. |
won't authors only be able to use component-local media after the course has been switched from GridFS over to openedx_content?
There was a problem hiding this comment.
Yes, I was trying to talk about the full migration for all XBlocks from modulstore to openedx_content, not just replacing GridFS.
|
|
||
| **We could implement dependency tracking for all XBlocks, even those currently in modulestore.** We can detect ``/static/...`` references in regular modulestore XBlocks, and then record which shared Assets are in use using :class:`PublishableEntityVersionDependency`, exactly the same way as we can for XBlocks/Components that are fully stored in ``openedx_content``. Whether and when we should do this is left to decide at implementation time; this would require changing decision 4 above, that "XBlocks with no attached assets never get" a shadow component. | ||
|
|
||
| **There are temporarily two sources of truth for a single XBlock**: modulestore for its fields, and ``openedx_content`` for its attached assets. Every Studio code path that creates, copies, publishes or deletes blocks needs to update both, and a missed code path will leave orphaned Shadow Components or XBlocks with missing assets. Orphaned Shadow Components are harmless apart from storage, and could be found and cleaned up by a periodic task that compares them to modulestore. |
There was a problem hiding this comment.
Do you think you'll have to play whack-a-mole with Studio code that creates/modifies/deletes modulestore XBlocks, or do you think the Shadow Components could be managed inside the modulestore API implementations, invisible to Studio?
There was a problem hiding this comment.
I'm hoping the latter; if it's not the case, then we can consider dropping Shadow Components for now. The whole feature is a "nice to have" while we're halfway between the current state and fully removing MongoDB, but it's not necessary just to move off GridFS. (It's not necessary for basic contentstore parity, that is, and it's also not necessary when we fully move off modulestore)
kdmccormick
left a comment
kdmccormick
left a comment
There was a problem hiding this comment.
alright, I've finished this pass. thanks for all you work so far in speccing this out!
| 3. References using ``/static/`` URLs are tracked as dependencies | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| When editing an XBlock/Component such as a Text (HTML) component, course authors can freely copy and paste ``/static/`` references into the HTML/OLX to reference any available shared assets in the course (including ones copied from content libraries). When the author saves their changes, the OLX will be scanned for ``/static/`` references using the improved detection from decision 1, and each reference will be resolved as described in decision 2. As part of saving the new ``ComponentVersion``, the system will create a :class:`PublishableEntityVersionDependency` from the component version to any shared assets used. (References that resolve to the component's own attached files need no dependency.) |
There was a problem hiding this comment.
I think we'll also need to fiddle with PublishableEntityVersionDependencys and placeholder Assets when:
- a Components references
/static/blah.png(a shared Asset), and then the author addsblah.pngas a local Media file, removing its dependency on the Sharedblah.png. - a Components references
/static/blah.png(a local Media file), and then the author removes theblah.pngMedia file, such that the reference now falls back to a sharedblah.pngAsset OR now needs a placeholder Asset
There was a problem hiding this comment.
True, but I think those cases will be handled naturally if our logic is essentially to re-create the dependencies from scratch every time a component is saved.
| Additional rules: | ||
|
|
||
| - No placeholder is created if the referencing component has an attached asset file with that name (decision 2). | ||
| - A placeholder is identified by being an :class:`Asset` with no versions at all. Placeholders are excluded from regular Asset listings, export, and search indexing. |
There was a problem hiding this comment.
if an Asset which is in-use is soft-deleted, what happens? is it replaced with a Placeholder of the same name but no versions? (will the data model let you do that?)
There was a problem hiding this comment.
Good point. In f249db4 I've clarified that a placeholder asset is an asset with no current versions. So if an asset exists and then you soft delete it, the Draft pointer no longer points at any specific version. We can't/shouldn't get rid of the previous versions that existed, but they won't be used.
The difference between a placeholder asset and a deleted asset is now just whether or not it ever existed, and the effect is the same: we can be aware of the broken links and report it on the Files page.
| - Placeholders that no longer have any dependents (e.g. typos that were later fixed) can be hard-deleted, since they never had any content. The ``on_delete=RESTRICT`` on ``referenced_entity`` ensures a placeholder that is still referenced cannot be deleted by mistake. | ||
| - When a component referencing a placeholder is copied to another Learning Package (decision 5), a placeholder is created in the destination too. | ||
|
|
||
| 5. Conflicting shared assets are renamed when copied to another Learning Package |
There was a problem hiding this comment.
this will change based on our WG discussion today, right? If I can try to summarize:
when a component with references to shared Assets is copied from one Package and pasted into another (including "linking", which is just paste + upstream), the pasted Assets will become Component-local, thus avoiding any conflicts with the shared Assets in the target Package. This is still storage-efficient, because the files are effectively de-deuped at the Media layer. but we will need to keep 2 things in mind:
- when the source Package is a library, there should be Dependencies relationships from its internal shared Assets to its Components which use them. that way, published edits to the library's Assets will appear as a dependency changes on its Components, giving course authors the ability to sync. (this is already noted in Decision 3)
- when exporting a course which heavily uses a library, we might wind up wasting disk space with many copies of the same file. I'm not sure if we need to handle this now (
.zipcompression will do a lot for us) but it is worth keeping track of.
| 6. Editors should de-reference full URLs on save | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| In the course of editing courseware, authors may use their browser's "Copy Image URL" to copy the URL of an image and paste it elsewhere, resulting in a full, rewritten URL like ``https://demo.openedx.org/assets/content_libraries/lib:Axim:200/xblock.v1:problem@multi_choice_8/v4/static/images/fig1.png`` ending up in the OLX. If any such URLs are detected, they should be automatically rewritten to the ``/static/`` format (subject to some sanity checks to ensure it will still reference the correct asset). | ||
|
|
||
| Note: Only rewrite full URLs that point to a shared asset in the same learning package, and don't conflict with any of the component's own asset files. |
| 8. Backwards compatibility in OLX export | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| TODO: document format for assets attached to components. |
There was a problem hiding this comment.
do you want to do this in a follow-up so we can merge the decisions that we have so far?
There was a problem hiding this comment.
Yeah, I'll remove 8 and 9 here to keep this ADR focused, and we can decide on the OLX format and python_lib issues later in follow-up.s
|
|
||
| 9. Support for multiple python libraries per course | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| The Problem component (also known as "capa") has support for writing advanced problems that use Python to compute parts of the problem or grade user answers. In particular, it allows authors to provide a library of python functions that will be available for use in any python code used to define a given problem. Currently, this is limited to one library file per course, which must be called ``python_lib.zip``. Once courseware is moved into ``openedx_content``, we should extend the Problem component so that the Python library is a field that can point to any shared asset (e.g. ``/static/alternative_python_lib.zip``), falling back to whichever asset has the filename ``python_lib.zip``. This will provide authors with more flexibility and simplify the process of linking advanced python-based Problems from a library into a course, without breaking backwards compatibility. |
There was a problem hiding this comment.
I think this issue is temporarily alleviated by the local-to-shared fallback mechanism, plus the new decision to include library assets as local rather than shared.
Still would be nice to consider in the future, but I think we could remove it from the scope of the ADR.
| - Publishing one component publishes the shared assets it uses, which changes what learners see in every other component that uses it. That's consistent with how unit children behave, but may be surprising. | ||
| - Deleting a shared asset needs to be handled carefully. Publishing a component publishes its dependencies' unpublished changes, and a soft-delete counts as one. So if an author deletes a still-referenced Asset and then publishes any component that uses it, the deletion goes live and breaks every other published component that uses it. To avoid this, we should warn users when they delete an asset that is still referenced. (Or, don't publish dependency deletions as a side effect.) |
There was a problem hiding this comment.
these are just a potential future gotchas, right? not one that will bite users right after the migration? my thinking:
- for courses, accidental-Asset-publishing would not be an issue, since every change to course assets is auto-published.
- for libraries, we don't yet have UI support for shared assets. when we do build that, hopefully it'll use the existing method of warnings users, a la "by publishing unit U you will also publish components X, Y and asset Z"
There was a problem hiding this comment.
Yeah, it's more a future concern. I just wanted to flag it since it's perhaps an unexpected effect from the interaction of our different publishing rules.
Co-authored-by: Kyle D McCormick <kyle@kylemccormick.me>
|
@kdmccormick Thanks for the review! I have pushed some commits that should address everything you mentioned, plus what we discussed at the last meeting. I've also generally tried to simplify the ADRs and shorten them where possible, leaving some details to implementation or future ADRs. CC @ChrisChV |
7 participants
This is an ADR for representing Course Files in openedx_content.
Builds on #812 and #818 (merged).
View the rendered ADRs here:
Private ref: FAL-4383
AI disclosure: Claude helped me with research, writing, and formatting the RST, but I wrote most of the content myself.