Merge pull request #7 from Crocmagnon/improve-error-handling

Improve error handling

.github/workflows/package.yml

@@ -6,6 +6,30 @@ on:
     types: [created]
 
 jobs:
+  sonarCloudTrigger:
+    name: SonarCloud Trigger
+    runs-on: ubuntu-latest
+    steps:
+    - uses: actions/checkout@v2
+    - name: Set up Python
+      uses: actions/setup-python@v1.1.1
+      with:
+        python-version: 3.8
+    - name: Install dependencies
+      run: |
+        python -m pip install --upgrade pip
+        pip install poetry
+        poetry install
+    - name: Test with pytest
+      run: |
+        poetry run coverage run --source=ofx_processor -m pytest --color=yes
+        poetry run coverage xml
+    - name: SonarCloud Scan
+      uses: sonarsource/sonarcloud-github-action@v1.1
+      env:
+        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
+        SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
+
   build:
     name: Python ${{ matrix.python-version }} / ${{ matrix.os }}
     runs-on: ${{ matrix.os }}
@@ -29,30 +53,7 @@ jobs:
         poetry install
     - name: Test with pytest
       run: |
-        poetry run coverage run --source=ofx_processor -m pytest --color=yes
-        poetry run coverage xml
-    - name: Coverage report upload
-      uses: actions/upload-artifact@v1
-      with:
-        name: coverage_${{ matrix.os }}_${{ matrix.python-version }}
-        path: coverage.xml
-
-  sonarCloudTrigger:
-    name: SonarCloud Trigger
-    needs: build
-    runs-on: ubuntu-latest
-    steps:
-    - uses: actions/checkout@v2
-    - name: Download coverage report
-      uses: actions/download-artifact@v1
-      with:
-        name: coverage_ubuntu-latest_3.8
-        path: coverage-reports
-    - name: SonarCloud Scan
-      uses: sonarsource/sonarcloud-github-action@v1.1
-      env:
-        GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
-        SONAR_TOKEN: ${{ secrets.SONAR_TOKEN }}
+        poetry run pytest --color=yes
 
   publish:
     name: Publish to PyPI

ofx_processor/utils/ynab.py

@@ -59,20 +59,21 @@ def push_transactions(transactions, account):
     try:
         config.read(config_file)
     except configparser.Error as e:
-        click.secho(f"Error while parsing config file: {str(e)}", fg="red", bold=True)
-        click.secho("Opening the file...")
-        click.pause()
-        click.edit(filename=config_file)
-        click.secho("Exiting...", fg="red", bold=True)
-        sys.exit(1)
+        return handle_config_file_error(config_file, e)
+
+    try:
         section = config[account]
         budget_id = section["budget"]
+        token = section["token"]
+        account = section["account"]
+    except KeyError as e:
+        return handle_config_file_error(config_file, e)
+
     url = f"{BASE_URL}/budgets/{budget_id}/transactions"
     for transaction in transactions:
-        transaction["account_id"] = section["account"]
+        transaction["account_id"] = account
 
     data = {"transactions": transactions}
-    token = section["token"]
     headers = {"Authorization": f"Bearer {token}"}
 
     res = requests.post(url, json=data, headers=headers)
@@ -95,3 +96,12 @@ def push_transactions(transactions, account):
         click.secho(
             f"{len(duplicates)} transactions ignored (duplicates).", fg="yellow"
         )
+
+
+def handle_config_file_error(config_file, e):
+    click.secho(f"Error while parsing config file: {str(e)}", fg="red", bold=True)
+    click.secho("Opening the file...")
+    click.pause()
+    click.edit(filename=config_file)
+    click.secho("Exiting...", fg="red", bold=True)
+    sys.exit(1)

sonar-project.properties

@@ -4,4 +4,4 @@ sonar.projectKey=Crocmagnon_ofx-processor
 # relative paths to source directories. More details and properties are described
 # in https://sonarcloud.io/documentation/project-administration/narrowing-the-focus/
 sonar.sources=ofx_processor
-sonar.python.coverage.reportPaths=coverage-reports/coverage.xml
+sonar.python.coverage.reportPaths=coverage.xml

tests/samples/config_broken_missing_account.ini

@@ -0,0 +1,9 @@
+[DEFAULT]
+token = <YOUR API TOKEN>
+budget = <YOUR BUDGET ID>
+
+[bpvf]
+account = <YOUR BPVF ACCOUNT ID>
+
+[ce]
+account = <YOUR CE ACCOUNT ID>

tests/samples/config_broken_missing_account_key.ini

@@ -0,0 +1,11 @@
+[DEFAULT]
+token = <YOUR API TOKEN>
+budget = <YOUR BUDGET ID>
+
+[bpvf]
+account = <YOUR BPVF ACCOUNT ID>
+
+[revolut]
+
+[ce]
+account = <YOUR CE ACCOUNT ID>

tests/samples/config_broken_missing_budget.ini

@@ -0,0 +1,11 @@
+[DEFAULT]
+token = <YOUR API TOKEN>
+
+[bpvf]
+account = <YOUR BPVF ACCOUNT ID>
+
+[revolut]
+account = <YOUR REVOLUT ACCOUNT ID>
+
+[ce]
+account = <YOUR CE ACCOUNT ID>

tests/samples/config_broken_missing_token.ini

@@ -0,0 +1,11 @@
+[DEFAULT]
+budget = <YOUR BUDGET ID>
+
+[bpvf]
+account = <YOUR BPVF ACCOUNT ID>
+
+[revolut]
+account = <YOUR REVOLUT ACCOUNT ID>
+
+[ce]
+account = <YOUR CE ACCOUNT ID>

tests/test_end_to_end.py

@@ -1,4 +1,5 @@
 import json
+import os
 import unittest
 from unittest import mock
 from unittest.mock import call
@@ -39,19 +40,24 @@ class UtilsTestCase(unittest.TestCase):
 
 
 class ConfigTestCase(unittest.TestCase):
-    @mock.patch("click.edit")
-    @mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", "tests/samples")
-    def test_config_edit(self, edit):
+    def setUp(self):
         from ofx_processor.main import cli
 
         # This is run at import time and the cli module is already imported before this test
         # so we need to re-run the add_command to make it available.
         cli.add_command(ynab.config, name="config")
+        utils.discover_processors(cli)
+        self.cli = cli
 
+    @mock.patch("click.edit")
+    @mock.patch(
+        "ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", os.path.join("tests", "samples")
+    )
+    def test_config_edit(self, edit):
         runner = CliRunner()
-        runner.invoke(cli, ["config", "edit"])
+        runner.invoke(self.cli, ["config", "edit"])
 
-        expected_filename = f"tests/samples/config.ini"
+        expected_filename = os.path.join("tests", "samples", "config.ini")
         edit.assert_called_once_with(filename=expected_filename)
 
     @mock.patch("click.edit")
@@ -59,40 +65,46 @@ class ConfigTestCase(unittest.TestCase):
         "ofx_processor.utils.ynab.DEFAULT_CONFIG_FILENAME",
         "config_broken_duplicate_key.ini",
     )
-    @mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", "tests/samples")
+    @mock.patch(
+        "ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", os.path.join("tests", "samples")
+    )
     def test_broken_config_file(self, edit):
-        from ofx_processor.main import cli
-
-        # This is run at import time and the cli module is already imported before this test
-        # so we need to re-run the add_command to make it available.
-        utils.discover_processors(cli)
-
+        broken_files = [
+            "config_broken_duplicate_key.ini",
+            "config_broken_missing_account.ini",
+            "config_broken_missing_account_key.ini",
+            "config_broken_missing_budget.ini",
+            "config_broken_missing_token.ini",
+        ]
+        for name in broken_files:
+            with mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_FILENAME", name):
                 runner = CliRunner()
-        result = runner.invoke(cli, ["revolut", "tests/samples/revolut.csv"])
-
-        expected_filename = "tests/samples/config_broken_duplicate_key.ini"
+                result = runner.invoke(
+                    self.cli, ["revolut", "tests/samples/revolut.csv"]
+                )
+                expected_filename = ynab.get_config_file_name()
                 self.assertIn("Error while parsing config file", result.output)
-        edit.assert_called_once_with(filename=expected_filename)
+                edit.assert_called_with(filename=expected_filename)
 
     @mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_FILENAME", "file.ini")
-    @mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", "some/config/folder")
+    @mock.patch(
+        "ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR",
+        os.path.join("some", "config", "folder"),
+    )
     def test_get_config_file_name(self):
-        expected = "some/config/folder/file.ini"
+        expected = os.path.join("some", "config", "folder", "file.ini")
         self.assertEqual(ynab.get_config_file_name(), expected)
 
     @mock.patch("click.edit")
     @mock.patch("os.makedirs")
     @mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_FILENAME", "notfound.ini")
-    @mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", "tests/samples")
+    @mock.patch(
+        "ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", os.path.join("tests", "samples")
+    )
     def test_missing_config_file(self, makedirs, edit):
-        from ofx_processor.main import cli
-
-        # This is run at import time and the cli module is already imported before this test
-        # so we need to re-run the add_command to make it available.
-        utils.discover_processors(cli)
         runner = CliRunner()
         with mock.patch("ofx_processor.utils.ynab.open", mock.mock_open()) as mock_file:
-            result = runner.invoke(cli, ["revolut", "tests/samples/revolut.csv"])
+            result = runner.invoke(self.cli, ["revolut", "tests/samples/revolut.csv"])
             mock_file.assert_called_once()
         makedirs.assert_called_once_with(ynab.DEFAULT_CONFIG_DIR, exist_ok=True)
         self.assertIn("Editing config file", result.output)
@@ -100,8 +112,19 @@ class ConfigTestCase(unittest.TestCase):
         edit.assert_called_once_with(filename=expected_filename)
 
 
-@mock.patch("ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", "tests/samples")
+@mock.patch(
+    "ofx_processor.utils.ynab.DEFAULT_CONFIG_DIR", os.path.join("tests", "samples")
+)
 class DataTestCase(unittest.TestCase):
+    def setUp(self):
+        from ofx_processor.main import cli
+
+        # This is run at import time and the cli module is already imported before this test
+        # so we need to re-run the add_command to make it available.
+        cli.add_command(ynab.config, name="config")
+        utils.discover_processors(cli)
+        self.cli = cli
+
     @mock.patch("requests.post")
     def test_revolut_sends_data_only_created(self, post):
         post.return_value.json.return_value = {
@@ -134,14 +157,9 @@ class DataTestCase(unittest.TestCase):
                 "duplicate_import_ids": [],
             }
         }
-        from ofx_processor.main import cli
-
-        # This is run at import time and the cli module is already imported before this test
-        # so we need to re-run the add_command to make it available.
-        utils.discover_processors(cli)
 
         runner = CliRunner()
-        result = runner.invoke(cli, ["revolut", "tests/samples/revolut.csv"])
+        result = runner.invoke(self.cli, ["revolut", "tests/samples/revolut.csv"])
 
         self.assertEqual(result.exit_code, 0)
         self.assertIn("Processed 9 transactions total.", result.output)
@@ -181,14 +199,9 @@ class DataTestCase(unittest.TestCase):
                 ],
             }
         }
-        from ofx_processor.main import cli
-
-        # This is run at import time and the cli module is already imported before this test
-        # so we need to re-run the add_command to make it available.
-        utils.discover_processors(cli)
 
         runner = CliRunner()
-        result = runner.invoke(cli, ["revolut", "tests/samples/revolut.csv"])
+        result = runner.invoke(self.cli, ["revolut", "tests/samples/revolut.csv"])
 
         self.assertEqual(result.exit_code, 0)
         self.assertIn("Processed 9 transactions total.", result.output)
@@ -213,14 +226,9 @@ class DataTestCase(unittest.TestCase):
                 ],
             }
         }
-        from ofx_processor.main import cli
-
-        # This is run at import time and the cli module is already imported before this test
-        # so we need to re-run the add_command to make it available.
-        utils.discover_processors(cli)
 
         runner = CliRunner()
-        result = runner.invoke(cli, ["revolut", "tests/samples/revolut.csv"])
+        result = runner.invoke(self.cli, ["revolut", "tests/samples/revolut.csv"])
 
         self.assertEqual(result.exit_code, 0)
         self.assertIn("Processed 9 transactions total.", result.output)
@@ -229,12 +237,6 @@ class DataTestCase(unittest.TestCase):
 
     @mock.patch("requests.post")
     def test_bpvf_sends_to_ynab(self, post):
-        from ofx_processor.main import cli
-
-        # This is run at import time and the cli module is already imported before this test
-        # so we need to re-run the add_command to make it available.
-        utils.discover_processors(cli)
-
         with open("tests/samples/bpvf_transactions.json", encoding="utf-8") as f:
             expected_data = json.load(f)
 
@@ -242,7 +244,7 @@ class DataTestCase(unittest.TestCase):
         expected_url = f"{ynab.BASE_URL}/budgets/<YOUR BUDGET ID>/transactions"
 
         runner = CliRunner()
-        runner.invoke(cli, ["bpvf", "tests/samples/bpvf.ofx"])
+        runner.invoke(self.cli, ["bpvf", "tests/samples/bpvf.ofx"])
 
         post.assert_called_once_with(
             expected_url, json=expected_data, headers=expected_headers
@@ -250,12 +252,6 @@ class DataTestCase(unittest.TestCase):
 
     @mock.patch("requests.post")
     def test_ce_sends_to_ynab(self, post):
-        from ofx_processor.main import cli
-
-        # This is run at import time and the cli module is already imported before this test
-        # so we need to re-run the add_command to make it available.
-        utils.discover_processors(cli)
-
         with open("tests/samples/ce_transactions.json", encoding="utf-8") as f:
             expected_data = json.load(f)
 
@@ -263,7 +259,7 @@ class DataTestCase(unittest.TestCase):
         expected_url = f"{ynab.BASE_URL}/budgets/<YOUR BUDGET ID>/transactions"
 
         runner = CliRunner()
-        runner.invoke(cli, ["ce", "tests/samples/ce.ofx"])
+        runner.invoke(self.cli, ["ce", "tests/samples/ce.ofx"])
 
         post.assert_called_once_with(
             expected_url, json=expected_data, headers=expected_headers