potiuk commented on PR #70243:
URL: https://github.com/apache/airflow/pull/70243#issuecomment-5151796788
The feature makes sense — having to bounce back to Show User just to reset a
password is a genuine annoyance — and I checked the part that matters most
here: there is no permission bypass. `lib.render_action_links` applies its own
filter as the first thing it does:
```jinja
{% macro render_action_links(actions, pk, modelview_name) %}
{% set actions = actions | get_actions_on_show(modelview_name) %}
```
so injecting the action into `template_args` cannot surface a button the
user is not entitled to, and the action endpoint is separately protected. Your
tests cover both directions, which is the right shape for this.
I'd like to suggest a different approach to the template, though, because
copying `form_vertical.html` brings a problem with it.
`render_action_links` emits its own `<form id="action_form" ...>` (see
`appbuilder/general/lib.html`). In the copied template the call sits inside the
model `<form class="form-vertical">`, so the rendered page has a form nested
inside another form. That is invalid HTML — browsers drop the inner element —
so the button may work only incidentally, and could stop working on a browser
or FAB version that handles it differently. FAB's own `show.html` does not hit
this because the Show view has no surrounding form, so the pattern does not
carry over as directly as it looks.
You are right that `form_vertical.html` cannot be extended — it defines no
blocks, which I assume is exactly why you copied it. But the *page* template
can be: `appbuilder/general/model/edit.html` wraps the widget in `{% block
edit_form %}`. Overriding that block puts the actions after the form rather
than inside it:
```jinja
{% extends "appbuilder/general/model/edit.html" %}
{% import 'appbuilder/general/lib.html' as lib %}
{% block edit_form %}
{{ super() }}
{% if actions %}
<div class="well well-sm">
{{ lib.render_action_links(actions, pk, modelview_name) }}
</div>
{% endif %}
{% endblock %}
```
and the view can pass the values straight to `render_template` rather than
reaching into widget internals:
```python
edit_template = "appbuilder/general/model/user_edit.html"
@expose("/edit/<pk>", methods=["GET", "POST"])
@has_access
def edit(self, pk):
pk = self._deserialize_pk_if_composite(pk)
widgets = self._edit(pk)
if not widgets:
return self.post_edit_redirect()
return self.render_template(
self.edit_template,
title=self.edit_title,
widgets=widgets,
related_views=self._related_views,
actions={"resetpasswords": self.actions.get("resetpasswords")},
pk=pk,
modelview_name=self.__class__.__name__,
)
```
That drops the ~45 lines of duplicated form markup, removes the need for
`UserEditFormWidget` altogether, and means any future FAB change to
`form_vertical.html` is picked up automatically instead of silently drifting
from our copy. The permission filtering is unchanged, since it is the same
macro doing the work.
Two smaller notes:
The description says the new template "extends FAB's built-in
`form_vertical.html`" — it is a copy rather than an `extends`. Worth correcting
before merge, since the body becomes the commit message and the maintenance
implications of the two are quite different.
"Mirroring the existing `show()` override pattern" is also a slightly
generous reading: `show()` *removes* entries FAB has already populated, whereas
this *adds* one FAB never populates. The result is safe because of the
macro-level filtering, but they aren't the same manoeuvre, and it's worth
knowing the safety comes from the macro rather than from symmetry with `show()`.
---
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
