Closed ormsbee closed 7 months ago
Okay, I think I've gotten all the basic changes in there. Now to fix old tests and add new ones for the new functionality. Then a final re-base and version bump.
Old tests are fixed, added a few new ones, still some more to go. I'll deal with mypy last.
@kdmccormick, @bradenmacdonald: If you have some time, please give this a review pass. The tests aren't fully there yet, but I'd appreciate your thoughts on the API additions especially.
Okay, I wrote the tests, made changes based on the review comments, and the linters are appeased. I'm going to do the _pk
refactoring now, but with any luck I'm hoping this can merge today.
Okay, slight delay because I noticed that it's always doing the work of loading all components no matter the pagination params in the REST API. I think this is a problem on the edx-platform side, but I'm looking into it to be sure.
Okay, I wrote the tests, made changes based on the review comments, and the linters are appeased. I'm going to do the _pk refactoring now, but with any luck I'm hoping this can merge today.
I went back and forth on _id
vs. _pk
for a bit and then decided to punt entirely by making these arguments positional-only for the moment. >_<
Also, I tracked down the query issue to something in edx-platform, and I'll have an update for that PR with the fix later. Part of it points to an issue that our internal APIs have with QuerySets and pagination. Namely, the pattern for DRF and Django in general is QuerySet -> Pagination -> Serialization. So it gets awkward when we're returning something from an API that we want to be QuerySet-like in that it's filterable and pageable, but also presents something that is not actually a model. I hacked up something before that wrapped the __iter()__
call and did serialization that seemed to work, but I feel like that's probably too much hackery to want to support in the longer term.
Anyhow, I'll discuss it more on the other PR.
It occurred to me while looking at some recent comments in the wiki w.r.t. library content needs that we probably want to have ComponentType
represented as its own model. Partly for saving space, but mostly because it gives us something to later hang policy off of on a per-type basis, e.g. "videos are okay to show in libraries, SGA blocks are experimental for library use".
I implemented that this evening. It was pleasantly straightforward, and it also gets rid of our having to tell pylint to ignore our use of type
as a variable name. Tests already pass, so hopefully I can just do it as a fast-follow to this PR tomorrow if this merges.
BTW, the only reason this build isn't green is because I was a doofus and mis-spelled a couple of conventional commit messages. Please know that it's ready for final review, and I'll squash it at the end.
Question that's not directly related: Can you use add_content_to_component_version
to modify a ComponentVersion
that has already been published?
Thanks folks! I squashed my commits and made a decent commit message out of it. Will merge once things are green.
Question that's not directly related: Can you use
add_content_to_component_version
to modify aComponentVersion
that has already been published?
Yes, you can. 95% of the time you shouldn't, but some real use cases came up where doing so seems like it would be beneficial:
Work to support https://github.com/openedx/edx-platform/pull/34066
This bumps the version for openedx-learning to 0.5.0.
Some highlights:
Component
.Squashed commit message: