Repository navigation
Conversation
- add `get_report_filters` tool to retrieve filter options for reports, parses js under the hood for filters. - add `run_report` tool for report generation.
- Removed the `get_report_filters` method from the toolset as it is no longer needed. - Updated the `run_report` function to directly retrieve filters and handle exceptions more gracefully. - Adjusted the agent's tool call logic to reflect the removal of the `get_report_filters` call.
- Reformatted the `JS_FILTER_KEYS` for improved readability. - Removed an unnecessary blank line in the `run_report` method of the toolset.
- Updated `get_report_filters` to include JavaScript filters alongside document filters. - Modified `run_report` to handle filters more robustly, ensuring compatibility with the new structure. - Added default values for "Company" and "Fiscal Year" link fields based on user settings and current date.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…in run_report - Updated `_find_js_for_ident` to improve efficiency by only using the first two parts of the identifier, its enough for ERPNext. - Enhanced `_read_quoted` to handle string decoding more robustly. - Improved error handling in `run_report` to provide clearer context in the returned error message.
- implemented a custom escape sequence handling.
- Removed the unnecessary _has_own_fieldname function to streamline the filter extraction process. - Directly appended valid rows from JavaScript objects to the results list.
- Move report JS filter parsing into extract_report_filters.py, with $.extend and date defaults. - On Report Builder, keep only those fieldnames and check Link permissions. - Cap every report at 200 rows. - If a run fails, retry without optional checks that match their default. - Install the ERPNext report skill so the agent passes the checks those reports expect. - Cover Report Builder filter rejection and the skill install with tests.
…kill to Ask ALYF User
| after_install = ["ask_alyf.ask_alyf.install.ensure_erpnext_report_skill"] | ||
| after_migrate = ["ask_alyf.ask_alyf.install.ensure_erpnext_report_skill"] |
There was a problem hiding this comment.
Instead of building the fixture sync manually, could we just reuse this frappe standard hook? https://docs.frappe.io/framework/user/en/python-api/hooks#fixtures
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Intention was to not override the Skill, if it once exists
|
Here is a summary of the
Similar Change-Size Groups
|
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | البحث عن عوامل تصفية التقرير |
bn (ask_alyf/locale/bn.po) — 1 entries
bn (ask_alyf/locale/bn.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | রিপোর্ট ফিল্টার খোঁজা হচ্ছে |
de (ask_alyf/locale/de.po) — 1 entries
de (ask_alyf/locale/de.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | Berichtsfilter werden gesucht |
es (ask_alyf/locale/es.po) — 1 entries
es (ask_alyf/locale/es.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | Buscando los filtros del informe |
hi (ask_alyf/locale/hi.po) — 1 entries
hi (ask_alyf/locale/hi.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | रिपोर्ट फ़िल्टर खोजे जा रहे हैं |
ja (ask_alyf/locale/ja.po) — 1 entries
ja (ask_alyf/locale/ja.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | レポートのフィルターを検索しています |
pt (ask_alyf/locale/pt.po) — 1 entries
pt (ask_alyf/locale/pt.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | A procurar filtros do relatório |
ru (ask_alyf/locale/ru.po) — 1 entries
ru (ask_alyf/locale/ru.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | Поиск фильтров отчета |
zh_Hans_CN (ask_alyf/locale/zh_CN.po) — 1 entries
zh_Hans_CN (ask_alyf/locale/zh_CN.po)
| Status | Msgid | Previous | Current |
|---|---|---|---|
| added | Looking up report filters | empty | 正在查找报表筛选条件 |
|
| Status | Previous | Current |
|---|---|---|
| added | empty | Looking up report filters |
barredterra
left a comment
barredterra
left a comment
There was a problem hiding this comment.
Review notes below. The first three (all in run_report error handling) look like the ones to fix before merge. Dropping the retry and letting errors go through _on_tool_error would solve all three.
| row | ||
| and row.get("fieldtype") == "Check" | ||
| and not row.get("reqd") | ||
| and cint(row.get("default")) == cint(value) |
There was a problem hiding this comment.
The retry drops optional Check filters whose value equals the default, also when the default is 1. In query_report.run, a missing Check is falsy, not "use the default". So the retry changes the meaning of the report.
Example: General Ledger with include_default_book_entries=1 (default 1) fails for another reason, e.g. a bad date. The retry runs without the filter, so default book entries are excluded. The agent gets different totals plus an omitted_filters note, and the original error is lost.
| try: | ||
| meta = get_report_filters(report_name) | ||
| return _execute_report(report_name, meta, applied_filters, are_default_filters=not filters) | ||
| except Exception as error: |
There was a problem hiding this comment.
This catches every exception and returns str(error) to the model. That gets around _on_tool_error in agent.py, which on purpose shows only the exception type for errors that are not Validation or Permission errors. SQL errors, KeyErrors etc. from script reports would then put query text, table/column names or internal values into the LLM context, the conversation and the traces.
| result = _execute_report(report_name, meta, narrowed, are_default_filters=False) | ||
| result["omitted_filters"] = omitted | ||
| return result | ||
| except Exception: |
There was a problem hiding this comment.
The retry runs after any exception: timeouts, lock waits, permission or DB errors. It reruns the full report without a rollback and swallows the second error. On a heavy report this doubles the load. On Postgres, the aborted transaction makes the retry fail too, and later DB calls in the same agent job can also fail.
| if token.type not in ("linecomment", "multilinecomment") | ||
| ] | ||
| rows = [] | ||
| for index, token in enumerate(tokens): |
There was a problem hiding this comment.
Every {...} in the report JS that has a depth-1 fieldname: key counts as a report filter. That includes frappe.prompt / dialog fields, get_query return values and column definitions in onload or button handlers. Such fields then show up as allowed filters, and for Report Builder _builder_filters turns them into WHERE conditions. Maybe limit parsing to the filters: [...] array of the frappe.query_reports[...] object?
| name = row["fieldname"] | ||
| if name in seen: | ||
| continue | ||
| seen.add(name) |
There was a problem hiding this comment.
Extended base scripts are read before the report's own script, and the first fieldname seen wins. When a report extends a base set of filters (e.g. erpnext.financial_statements) and then redefines one with a different default or reqd, the base version is returned. Reading the report's own script first (or letting later definitions win) would fix this.
| break | ||
| elif ( | ||
| depth == 1 | ||
| and tokens[index].type == "name" |
There was a problem hiding this comment.
Only name tokens are matched as keys. Quoted keys such as "fieldname": "company" are string tokens, so filters written in JSON style (common in custom report javascript) are not found. The report then shows no filters.
| """Install the ERPNext report skill once. Skip when ERPNext is absent or the skill was edited.""" | ||
| if "erpnext" not in frappe.get_installed_apps(): | ||
| return | ||
| if frappe.db.exists("Ask ALYF Skill", REPORT_SKILL_NAME): |
There was a problem hiding this comment.
The docstring says "skip when ... the skill was edited", but this skips whenever the skill exists, edited or not. Two effects: existing sites never get an improved REPORT_SKILL_DESCRIPTION, and a skill that an admin deleted on purpose comes back on every bench migrate. At least the docstring should match the behaviour.
| from frappe.desk.query_report import get_report_doc, get_script | ||
|
|
||
| report = get_report_doc(report_name) # checks permissions | ||
| script = get_script(report_name)["script"] or "" |
There was a problem hiding this comment.
Each run_report call runs get_report_filters again: get_report_doc twice (directly and inside get_script), JS render plus tokenizing (also of extended files), maybe fiscal year queries. Then run() loads the Report again. A normal "get filters, then run" turn parses the JS twice and loads the Report about 5 times. Not urgent, but only js_filters is needed for the run.
| } | ||
|
|
||
|
|
||
| def _cap_report_rows(payload): |
There was a problem hiding this comment.
Keeping only the first rows cuts off the totals row that many query reports (and add_totals_row) add at the end, e.g. Trial Balance or General Ledger. The agent may then add up the partial rows and report a wrong total. Maybe keep the last row when it is a total, or tell the agent that totals were cut off.
| @@ -0,0 +1,362 @@ | |||
| """Extract `filters` from `frappe.query_reports["X"] = {...}` JS files.""" | |||
There was a problem hiding this comment.
This is a new 362-line JS tokenizer and expression evaluator (dates, fiscal years, $.extend lookup) with no tests. The new tests cover only the Report Builder path, which does not use this parser. A few tests that parse real ERPNext report JS files (e.g. General Ledger, a financial statement) would catch regressions when ERPNext changes its JS.
2 participants
Adds a read-only
run_reportagent tool so Ask ALYF can execute Report queries with the same permission and filter rules as Desk.get_report_filters(tool) returns the declared filters for a specific report. The agent is prompted to call this beforerun_report.run_reporttakes a report name and optionalfilters. Query Report, Script Report, and Custom Report go throughfrappe.desk.query_report.run, withignore_prepared_reportalways on. With no filters,are_default_filtersstays true, so a custom report keeps its saved filters.run_reportthe Report Report Builder usesReport.get_datainstead. Unknown fieldnames are rejected. MultiSelectList values become["in", values]. A Link value is rejected unless the user can read or select that document. Results stop at 200 rows.truncatedis set when more rows existed.extract_report_filters, including objects pulled in with$.extendorObject.assign. Date defaults are resolved to concrete values. A Company link without a default uses the user default. A Fiscal Year default is filled (only when ERPNext is installed).omitted_filters. If it still fails, the tool returnserror,report_name,applied_filters, andallowed_filters, so the agent can handle the failed request.erpnext-report-filtersis a first pass. It is not fully checked and is meant to be adjusted in use. It is installed on install and migrate when ERPNext is present and the skill does not exist yet, for the Ask ALYF User role only. An existing skill is left unchanged. When the skill is available, the agent reads it before running an ERPNext report.Frappe core:
query_report.runtakes optionaljs_filtersand passes them tovalidate_filters_permissions. Ask ALYF uses that only for Query Report, Script Report, and Custom Report. Report Builder does not callrun. It checks Link permissions throughvalidate_filters_permissionson its own.resolves #135