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

Hide parameter in Validators to hide Piccolo Admin table link from sidebar if the validators fail. #281

Open
AmazingAkai opened this issue Apr 8, 2024 · 8 comments

Comments

@AmazingAkai
Copy link

AmazingAkai commented Apr 8, 2024

There should be a hide parameter in Validators which would hide Piccolo Admin table link from sidebar if the validator fails.

For Example, the following table won't be shown if the validator fails:

VALIDATOR_FUNCTIONS: list[ValidatorFunction] = ...

TABLE_CONFIG = TableConfig(
    MyTable,
    validators=Validators(every=VALIDATOR_FUNCTIONS, hide=True),
)
@sinisaos
Copy link
Member

sinisaos commented Apr 8, 2024

@AmazingAkai Since it's a Piccolo Admin issue, I think the best way to fix it is to raise an error and display that error in the admin UI. If, for example, we write a validator that only the superuser can access the table like this (user piccolo is superuser and user john is not)

from piccolo_api.crud.endpoints import PiccoloCRUD
from piccolo_api.crud.validators import Validators
from piccolo_admin.endpoints import TableConfig  

def validator_superuser(piccolo_crud: PiccoloCRUD, request: Request):
    if not request.user.user.superuser:
        raise HTTPException(
            detail="Only a superuser can do this",
            status_code=403,
        )

director_config = TableConfig(
    validators=Validators(every=[validator_superuser]),
)

APP = create_admin([director_config])

This is how it could look like.

validators.webm

@AmazingAkai
Copy link
Author

@sinisaos That would also be fine if we can customize the message.

@dantownsend
Copy link
Member

dantownsend commented Apr 8, 2024

@sinisaos That's a smart solution.

In terms of showing the error message, this is the bit of code which shows the error message:

https://github.com/piccolo-orm/piccolo_admin/blob/master/admin_ui/src/store.ts#L186-L189

The problem we have though is knowing how to extract the error message from the response.

For example, for forms we have all of this logic to try and extract the error message from the response:

https://github.com/piccolo-orm/piccolo_admin/blob/1157c120415b54e509f7720daaa587cd2cc9d450/admin_ui/src/utils.ts#L84

It gets pretty messy. We could look for a certain header in the response.

        raise HTTPException(
            detail="Only a superuser can do this",
            status_code=403,
            headers={'Piccolo-Admin-Error': 'Only a superuser can do this'}
        )

Or if it's a text response, just show whatever the response body is as the error message.

I'm not sure - what do you think?

@dantownsend
Copy link
Member

dantownsend commented Apr 8, 2024

Alternatively, if it's just a matter of hiding certain tables from the sidebar based on whether the user is an admin or superuser, we could do this:

TableConfig(MySecretTable, visible_to=['superuser', 'admin'])

And then just hide tables from the /api/tables/grouped/ accordingly.

Just hiding them isn't enough by itself though, because a user could still follow a URL to the table. So validators are required too.

@sinisaos
Copy link
Member

sinisaos commented Apr 8, 2024

In terms of showing the error message, this is the bit of code which shows the error message:

https://github.com/piccolo-orm/piccolo_admin/blob/master/admin_ui/src/store.ts#L186-L189

The problem we have though is knowing how to extract the error message from the response.

@dantownsend Thanks. I used that generic error message also in the fetchSchema and fetchCount methods to get the results like in the video.

Or if it's a text response, just show whatever the response body is as the error message.

We could just return error message like this

context.commit("updateApiResponseMessage", {
  contents: `Problem fetching ${tableName} rows. ${error.message}.`,
  type: "error"
})

This would result in a pop-up message like this Problem fetching director rows. Request failed with status code 400.. Other information about the error is visible in the browser console. I think the main purpose of this is to disable access to the table if the user doesn't have the right permission.

@sinisaos
Copy link
Member

sinisaos commented Apr 8, 2024

@dantownsend Or we can try something like this

context.commit("updateApiResponseMessage", {
    contents: `Problem fetching ${tableName} rows. 
    ${JSON.parse(JSON.stringify(error.response?.data.detail))}.`,
    type: "error"
})

Result is Problem fetching director rows. Only a superuser can do this. in message pop-up.

@dantownsend
Copy link
Member

@sinisaos What do you think of the header idea?

        raise HTTPException(
            detail="Only a superuser can do this",
            status_code=403,
            headers={'Piccolo-Admin-Error': 'Only a superuser can do this'}
        )

It means we don't have to worry about parsing anything, or looking for specific error codes.

@sinisaos
Copy link
Member

sinisaos commented Apr 10, 2024

@dantownsend The headers idea is good but that will solve only errors where we specify headers. With the approach I suggested we get more generic messages that would parse all other Axios errors eg 404 Not Found etc. Here is a branch you can test if you want (of course, the validator code in example.py should be removed if it is good enough for PR). I agree with any solutions that will clearly show the user what the error is and where the user won't have to write too much code to get it.

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

No branches or pull requests

3 participants