refactor: [FC-0063] Applying code review suggestions

README.rst

@@ -71,7 +71,7 @@ such xBlocks can render are found during the course converting, they will be
 used. The argument values correspond to the xBlock names to specify in
 `advanced_modules` inside a course advanced settings.
 
-Supported xBlocks :
+Supported Custom xBlocks:
 
 - `pdf <https://github.com/raccoongang/xblock-pdf>`_
 

src/cc2olx/cli.py

@@ -1,4 +1,5 @@
 import argparse
+import logging
 
 from pathlib import Path
 
@@ -8,6 +9,31 @@
 RESULT_TYPE_FOLDER = "folder"
 RESULT_TYPE_ZIP = "zip"
 
+logger = logging.getLogger()
+
+
+class AppendIfAllowedAction(argparse._AppendAction):
+    """
+    Store a list and append only allowed argument values to the list.
+    """
+
+    NOT_ALLOWED_CHOICE_MESSAGE = (
+        "The choice {choice_name!r} is not allowed for {argument_name} argument. It will be ignored during processing."
+    )
+
+    def __init__(self, *args, **kwargs):
+        super().__init__(*args, **kwargs)
+
+        self._choices = self.choices
+        self.choices = None
+
+    def __call__(self, parser, namespace, values, option_string=None):
+        if values in self._choices:
+            super().__call__(parser, namespace, values, option_string)
+        else:
+            argument_name = "/".join(self.option_strings)
+            logger.warning(self.NOT_ALLOWED_CHOICE_MESSAGE.format(choice_name=values, argument_name=argument_name))
+
 
 def parse_args(args=None):
     parser = argparse.ArgumentParser(
@@ -82,7 +108,7 @@ def parse_args(args=None):
     parser.add_argument(
         "-c",
         "--content_types_with_custom_blocks",
-        action="append",
+        action=AppendIfAllowedAction,
         default=[],
         choices=list(SupportedCustomBlockContentType.__members__.values()),
         help="Names of content types for which custom xblocks will be used.",

src/cc2olx/content_parsers/__init__.py

@@ -1,4 +1,4 @@
-from cc2olx.content_parsers.abc import AbstractContentParser
+from cc2olx.content_parsers.abc import AbstractContentParser, AbstractContentTypeWithCustomBlockParser
 from cc2olx.content_parsers.discussion import DiscussionContentParser
 from cc2olx.content_parsers.html import HtmlContentParser
 from cc2olx.content_parsers.lti import LtiContentParser
@@ -8,6 +8,7 @@
 
 __all__ = [
     "AbstractContentParser",
+    "AbstractContentTypeWithCustomBlockParser",
     "DiscussionContentParser",
     "HtmlContentParser",
     "LtiContentParser",

src/cc2olx/content_parsers/abc.py

@@ -3,6 +3,7 @@
 
 from cc2olx.content_parsers.utils import StaticLinkProcessor
 from cc2olx.dataclasses import ContentParserContext
+from cc2olx.enums import SupportedCustomBlockContentType
 from cc2olx.models import Cartridge
 
 
@@ -29,3 +30,23 @@ def _parse_content(self, idref: Optional[str]) -> Optional[Union[list, dict]]:
         """
         Parse content of the resource with the specified identifier.
         """
+
+
+class AbstractContentTypeWithCustomBlockParser(AbstractContentParser, ABC):
+    """
+    Abstract base class for content type with custom block parsing.
+    """
+
+    CUSTOM_BLOCK_CONTENT_TYPE: SupportedCustomBlockContentType
+
+    def _parse_content(self, idref: Optional[str]) -> Optional[Union[list, dict]]:
+        if idref and self._context.is_content_type_with_custom_block_used(self.CUSTOM_BLOCK_CONTENT_TYPE):
+            if resource := self._cartridge.define_resource(idref):
+                return self._parse_resource_content(resource)
+        return None
+
+    @abstractmethod
+    def _parse_resource_content(self, resource: dict) -> Optional[Union[list, dict]]:
+        """
+        Parse resource content.
+        """

src/cc2olx/content_parsers/html.py

@@ -9,7 +9,7 @@
 from cc2olx.constants import LINK_HTML
 from cc2olx.content_parsers import AbstractContentParser
 from cc2olx.content_parsers.mixins import WebLinkParserMixin
-from cc2olx.content_parsers.web_content import WebContent
+from cc2olx.content_parsers.utils import WebContent
 from cc2olx.enums import CommonCartridgeResourceType
 
 logger = logging.getLogger()

src/cc2olx/content_parsers/pdf.py

@@ -1,24 +1,24 @@
 import urllib
 from typing import Dict, Optional
 
-from cc2olx.content_parsers import AbstractContentParser
+from cc2olx.content_parsers import AbstractContentTypeWithCustomBlockParser
 from cc2olx.content_parsers.mixins import WebLinkParserMixin
-from cc2olx.content_parsers.web_content import WebContent
+from cc2olx.content_parsers.utils import WebContent
 from cc2olx.enums import CommonCartridgeResourceType, SupportedCustomBlockContentType
 
 
-class PdfContentParser(WebLinkParserMixin, AbstractContentParser):
+class PdfContentParser(WebLinkParserMixin, AbstractContentTypeWithCustomBlockParser):
     """
     PDF resource content parser.
     """
 
-    def _parse_content(self, idref: Optional[str]) -> Optional[Dict[str, str]]:
-        if idref and self._context.is_content_type_with_custom_block_used(SupportedCustomBlockContentType.PDF):
-            if resource := self._cartridge.define_resource(idref):
-                if resource["type"] == CommonCartridgeResourceType.WEB_CONTENT:
-                    return self._parse_webcontent(resource)
-                elif web_link_content := self._parse_web_link_content(resource):
-                    return self._transform_web_link_content_to_pdf(web_link_content)
+    CUSTOM_BLOCK_CONTENT_TYPE = SupportedCustomBlockContentType.PDF
+
+    def _parse_resource_content(self, resource: dict) -> Optional[Dict[str, str]]:
+        if resource["type"] == CommonCartridgeResourceType.WEB_CONTENT:
+            return self._parse_webcontent(resource)
+        elif web_link_content := self._parse_web_link_content(resource):
+            return self._transform_web_link_content_to_pdf(web_link_content)
         return None
 
     def _parse_webcontent(self, resource: dict) -> Optional[Dict[str, str]]:

src/cc2olx/content_parsers/utils.py

@@ -2,10 +2,12 @@
 import logging
 import re
 import urllib
+from pathlib import Path
 from typing import TypeVar, Optional
 
+from cc2olx.constants import OLX_STATIC_PATH_TEMPLATE, WEB_RESOURCES_DIR_NAME
 from cc2olx.dataclasses import LinkKeywordProcessor
-from cc2olx.models import Cartridge
+from cc2olx.models import Cartridge, ResourceFile
 
 logger = logging.getLogger()
 
@@ -125,3 +127,49 @@ def _process_relative_external_links(self, link: str, html: str) -> str:
 
         url = urllib.parse.urljoin(self._relative_links_source, link)
         return html.replace(link, url)
+
+
+class WebContent:
+    """
+    Represent Common Cartridge web content resource type.
+    """
+
+    def __init__(self, cartridge: Cartridge, resource_file: ResourceFile) -> None:
+        self._resource_relative_path = resource_file.href
+        self._resource_file_path = cartridge.build_resource_file_path(self._resource_relative_path)
+
+    @property
+    def resource_relative_path(self) -> str:
+        """
+        Resource file path inside .imscc file.
+        """
+        return self._resource_relative_path
+
+    @property
+    def resource_file_path(self) -> Path:
+        """
+        Absolute file path of unpacked resource in the filesystem.
+        """
+        return self._resource_file_path
+
+    @property
+    def static_file_path(self) -> str:
+        """
+        File path inside OLX_STATIC_DIR.
+        """
+        if self.is_from_web_resources_dir():
+            return str(self._resource_file_path).split(f"{WEB_RESOURCES_DIR_NAME}/")[1]
+        return self._resource_relative_path
+
+    @property
+    def olx_static_path(self) -> str:
+        """
+        OLX static file path.
+        """
+        return OLX_STATIC_PATH_TEMPLATE.format(static_file_path=self.static_file_path)
+
+    def is_from_web_resources_dir(self) -> bool:
+        """
+        Whether resource file is located in "web_resources" directory.
+        """
+        return WEB_RESOURCES_DIR_NAME in str(self._resource_file_path)

src/cc2olx/main.py

@@ -23,8 +23,7 @@ def convert_one_file(
     relative_links_source=None,
     content_types_with_custom_blocks=None,
 ):
-    if content_types_with_custom_blocks is None:
-        content_types_with_custom_blocks = []
+    content_types_with_custom_blocks = content_types_with_custom_blocks or []
 
     filesystem.create_directory(workspace)
 

tests/test_cli.py

@@ -1,5 +1,6 @@
 from argparse import Namespace
 from pathlib import Path
+from unittest.mock import MagicMock, patch
 
 import pytest
 
@@ -122,12 +123,20 @@ def test_parse_args_with_correct_content_types_with_custom_blocks(imscc_file: Pa
     "content_type_with_custom_block",
     ["word_document", "poll", "survey", "feedback", "image", "audio", "llm"],
 )
+@patch("cc2olx.cli.logger")
 def test_parse_args_with_incorrect_content_types_with_custom_blocks(
+    logger_mock: MagicMock,
     imscc_file: Path,
     content_type_with_custom_block: str,
 ) -> None:
     """
-    Test arguments parser detects incorrect content types with custom blocks.
+    Test arguments parser logs incorrect content types with custom blocks.
     """
-    with pytest.raises(SystemExit):
-        parse_args(["-i", str(imscc_file), "-c", content_type_with_custom_block])
+    expected_log_message = (
+        f"The choice '{content_type_with_custom_block}' is not allowed for -c/--content_types_with_custom_blocks "
+        f"argument. It will be ignored during processing."
+    )
+
+    parse_args(["-i", str(imscc_file), "-c", content_type_with_custom_block])
+
+    logger_mock.warning.assert_called_once_with(expected_log_message)

tests/test_content_parsers/test_abc.py

@@ -0,0 +1,28 @@
+from unittest.mock import Mock, patch
+
+from cc2olx.content_parsers import AbstractContentTypeWithCustomBlockParser
+
+
+@patch("cc2olx.content_parsers.abc.AbstractContentTypeWithCustomBlockParser.__abstractmethods__", frozenset())
+class TestAbstractContentTypeWithCustomBlockParser:
+    parser_type = AbstractContentTypeWithCustomBlockParser
+
+    def test_parse_content_returns_none_if_idref_is_none(self):
+        parser = self.parser_type(Mock(), Mock())
+
+        assert parser._parse_content(None) is None
+
+    def test_parse_content_returns_none_if_content_type_with_custom_block_is_not_used(self):
+        parser = self.parser_type(Mock(), Mock())
+        parser._context = Mock(is_content_type_with_custom_block_used=Mock(return_value=False))
+        parser.CUSTOM_BLOCK_CONTENT_TYPE = Mock()
+
+        assert parser._parse_content(Mock()) is None
+
+    def test_parse_content_returns_none_if_resource_is_not_found(self):
+        parser = self.parser_type(Mock(), Mock())
+        parser._context = Mock(is_content_type_with_custom_block_used=Mock(return_value=True))
+        parser._cartridge = Mock(define_resource=Mock(return_value=None))
+        parser.CUSTOM_BLOCK_CONTENT_TYPE = Mock()
+
+        assert parser._parse_content(Mock()) is None