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

added TableConfig #101

Closed
wants to merge 6 commits into from
Closed

added TableConfig #101

wants to merge 6 commits into from

Conversation

dantownsend
Copy link
Member

Related to #73

Comment on lines +226 to +228
for table_config in table_configs:
table_class = table_config.table_class
visible_column_names = table_config.get_visible_column_names()
Copy link
Member

Choose a reason for hiding this comment

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

This looks great but but visible_columns_names return all table columns

@sinisaos
Copy link
Member

sinisaos commented Oct 22, 2021

@dantownsend Sorry for the late reply, but I wasn't at home.I need to add one line to get things work get_all_tables

for table in tables:
    table = table.table_class if isinstance(table, TableConfig) else table <- must add this line to get things work
    if table not in output:
        output.append(table)
    get_references(table)

Everything works great but visible_columns return all columns. I use TableConfig like this in example.py

from piccolo_admin.endpoints import FormConfig, TableConfig, create_admin

table_movie = TableConfig(
    table_class=Movie,
    visible_columns=[
        Movie.id,
        Movie.name,
        Movie.director,
    ],
)

APP = create_admin(
    [
        table_movie,
        Director,
        Studio,
    ],

Maybe the line I added is causing the problem.

@dantownsend
Copy link
Member Author

@sinisaos You're right, there was a bug - I've pushed a fix. I forgot to update create_admin to accept a Table or TableConfig.

visible_columns seems to be working OK for me:

from piccolo_admin.example import Director
from piccolo_admin.endpoints import TableConfig

config = TableConfig(table_class=Director, visible_columns=[Director.id])
>>> config.get_visible_columns()
[Director.id - Serial]
>>> config.get_visible_column_names()
('id',)

Or is it something else? I put it together quite quickly - it could do with some more unit tests.

@sinisaos
Copy link
Member

@dantownsend I found it. Problem was get_references() in get_all_tables() raise AttributeError. If we add one line everything works great

def get_all_tables(
    tables: t.Sequence[t.Type[Table]],
) -> t.Sequence[t.Type[Table]]:
    """
    Fetch any related tables, and include them.
    """
    output: t.List[t.Type[Table]] = []

    def get_references(table: t.Type[Table]):
        table = table.table_class if isinstance(table, TableConfig) else table <-- this line
        references: t.List[t.Union[t.Type[Table], t.Any]] = [
            i._foreign_key_meta.references
            for i in table._meta.foreign_key_columns
        ]
        for reference in references:
            table = (
                reference.resolve()
                if isinstance(reference, LazyTableReference)
                else reference
            )

            if table not in output:
                output.append(table)
                get_references(table)

    for table in tables:
        if table not in output:
            output.append(table)
        get_references(table)

    return output

@sinisaos
Copy link
Member

@dantownsend You can also add this to RowListing.vue and everything works great.

cellNames() {
    const keys = []
    for (const key in this.rows[0]) {
        if (!key.endsWith("_readable")) {
            keys.push(key)
        }
    }
    // display Piccolo ORM visible_columns
    const visibleColumns = keys.filter((column) =>
        this.schema.visible_column_names.includes(column)
    )
    return visibleColumns
},

@dantownsend
Copy link
Member Author

@sinisaos Cool, thanks.

@sinisaos
Copy link
Member

@dantownsend But you have to remove the code from the last commit for get_references() to work. Now a new problem has occurred. This code works if the table has a FK. If not the table appears twice with visible_columns and without. For example

table_movie = TableConfig(
    table_class=Movie,
    visible_columns=[
        Movie.id,
        Movie.name,
        Movie.rating,
        Movie.director,
    ],
)

table_director = TableConfig(
    table_class=Director,
    visible_columns=[
        Director.id,
        Director.name,
    ],
)

APP = create_admin(
    [
        table_movie,
        table_director,
        Studio,
    ],

Director table now occurs twice with ('id', 'name', 'years_nominated', 'gender') and also with ('id', 'name')

Screenshot from 2021-10-22 21-23-12

@sinisaos
Copy link
Member

@dantownsend This works. Sorry for this long comments.

# previous code is unchanged
def get_all_tables(
    tables: t.Sequence[t.Type[Table]],
) -> t.Sequence[t.Type[Table]]:
    """
    Fetch any related tables, and include them.
    """
    output: t.List[t.Type[Table]] = []

    def get_references(table: t.Type[Table]):
        table = table.table_class if isinstance(table, TableConfig) else table
        references: t.List[t.Union[t.Type[Table], t.Any]] = [
            i._foreign_key_meta.references
            for i in table._meta.foreign_key_columns
        ]
        for reference in references:
            table = (
                reference.resolve()
                if isinstance(reference, LazyTableReference)
                else reference
            )

    for table in tables:
        if table not in output:
            output.append(table)
        get_references(table)

    return output


def create_admin(
    tables: t.Sequence[t.Type[Table]],
    forms: t.List = [],
    auth_table: t.Type[BaseUser] = BaseUser,
    session_table: t.Type[SessionsBase] = SessionsBase,
    session_expiry: timedelta = timedelta(hours=1),
    max_session_expiry: timedelta = timedelta(days=7),
    increase_expiry: t.Optional[timedelta] = timedelta(minutes=20),
    page_size: int = 15,
    read_only: bool = False,
    rate_limit_provider: t.Optional[RateLimitProvider] = None,
    production: bool = False,
    site_name: str = "Piccolo Admin",
    auto_include_related: bool = True,
    allowed_hosts: t.Sequence[str] = [],
):
    """
    :param tables:
        Each of the tables will be added to the admin.
    :param auth_table:
        Either a BaseUser, or BaseUser subclass table, which is used for
        fetching users.
    :param session_table:
        Either a SessionBase, or SessionBase subclass table, which is used
        for storing and querying session tokens.
    :param session_expiry:
        How long a session is valid for.
    :param max_session_expiry:
        The maximum time a session is valid for, taking into account any
        refreshes using `increase_expiry`.
    :param increase_expiry:
        If set, the `session_expiry` will be increased by this amount if it's
        close to expiry.
    :param page_size:
        The admin API paginates content - this sets the default number of
        results on each page.
    :param read_only:
        If True, all non auth endpoints only respond to GET requests - the
        admin can still be viewed, and the data can be filtered. Useful for
        creating online demos.
    :param rate_limit_provider:
        Rate limiting middleware is used to protect the login endpoint
        against brute force attack. If not set, an InMemoryLimitProvider
        will be configured with reasonable defaults.
    :param production:
        If True, the admin will enforce stronger security - for example,
        the cookies used will be secure, meaning they are only sent over
        HTTPS.
    :param site_name:
        Specify a different site name in the admin UI (default Piccolo Admin).
    :param auto_include_related:
        If a table has foreign keys to other tables, those tables will also be
        included in the admin by default, if not already specified. Otherwise
        the admin won't work as expected.
    :param allowed_hosts:
        This is used by the CSRF middleware as an additional layer of
        protection when the admin is run under HTTPS. It must be a sequence of
        strings, such as ['my_site.com'].
    """

    if auto_include_related:
        tables = get_all_tables(tables)

    return ExceptionMiddleware(
        CSRFMiddleware(
            AdminRouter(
                *tables,
                forms=forms,
                auth_table=auth_table,
                session_table=session_table,
                session_expiry=session_expiry,
                max_session_expiry=max_session_expiry,
                increase_expiry=increase_expiry,
                page_size=page_size,
                read_only=read_only,
                rate_limit_provider=rate_limit_provider,
                production=production,
                site_name=site_name,
            ),
            allowed_hosts=allowed_hosts,
        )
    )

I removed this 3 lines of code from get_references()

if table not in output:
    output.append(table)
    get_references(table)

because it's adding twice table from FK relation (Director table).

@dantownsend
Copy link
Member Author

Closing in preference of #102

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants