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

FIX: render form fields have switch condition for theme #13779

Merged
merged 5 commits into from
Jun 6, 2024

Conversation

luk4s
Copy link
Contributor

@luk4s luk4s commented May 24, 2024

If conditional form is rendered, themes modified templates did not used. It seems because condition here is switched. In theory if for should be rendered without any theme, its must raise exception from my point of view.

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

Description:

Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)

If conditional form is rendered, theme modified templates did not used. It seems because condition here is in opposite
kuzmany
kuzmany previously approved these changes May 28, 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.

Good catch 👍 Thanks

@kuzmany kuzmany added bug Issues or PR's relating to bugs pending-test-confirmation PR's that require one test before they can be merged labels May 28, 2024
escopecz
escopecz previously approved these changes Jun 6, 2024
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.

Makes sense 👍

@escopecz escopecz added code-review-passed PRs which have passed code review forms Anything related to forms and removed pending-test-confirmation PR's that require one test before they can be merged labels Jun 6, 2024
@escopecz escopecz added this to the 5.1.0 milestone Jun 6, 2024
@escopecz escopecz added the ready-to-commit PR's with 2 successful tests, 1 approval, automated tests and docs and is ready to be merged label Jun 6, 2024
@escopecz escopecz self-assigned this Jun 6, 2024
@escopecz escopecz dismissed stale reviews from kuzmany and themself via a9cd85d June 6, 2024 17:15
@escopecz
Copy link
Sponsor Member

escopecz commented Jun 6, 2024

The tests were failing because the theme didn't have that twig template. I fixed it with some twig magic

Copy link

codecov bot commented Jun 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.17%. Comparing base (5bf1b10) to head (279f35e).

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##                5.x   #13779      +/-   ##
============================================
- Coverage     62.17%   62.17%   -0.01%     
  Complexity    34211    34211              
============================================
  Files          2249     2249              
  Lines        102309   102309              
============================================
- Hits          63608    63607       -1     
- Misses        38701    38702       +1     

see 1 file with indirect coverage changes

@escopecz escopecz merged commit e8d5b6c into mautic:5.x Jun 6, 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants