Overview

In #3487284: [META] Add explicit end-to-end test coverage for using all core field widgets including multiple cardinalities in the "Page Data" (content entity) form we're adding e2e tests for core widgets.

Proposed resolution

Add explicit e2e coverage to entity-form-field-types-test.cy.js for multi-value textfield widget

User interface changes

Issue fork experience_builder-3517868

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » larowlan

larowlan opened merge request !875

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

We're getting fatals from the backend on adding a third item, so this is catching an actual regression.

Will dig into it tomorrow

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Active » Needs review

Got to the bottom of this, the code we added for the media library to keep track of form state cache didn't work if the triggering element in an ajax callback had limit validation errors.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » bnjmnm
Category: Task » Bug report
Priority: Normal » Critical

Bumping to critical bug because I marked #3519123: Page Data Form + Options as Buttons error on AJAX submits as a duplicate
Assigning to @bnjmnm to review

bnjmnm made their first commit to this issue’s fork.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

🤪🤯 Wow wow wow @ ClientDataToEntityConverter::setEntityFields(), but given the detailed (and essential!) comments left by @larowlan on there, I feel zero need to step through all that again to verify it. This looks ready to me.

Unfortunately the widget-multivalue.cy.js e2e test failed with

      cy:command ✘  assert	expected **<input#edit-field-xbt-unlimited-text-0-value--5Ex2yiBamp4.rt-reset.rt-TextFieldInput.js-text-full.text-full.form-text>** to have value **item five**, but the value was **item one**

Retesting… 🤞

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Reviewed & tested by the community » Needs work

I'm running into the same test failure locally , which is an existing test that also deals with the multi-value textfield widget. It failing on the portion of the test where the items in the multivalue field are reordered. The dragging of the lower item to the top no longer seems to register.

This still works manually, though, so hopefully it's just a matter of tweaking widget-multivalue.cy.js a bit. Unassigning myself for now.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: Unassigned » larowlan

This still works manually, though, so hopefully it's just a matter of tweaking widget-multivalue.cy.js a bit. Unassigning myself for now.

I had a thought about this while I was on leave - I think the issue is the realDnd code we have is confused by the fact the field is in a fixed height scrollable pane. When the test was written we had a small number of fields but as we've expanded things it has now grown. I'll see if making the weight of this field lighter so that is shows in the viewport without scrolling makes a difference.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#3521641: AJAX race condition: implement form cache locking protection between the ApiLayoutController and the EntityFormController

Added a related issue for locking

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Assigned: larowlan » Unassigned
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Related issues: +#3514900: EntityFormController should load auto-save state if it exists

Postponed #3514900: EntityFormController should load auto-save state if it exists on this as it is very obvious once this is in (you lose data)

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: Unassigned » bnjmnm
Status: Needs review » Reviewed & tested by the community

I'm really curious to see if this will also make the multi-value widget for the new sparkline SDC that #3467870: Support `{type: array, …}` prop shapes introduced work! 🤓

But: out of scope here, assigning to @bnjmnm for final sign-off!

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Assigned: bnjmnm » Unassigned
Status: Reviewed & tested by the community » Needs work

I spotted one small thing mentioned in an MR comment that I think I can just remove, but I wanted to confirm with the original author. If it's as simple as that, removing and re-rtbcing seems fine to me and I can commit.

  • bnjmnm committed 9f70cd14 on 0.x authored by larowlan 
    Issue #3517868 by larowlan, bnjmnm, wim leers: Add e2e tests for multi-...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs work » Fixed

Merged!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.