Improved code based on review feedback.

- Added docstringsu
- Improved naming.
- Removed redundant arguments.
- Split large method into two smaller.

org_fedora_oscap/gui/spokes/oscap.py

@@ -377,7 +377,7 @@ def _fetch_data_and_initialize(self):
         if self._addon_data.content_url and self._addon_data.content_type != "scap-security-guide":
             self.model.CONTENT_DOWNLOAD_LOCATION = pathlib.Path(common.INSTALLATION_CONTENT_DIR)
             self.model.content_uri = self._addon_data.content_url
-            thread_name = self.model.fetch_content(self._addon_data.certificates, self._handle_error)
+            thread_name = self.model.fetch_content(self._handle_error, self._addon_data.certificates)
 
         # pylint: disable-msg=E1101
         hubQ.send_message(self.__class__.__name__,
@@ -399,13 +399,18 @@ def _init_after_data_fetch(self, wait_for):
         :type wait_for: str or None
 
         """
+        def update_progress_label(msg):
+            fire_gtk_action(self._progress_label.set_text(msg))
+
         content_path = None
         actually_fetched_content = wait_for is not None
 
         if actually_fetched_content:
             content_path = self._addon_data.raw_preinst_content_path
 
-        content = self.model.finish_content_fetch(wait_for, self._addon_data.fingerprint, lambda msg: log.info(msg), content_path, self._end_fetching, self._handle_error)
+        content = self.model.finish_content_fetch(
+            wait_for, self._addon_data.fingerprint, update_progress_label,
+            content_path, self._handle_error)
         if not content:
             with self._fetch_flag_lock:
                 self._fetching = False

org_fedora_oscap/ks/oscap.py

@@ -451,13 +451,14 @@ def setup(self, storage, ksdata, payload):
         if not os.path.exists(self.preinst_content_path) and not os.path.exists(self.raw_preinst_content_path):
             # content not available/fetched yet
             self.model.content_uri = self.content_url
-            thread_name = self.model.fetch_content(self.certificates, self._handle_error)
+            thread_name = self.model.fetch_content(self._handle_error, self.certificates)
 
         content_dest = None
         if self.content_type != "scap-security-guide":
             content_dest = self.raw_preinst_content_path
 
-        content = self.model.finish_content_fetch(thread_name, self.fingerprint, lambda msg: log.info(msg), content_dest, lambda content: content, self._handle_error)
+        content = self.model.finish_content_fetch(
+            thread_name, self.fingerprint, lambda msg: log.info(msg), content_dest, self._handle_error)
 
         if not content:
             return

org_fedora_oscap/model.py

@@ -12,6 +12,8 @@
 from org_fedora_oscap import common
 from org_fedora_oscap import content_handling
 
+from org_fedora_oscap.common import _
+
 log = logging.getLogger("anaconda")
 
 
@@ -23,6 +25,7 @@ def is_network(scheme):
 
 class Model:
     CONTENT_DOWNLOAD_LOCATION = pathlib.Path(common.INSTALLATION_CONTENT_DIR) / "content-download"
+    DEFAULT_CONTENT = f"{common.SSG_DIR}/{common.SSG_CONTENT}"
 
     def __init__(self, policy_data):
         self.content_uri_scheme = ""
@@ -52,32 +55,43 @@ def content_uri(self, uri):
         self.content_uri_path = path
         self.content_uri_scheme = scheme
 
-    def fetch_content(self, cert, what_if_fail):
+    def fetch_content(self, what_if_fail, cert=""):
+        """
+        Initiate fetch of the content into an appropriate directory
+
+        Args:
+            what_if_fail: Callback accepting exception as an argument that
+                should handle them in the calling layer.
+            cert: HTTPS certificates
+        """
         shutil.rmtree(self.CONTENT_DOWNLOAD_LOCATION, ignore_errors=True)
         self.CONTENT_DOWNLOAD_LOCATION.mkdir(parents=True, exist_ok=True)
-        return self.fetch_files(self.content_uri_scheme, self.content_uri_path, self.CONTENT_DOWNLOAD_LOCATION, cert, what_if_fail)
+        fetching_thread_name = self._fetch_files(
+            self.content_uri_scheme, self.content_uri_path,
+            self.CONTENT_DOWNLOAD_LOCATION, cert, what_if_fail)
+        return fetching_thread_name
 
-    def fetch_files(self, scheme, path, destdir, cert, what_if_fail):
+    def _fetch_files(self, scheme, path, destdir, cert, what_if_fail):
         with self.activity_lock:
             if self.now_fetching_or_processing:
                 msg = "Strange, it seems that we are already fetching something."
                 log.warn(msg)
                 return
             self.now_fetching_or_processing = True
 
-        thread_name = None
+        fetching_thread_name = None
         try:
-            thread_name = self._start_actual_fetch(scheme, path, destdir, cert)
+            fetching_thread_name = self._start_actual_fetch(scheme, path, destdir, cert)
         except Exception as exc:
             with self.activity_lock:
                 self.now_fetching_or_processing = False
             what_if_fail(exc)
 
         # We are not finished yet with the fetch
-        return thread_name
+        return fetching_thread_name
 
     def _start_actual_fetch(self, scheme, path, destdir, cert):
-        thread_name = None
+        fetching_thread_name = None
         url = scheme + "://" + path
 
         if "/" not in path:
@@ -91,35 +105,48 @@ def _start_actual_fetch(self, scheme, path, destdir, cert):
         dest = destdir / basename
 
         if is_network(scheme):
-            thread_name = data_fetch.wait_and_fetch_net_data(
+            fetching_thread_name = data_fetch.wait_and_fetch_net_data(
                 url,
                 dest,
                 cert
             )
         else:  # invalid schemes are handled down the road
-            thread_name = data_fetch.fetch_local_data(
+            fetching_thread_name = data_fetch.fetch_local_data(
                 url,
                 dest,
             )
-        return thread_name
+        return fetching_thread_name
 
-    def finish_content_fetch(self, thread_name, fingerprint, report_callback, dest_filename, after_fetch, what_if_fail):
+    def finish_content_fetch(self, fetching_thread_name, fingerprint, report_callback, dest_filename,
+                             what_if_fail):
         """
+        Finish any ongoing fetch and analyze what has been fetched.
+
+        After the fetch is completed, it analyzes verifies fetched content if applicable,
+        analyzes it and compiles into an instance of ObtainedContent.
+
         Args:
-            what_if_fail: Callback accepting exception.
-            after_fetch: Callback accepting the content class.
+            fetching_thread_name: Name of the fetching thread
+                or None if we are only after the analysis
+            fingerprint: A checksum for downloaded file verification
+            report_callback: Means for the method to send user-relevant messages outside
+            dest_filename: The target of the fetch operation. Can be falsy -
+                in this case there is no content filename defined
+            what_if_fail: Callback accepting exception as an argument
+                that should handle them in the calling layer.
+
+        Returns:
+            Instance of ObtainedContent if everything went well, or None.
         """
         try:
-            content = self._finish_actual_fetch(thread_name, fingerprint, report_callback, dest_filename)
+            content = self._finish_actual_fetch(fetching_thread_name, fingerprint, report_callback, dest_filename)
         except Exception as exc:
             what_if_fail(exc)
             content = None
         finally:
             with self.activity_lock:
                 self.now_fetching_or_processing = False
 
-        after_fetch(content)
-
         return content
 
     def _verify_fingerprint(self, dest_filename, fingerprint=""):
@@ -131,35 +158,47 @@ def _verify_fingerprint(self, dest_filename, fingerprint=""):
                                             hash_obj)
         if digest != fingerprint:
             log.error(
-                "File {dest_filename} failed integrity check - assumed a "
-                "{hash_obj.name} hash and '{fingerprint}', got '{digest}'"
+                f"File {dest_filename} failed integrity check - assumed a "
+                f"{hash_obj.name} hash and '{fingerprint}', got '{digest}'"
             )
-            msg = f"Integrity check of the content failed - {hash_obj.name} hash didn't match"
+            msg = _(f"Integrity check of the content failed - {hash_obj.name} hash didn't match")
             raise content_handling.ContentCheckError(msg)
 
     def _finish_actual_fetch(self, wait_for, fingerprint, report_callback, dest_filename):
         threadMgr.wait(wait_for)
         actually_fetched_content = wait_for is not None
 
-        self._verify_fingerprint(dest_filename, fingerprint)
+        if fingerprint and dest_filename:
+            self._verify_fingerprint(dest_filename, fingerprint)
+
+        fpaths = self._gather_available_files(actually_fetched_content, dest_filename)
+
+        structured_content = ObtainedContent(self.CONTENT_DOWNLOAD_LOCATION)
+        content_type = self.get_content_type(str(dest_filename))
+        if content_type in ("archive", "rpm"):
+            structured_content.add_content_archive(dest_filename)
+
+        labelled_files = content_handling.identify_files(fpaths)
+        for fname, label in labelled_files.items():
+            structured_content.add_file(fname, label)
+
+        if fingerprint and dest_filename:
+            structured_content.record_verification(dest_filename)
 
-        content = ObtainedContent(self.CONTENT_DOWNLOAD_LOCATION)
+        return structured_content
 
-        report_callback("Analyzing content.")
+    def _gather_available_files(self, actually_fetched_content, dest_filename):
+        fpaths = []
         if not actually_fetched_content:
             if not dest_filename:  # using scap-security-guide
-                fpaths = [f"{common.SSG_DIR}/{common.SSG_CONTENT}"]
-                labelled_files = content_handling.identify_files(fpaths)
+                fpaths = [self.DEFAULT_CONTENT]
             else:  # Using downloaded XCCDF/OVAL/DS/tailoring
                 fpaths = glob(str(self.CONTENT_DOWNLOAD_LOCATION / "*.xml"))
-                labelled_files = content_handling.identify_files(fpaths)
         else:
             dest_filename = pathlib.Path(dest_filename)
             # RPM is an archive at this phase
             content_type = self.get_content_type(str(dest_filename))
             if content_type in ("archive", "rpm"):
-                # extract the content
-                content.add_content_archive(dest_filename)
                 try:
                     fpaths = common.extract_data(
                         str(dest_filename),
@@ -170,24 +209,20 @@ def _finish_actual_fetch(self, wait_for, fingerprint, report_callback, dest_file
                     log.error(msg)
                     raise err
 
-                # and populate missing fields
-                labelled_files = content_handling.identify_files(fpaths)
-
             elif content_type == "file":
-                labelled_files = content_handling.identify_files([str(dest_filename)])
+                fpaths = [str(dest_filename)]
             else:
                 raise common.OSCAPaddonError("Unsupported content type")
-
-        for f, l in labelled_files.items():
-            content.add_file(f, l)
-
-        if fingerprint:
-            content.record_verification(dest_filename)
-
-        return content
+        return fpaths
 
 
 class ObtainedContent:
+    """
+    This class aims to assist the gathered files discovery -
+    the addon can downloaded files directly, or they can be extracted for an archive.
+    The class enables user to quickly understand what is available,
+    and whether the current set of contents is usable for further processing.
+    """
     def __init__(self, root):
         self.labelled_files = dict()
         self.datastream = ""
@@ -199,10 +234,17 @@ def __init__(self, root):
         self.root = pathlib.Path(root)
 
     def record_verification(self, path):
+        """
+        Declare a file as verified (typically by means of a checksum)
+        """
+        path = pathlib.Path(path)
         assert path in self.labelled_files
         self.verified = path
 
     def add_content_archive(self, fname):
+        """
+        If files come from an archive, record this information using this function.
+        """
         path = pathlib.Path(fname)
         self.labelled_files[path] = None
         self.archive = path
@@ -244,9 +286,9 @@ def _xccdf_content(self):
 
     def find_expected_usable_content(self, relative_expected_content_path):
         content_path = self.root / relative_expected_content_path
-        elligible_main_content = (self._datastream_content(), self._xccdf_content())
+        eligible_main_content = (self._datastream_content(), self._xccdf_content())
 
-        if content_path in elligible_main_content:
+        if content_path in eligible_main_content:
             return content_path
         else:
             if not content_path.exists():
@@ -258,13 +300,12 @@ def find_expected_usable_content(self, relative_expected_content_path):
             raise content_handling.ContentHandlingError(msg)
 
     def select_main_usable_content(self):
-        elligible_main_content = (self._datastream_content(), self._xccdf_content())
-        if not any(elligible_main_content):
+        if self._datastream_content():
+            return self._datastream_content()
+        elif self._xccdf_content():
+            return self._xccdf_content()
+        else:
             msg = (
                 "Couldn't find a valid datastream or a valid XCCDF-OVAL file tuple "
                 "among the available content")
             raise content_handling.ContentHandlingError(msg)
-        if elligible_main_content[0]:
-            return elligible_main_content[0]
-        else:
-            return elligible_main_content[1]