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

Removed a layer of app manager template hierarchy #35664

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

orangejenny
Copy link
Contributor

Technical Summary

This is the template hierarchy in app manager that descends from apps_base.html, which is the template that sets up the left-hand menu of the app's modules & forms (and various other UI infrastructure):

apps_base.html
    no_longer_supported.html
    import_app.html
    managed_app.html
        form_designer.html
        app_view.html
            app_view_settings.html
            app_view_release_manager.html
        module_view.html
            module_view_advanced.html
        module_view_report.html
        form_view.html

This PR removes no_longer_supported.html and import_app.html from this tree, since they're simple templates that don't need the app menu. That makes managed_app.html the only child of apps_base.html, so then I combine those into a single template.

For fun, I also migrated the import app page to webpack.

Safety Assurance

Safety story

These changes are fairly mehanical. The main risk is making app manager pages 500. I've smoke tested the various pages - releases, app settings, module, advanced module, report module, form settings, form edit - to verify they still load.

Automated test coverage

There are some view level tests in app manager. I doubt this change is fully covered.

QA Plan

Not requesting QA.

Rollback instructions

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@orangejenny orangejenny added the product/invisible Change has no end-user visible impact label Jan 22, 2025
<h1>This app is no longer supported</h1>
<p>If you believe this to be in error, please contact support.</p>
{% block page_content %}
{% blocktrans %}
Copy link
Member

Choose a reason for hiding this comment

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

nit: i generally prefer to blocktrans INSIDE of block tags like <p> and <h1> tags, as that relies on the translator to get those tags (and closing tags) correct in the translated text...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense, fixed in e9065ef

{% initial_page_data 'export_json' app.export_json %}
<form action="{% url "import_app" domain %}" id="app-import-form" method="post" enctype="multipart/form-data">
{% csrf_token %}
{% if app %}
<p>Import application <strong>{{ app.name }}</strong> from domain <strong>{{ app.domain }}</strong>?</p>
{% blocktrans %}
Copy link
Member

@biyeun biyeun Jan 23, 2025

Choose a reason for hiding this comment

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

my nit of blocktrans inside of <p> still applies. additionally, if i remember correctly blocktrans doesn't play well with app.name type variables (e.g. properties of variables). you generally need to do something like:

<p>
  {% blocktrans with app_name=app.name app_domain=app.domain %}
    Import application <strong>{{ app_name }}</strong> from domain <strong>{{ app_domain }}</strong>?
  {% endblocktrans %}
</p>

or those values will actually show up blank when rendered (regardless of translation). resolving this is a blocker for a ✅

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Huh, that's unfortunate if values get blanked out but there's no error. Updated in e9065ef

but one example would be adding a new page to app manager that inherits
from ``managed_app.html``.
from ``apps_base.html.html``.
Copy link
Member

Choose a reason for hiding this comment

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

typo... i think this should be apps_base.html rather than apps_base.html.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks - e9065ef

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/invisible Change has no end-user visible impact
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants