Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form field automatically remapping after being unmapped #13744

Conversation

fedys
Copy link
Member

@fedys fedys commented May 13, 2024

Q A
Bug fix? (use the a.b branch) [✅]
New feature/enhancement? (use the a.x branch) [❌]
Deprecations? [❌]
BC breaks? (use the c.x branch) [❌]
Automated tests included? [✅]
Related user documentation PR URL
Related developer documentation PR URL
Issue(s) addressed

Description:

This PR fixes the following bugs:

  1. Icons for mapped form fields are not displayed in the form field listing.
Screen.Recording.2024-05-13.at.15.00.42.mov
  1. A non-mapped existing form field is auto-mapped when being edited.
Screen.Recording.2024-05-13.at.15.53.43.mov

Steps to test this PR:

  1. Icons for mapped form fields are not displayed in the form field listing.
    1. When adding a new mapped form field the icon indicating mapping should be visible right after clicking the "Add" button. See the first screen recording in the description.
    1. Create a form with at least one non-mapped field of type "Email" and click “Save & Close”.
    2. Go back to the form edit page and refresh your browser.
    3. Click the "Pencil" icon to update the form field you created at 1. step.
    4. Navigate to the "Mapped Field" tab. The field should not be mapped to a contact's email. See the second screen recording in the description.

Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.72%. Comparing base (4883fa0) to head (55c976d).

Current head 55c976d differs from pull request most recent head 47cb187

Please upload reports for the commit 47cb187 to get more accurate results.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13744      +/-   ##
============================================
+ Coverage     61.62%   61.72%   +0.10%     
+ Complexity    34145    34101      -44     
============================================
  Files          2245     2241       -4     
  Lines        102077   101937     -140     
============================================
+ Hits          62909    62925      +16     
+ Misses        39168    39012     -156     
Files Coverage Δ
app/bundles/FormBundle/Entity/Field.php 91.28% <100.00%> (+12.21%) ⬆️
app/bundles/FormBundle/Form/Type/FieldType.php 67.57% <100.00%> (+0.91%) ⬆️

... and 17 files with indirect coverage changes

@fedys fedys requested review from escopecz and kuzmany May 13, 2024 14:04
Copy link
Sponsor Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no issue in the code 👍 Thanks!

@escopecz escopecz added bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged forms Anything related to forms unforking Used for PRs in the Acquia's unforking initiative code-review-passed PRs which have passed code review labels May 13, 2024
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found another issue, do not investigate if it's related to code in the PR, but it's worth checking.
If the form returns an error, then the last added field disappears.

Zaznam.2024-05-13.202639.mp4

@fedys
Copy link
Member Author

fedys commented May 14, 2024

I found another issue, do not investigate if it's related to code in the PR, but it's worth checking. If the form returns an error, then the last added field disappears.

Zaznam.2024-05-13.202639.mp4

@kuzmany Thanks for point it out! This issue is reproducible in 5.x too.

@fedys fedys requested a review from kuzmany May 14, 2024 06:08
@kuzmany kuzmany added ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged and removed pending-test-confirmation PR's that require one test before they can be merged labels May 16, 2024
@escopecz escopecz added this to the 5.1.0 milestone May 17, 2024
@escopecz escopecz merged commit 52f097b into mautic:5.x May 17, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues or PR's relating to bugs code-review-passed PRs which have passed code review forms Anything related to forms ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged unforking Used for PRs in the Acquia's unforking initiative
Projects
Status: 🥳 Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants