feat: Source specifying for relative links is allowed #232
Conversation
openedx-webhooks
added
the
open-source-contribution
|
Thanks for the pull request, @myhailo-chernyshov-rg! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently unmaintained. To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:
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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
|
Hi @myhailo-chernyshov-rg! Thanks for this contribution! It looks like you're contributing on behalf of Raccoon Gang - please have your manager reach out to [email protected] to have you added to Raccoon Gang's existing entity agreement with us. Thank you! |
mphilbrick211
added
the
needs test run
src/cc2olx/olx.py
Outdated
| @@ -250,6 +252,18 @@ def process_external_tools_link(item, html): | |||
| html = html.replace(item, external_tool_url) | |||
| return html | |||
|
|
|||
| def process_extra_static_files(item, html): | |||
There was a problem hiding this comment.
Please add a docstring here explaining what this function is for.
| self.cartridge = cartridge | ||
| self.doc = None | ||
| self.link_file = link_file | ||
| self.passport_file = passport_file | ||
| self.relative_links_source = relative_links_source |
There was a problem hiding this comment.
Please add some validation for the format of relative_links_source.
There was a problem hiding this comment.
Validation is added on the argument parsing step.
src/cc2olx/olx.py
Outdated
| if self.relative_links_source is None: | ||
| return html | ||
|
|
||
| for static_file in self.cartridge.extra_static_files: |
There was a problem hiding this comment.
How large can extra_static_files get? This looks like it could get pretty expensive to loop through every entity in this list for each item.
There was a problem hiding this comment.
During the resource content collection, when the external file is found, it's added to this list. So, the list size isn't limited and depends on CC archive content. But we need to process each such link to make it absolute, so we do need to iterate over the list. I don't think it's too expensive because other exporter tasks require much more actions to perform. Do you have any ideas how we can handle all the links without iterating over them?
There was a problem hiding this comment.
I might be misunderstanding this at a basic level, but here is how I interpret this:
- There is a shared list of
extra_static_files. - There are many items.
- This is lookup n^2, i.e. (number of items) * (number of extra static files).
- Each static file maps to the same OLX static path every time.
If that is accurate, then could create a dict that has the mapped values, where the keys are OLX_STATIC_PATH_TEMPLATE.format(static_filename=static_file) and the values are the static_file? That dict could be initialized once and stored on the object, so that the lookup here is just if item in {my lookup dict}, i.e. the whole thing goes from n^2 to n.
The thing is, if extra_static_files is small, this doesn't matter at all. In the past this sort of thing has been a problem when the list grew to thousands.
There was a problem hiding this comment.
openedx/edx-platform#11095 (comment) is an example of something like this that has come up before.
There was a problem hiding this comment.
It's refactored and the bug with making web_resources static links absolute is fixed.
mphilbrick211
removed
the
needs test run
myhailo-chernyshov-rg
force-pushed
the
myhailochernyshov/source-specifying-for-relative-links-is-allowed
branch
from
ff473b9 to
751e8ed
Compare
myhailo-chernyshov-rg
force-pushed
the
myhailochernyshov/source-specifying-for-relative-links-is-allowed
branch
from
9da9855 to
1b67ba1
Compare
Description
It is possible that the
.imscccourse dump contains files with relative links to resources for some reason not included into it (e.g. some static files of LMS that are not processed by its exporting tool). It can become a reason of broken images, not loaded styles etc.To handle such situations, it is allowed to specify the relative links source during converting.Usage instructions
Specify relative links source using
-sflag:Deadline
"None"