Ability to use Jinja2 templates in action forms

[geoolekom]

This PR allows the use of Jinja2 templates in action forms, making it more flexible than plain HTML.

• Moved form initialization to HTML from JS to use Jinja2 for rendering.
• Separated modal windows for each action. This is also allows to use select2 controls the same way it is done in editing/creation forms, without duplicating logic and without re-creating controls.
• Added a base form template that will be rendered if you pass form_fields to the rendering context.
• Separated the action and request_action logic so that these don't get mixed up in different parts of the code.
• Added an example of use and documented the feature.

[geoolekom]

This also partially solves the problem described in #375.
This allows fields to be used for rendering, but not for validation.

[hasansezertasan]

This also partially solves the problem described in #375.
This allows fields to be used for rendering, but not for validation.

You're implementation looks quite interesting and it kinda makes #375 obsolete. Let's hear what @jowilf is thinking about it.

Edit: At #444, we talked about that validation issue. I recommend checking it. It would be awesome if you could integrate validation feature to the ActionManager API 🙏.

Great work 💯

[hasansezertasan]

Suggested change

	form_context: Optional[dict] = None,
	form_context: Optional[Dict] = None,

To support older Python versions, you should import Dict from typing or typing_extensions and use it.

[geoolekom]

Fixed

[geoolekom]

Edit: At #444, we talked about that validation issue. I recommend checking it. It would be awesome if you could integrate validation feature to the ActionManager API 🙏.

I believe that client validation is not enough, and furthermore, it cannot be trusted. I can suggest a temporary workaround to validate forms, which you may see in examples:

https://github.com/jowilf/starlette-admin/pull/505/files#diff-020e73342e8e6a1a797cfe0dbdaaeb533146e3e57e8373a9059a6e5b4855f55eR85

    async def add_tag(self, request: Request, pks: List[Any]):
        form_fields = [HasOne("tag", identity="tag")]
        try:
            form = (
                await request.form()
            )  # from starlette_admin.base.BaseAdmin._render_edit
            data = {
                f.name: f.parse_form_data(request, form, request.state.action)
                for f in form_fields
            }  # from starlette_admin.base.BaseAdmin.form_to_dict
        except Exception as e:
            raise ActionFailed(f"Form processing error: {e}") from e

        data = await self.arrange_data_for_fields(request, data, form_fields)
        await self.validate_add_tag(request, data)

Of course, I would like to be able to hide all this boilerplate code inside handle_action and handle_row_action, and have the validated form as one of the action arguments. But this would require significant refactoring.

I would like to be able to validate the form in the same way as in, for example, starlette_admin.contrib.sqla.view.ModelView.create, but the logic of this validation is partly contained in BaseAdmin, partly in View, partly in ModelView. Therefore, I would leave this refactoring for later, and now limit myself to only processing FormValidationError.

UPD. Still needs testing. Perhaps it should be a separate PR, thoughts?

[geoolekom]

But this would require significant refactoring.

Taking a fresh look, I realized that there wasn't much refactoring. Added form parsing in BaseView. Now, if the action has form_fields specified, then the function must take three arguments, the third of which is validated form data.

if you want to add extra validation, you can write a function:

    async def validate_my_action(self, request: Request, data: Dict[str, Any]):
        ...
        if len(errors) > 0:
            raise FormValidationError(errors)
    
    @action(...)
    async def my_action(self, request: Request, pks: List[Any], data: Dict):
        await self.validate_my_action(request, data)

So, this solves the problem stated at #444

Also, I realized that module-level form_context makes no sense. I can't think of a single example in which something is already defined at the module-level, but it cannot be specified directly in a custom form_template.

I left fixups to simplify your review. Will squash them later after you will check the changes.

[hasansezertasan]

I would like to be able to validate the form in the same way as in, for example, starlette_admin.contrib.sqla.view.ModelView.create, but the logic of this validation is partly contained in BaseAdmin, partly in View, partly in ModelView. Therefore, I would leave this refactoring for later, and now limit myself to only processing FormValidationError.

Well, that separation bothers me a lot...

So, this solves the problem stated at #444

But the login is in the backend instead of frontend right?

Also, I realized that module-level form_context makes no sense. I can't think of a single example in which something is already defined at the module-level, but it cannot be specified directly in a custom form_template.

I totally agree with you, if form_context is going to take one keyword (form_fields), why not just use that keyword?

I left fixups to simplify your review. Will squash them later after you will check the changes.

I'll take a look at it as soon as possible, great work!

[geoolekom]

But the login is in the backend instead of frontend right?

I don't see anything about login at #444. It's a typo? However, validation is completely in the backend, and it is completely similar to how it is done in create, except that the validate_* function must be called explicitly.

I'll take a look at it as soon as possible, great work!

Thank you, looking forward to your review!

[hasansezertasan]

I don't see anything about login at #444. It's a typo?

Yep, it's a typo. I was walking about validation logic.

[hasansezertasan]

Well, you've done a great job 👏.

I cloned your fork and tried the example. It seems to me that everything is working as it supposed to be but I don't think my approval will (and should not) be enough to be merged. Let's hear what @jowilf has to say about this, as for me, it's far better implementation than #375.

I'll definetely use the changes you made. 🚀.

[hasansezertasan]

There is one more thing. What do you @geoolekom think about #463 and #472? Do you think we can find a more Pythonic/python-first (instead of overriding Jinja2 templates) solution for this?

[geoolekom]

What do you think about #463 and #472?

I will look into these issues and write my comment as soon as possible. Since you have already did the review, I will squash the fixups.

[geoolekom]

@jowilf @hasansezertasan Any updates on this?

[hasansezertasan]

@jowilf @hasansezertasan Any updates on this?

Hey there, sorry for late response. I don't know what the author thinks about it but it looks good enough for me.