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

Check for form attribute before rendering role matrix #1553

Merged
merged 1 commit into from
Jul 19, 2022

Conversation

wluijt-endertech
Copy link
Contributor

@wluijt-endertech wluijt-endertech commented Jul 14, 2022

Subject

I am targeting this branch, because it appears that a recent change in the same branch might need additional logic.

Closes #1552.

Changelog

### Fixed
- Fix user edit page to check for a form attribute before rendering the role matrix.

@@ -9,7 +9,7 @@ file that was distributed with this source code.

#}
{% for role, attributes in roles|sort %}
<li>{{ form_widget(attributes.form, {label: attributes.role_translated, value: attributes.role}) }}</li>
<li>{{ attributes.form is defined ? form_widget(attributes.form, {label: attributes.role_translated, value: attributes.role}) }}</li>
Copy link
Member

Choose a reason for hiding this comment

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

Shouldnt the whole li be hidden if the form is not defined ?

(Also are you able to add a functionnal test for this bug ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the code to surround the <li> element. I think it will take me some time to properly setup for creating tests. I don't know if you should wait on me for that though.

Copy link
Member

Choose a reason for hiding this comment

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

I wont be able to patch this before sunday/monday anyway since I currently doesnt have a computer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries. I was able to add an initial functional test.

VincentLanglet
VincentLanglet previously approved these changes Jul 15, 2022
@VincentLanglet
Copy link
Member

@wluijt-endertech Can you take a look at the psalm and phpstan failures.
I assume you have to ignore some error (with the baseline and a comment, or with a phpstan-ignore, psalm-suppress) because you added compatiblity with old symfony version and tools only checks with the latest.

@wluijt-endertech
Copy link
Contributor Author

I added the phpstan-ignore and psalm-suppress comments.

@@ -9,7 +9,7 @@ file that was distributed with this source code.

#}
{% for role, attributes in roles|sort %}
<li>{{ form_widget(attributes.form, {label: attributes.role_translated, value: attributes.role}) }}</li>
{% if attributes.form is defined %}<li>{{ form_widget(attributes.form, {label: attributes.role_translated, value: attributes.role}) }}</li>{% endif %}
{% if not attributes.is_granted %}
Copy link
Member

Choose a reason for hiding this comment

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

do this if need to be updated too? what happens If I exclude a role for which I have is_granted = false. It will try to print a script that will fail because there is no form, right?

Maybe something like:

    {% if attributes.form is defined %}
        <li>{{ form_widget(attributes.form, {label: attributes.role_translated, value: attributes.role}) }}</li>
        {% if not attributes.is_granted %}
            <script>
                $('input[value="{{ role }}"]').iCheck('disable');
                $('form').on('submit', function() {
                    $('input[value="{{ role }}"]').iCheck('enable');
                });
            </script>
        {% endif %}
    {% endif %}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the template to surround the content with the form attribute check (with a minimal change, though).

@jordisala1991 jordisala1991 merged commit aca99a3 into sonata-project:5.x Jul 19, 2022
@jordisala1991
Copy link
Member

Thanks @wluijt-endertech

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants