SE-3540 [LX-1653] Fix create_transcripts_xml when resource_fs is WrapFS #267
Conversation
`resource_fs` can be set to a WrapFS instance in some cases, notably in override_export_fs, which is used when exporting content via the olx_rest_api. This bug was introduced in openedx#258, which in turn fixed other bugs. So this commit preserves the behaviour from that PR, while ensuring that the original static_file_dir logic is used when a WrapFS is passed (ie. no _sub_dir attr). An alternate option would be to add AttributeError to an except clause, but it feels safer to be more specific and proactive in this case.
|
Thanks for the pull request, @swalladge! I've created OSPR-5109 to keep track of it in JIRA, where we prioritize reviews. Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
openedx-webhooks
added
needs triage
open-source-contribution
|
👍 🎉
PS: Could you please add tests and update the branch ("This branch is out-of-date with the base branch")? |
|
@natabene this is ready for edX review. @DawoudSheraz may wish to look at this too since it relates to #258 |
|
@swalladge Thank you for your contribution. @DawoudSheraz Would you like to review this? |
| @@ -964,7 +964,7 @@ def create_transcripts_xml(video_id, video_el, resource_fs, static_dir): | |||
| video_id (str): Video id of the video. | |||
| video_el (Element): lxml Element object | |||
| static_dir (str): The Directory to store transcript file. | |||
| resource_fs (SubFS): The file system to store transcripts. | |||
| resource_fs (SubFS|WrapFS): The file system to store transcripts. | |||
There was a problem hiding this comment.
A clarification of how this can be different will be helpful for future devs.
edxval/tests/test_api.py
Outdated
| exported_xml = exported_metadata['xml'] | ||
| self.assert_xml_equal(exported_xml, expected) | ||
|
|
||
| # Verify that no transcript is present in the XML. |
There was a problem hiding this comment.
No need for this as the unit test docstring is already explaining this.
edxval/tests/test_api.py
Outdated
| @unpack | ||
| def test_basic_wrapfs(self, course_id, image): | ||
| """ | ||
| Test that video export works as expected, with wrapfs as sometimes used by edx-platform. |
There was a problem hiding this comment.
nit: move , after wrapfs and mention olx_rest_api after edx-platform to be more specific
edxval/tests/test_api.py
Outdated
| fs = WrapFS(MemoryFS()) | ||
| fs.makedir('course') | ||
| fs.makedir('course/static') # Video XBlock requires this directory to exists, to put srt files etc. |
There was a problem hiding this comment.
These lines can be converted into a helper function, like _setup_wrap_fs() to avoid code duplication.
- explain SubFS vs WrapFS better - helper function to set up a WrapFS for tests (code deduplication)
|
@DawoudSheraz thanks! I addressed your comments. Ready for another pass. 😄 |
|
@swalladge PR approved. If it is not waiting for another approval on your end, I will merge it. Please let me know. Thanks for your contribution. |
|
@DawoudSheraz thanks! We have completed our internal review process (@gabor-boros approved above), so will be great to get this merged! 😄 |
|
@swalladge 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
|
PR has been merged and a release tag https://github.com/edx/edx-val/releases/tag/1.4.4 has been created. |
|
Thanks @DawoudSheraz ! Could you let us know when this has been deployed to edX staging and production please? 😄 |
|
@swalladge Hi. This has reached production after the requirements update in https://github.com/edx/edx-platform/pull/25554. Thank You |
|
@DawoudSheraz thanks, that's good to know. 👍 |
resource_fscan be set to a WrapFS instance in some cases,notably in override_export_fs, which is used when exporting content via
the olx_rest_api.
This bug was introduced in #258,
which in turn fixed other bugs.
So this commit preserves the behaviour from that PR,
while ensuring that the original static_file_dir logic is used when
a WrapFS is passed (ie. no _sub_dir attr).
An alternate option would be to add AttributeError to an except clause,
but it feels safer to be more specific and proactive in this case.
JIRA tickets: OSPR-5109
Merge deadline: ASAP; this is breaking exporting videos, which is used by our clients.
Test instructions:
Reviewers: