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: Empty all placeholder menu #7403

Merged
merged 2 commits into from
Sep 28, 2022
Merged

fix: Empty all placeholder menu #7403

merged 2 commits into from
Sep 28, 2022

Conversation

fsbraun
Copy link
Sponsor Member

@fsbraun fsbraun commented Sep 26, 2022

Description

The empty placeholder menu currently does not work (#7389). The url is not rendered into the template.

This PR corrects the template and uses the url template tag to retrieve the admin url for "Empty all".

empty-all

Checklist

  • I have opened this pull request against develop-4
  • I have added or modified the tests when changing logic
  • I have followed the conventional commits guidelines to add meaningful information into the changelog
  • I have read the contribution guidelines and I have joined #workgroup-pr-review on Slack to find a “pr review buddy” who is going to review my pull request.

@fsbraun fsbraun added the 4.1 label Sep 26, 2022
Copy link
Contributor

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

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

The change looks good, although I would question why no tests failed for this issue. Maybe it has no coverage. Here is an example of a test that checks the plugin menu, hopefully this will be super easy to implement:

def test_get_placeholder_plugin_menu(self):

I think when any bugs like this are found and no tests failed, we should implement a test to ensure it doesn't happen again. Paulo would regularly state that he had little confidence in the cms test suite, due to that we have tried to add confidence in the test suite early on in v4.

@fsbraun
Copy link
Sponsor Member Author

fsbraun commented Sep 27, 2022

I see your point. This was not detected since {{ }} expansions in templates fail silently. So what we would need to do is to find all those expansions and generate a test suite for those. Luckily {% %} template tags fail loudly and with this particular item I guess we have a good chance of being fine.
I am actually thinking of a linter to identify those variable expansions. (There's a longstanding bug #7188 where page ids are put into an url in localized form rendering the page tree unusable in localizations with thousands separator - that's probably something to port to v4, too.)
Will look into other variable expansions at risk and see what tests make sense.

@fsbraun
Copy link
Sponsor Member Author

fsbraun commented Sep 27, 2022

@Aiky30 It turns out there was a test for the extended menu items. The new test also includes the standard items. (Copy, Paste, Empty all) Other critical template elements ({{ object.property }}) can be found in these files:

(.venv) $ find cms -name "*.html" -print -exec egrep 'href="{{\s*[a-zA-Z0-9_.]+\.[a-zA-Z0-9_]+\s*}}"' {} \;
cms/templates/cms/toolbar/toolbar_with_structure.html
    <p>{% trans "It is being redirected to:" %} <a href="{{ cms_toolbar.redirect_url }}">{{ cms_toolbar.redirect_url }}</a></p>
cms/templates/cms/toolbar/items/live_draft.html
    href="{{ cms_toolbar.get_object_preview_url }}">
    href="{{ cms_toolbar.get_object_edit_url }}">
cms/templates/cms/toolbar/dragitem_extra_menu.html
    <div class="cms-submenu-item"><a href="{{ item.url }}" data-rel="{{ item.action }}"{% if item.data %} data-post="{{ item.data }}"{% endif %}{% if question %} data-text="{{ question }}"{% endif %} data-on-success="REFRESH_PAGE" {% for key, attr in item.attributes.items %}data-{{ key }}="{{ attr }}" {% endfor %}>{{ item.name }}</a></div>
cms/templates/cms/toolbar/dragitem_menu.html
    <div class="cms-submenu-item"><a data-rel="add" href="{{ p.value }}">{{ p.name }}</a></div>

Afaics these are covered with tests.

Copy link
Contributor

@Aiky30 Aiky30 left a comment

Choose a reason for hiding this comment

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

Awesome, and just like that, the world is a better place for all. :-)

@fsbraun
Copy link
Sponsor Member Author

fsbraun commented Sep 27, 2022

Wait: The first two are tested but do not detect missing links. I'll add two more tests.

@fsbraun
Copy link
Sponsor Member Author

fsbraun commented Sep 27, 2022

@Aiky30 Do you know if cms/templates/toolbar/items/live_draft.html is used at all in v4? I believe the edit, preview and publish buttons are generated programmatically in python.

Same with cms/templates/cms/toolbar/toolbar_with_structure.html (which is in use, obviously): it seems that the cms_toolbar.redirect_url property is never used. Hence you never get the message "This page redirects to ...". I cannot go to edit a language that does not have a page content, correct? Hence the last part is not rendered (if not a custom toolbar causes it to be rendered):

    {% if cms_toolbar.redirect_url %}
        <div class="cms-screenblock">
            <div class="cms-screenblock-inner">
                <h1>{% trans "This page has no preview!" %}</h1>
                <p>{% trans "It is being redirected to:" %} <a href="{{ cms_toolbar.redirect_url }}">{{ cms_toolbar.redirect_url }}</a></p>
            </div>
        </div>
    {% endif %}

@Aiky30
Copy link
Contributor

Aiky30 commented Sep 27, 2022

@Aiky30 Do you know if cms/templates/toolbar/items/live_draft.html is used at all in v4? I believe the edit, preview and publish buttons are generated programmatically in python.

Same with cms/templates/cms/toolbar/toolbar_with_structure.html (which is in use, obviously): it seems that the cms_toolbar.redirect_url property is never used. Hence you never get the message "This page redirects to ...". I cannot go to edit a language that does not have a page content, correct? Hence the last part is not rendered (if not a custom toolbar causes it to be rendered):

    {% if cms_toolbar.redirect_url %}
        <div class="cms-screenblock">
            <div class="cms-screenblock-inner">
                <h1>{% trans "This page has no preview!" %}</h1>
                <p>{% trans "It is being redirected to:" %} <a href="{{ cms_toolbar.redirect_url }}">{{ cms_toolbar.redirect_url }}</a></p>
            </div>
        </div>
    {% endif %}

You could possibly get this message if you have content in one language but you've not yet created content in another.

cms/templates/toolbar/items/live_draft.html is possibly not used, although it could still be used and the name wasn't updated ...

@fsbraun
Copy link
Sponsor Member Author

fsbraun commented Sep 27, 2022

You could possibly get this message if you have content in one language but you've not yet created content in another.

Is there an endpoint for that? It seems that the page content object needs to be created (and thereby the language) to edit it. If you can give me an edit endpoint for a non-existing language I can add a test for the message.

OK, it it could be used we better keep the live_draft.html template. There are no tests for it (and I see no way of testing it if it is not rendered at some point 😉).

@fsbraun fsbraun merged commit edeb03a into develop-4 Sep 28, 2022
@fsbraun fsbraun self-assigned this Oct 5, 2022
@fsbraun fsbraun deleted the fix/empty-all branch October 5, 2022 13:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants