starlette-admin icon indicating copy to clipboard operation
starlette-admin copied to clipboard

'column_formatter' feature from Flask Admin

Open hasansezertasan opened this issue 1 year ago • 9 comments

Resolves 344.

FormattedField is a reusable, configurable, customizable by far the superior alternative to Flask Admins column_property thanks to Starlette Admin Fields.

I can extend examples.sqla with this field, it is totally suitable.

class UserView(ModelView):
    fields = [
        "id",
        "last_name",
        "first_name",
        FormattedField(
            "full_name",
            label="Full Name",
            func=lambda request, obj: f"{obj.first_name} {obj.last_name}",
        ),
        EnumField("type", choices=AVAILABLE_USER_TYPES, select2=False),
        "posts",
        FormattedField(
            "post_count",
            label="Post count",
            func=lambda request, obj: len(obj.posts),
        ),
    ]
    fields_default_sort = [User.last_name, ("first_name", True)]

PS: I'm not very good at writing tests, it's kind of a copy of the test_list_field.py.

hasansezertasan avatar Oct 21 '23 07:10 hasansezertasan

@jowilf What do you think about this?

I think it's not necessary to come up with a Field for this one. I was tinkering BaseField and I think we can make this feature more generic. I think we can add a new attribute to BaseField and call it parse_func and this attribute will be a callable. This callable will take two arguments: request and obj. request will be the current request and obj will be the current object. This callable will return the value of the field. If the callable is not provided, we will use the default behavior which is getattr(obj, self.name, None).

By this way, using FormattedField won't be necessary, which inherits from StringField. I think inheriting from a strict type like StringField is not a good idea. My return type could be a boolean or a number or a date, so inheriting from StringField is not a good idea. I can make the changes and commit to this PR.

@dataclass
class BaseField:
    """
    Base class for fields

    Parameters:
        ...
        parse_func: function to parse the value of this field from 
    """

    # ...
    func: Optional[Callable[[Request, Any], Any]] = None


    async def parse_obj(self, request: Request, obj: Any) -> Any:
        # ...
        # ! I believe we should force this field to be read_only and excluded from create and edit.
        return self.parse_func(request, obj) if self.parse_func else getattr(obj, self.name, None)

By this way, we will have column_formatters feature from Flask Admin with a lot of upsides. We won't have to create an instance of this class like you did in the starlette-admin-demo.

It'll probably help us to get a relationship attribute without a hussle. Probably a partial solution to #240

Implementing EndpointLinkRowAction and LinkRowAction from Flask Admin, in column level will be easier with this one.

hasansezertasan avatar Oct 29 '23 04:10 hasansezertasan

I previously answered this question in #200. So In my opinion we already have a way to do it, it's not really necessary to have two ways to do the same thing. Additionally, the async parse_obj function can handle more complex use cases such as #219, which may require an async function.

Edit:

We won't have to create an instance of this class like you did in the starlette-admin-demo.

I didn't pay attention to this part, I understand your point now and I believe that having field_formatters in the BaseModelView that functions like column_formatters in Flask-Admin would be a better solution. For more complex use cases, the class can be overridden like demo. wdyt @hasansezertasan ?

jowilf avatar Oct 29 '23 04:10 jowilf

I think inheriting from a strict type like StringField is not a good idea

Agree

jowilf avatar Oct 29 '23 05:10 jowilf

Implementing EndpointLinkRowAction and LinkRowAction from Flask Admin, in column level will be easier with this one.

I'm not sure I understand. Please, can you clarify it?

jowilf avatar Oct 29 '23 05:10 jowilf

I previously answered this question in #200. So In my opinion we already have a way to do it, it's not really necessary to have two ways to do the same thing. Additionally, the async parse_obj function can handle more complex use cases such as #219, which may require an async function.

I see. Let me tell you why I wrote this FormattedField at the begining. I saw #200 and checked the full example that you referenced. Its great to have a custom Field feature. Let's say that I have a table with a bunch of child objects. What do I do? In your example, you wrote CommentCounterField to get the length of comments attribute. What do I do for likes, reactions, etc? I have to write a new field for each of them, which I'm going to be using it for only a table. That's why I wrote FormattedField because it acted like column_formatter from Flask Admin. Why lambda functions? I don't have to create a class or define a function for a functionality that I will need for only one time. It's a one-liner, easy to write.

I aggree with you, async parse_obj function can handle more complex use cases but my approach doesn't prevent anyone from overriding parse_obj method. The way we already have is a great solution but I think it's not enough. In addition, adding a feature like column_formatter that accepts a lambda function will offer a great experience for those people migrating/coming from Flask Admin ecosystem.

I didn't pay attention to this part, I understand your point now and I believe that having field_formatters in the BaseModelView that functions like column_formatters in Flask-Admin would be a better solution. For more complex use cases, the class can be overridden like demo. wdyt @hasansezertasan ?

My toughts about field_formatters are here: I never liked defining column_list, column_details_list, column_sortable_list, column_filters, column_labels, column_descriptions and column_formatter over and over again for different views when I used Flask Admin. For larged models, I had to scroll a lot to change the description or label for a field/column. I don't like the idea of seperating them. Youre approach is great, I can define a field no matter what my field's type is, I can say BooleanField but read_only for an integer field which restricts people from editing this. I can define TextAreaField for a string field and restrict people from editing this, can set the label, description in one place. Your approach helps me to keep things in order. I like to keep control of things, so I decided to use subclasses of BaseField instead of just typing Model.id. In Flask Admin, when I define a formatter, I had to add this to a column_list, add a label for it in the column_labels, add a description for it in the column_descriptions, there are changes in three different places for only one field and I'm setting key/value pairs. It makes harder to keep track of things. And I agree, for more complex use cases, the class can be overriden. I highly reccomend not adding field_formatters but add a formatter attribute to the BaseField to act as a column_formatter in Flask Admin.

I'm not sure I understand. Please, can you clarify it?

LinkRowAction acts like link_row_action but what if I want to add a link action as a seperate column in the table? This is why formatter or parse_func attribute could work better. I can't see how to let the admin panel know what type of data that field_formatters returns. I also think formatter or parse_func accepting a lambda function is a quite good approach.

Let's say that I want to add # at the begining of each ID Field, no calculations or any logic. Just add # at the begining of each ID Field. What should I do? Inherit from IntegerField and override parse_obj method? I think we should seperate the logic for parse_obj. I think we can have parse_obj_for_edit, parse_obj_for_details, parse__obj_for_list in addition to parse_obj.

I hope I have explained myself clearly. I'm not a native speaker...

Sorry for closing the pr and re-opening it. It was my mistake.

hasansezertasan avatar Oct 29 '23 06:10 hasansezertasan

Let's say that I want to add # at the begining of each ID Field, no calculations or any logic. Just add # at the begining of each ID Field. What should I do? Inherit from IntegerField and override parse_obj method? I think we should seperate the logic for parse_obj. I think we can have parse_obj_for_edit, parse_obj_for_details, parse__obj_for_list in addition to parse_obj.

parse_obj should only be used to extract the value of a field from a model instance. Also, I forgot this, but you can simply add the @property decorator in your model to avoid overridden parse_obj. Here is a full example:

import uvicorn
from sqlalchemy import Column, Integer, String, create_engine
from sqlalchemy.ext.declarative import declarative_base
from starlette.applications import Starlette

from starlette_admin import StringField
from starlette_admin.contrib.sqla import Admin, ModelView
from starlette_admin.exceptions import FormValidationError

Base = declarative_base()
engine = create_engine("sqlite:///test.db", connect_args={"check_same_thread": False})


# Define your model
class User(Base):
    __tablename__ = "user"

    id = Column(Integer, primary_key=True)
    first_name = Column(String)
    last_name = Column(String)

    @property
    def full_name(self):
        return self.first_name + " " + self.last_name

    @full_name.setter
    def full_name(self, value: str):
        try:
            self.first_name, self.last_name = value.split(" ", maxsplit=2)
        except:
            raise FormValidationError({"full_name": "Bad format"})


class UserView(ModelView):
    fields = ["id", StringField("full_name")]


Base.metadata.create_all(engine)

app = Starlette()  # FastAPI()

# Create admin
admin = Admin(engine, title="Example: SQLAlchemy")

# Add view
admin.add_view(UserView(User))

# Mount admin to your app
admin.mount_to(app)

if __name__ == "__main__":
    uvicorn.run("test:app", reload=True)

To control which value is rendered in the list, detail, etc., you should override the BaseField.serialize_value instead. Here is the full example:

from typing import Any

import uvicorn
from sqlalchemy import Column, Integer, String, create_engine
from sqlalchemy.ext.declarative import declarative_base
from starlette.applications import Starlette
from starlette.requests import Request

from starlette_admin import StringField, RequestAction
from starlette_admin.contrib.sqla import Admin, ModelView

Base = declarative_base()
engine = create_engine("sqlite:///test.db", connect_args={"check_same_thread": False})


# Define your model
class User(Base):
    __tablename__ = "user"

    id = Column(Integer, primary_key=True)
    first_name = Column(String)
    last_name = Column(String)


class CustomField(StringField):
    async def serialize_value(
        self, request: Request, value: Any, action: RequestAction
    ) -> Any:
        # value is the same value returned by self.parse_obj
        if action == RequestAction.LIST:
            return value + " in list"
        #...
        return value


class UserView(ModelView):
    fields = ["id", CustomField("first_name"), "last_name"]


Base.metadata.create_all(engine)

app = Starlette()  # FastAPI()

# Create admin
admin = Admin(engine, title="Example: SQLAlchemy")

# Add view
admin.add_view(UserView(User))

# Mount admin to your app
admin.mount_to(app)

if __name__ == "__main__":
    uvicorn.run("test:app", reload=True)

Note: This should be added somewhere in the documentation

jowilf avatar Oct 29 '23 07:10 jowilf

parse_obj should only be used to extract the value of a field from a model instance. Also, I forgot this, but you can simply add the @property decorator in your model to avoid overridden parse_obj. Here is a full example:

I'm using column_property (mostly one-liner, gives ability to filter with it) when I need to add a property to a model. Here's the documentation: SQL Expressions as Mapped Attributes — SQLAlchemy 2.0 Documentation and there is another approach we can use model wise: Hybrid Attributes — SQLAlchemy 2.0 Documentation. These are SQLAlchemy goodies. Think about it, I'm commiting a code block that's related to ModelView but the change is in the models folder. I'm not sure if it's a good idea. That's why I'm using repr in the ModelView instead of __admin_repr__ in the model and same goes for select2_result. I could be using same model in a script, or another API or somewhere else and putting __admin_select2_repr__ into my model doesn't look good because it's not related to the model itself. I'm not sure if I'm making sense here but I hope you get the idea.

Also the represantation could depend on the User's role.

The example you have given is great but it's still requires to write same thing over and over again when it's required in different places but mostly used in ModelViews. I believe an Administrative Interface Module like this should offer an internal alternative for these kind of things.

To control which value is rendered in the list, detail, etc., you should override the BaseField.serialize_value instead. Here is the full example:

Thanks for letting me know, I'm learning more about starlette-admin thanks to this conversation. So my suggestion with parse_obj is not very suitable for starlette-admins design. What do you think about not following Flask Admin's design approach for formatters? Should it be a seperate attribute or should we define those formatters in the fields attribute with Field based formatters?

I believe we could pass action parameter to parse_obj too.

hasansezertasan avatar Oct 29 '23 08:10 hasansezertasan

Syntax proposal:

class UserView(ModelView):
    fields = [
        "id",
        "last_name",
        "first_name",
        FormattedField(
            func=lambda request, obj: f"{obj.first_name} {obj.last_name}",
            field=TextField,
            name="full_name",  # Passes to IntegerField
            label="Full Name",  # Passes to IntegerField
            # **kwargs passes to TextField
        ),
        "posts",
        FormattedField(
            func=lambda request, obj: len(obj.posts),
            field=IntegerField,
            name="post_count",  # Passes to IntegerField
            label="Post count",  # Passes to IntegerField
            # **kwargs passes to IntegerField
        ),
    ]

  • All FormattedField fields are going to be read_only.
  • func is the only parameter it gets, it constructs a field with the given field and kwargs.

hasansezertasan avatar Dec 29 '23 03:12 hasansezertasan

Let's take a look at this PR.

What do we need?

column_formatter feature from Flask Admin.

What does column_formatter do?

It's simply a function that returns some value and, it's mostly read-only.

What workarounds/alternatives do we have to achieve this?

  • We can use a property decorator in the model class.
  • We can create a custom field.

What is wrong with creating a custom field?

  • We might have to do it over and over again, it's not so efficient. Take a look at this comment: https://github.com/jowilf/starlette-admin/pull/345#issuecomment-1784012009
  • We need to exclude these fields from the create and edit views manually, every time.

What is wrong with a property decorator?

  • The column_formatter feature is read-only but the property decorator is commonly used for getting and setting class attributes.
  • We need to exclude these fields from the create and edit views manually, every time.

Flask Admin knows that column_formatters are read-only and it automatically excludes these from the create and edit views. column_formatter fields are not "settable".


It's been months since I created this pull request. Looking at the code now, it's not quite good, isn't it? The foundation looks good, it's based on Starlette Admin Fields, which is lovely... but users are going to have to import those fields and use them.

What can we do?

Let's take a look at an example.

class User(Base):
    __tablename__ = "user"

    id = Column(Integer, primary_key=True)
    first_name = Column(String)
    last_name = Column(String)

    def full_name(self) -> str:
        return self.first_name + " " + self.last_name


class UserView(ModelView):
    fields = ["id", "full_name"]

We need to update ModelConverter to detect that the full_name is a method and its return type (to figure out what kind of field should it be) and because it's a method (callable), the converter can pass exclude_from_edit=True, exclude_from_create=True to the detected field type.

We probably need to add the is_callable attribute to the BaseClass to "call" the full_name method to get the value.

https://github.com/jowilf/starlette-admin/blob/d30fb6433b4530fbd20bbe11ca1a32aa48e61fdd/starlette_admin/fields.py#L104-L127

     async def parse_obj(self, request: Request, obj: Any) -> Any: 
         if self.is_callable:
             func = getattr(obj, self.name, None)
             value = func() if func else None
         else:
             value = getattr(obj, self.name, None)
        return value

What is the gain?

  • We don't need to create a custom field.
  • Users don't have to know anything about column_formatters.
  • full_name is not defined in the ModelView as a column_formatter. It's defined in the model as a method so we can use it outside of the starlette-admin scope. It's more generic.
  • column_formatters are redundant now.

To summarize. Support adding callables to the fields.

In the end, it's what column_formatters do, call a function.

WDYT about this approach @jowilf?

hasansezertasan avatar Jan 15 '24 09:01 hasansezertasan

Closing due to inactivity.

hasansezertasan avatar May 06 '24 16:05 hasansezertasan