Repository: Senth/minecraft-mod-manager (python)

Issue:
Downloading via slug doesn't persist if mod name attribute varies
For example, if you download unearthed-fabric, it will pull from curse, but store in the sqlite as simply unearthed, requiring a further ```configure unearthed=unearthed-fabric``` in order for an update to work in the future.

Pull request description:
Downloading via slug does not persist if mod name varies
Downloading via slug now saves correctly in the db

### What changed?
- Now checks the mod_id after installing the mod so it's saved correctly in the mod id. Basically, it's possible to install through searching on the slug directly
- possible_names have expanded to include split names (by `-`) Meaning `unearthed-fabric` will add `unearthed` and `fabric` to the search query.

### Testing
- [X] Added unit tests
- [X] Added integration tests

### Related Issues
Fixes #32 


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 7e3c12c..680148c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## Upcoming
+
+### Fixed
+
+- Installing via slug now works correctly (saves the mod correctly in db) [#32](https://github.com/Senth/minecraft-mod-manager/issues/32)
+
 ## [1.2.3] - 2021-06-16
 
 ### Fixed
diff --git a/minecraft_mod_manager/adapters/repo_impl.py b/minecraft_mod_manager/adapters/repo_impl.py
index 3a4124c..546c709 100644
--- a/minecraft_mod_manager/adapters/repo_impl.py
+++ b/minecraft_mod_manager/adapters/repo_impl.py
@@ -23,6 +23,7 @@ class RepoImpl(ConfigureRepo, UpdateRepo, InstallRepo, ShowRepo):
 
     def __init__(self, jar_parser: JarParser, sqlite: Sqlite, downloader: Downloader) -> None:
         self.db = sqlite
+        self.jar_parser = jar_parser
         self.mods = self.db.sync_with_dir(jar_parser.mods)
         self.downloader = downloader
         self.apis = (
@@ -49,6 +50,9 @@ class RepoImpl(ConfigureRepo, UpdateRepo, InstallRepo, ShowRepo):
     def get_all_mods(self) -> Sequence[Mod]:
         return self.mods
 
+    def get_mod_from_file(self, filepath: str) -> Union[Mod, None]:
+        return self.jar_parser.get_mod(filepath)
+
     def remove_mod_file(self, filename: str) -> None:
         path = Path(config.dir).joinpath(filename)
         path.unlink(missing_ok=True)
diff --git a/minecraft_mod_manager/app/download/download.py b/minecraft_mod_manager/app/download/download.py
index 9d32c1b..c84c67d 100644
--- a/minecraft_mod_manager/app/download/download.py
+++ b/minecraft_mod_manager/app/download/download.py
@@ -9,21 +9,6 @@ from ...utils.logger import LogColors, Logger
 from .download_repo import DownloadRepo
 
 
-class DownloadInfo:
-    def __init__(self, mod: Mod, version_info: VersionInfo):
-        self.mod = mod
-        self.version_info = version_info
-
-    @property
-    def name(self) -> str:
-        name = f"{self.mod.id}"
-
-        if self.mod.version:
-            name += f" {self.mod.version}"
-
-        return name
-
-
 class Download:
     def __init__(self, repo: DownloadRepo):
         self._repo = repo
@@ -41,11 +26,11 @@ class Download:
                 latest_version = LatestVersionFinder.find_latest_version(mod, versions, filter=True)
 
                 if latest_version:
-                    download_info = DownloadInfo(mod, latest_version)
                     Logger.verbose("⬇ Downloading...", indent=1)
-                    self._download_and_install(download_info)
-                    # TODO #32 read mod again to get version number etc.
-                    self.on_version_found(download_info)
+                    downloaded_mod = self._download(mod, latest_version)
+                    self._update_mod_from_file(downloaded_mod)
+                    self._repo.update_mod(downloaded_mod)
+                    self.on_version_found(mod, downloaded_mod)
                 else:
                     self.on_version_not_found(mod, versions)
 
@@ -59,15 +44,19 @@ class Download:
             for error in mods_not_found:
                 error.print_message()
 
-    def on_version_found(self, download_info: DownloadInfo) -> None:
+    def on_version_found(self, old: Mod, new: Mod) -> None:
         raise NotImplementedError("Not implemented in subclass")
 
     def on_version_not_found(self, mod: Mod, versions: List[VersionInfo]) -> None:
         raise NotImplementedError("Not implemented in subclass")
 
-    def _download_and_install(self, download_info: DownloadInfo) -> None:
-        downloaded_mod = self._download(download_info.mod, download_info.version_info)
-        self._repo.update_mod(downloaded_mod)
+    def _update_mod_from_file(self, mod: Mod) -> None:
+        if mod.file:
+            installed_mod = self._repo.get_mod_from_file(mod.file)
+            if installed_mod:
+                mod.id = installed_mod.id
+                mod.name = installed_mod.name
+                mod.version = installed_mod.version
 
     def _download(self, mod: ModArg, latest_version: VersionInfo) -> Mod:
         downloaded_file = self._repo.download(latest_version.download_url, latest_version.filename)
diff --git a/minecraft_mod_manager/app/download/download_repo.py b/minecraft_mod_manager/app/download/download_repo.py
index 384a6e7..3b0df36 100644
--- a/minecraft_mod_manager/app/download/download_repo.py
+++ b/minecraft_mod_manager/app/download/download_repo.py
@@ -1,5 +1,5 @@
 from pathlib import Path
-from typing import Dict, List
+from typing import Dict, List, Union
 
 from ...core.entities.mod import Mod
 from ...core.entities.sites import Site, Sites
@@ -18,3 +18,6 @@ class DownloadRepo:
 
     def update_mod(self, mod: Mod) -> None:
         raise NotImplementedError()
+
+    def get_mod_from_file(self, filepath: str) -> Union[Mod, None]:
+        raise NotImplementedError()
diff --git a/minecraft_mod_manager/app/install/install.py b/minecraft_mod_manager/app/install/install.py
index a234b53..6a6cd6b 100644
--- a/minecraft_mod_manager/app/install/install.py
+++ b/minecraft_mod_manager/app/install/install.py
@@ -7,7 +7,7 @@ from ...core.entities.mod_loaders import ModLoaders
 from ...core.entities.version_info import VersionInfo
 from ...core.utils.latest_version_finder import LatestVersionFinder
 from ...utils.logger import LogColors, Logger
-from ..download.download import Download, DownloadInfo
+from ..download.download import Download
 from .install_repo import InstallRepo
 
 
@@ -66,10 +66,9 @@ class Install(Download):
 
         return loader_max
 
-    def on_version_found(self, download_info: DownloadInfo) -> None:
-        # TODO #32 improve message
+    def on_version_found(self, old: Mod, new: Mod) -> None:
         Logger.info(
-            f"🟢 Installed ({download_info.version_info.filename})",
+            f"🟢 Installed version {new.version}",
             LogColors.green,
             indent=1,
         )
diff --git a/minecraft_mod_manager/app/update/update.py b/minecraft_mod_manager/app/update/update.py
index c68c0f1..cc2bc33 100644
--- a/minecraft_mod_manager/app/update/update.py
+++ b/minecraft_mod_manager/app/update/update.py
@@ -4,7 +4,7 @@ from ...config import config
 from ...core.entities.mod import Mod, ModArg
 from ...core.entities.version_info import VersionInfo
 from ...utils.logger import LogColors, Logger
-from ..download.download import Download, DownloadInfo
+from ..download.download import Download
 from .update_repo import UpdateRepo
 
 
@@ -27,15 +27,14 @@ class Update(Download):
 
         self.find_download_and_install(mods_to_update)
 
-    def on_version_found(self, download_info: DownloadInfo) -> None:
-        if download_info.mod.file:
-            if Update._has_downloaded_new_file(download_info):
-                if not config.pretend:
-                    self._update_repo.remove_mod_file(download_info.mod.file)
+    def on_version_found(self, old: Mod, new: Mod) -> None:
+        if new.file:
+            if Update._has_downloaded_new_file(old, new):
+                if not config.pretend and old.file:
+                    self._update_repo.remove_mod_file(old.file)
 
-                # TODO #32 improve message
                 Logger.info(
-                    f"🟢 Updated {download_info.mod.version} -> {download_info.version_info.filename}",
+                    f"🟢 Updated {old.version} -> {new.version}",
                     LogColors.green,
                     indent=1,
                 )
@@ -44,13 +43,9 @@ class Update(Download):
         Logger.verbose("🟨 No new version found", LogColors.skip, indent=1)
 
     @staticmethod
-    def _has_downloaded_new_file(download_info: DownloadInfo) -> bool:
-        mod = download_info.mod
-        version_info = download_info.version_info
-
-        if mod.file:
-            if len(version_info.filename) > 0:
-                if mod.file != version_info.filename:
-                    return True
+    def _has_downloaded_new_file(old: Mod, new: Mod) -> bool:
+        if new.file and len(new.file) > 0:
+            if old.file != new.file:
+                return True
 
         return False
diff --git a/minecraft_mod_manager/core/entities/mod.py b/minecraft_mod_manager/core/entities/mod.py
index ba254c4..89aa402 100644
--- a/minecraft_mod_manager/core/entities/mod.py
+++ b/minecraft_mod_manager/core/entities/mod.py
@@ -113,6 +113,12 @@ class Mod(ModArg):
                 without_fabric = re.sub(r"-fabric\w*|fabric\w*-", "", filename)
                 possible_names.add(without_fabric)
 
+        # Split parts of name
+        split_names: Set[str] = set()
+        for possible_name in possible_names:
+            split_names.update(possible_name.split("-"))
+        possible_names.update(split_names)
+
         return possible_names
 
     def __members(self):
diff --git a/minecraft_mod_manager/gateways/api/curse_api.py b/minecraft_mod_manager/gateways/api/curse_api.py
index c94605b..95baa56 100644
--- a/minecraft_mod_manager/gateways/api/curse_api.py
+++ b/minecraft_mod_manager/gateways/api/curse_api.py
@@ -39,7 +39,9 @@ class CurseApi(Api):
 
     @staticmethod
     def _make_files_url(mod: Mod) -> str:
-        return f"{_base_url}/{mod.sites[Sites.curse].id}/files"
+        if mod.sites:
+            return f"{_base_url}/{mod.sites[Sites.curse].id}/files"
+        return ""
 
     @staticmethod
     def _file_to_version_info(file_data: Any) -> VersionInfo:
diff --git a/minecraft_mod_manager/gateways/jar_parser.py b/minecraft_mod_manager/gateways/jar_parser.py
index 19c4932..7b73152 100644
--- a/minecraft_mod_manager/gateways/jar_parser.py
+++ b/minecraft_mod_manager/gateways/jar_parser.py
@@ -27,15 +27,18 @@ class JarParser:
         # Iterate through all files
         for file in self._dir.glob("*.jar"):
             Logger.debug(f"Found file {file}")
-            mod = JarParser.get_mod_info(file)
+            mod = JarParser._get_mod_info(file)
             if mod:
                 JarParser._log_found_mod(mod)
                 self._mods.append(mod)
 
         return self._mods
 
+    def get_mod(self, file: str) -> Union[Mod, None]:
+        return JarParser._get_mod_info(self._dir.joinpath(file))
+
     @staticmethod
-    def get_mod_info(file: Path) -> Union[Mod, None]:
+    def _get_mod_info(file: Path) -> Union[Mod, None]:
         mod: Union[Mod, None] = None
 
         try:


Test patch:
diff --git a/minecraft_mod_manager/app/download/download_test.py b/minecraft_mod_manager/app/download/download_test.py
index 03bf853..a61ea0f 100644
--- a/minecraft_mod_manager/app/download/download_test.py
+++ b/minecraft_mod_manager/app/download/download_test.py
@@ -29,6 +29,7 @@ def test_download_and_install_when_found(mock_repo):
     when(mock_repo).search_for_mod(...).thenReturn({Sites.curse: Site(Sites.curse, "", "")})
     when(mock_repo).get_versions(...).thenReturn([version_info])
     when(mock_repo).download(...).thenReturn(Path("mod.jar"))
+    when(mock_repo).get_mod_from_file(...).thenReturn(Mod("found", ""))
     when(mock_repo).update_mod(...)
 
     download = Download(mock_repo)
@@ -37,3 +38,36 @@ def test_download_and_install_when_found(mock_repo):
 
     verifyStubbedInvocationsAreUsed()
     unstub()
+
+
+def test_update_mod_id_after_download(mock_repo):
+    download = Download(mock_repo)
+
+    mod_arg = Mod("dummy", "name")
+    installed_mod = Mod("validid", "name", version="1.0.0")
+    expected_mod = Mod(
+        "validid",
+        "name",
+        sites={Sites.curse: Site(Sites.curse, "", "")},
+        version="1.0.0",
+        file="mod.jar",
+    )
+    version_info = VersionInfo(
+        stability=Stabilities.release,
+        mod_loaders={ModLoaders.fabric},
+        site=Sites.curse,
+        upload_time=0,
+        minecraft_versions=[],
+        download_url="",
+    )
+    when(mock_repo).search_for_mod(...).thenReturn({Sites.curse: Site(Sites.curse, "", "")})
+    when(mock_repo).get_versions(...).thenReturn([version_info])
+    when(mock_repo).download(...).thenReturn(Path("mod.jar"))
+    when(mock_repo).update_mod(expected_mod)
+    when(mock_repo).get_mod_from_file("mod.jar").thenReturn(installed_mod)
+    when(download).on_version_found(...)
+
+    download.find_download_and_install([mod_arg])
+
+    verifyStubbedInvocationsAreUsed()
+    unstub()
diff --git a/minecraft_mod_manager/app/update/update_test.py b/minecraft_mod_manager/app/update/update_test.py
index d24479e..aca0821 100644
--- a/minecraft_mod_manager/app/update/update_test.py
+++ b/minecraft_mod_manager/app/update/update_test.py
@@ -1,7 +1,6 @@
 from typing import List, Union
 
 import pytest
-from minecraft_mod_manager.app.download.download import DownloadInfo
 from mockito import mock, unstub, verifyStubbedInvocationsAreUsed, when
 
 from ...config import config
@@ -52,60 +51,66 @@ def version_info(filename: str) -> VersionInfo:
     )
 
 
-def download_info(old: Union[str, None], new: str) -> DownloadInfo:
-    return DownloadInfo(Mod("", "", file=old), version_info(new))
-
-
 @pytest.mark.parametrize(
-    "name,input,pretend,expected",
+    "name,old,new,pretend,expected",
     [
         (
             "Remove file when new file has been downloaded",
-            download_info("old", "new"),
+            "old",
+            "new",
             False,
             True,
         ),
         (
             "Keep file when no new file has been downloaded",
-            download_info("old", "old"),
+            "old",
+            "old",
             False,
             False,
         ),
         (
             "Keep old file when new filename is empty",
-            download_info("old", ""),
+            "old",
+            "",
             False,
             False,
         ),
         (
             "Don't remove old file when it doesn't exist",
-            download_info(None, "new"),
+            None,
+            "new",
             False,
             False,
         ),
         (
             "Don't remove old file when it's empty",
-            download_info("", "new"),
+            "",
+            "new",
             False,
             False,
         ),
         (
             "Don't remove old file when --pretend is on",
-            download_info("old", "new"),
+            "old",
+            "new",
             True,
             False,
         ),
     ],
 )
-def test_on_version_found(name: str, input: DownloadInfo, pretend: bool, expected: bool, mock_repo):
+def test_on_version_found(
+    name: str, old: Union[str, None], new: Union[str, None], pretend: bool, expected: bool, mock_repo
+):
     print(name)
 
     config.pretend = pretend
     if expected:
-        when(mock_repo).remove_mod_file(input.mod.file)
+        when(mock_repo).remove_mod_file(old)
 
     update = Update(mock_repo)
-    update.on_version_found(input)
+    old_mod = Mod("", "", file=old)
+    new_mod = Mod("", "", file=new)
+    update.on_version_found(old_mod, new_mod)
 
     config.pretend = False
     verifyStubbedInvocationsAreUsed()
diff --git a/minecraft_mod_manager/gateways/api/api_test.py b/minecraft_mod_manager/gateways/api/api_test.py
index f7c8e16..03e8e47 100644
--- a/minecraft_mod_manager/gateways/api/api_test.py
+++ b/minecraft_mod_manager/gateways/api/api_test.py
@@ -47,9 +47,11 @@ def test_find_mod_id_from_mod_id(api: Api):
 
 
 def test_mod_not_found_uses_all_possible_names(api: Api):
-    input = Mod("mod-id", "", file="some-filename")
+    input = Mod("unearthed-fabric", "", file="some-filename")
 
-    when(api)._find_mod_id_by_slug("mod-id", input.get_possible_slugs())
+    when(api)._find_mod_id_by_slug("unearthed-fabric", input.get_possible_slugs())
+    when(api)._find_mod_id_by_slug("unearthed", input.get_possible_slugs())
+    when(api)._find_mod_id_by_slug("fabric", input.get_possible_slugs())
     when(api)._find_mod_id_by_slug("some", input.get_possible_slugs())
 
     with pytest.raises(ModNotFoundException) as e:
diff --git a/minecraft_mod_manager/gateways/jar_parser_test.py b/minecraft_mod_manager/gateways/jar_parser_test.py
index c9ba81f..a2a8c81 100644
--- a/minecraft_mod_manager/gateways/jar_parser_test.py
+++ b/minecraft_mod_manager/gateways/jar_parser_test.py
@@ -81,7 +81,7 @@ def test_get_mod_info(name, file, expected):
     print(name)
     input = path(file)
 
-    result = JarParser.get_mod_info(input)
+    result = JarParser._get_mod_info(input)
 
     assert expected == result
 
diff --git a/tests/install_test.py b/tests/install_test.py
index 9972bd1..00878db 100644
--- a/tests/install_test.py
+++ b/tests/install_test.py
@@ -21,3 +21,14 @@ def test_install_mod_that_has_no_mod_loader(helper: Helper):
 
     assert code == 0
     assert jei is not None
+
+
+def test_remember_slug_for_installed_mod_if_mod_id_varies(helper: Helper):
+    """When installing and supplying a slug it used to save the slug as an id.
+    This test makes sure that we actually check what the mod id is before saving
+    to the database.
+    """
+    code = helper.run("install", "unearthed-fabric")
+
+    assert code == 0
+    assert helper.exists_in_db("unearthed")
diff --git a/tests/util/helper.py b/tests/util/helper.py
index 91de03d..8b388b1 100644
--- a/tests/util/helper.py
+++ b/tests/util/helper.py
@@ -26,6 +26,10 @@ class Helper:
             return file
         return None
 
+    def exists_in_db(self, id: str) -> bool:
+        self.cursor.execute("SELECT * FROM mod WHERE id=?", [id])
+        return self.cursor.fetchone() is not None
+
     def unstub(self) -> None:
         self.cursor.close()
         self.db.close()