fix(#261): empty date range suggestion handling (cherry-pick + tests)#322
Merged
Conversation
…d out when there is no date range found in the dataset
The cherry-picked fix from Minhajuddin (commit 62c18ea, parent of this commit on this branch) addresses the "Automatic Empty Date Range handling" bug at two layers: 1. ``FormulaProcessor.process_formulae`` coerces Jinja2's string ``"None"`` / ``""`` outputs back to actual Python ``None`` so ``dpp_suggestions`` JSON serializes a real ``null`` instead of the literal string ``"None"``. 2. ``scheming-suggestions.js`` + ``suggestions.css`` greys out the per-field suggestion button and short-circuits its click handler when the suggestion value is ``null`` / ``undefined`` / ``"None"`` / ``""``. This commit adds 4 pytest regression tests for the Python side — the layer most likely to silently regress in a future refactor (e.g. someone tightening the formula pipeline and dropping the coercion as "unnecessary string handling"). The JS side is empirically validated in the issue thread (screenshot of greyed-out icon); a future vitest spec for the legacy ``scheming-suggestions.js`` module could lock that down separately (out of scope here — there's no JS test coverage for the legacy DRUF suggestions UI yet). Tests pin: * ``{{ none }}`` template → Python ``None`` in the output dict (the #261 reporter's exact failure mode — date-related suggest_formula branches on ``dpp.NO_DATE_FIELDS`` and returns ``none`` when the dataset has no date columns). * Empty / undefined-variable / empty-conditional Jinja2 output → Python ``None``. * Real non-empty values pass through unchanged (negative control — the coercion must not swallow legitimate formula outputs). * Even an explicit ``"None"`` string literal in the formula coerces to Python ``None`` per the fix as written. Documents the choice as deliberate — operators emitting the literal text "None" gets treated as a sentinel because the front-end relies on it. The tests bypass ``FormulaProcessor.__init__`` (which does heavy lat/lon/date inference + pulls in CKAN config) by constructing the object via ``__new__`` and setting attributes directly — same pattern as ``test_security.test_formula_processor_uses_sandboxed_environment``. Full unit suite: 245 → 249 passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Roborev's review of the cherry-picked commit flagged 5 LOW findings.
Three applied here, two left as-is with documented rationale.
**Applied (3):**
* ``jinja2_helpers.py:216`` — strip whitespace before the empty/None
check. The prior ``rendered_formula == ""`` missed common cases
like ``" "`` and ``"\n"`` (Jinja2 ``{% if %}`` blocks without
``-`` trim markers render to whitespace, not empty string).
Switched to ``rendered_formula.strip() in ("", "None")``. New
regression test ``test_whitespace_only_template_render_is_coerced_to_python_none``
pins the three sub-cases (publisher with stray newlines, three
spaces, mixed ``\n\t ``).
* ``scheming-suggestions.js:149`` — strip stale state classes
(``suggestion-btn-error``, ``suggestion-btn-ready``) before
applying ``suggestion-btn-disabled``. Prevents mixed visual state
on re-renders (polling updates, edit-then-re-render flows).
* ``scheming-suggestions.js:149`` (same line, different concern) —
added a code comment marking the ``=== 'None'`` arm as a
backwards-compat guard for resources ingested BEFORE the #261
Python fix landed (those still have the literal string ``"None"``
stored in ``dpp_suggestions`` from when ``str(None)`` passed
through verbatim). Safe to drop once cached/legacy data has been
re-ingested.
**Skipped with rationale (2):**
* ``jinja2_helpers.py:216`` — string literal ``"None"`` in formula
output coerces to Python ``None``, swallowing legitimate "None"
values (e.g. license fields). Documented in an expanded code
comment as a **deliberate** trade-off: the front-end relies on
``null`` (not ``"None"``) to distinguish "no suggestion" from a
real value, and the cost (operators picking ``"none"`` /
``"N/A"`` for the literal-text case) is much smaller than the
alternative (every scheming YAML adding ``| my_filter`` wrappers).
Existing test ``test_string_literal_none_in_formula_output_is_coerced``
already pins this as intentional.
* ``scheming-suggestions.js:516`` — double guard (``prop('disabled')``
+ click handler ``hasClass`` check) is redundant per roborev. Kept
as defense-in-depth: jQuery event delegation can sometimes
bypass the native ``disabled`` attribute, and the cost of two
short checks is negligible vs the cost of a button accidentally
becoming clickable.
Full unit suite: 249 → 250 passing.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #261 where empty/None-valued suggestion formula renders were being stored as the literal string "None" (or empty strings) in dpp_suggestions, causing the UI to display clickable “None”/blank suggestions.
Changes:
- Add backend coercion in
FormulaProcessor.process_formulae()to convert rendered""/"None"(including whitespace-only) into PythonNoneso JSON serializesnull. - Update the scheming suggestions frontend to disable suggestion buttons when suggestion values are empty/
None. - Add a new pytest regression suite covering backend coercion behavior for #261.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
ckanext/datapusher_plus/jinja2_helpers.py |
Coerces rendered empty/None suggestion outputs to Python None. |
ckanext/datapusher_plus/assets/js/scheming-suggestions.js |
Disables suggestion buttons when suggestion data is missing/empty. |
ckanext/datapusher_plus/assets/styles/suggestions.css |
Adds styling for disabled suggestion buttons. |
tests/test_issue_261_empty_date_range.py |
Adds regression tests to pin backend coercion behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…tness)
Six findings, all applied. Two were substantive correctness fixes, the
rest were polish — but the test-suite ``suggest_formula`` finding was
particularly valuable: it exposed that my regression tests had been
exercising a phantom schema key (production uses
``suggestion_formula``, not ``suggest_formula``).
**Python — gated coercion (HIGHEST IMPACT):**
* ``jinja2_helpers.py`` — ``process_formulae`` is called four times in
``jobs/stages/formula.py``: twice with ``formula_type="formula"``
(lines 290, 321) for direct field updates that write into the
package/resource dict, and twice with ``"suggestion_formula"``
(lines 355, 398) for dpp_suggestions. The empty/None coercion now
gates on the suggestion path only — direct ``formula`` outputs
preserve their rendered value verbatim. Coercing ``""`` → ``None``
on the direct path would have changed patch semantics for CKAN
fields where validators distinguish empty string from null.
* ``jinja2_helpers.py`` — stripped trailing whitespace from the blank
line introduced by the cherry-pick.
**JS — symmetric handling + whitespace:**
* ``scheming-suggestions.js`` — added explicit
``removeClass('suggestion-btn-disabled') + prop('disabled', false)``
on the non-disabled branch so a button that transitions from null
→ real-suggestion across re-renders (polling, edit flows) gets
re-enabled. Without this, once a field was disabled it stayed
disabled even when a later pass had a real suggestion.
* ``scheming-suggestions.js`` — whitespace-only suggestion strings
(``" "``, ``"\n"``) now treated as no-suggestion via
``typeof === 'string' && trim() === ''``. Mirrors the Python-side
``.strip()`` from roborev #2289 — legacy data and templates without
trim markers are now covered on both sides.
**Tests — phantom key + drift + new gating test:**
* ``test_issue_261_empty_date_range.py`` — ``SUGGEST_FORMULA``
constant was ``"suggest_formula"``, but the production schema key
(verified in ``dataset-druf.yaml``, ``docs/dataset_schema.yaml``,
and the four ``jobs/stages/formula.py`` call sites) is
``"suggestion_formula"``. My tests passed because
``process_formulae`` reads whatever key is in the YAML without
validating the name — the coverage was real but misleading. Fixed
with a comment explaining the catch.
* ``test_issue_261_empty_date_range.py`` — module docstring claimed
"three properties pinned" but the file now covers 6. Listed them
all explicitly.
* ``test_issue_261_empty_date_range.py`` — new
``test_coercion_is_gated_on_suggestion_formula_type`` pins the
asymmetry: identical ``{{ none }}`` template gives ``"None"`` on
the direct ``formula`` path and ``None`` on the
``suggestion_formula`` path. Locks the Copilot-caught fix in
against future refactors that might broaden the coercion again.
Full unit suite: 250 → 251 passing (no regressions).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #261. Cross-referenced from TNRIS/texaswaterhub_CKAN#1035.
Summary
The "Automatic Empty Date Range handling" bug fix from Minhajuddin (commit `62c18ea`, 2025-11-21) was authored on a branch that never reached `main`. This PR:
What was broken
A scheming `suggest_formula` like `{{ dpp.DATE_FIELDS[0] if not dpp.NO_DATE_FIELDS else none }}` rendered `None` (Python) → `"None"` (Jinja2 string output). That string then got stored in the `dpp_suggestions` JSON, and the frontend rendered it as a clickable "Suggestion" that — when clicked — inserted the literal text `"None"` into the metadata field. Reporter screenshot showed the broken state.
What the fix does (two layers)
Why a follow-up commit with tests
The cherry-picked commit covers the fix. The follow-up commit (`c1b1035`) adds 4 Python regression tests that pin the Python coercion specifically — the layer most likely to silently regress (e.g. someone tightening the formula pipeline and dropping the coercion as "unnecessary string handling"). The JS side is empirically validated in the issue thread (Minhajuddin's screenshot of the greyed-out icon); locking it down with a vitest spec would mean introducing a brand-new test target for the legacy `scheming-suggestions.js` module (no JS coverage exists for it today). Out of scope here, would belong in a follow-up.
Test plan
🤖 Generated with Claude Code