specify / specify7

Specify 7
https://www.specifysoftware.org/products/specify-7/
GNU General Public License v2.0
66 stars 36 forks source link

`xml-editor`: View definitions that include `<definition>` cannot be rendered #4423

Open grantfitzsimmons opened 9 months ago

grantfitzsimmons commented 9 months ago

Describe the bug When a <definition> is present and linking to another view definition Specify is unable to display the form when making changes.

To Reproduce Steps to reproduce the behavior:

  1. Import the following form definition: form.xml.zip

  2. See that the OtherIdentifiers view definition points to the OtherIdentifiers_Grid view definition using the <definition> tag within the <viewdef>

  3. Notice that the preview of the form now cannot finish loading

<viewdef name="OtherIdentifiers" class="edu.ku.brc.specify.datamodel.OtherIdentifier" type="form" gettable="edu.ku.brc.af.ui.forms.DataGetterForObj" settable="edu.ku.brc.af.ui.forms.DataSetterForObj" useresourcelabels="true">
    <desc>subform on the Collection Object form.</desc>
    <definition>OtherIdentifiers_Grid</definition>
    <enableRules/>
    <columnDef>100px,2px,473px,5px,120px,2px,125px,0px,15px,p:g</columnDef>
    <columnDef os="lnx">115px,2px,410px,5px,85px,2px,198px,0px,15px,p:g</columnDef>
    <columnDef os="mac">130px,2px,530px,5px,105px,2px,218px,0px,15px,p:g</columnDef>
    <columnDef os="exp">p,2px,473px,5px:g,p,2px,125px,0px,15px,p:g</columnDef>
    <rowDef auto="true" cell="p" sep="2dlu"/>
    <rows>
        <row>
            <cell type="label" labelfor="2"/>
            <cell type="field" id="2" name="institution" uitype="text"/>
            <cell type="label" labelfor="1"/>
            <cell type="field" id="1" name="identifier" uitype="text"/>
        </row>
    </rows>
</viewdef>

https://crazyfish-xml-editor.test.specifysystems.org/specify/resources/view-set/8/OtherIdentifier/OtherIdentifiers

Expected behavior The form should be rendered, ignoring the referenced <definition>.

Screenshots

Notice that commenting out or removing this <definition> resolves the issue.

https://github.com/specify/specify7/assets/37256050/aeb98717-c154-4c29-95e2-1ec231813bf2

Arc, macOS

maxpatiiuk commented 9 months ago

I might be wrong, but your form is invalid

It should either include <definition>, or the form definition itself - it included both. (does not mean form editor shouldn't handle this case better)

The form editor currently assumes that <definition> is only present on the form table viewdefs that are referring to the form viewdef - which are the only cases I have seen in the https://github.com/specify/viewset-schema

@grantfitzsimmons can you clarify what is this viewdef trying to achieve, and is this just a mistake or a common use case we are supposed to handle?

grantfitzsimmons commented 9 months ago

@maxpatiiuk See this article for more information on it: https://discourse.specifysoftware.org/t/edit-the-form-grid-to-display-different-fields-than-the-subform/435

This is the guidance we've provided for users and I have found several forms with this configuration. This one was sent to me by CSIRO after following these instructions. Let me know if you believe this is misconfigured after reviewing this material and potentially evaluating this in Specify 6!

grantfitzsimmons commented 9 months ago

I do see what you mean after reviewing the material myself, but I have found this to be the configuration used by a number of users. I'll review this myself and get back to you, but thank you for your message!

maxpatiiuk commented 9 months ago

The steps described in that article look good as far as I can tell, and the forms in that article are correct.

General idea:

image

But the form you have in this issue is not correct - <viewdef> with type="form" is not supposed to have <definition> In fact, sp6 does not even look whether <definition> is present if <viewdef> has type="form" (if I read the source code correctly)

grantfitzsimmons commented 9 months ago

In that case, we should prevent Specify from reading the <definition> if the <viewdef> has type="form" since I've stumbled upon this a handful of times since we started working onxml-editor. Not sure this is worth including in 7.9.4 if we're focused entirely on more important bug fixes, but I imagine it will come up.

emenslin commented 2 months ago

Can recreate in edge (7.9.6)