fix(ui): keep single-option preselect off Frappe reserved fields - #110
fix(ui): keep single-option preselect off Frappe reserved fields#110Omprakash-48 wants to merge 1 commit into
Conversation
The Select/Link single-option preselect fired for every field, including
the ones Frappe owns. Verified against frappe/develop rather than assumed:
- amended_from Link -> self, read_only:1, NOT hidden (renders)
- auto_repeat Link -> Auto Repeat, read_only:1, NOT hidden (renders)
- parent_<x> Link -> self, neither hidden nor read-only
- old_parent Link -> self, hidden:1 (never reaches a widget)
- naming_series Select, assigned server-side at insert
- name/owner/modified_by are std_fields pseudo-docfields; Frappe's
`restricted` tuple refuses them as real DocFields
parent_<x> is the dangerous one: editable and visible on every is_tree
DocType, so a tree with one existing node silently acquired a parent the
user never picked. naming_series matters too -- get_default_naming_series
returns the first TRUTHY option and notes "Empty strings are used to avoid
populating forms by default", so the server, not the client, picks it.
The nestedset parent fieldname is frappe.scrub(f"Parent {self.name}").
frappe.scrub is only .replace(" ","_").replace("-","_").lower(), which is
WEAKER than normalizeDoctypeTableName -- that one also collapses
apostrophes, so reusing it would mispredict parent_item's_group as
parent_item_s_group and miss the field. Ported separately as frappeScrub.
Also fixes three preselect defects found alongside:
- A single-option multi-select could not be unchecked. The preselect
lived in build(), so the clear (which emits '') re-fired it on the next
frame. Now gated on value == null, distinguishing "never set" from
"explicitly cleared". With reqd:1 this had produced a required error
while _formData still held the value, and formValues.addAll(_formData)
let that win on save.
- Preselect ignored readOnly/enabled, which is what actually reaches
amended_from and auto_repeat.
- Duplicate options crashed FormBuilderDropdown ("There should be exactly
one item with [DropdownButton]'s value") and defeated preselect by
making the count 2. Both Select options and LinkField's host-supplied
static list are now deduped, order-preserving.
allowPreselect defaults to true so a host constructing these widgets
directly is unaffected. FieldFactory carries `doctype` as a mutable
instance field rather than a createField parameter, per the extension
contract documented on the class -- the override compat test still passes.
Not addressed here: the dirty-on-open baseline is only partly improved.
That is a dirty-baseline issue, not a preselect one (a Table MultiSelect
field or any defaultValue dirties an untouched doc the same way), so it
needs its own design call.
Tests: 40 new across 6 files; 2453/2453 pass, analyze and format clean.
deepak-dhwani
left a comment
There was a problem hiding this comment.
PR 110 — b617791 · The code is right. Two documentation items before merge.
Thanks for splitting it — and it split perfectly: same SHA, no cherry-pick, no conflict, because #107
merged first and this sits directly on 1ee7dfa.
Because the SHA is unchanged, everything I verified in the #107 thread applies here without re-checking.
Recording it in this PR so it lives where the change does.
Verified
The Frappe research holds — all eight claims, checked against frappe/develop independently.
| Claim | Source | |
|---|---|---|
scrub = .replace(" ","_").replace("-","_").lower() |
utils/data.py:3126 |
✅ |
restricted contains name, owner, modified_by |
doctype.py:480-492 — owner at :484 |
✅ |
name/owner/modified_by are std_fields Links |
model/__init__.py:142-158 |
✅ |
amended_from — Link → self, read_only:1, not hidden |
doctype.py:963-975 |
✅ |
auto_repeat — Link → Auto Repeat, read_only:1, not hidden |
doctype.py:984-993 |
✅ |
old_parent — Link → self, hidden:1 |
add_nestedset_fields |
✅ |
parent field = frappe.scrub(f"Parent {self.name}"), neither flag |
add_nestedset_fields — literally parent_field_label = f"Parent {self.name}" |
✅ |
get_default_naming_series returns the first truthy option |
naming.py:501-509, with the comment you quote verbatim |
✅ |
I went looking for the one I expected to be wrong — owner in the restricted tuple — and it is there.
The guard set has no holes. Frappe's other reserved names (parent, parentfield, parenttype,
creation, modified, docstatus, idx) are none of them Link or Select in std_fields, so none can
reach a preselect. Complete for the fieldtypes the guard covers.
frappeScrub is justified. table_name.dart:24-27 collapses [^a-z0-9]+, so reusing it really
would mispredict. Probed: isFrappeReservedField("parent_item's_group", doctype: "Item's Group") → true;
isFrappeReservedField("parent_company") → false, so an ordinary user Link on a non-tree DocType is
untouched.
parent_<x> being the dangerous one is the right call — the only one of the seven that is both editable
and visible, so the only one where a user could ship a wrong value without ever seeing a field.
doctype as instance state is the createField contract applied correctly, and assigning it
unconditionally in _configureFieldFactoryForMeta is right for the reason established in #107:
meta-derived values are unconditional, host choices are guarded. You drew that line correctly without
being asked.
I tested the merge result, not just the branch. Worth flagging: this PR's parent is 1ee7dfa, not
develop's tip, so develop..pr110 is one commit but pr110..develop is seven — the CI run on this branch
tested a tree that is not what lands. I merged locally and ran it:
flutter analyze → clean for lib/ (2 pre-existing, neither yours)
flutter test → 2467 passed, 3 skipped, exit 0
Clean. The 3 skips are pre-existing from develop (3905592, in
form_builder_reactive_fetchfrom_link_test.dart) and this PR does not touch that file. Still worth
rebasing onto develop so CI exercises the tree that actually merges.
🟡 M1 — the value == null gate does more than the description says
Carried unchanged from #107, since the commit is unchanged. This is a documentation fix, not a code
one — I am not asking you to change the behaviour.
The description presents value == null as the multi-select uncheck fix. It gates both branches, and
what it actually distinguishes is absent key from empty string — not "cleared" from "never touched".
Those coincide only if '' can only arrive from a clear, and it cannot: Frappe stores an unset Select
as varchar NOT NULL DEFAULT '' and returns "", the pull writes it verbatim, and
form_builder.dart:560 does _formData.addAll(widget.initialData ?? {}) with no normalisation between.
Probed against the widget:
value = null → emitted [Only] ← new document, key absent
value = '' → emitted [] ← pulled document, single-select
value = '' → emitted [] ← pulled document, multi-select
So the effective rule is now preselect fires only for a key absent from form data — in practice, only
on documents this device created. A pulled record with a one-option Select stays empty, and if it is
reqd the user must pick the only choice by hand before the form will save.
The behaviour is right. Auto-filling a synced record writes a value the user never chose and dirties
a document on open — the same category as the bug this PR fixes, and your own note about the dirty
baseline says you see it.
And I withdraw the alternative I offered in the #107 thread. I suggested gating on
!formData.containsKey(fieldname) if you wanted the narrow version. Develop now contains 843b86b
"seed std eval fields on a non-null value, not key presence", whose whole point is that keying on
containsKey is the trap — an explicit {'docstatus': null} is "how an unsaved doc is routinely
assembled". value == null is not just acceptable, it matches the convention this repo just fixed a
data-loss bug to establish. Keep it.
What is missing is only the naming. The single thing pinning this population is a test titled
"an explicitly cleared value is not re-preselected on mount", which describes one of its two callers —
and the more common one, a pulled document, appears nowhere in the commit message, the test suite or the
PR description. A field team notices within a day of rollout that a form stopped filling something in.
Rename the test to cover the pulled case, and say it in the description.
🟡 M2 — no CHANGELOG entry at all
b617791 does not touch CHANGELOG.md, and grep -in "preselect\|naming_series\|amended_from\|nestedset"
over the file returns nothing. That is conspicuous next to #107, where every fix got a paragraph — and
this is the change with the widest blast radius of anything merged this week: single-option preselect
for Select and Link, on every DocType, in every deployment.
It needs an entry covering all four parts, with the fourth marked as a behaviour change rather than a fix:
- the reserved-field guard (fix),
- the
readOnly/enabledgate (fix), - duplicate-option dedup (fix — it was a crash),
- the population change (behaviour, per M1).
Verdict
The code needs no changes and I would merge it as it stands. I am holding only on the two documentation
items, because both are the kind that never get written after a merge, and because #4 above changes what
users see in every deployment while currently appearing in no changelog and no test title.
Both are small. Rename one test, add one CHANGELOG entry, add a paragraph to the description — and rebase
onto develop while you are there so CI tests the tree that lands. Re-approve on sight.
The Select/Link single-option preselect fired for every field, including the ones Frappe owns. Verified against frappe/develop rather than assumed:
restrictedtuple refuses them as real DocFieldsparent_ is the dangerous one: editable and visible on every is_tree DocType, so a tree with one existing node silently acquired a parent the user never picked. naming_series matters too -- get_default_naming_series returns the first TRUTHY option and notes "Empty strings are used to avoid populating forms by default", so the server, not the client, picks it.
The nestedset parent fieldname is frappe.scrub(f"Parent {self.name}"). frappe.scrub is only .replace(" ","").replace("-","").lower(), which is WEAKER than normalizeDoctypeTableName -- that one also collapses apostrophes, so reusing it would mispredict parent_item's_group as parent_item_s_group and miss the field. Ported separately as frappeScrub.
Also fixes three preselect defects found alongside:
allowPreselect defaults to true so a host constructing these widgets directly is unaffected. FieldFactory carries
doctypeas a mutable instance field rather than a createField parameter, per the extension contract documented on the class -- the override compat test still passes.Not addressed here: the dirty-on-open baseline is only partly improved. That is a dirty-baseline issue, not a preselect one (a Table MultiSelect field or any defaultValue dirties an untouched doc the same way), so it needs its own design call.
Tests: 40 new across 6 files; 2453/2453 pass, analyze and format clean.