Repository: beancount/fava

Issue:
`use-external-editor` feature doesn't actually use an external editor
This feature is supposed to result in a `beancount://` url instead of opening the built-in editor, when clicking on the file link on the top of this example:

![image](https://user-images.githubusercontent.com/691755/206845847-c30b0aaa-7716-4aee-b267-3cf03540ee0b.png)

However, even when `use-external-editor` is set to True, this does not happen. I haven't dug into why. [Here is the relevant code](https://github.com/beancount/fava/blob/c46d4498615e5dbeb9590653200ac361cb045fe7/src/fava/application.py#L201).

Discussion:

Patch:
diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml
index 7082e386..90aa4224 100644
--- a/.pre-commit-config.yaml
+++ b/.pre-commit-config.yaml
@@ -27,6 +27,7 @@ repos:
             name: prettier
             language: node
             entry: prettier --write --list-different --ignore-unknown
+            require_serial: true
             additional_dependencies:
                 - "prettier@2.8.1"
                 - "prettier-plugin-svelte@2.9.0"
@@ -36,6 +37,7 @@ repos:
             language: node
             entry: stylelint --fix
             files: \.(css|svelte)$
+            require_serial: true
             additional_dependencies:
                 - "stylelint@14.16.0"
                 - "stylelint-config-prettier@9.0.4"
diff --git a/frontend/src/api/validators.ts b/frontend/src/api/validators.ts
index ceaeeae1..73006d18 100644
--- a/frontend/src/api/validators.ts
+++ b/frontend/src/api/validators.ts
@@ -47,7 +47,12 @@ export const getAPIValidators = {
     })
   ),
   documents: array(object({ account: string, filename: string, date: string })),
-  errors: number,
+  errors: array(
+    object({
+      message: string,
+      source: object({ filename: string, lineno: number }),
+    })
+  ),
   extract: array(entryValidator),
   events: array(object({ type: string, description: string, date })),
   imports: importable_files_validator,
diff --git a/frontend/src/helpers.ts b/frontend/src/helpers.ts
index 4c658a53..93cf39cd 100644
--- a/frontend/src/helpers.ts
+++ b/frontend/src/helpers.ts
@@ -1,7 +1,6 @@
 import { get } from "svelte/store";
 
-import type { EntryBaseAttributes } from "./entries";
-import { base_url } from "./stores";
+import { base_url, fava_options } from "./stores";
 import { urlSyncedParams } from "./stores/url";
 
 /**
@@ -35,10 +34,10 @@ export function urlFor(
 }
 
 /** URL for the editor to the source location of an entry. */
-export function urlForSource(entry: EntryBaseAttributes): string {
-  const file_path = entry.meta.filename?.toString() ?? "";
-  const line = entry.meta.lineno?.toString() ?? "";
-  return urlFor("editor/", { file_path, line });
+export function urlForSource(file_path: string, line: string): string {
+  return get(fava_options).use_external_editor
+    ? `beancount://${file_path}?lineno=${line}`
+    : urlFor("editor/", { file_path, line });
 }
 
 /** URL for the account report. */
diff --git a/frontend/src/main.ts b/frontend/src/main.ts
index 43f7ae6c..3d062c09 100644
--- a/frontend/src/main.ts
+++ b/frontend/src/main.ts
@@ -83,7 +83,10 @@ function pollForChanges(): void {
       if (store_get(fava_options).auto_reload) {
         router.reload();
       } else {
-        get("errors").then((count) => errorCount.set(count), log_error);
+        get("errors").then(
+          (errors) => errorCount.set(errors.length),
+          log_error
+        );
         notify(_("File change detected. Click to reload."), "warning", () => {
           router.reload();
         });
diff --git a/frontend/src/modals/EntryContext.svelte b/frontend/src/modals/EntryContext.svelte
index 375e5a21..3aef4ea8 100644
--- a/frontend/src/modals/EntryContext.svelte
+++ b/frontend/src/modals/EntryContext.svelte
@@ -12,7 +12,12 @@
 <p>
   {_("Location")}:
   <code>
-    <a href={urlForSource(entry)}>
+    <a
+      href={urlForSource(
+        entry.meta.filename?.toString() ?? "",
+        entry.meta.lineno?.toString() ?? ""
+      )}
+    >
       {entry.meta.filename}:{entry.meta.lineno}
     </a>
   </code>
diff --git a/frontend/src/reports/editor/Editor.svelte b/frontend/src/reports/editor/Editor.svelte
index 6dd6b810..5e6830f7 100644
--- a/frontend/src/reports/editor/Editor.svelte
+++ b/frontend/src/reports/editor/Editor.svelte
@@ -42,7 +42,7 @@
       });
       changed = false;
       cm.focus();
-      get("errors").then((count) => errorCount.set(count), log_error);
+      get("errors").then((errors) => errorCount.set(errors.length), log_error);
     } catch (error) {
       notify_err(error, (e) => e.message);
     } finally {
diff --git a/frontend/src/reports/errors/Errors.svelte b/frontend/src/reports/errors/Errors.svelte
new file mode 100644
index 00000000..12eb1741
--- /dev/null
+++ b/frontend/src/reports/errors/Errors.svelte
@@ -0,0 +1,66 @@
+<script lang="ts">
+  import { urlForAccount, urlForSource } from "../../helpers";
+  import { _, format } from "../../i18n";
+  import { sortableTable } from "../../sort";
+  import { accounts } from "../../stores";
+
+  import type { PageData } from "./load";
+
+  export let data: PageData;
+
+  $: account_re = new RegExp(`(${$accounts.join("|")})`);
+
+  /** Replace all account names with links to the corresponding account page. */
+  function addAccountLinks(msg: string): string {
+    return msg
+      .split(account_re)
+      .map((s, idx) => {
+        if (idx % 2 === 0) {
+          const el = document.createElement("span");
+          el.textContent = s;
+          return el;
+        }
+        const el = document.createElement("a");
+        el.href = urlForAccount(s);
+        el.textContent = s;
+        return el;
+      })
+      .map((el) => el.outerHTML)
+      .join("");
+  }
+</script>
+
+{#if data.length}
+  <table use:sortableTable class="errors">
+    <thead>
+      <tr>
+        <th data-sort="string">{_("File")}</th>
+        <th data-sort="num">{_("Line")}</th>
+        <th data-sort="string">{_("Error")}</th>
+      </tr>
+    </thead>
+    <tbody>
+      {#each data as { message, source }}
+        {@const url = urlForSource(source.filename, `${source.lineno}`)}
+        {@const title = format(_("Show source %(file)s:%(lineno)s"), {
+          file: source.filename,
+          lineno: `${source.lineno}`,
+        })}
+        <tr>
+          <td>
+            {source.filename}
+          </td>
+          <td class="num">
+            <a class="source" href={url} {title}>{source.lineno}</a>
+          </td>
+          <!-- eslint-disable-next-line svelte/no-at-html-tags -->
+          <td class="pre">{@html addAccountLinks(message)}</td>
+        </tr>
+      {/each}
+    </tbody>
+  </table>
+{:else}
+  <p>
+    {_("No errors.")}
+  </p>
+{/if}
diff --git a/frontend/src/reports/errors/load.ts b/frontend/src/reports/errors/load.ts
new file mode 100644
index 00000000..17bf5d66
--- /dev/null
+++ b/frontend/src/reports/errors/load.ts
@@ -0,0 +1,5 @@
+import { get } from "../../api";
+
+export const load = () => get("errors");
+
+export type PageData = Awaited<ReturnType<typeof load>>;
diff --git a/frontend/src/reports/routes.ts b/frontend/src/reports/routes.ts
index 3e76052f..3cef1caf 100644
--- a/frontend/src/reports/routes.ts
+++ b/frontend/src/reports/routes.ts
@@ -10,6 +10,8 @@ import DocumentsSvelte from "./documents/Documents.svelte";
 import { load as load_documents } from "./documents/load";
 import EditorSvelte from "./editor/Editor.svelte";
 import { load as load_editor } from "./editor/load";
+import ErrorsSvelte from "./errors/Errors.svelte";
+import { load as load_errors } from "./errors/load";
 import EventsSvelte from "./events/Events.svelte";
 import { load as load_events } from "./events/load";
 import ImportSvelte from "./import/Import.svelte";
@@ -40,6 +42,7 @@ const routes: [
   ["commodities", CommoditiesSvelte, load_commodities, () => _("Commodities")],
   ["documents", DocumentsSvelte, load_documents, () => _("Documents")],
   ["editor", EditorSvelte, load_editor, () => _("Editor")],
+  ["errors", ErrorsSvelte, load_errors, () => _("Errors")],
   ["events", EventsSvelte, load_events, () => _("Events")],
   ["import", ImportSvelte, load_import, () => _("Import")],
   ["query", QuerySvelte, noload, () => _("Query")],
diff --git a/frontend/src/stores/index.ts b/frontend/src/stores/index.ts
index 2c14d01d..77cb08ca 100644
--- a/frontend/src/stores/index.ts
+++ b/frontend/src/stores/index.ts
@@ -42,13 +42,13 @@ export const ledgerDataValidator = object({
     currency_column: number,
     conversion_currencies: array(string),
     import_config: optional(string),
-
     indent: number,
     locale: union(string, constant(null)),
     uptodate_indicator_grey_lookback_days: number,
     insert_entry: array(
       object({ date: string, filename: string, lineno: number, re: string })
     ),
+    use_external_editor: boolean,
   }),
   have_excel: boolean,
   incognito: boolean,
diff --git a/src/fava/application.py b/src/fava/application.py
index ffd569a5..d53fb27a 100644
--- a/src/fava/application.py
+++ b/src/fava/application.py
@@ -89,7 +89,6 @@ app.config["ACCOUNT_RE"] = ACCOUNT_RE
 
 SERVER_SIDE_REPORTS = [
     "balance_sheet",
-    "errors",
     "holdings",
     "income_statement",
     "journal",
@@ -102,6 +101,7 @@ CLIENT_SIDE_REPORTS = [
     "commodities",
     "documents",
     "editor",
+    "errors",
     "events",
     "import",
     "query",
@@ -196,16 +196,6 @@ def url_for(endpoint: str, **values: str) -> str:
     return CACHED_URL_FOR(endpoint, **values)
 
 
-def url_for_source(**kwargs: str) -> str:
-    """URL to source file (possibly link to external editor)."""
-    if g.ledger.fava_options.use_external_editor:
-        return (
-            f"beancount://{kwargs.get('file_path')}"
-            + f"?lineno={kwargs.get('line', 1)}"
-        )
-    return url_for("report", report_name="editor", **kwargs)
-
-
 def translations() -> Any:
     """Get translations catalog."""
     # pylint: disable=protected-access
@@ -215,7 +205,6 @@ def translations() -> Any:
 app.add_template_global(static_url, "static_url")
 app.add_template_global(date.today, "today")
 app.add_template_global(url_for, "url_for")
-app.add_template_global(url_for_source, "url_for_source")
 app.add_template_global(translations, "translations")
 app.add_template_global(get_ledger_data, "get_ledger_data")
 
diff --git a/src/fava/json_api.py b/src/fava/json_api.py
index 31174522..67aca32f 100644
--- a/src/fava/json_api.py
+++ b/src/fava/json_api.py
@@ -162,9 +162,9 @@ def get_changed() -> bool:
 
 
 @api_endpoint
-def get_errors() -> int:
-    """Number of errors."""
-    return len(g.ledger.errors)
+def get_errors() -> list[Any]:
+    """Errors of the ledger."""
+    return g.ledger.errors
 
 
 api_endpoint(get_ledger_data)
diff --git a/src/fava/template_filters.py b/src/fava/template_filters.py
index 76c25fa1..9f26a5a0 100644
--- a/src/fava/template_filters.py
+++ b/src/fava/template_filters.py
@@ -5,7 +5,6 @@ All functions in this module will be automatically added as template filters.
 from __future__ import annotations
 
 import datetime
-import re
 from os.path import basename as path_basename
 from typing import Any
 from typing import MutableMapping
@@ -14,13 +13,10 @@ from unicodedata import normalize
 
 from beancount.core import compare
 from beancount.core import realization
-from beancount.core.account import ACCOUNT_RE
 from beancount.core.data import Directive
 from beancount.core.inventory import Inventory
 from beancount.core.number import Decimal
 from beancount.core.number import ZERO
-from flask import url_for
-from markupsafe import Markup
 
 from fava.context import g
 from fava.core.conversion import cost
@@ -141,20 +137,6 @@ def basename(file_path: str) -> str:
     return normalize("NFC", path_basename(file_path))
 
 
-def format_errormsg(message: str) -> Markup:
-    """Match account names in error messages and insert HTML links for them."""
-    match = re.search(ACCOUNT_RE, message)
-    if not match:
-        return Markup(message)
-    account = match.group()
-    url = url_for("account", name=account)
-    return Markup(
-        message.replace(account, f'<a href="{url}">{account}</a>')
-        .replace("for '", "for ")
-        .replace("': ", ": ")
-    )
-
-
 def collapse_account(account_name: str) -> bool:
     """Return true if account should be collapsed."""
     collapse_patterns = g.ledger.fava_options.collapse_pattern
@@ -172,7 +154,6 @@ FILTERS = [
     format_currency,
     format_date,
     format_date_filter,
-    format_errormsg,
     get_or_create,
     hash_entry,
     remove_keys,
diff --git a/src/fava/templates/errors.html b/src/fava/templates/errors.html
deleted file mode 100644
index 0a04db6a..00000000
--- a/src/fava/templates/errors.html
+++ /dev/null
@@ -1,26 +0,0 @@
-{% if ledger.errors %}
-<table is="sortable-table" class="errors">
-  <thead>
-    <tr>
-      <th data-sort="string" data-order="asc">{{ _('File') }}</th>
-      <th data-sort="num">{{ _('Line') }}</th>
-      <th data-sort="string">{{ _('Error') }}</th>
-    </tr>
-  </thead>
-  <tbody>
-    {% for error in ledger.errors %}
-    <tr>
-      {% with link=url_for_source(file_path=error.source['filename'], line=error.source['lineno']) %}
-      <td><a class="source" href="{{ link }}" title="{{ _('Show source %(file)s:%(lineno)s', file=error.source['filename'], lineno=error.source['lineno']) }}">{{ error.source['filename'] }}</a></td>
-      <td class="num"><a class="source" href="{{ link }}" title="{{ _('Show source %(file)s:%(lineno)s', file=error.source['filename'], lineno=error.source['lineno']) }}">{{ error.source['lineno'] }}</a></td>
-      <td class="pre">{{ error.message|format_errormsg }}</td>
-      {% endwith %}
-    </tr>
-    {% endfor %}
-  </tbody>
-</table>
-{% else %}
-<p>
-{{ _('No errors.') }}
-</p>
-{% endif %}


Test patch:
diff --git a/tests/test_json_api.py b/tests/test_json_api.py
index eed71819..6f7d0545 100644
--- a/tests/test_json_api.py
+++ b/tests/test_json_api.py
@@ -107,7 +107,7 @@ def test_api_add_document(
 
 def test_api_errors(test_client: FlaskClient) -> None:
     response = test_client.get("/long-example/api/errors")
-    assert_api_success(response, 0)
+    assert_api_success(response, [])
 
 
 def test_api_context(
diff --git a/tests/test_template_filters.py b/tests/test_template_filters.py
index 9f6e9db2..d830e98a 100644
--- a/tests/test_template_filters.py
+++ b/tests/test_template_filters.py
@@ -21,7 +21,6 @@ from fava.template_filters import collapse_account
 from fava.template_filters import format_currency
 from fava.template_filters import format_date
 from fava.template_filters import format_date_filter
-from fava.template_filters import format_errormsg
 from fava.template_filters import get_or_create
 from fava.template_filters import remove_keys
 from fava.template_filters import should_show
@@ -95,22 +94,6 @@ def test_should_show(app: Flask) -> None:
         assert should_show(g.filtered.root_tree.get("Expenses")) is False
 
 
-def test_format_errormsg(app: Flask) -> None:
-    with app.test_request_context("/long-example/"):
-        app.preprocess_request()
-        assert (
-            format_errormsg("Test for 'Expenses:Acme:Cash': Test")
-            == 'Test for <a href="/long-example/account/Expenses:'
-            'Acme:Cash/">Expenses:Acme:Cash</a>: Test'
-        )
-        assert (
-            format_errormsg("Test Expenses:Acme:Cash Test")
-            == 'Test <a href="/long-example/account/Expenses:'
-            'Acme:Cash/">Expenses:Acme:Cash</a> Test'
-        )
-        assert format_errormsg("Test: Test") == "Test: Test"
-
-
 def test_collapse_account(app: Flask, monkeypatch: MonkeyPatch) -> None:
     with app.test_request_context("/long-example/"):
         app.preprocess_request()