piccolo_admin icon indicating copy to clipboard operation
piccolo_admin copied to clipboard

Adding support for custom actions per table

Open haffi96 opened this issue 2 years ago • 14 comments

  • The general idea to allow custom logic being run for each table, this is more of a POC. So if there is anything wrong with doing this. Let me know.
  • I think this is different to hooks. Because hooks run only when a table row is modified 🤔
  • First time working with vue and typescript, so would be great to have some suggestions for improvement

For this to work, would need this https://github.com/piccolo-orm/piccolo_api/pull/200. These changes would allow us to send row information to the piccolo_api and run the actions

image

haffi96 avatar Nov 05 '22 19:11 haffi96

This pull request introduces 1 alert when merging 4246898e038db6644549b772631e2bd4f8634085 into 2194e4399db37a279c81e4d3673713a0c4cb090a - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a class instantiation

lgtm-com[bot] avatar Nov 05 '22 19:11 lgtm-com[bot]

@haffi96 This is cool, thanks.

I agree that it's something we need to add.

Some initial thoughts:

  • Rather than callbacks, how about we call them 'actions'? I might be biased on the name, as this is what I used in the past with Django.
  • It would be good if multiple actions / callbacks could be registered for a single table. So the the API call would be POST /actions/<some-action-name/. Calling GET /actions/ could return the name of all available actions.
  • At the moment it seems like the action can only accept a single row as an argument, but it would be great if multiple rows could be passed in.

First time working with vue and typescript, so would be great to have some suggestions for improvement

The Vue and Typescript code looks good.

I think this is different to hooks. Because hooks run only when a table row is modified 🤔

You're right - this is different to hooks, and definitely has it's own unique purpose.

dantownsend avatar Nov 05 '22 20:11 dantownsend

Rather than callbacks, how about we call them 'actions'?

haha actions was actually my initial choice too (inspired by flask-admin for myself 🤓 ). Decided against it as i saw that there was already some actions related components so didnt want to overlap. But i agree, actions sounds better. Will update

It would be good if multiple actions / callbacks could be registered for a single table

👍🏽

At the moment it seems like the action can only accept a single row as an argument, but it would be great if multiple rows could be passed in.

Yep, that was intentional as i wasn't sure if waiting for the response of an action to process for multiple rows may be kinda weird. But will experiment with this. I agree, would definitely be nice to have it for multiple rows

haffi96 avatar Nov 05 '22 21:11 haffi96

This pull request introduces 1 alert when merging b3dcd1d3aa6f99bd0af3e2c935e77b8cf8934964 into 2194e4399db37a279c81e4d3673713a0c4cb090a - view on LGTM.com

new alerts:

  • 1 for Wrong name for an argument in a class instantiation

lgtm-com[bot] avatar Nov 08 '22 19:11 lgtm-com[bot]

Can register multiple actions per table now and also select multiple rows at once.

Atm, the admin only sends this -> {"table_name": ..... , "row_ids": [...]} back to the user registered custom action functions. We could also send all the row data

And as you can see i've struggled abit with the css 😆 Will have another go at better aligning the drop down buttons

image

haffi96 avatar Nov 19 '22 18:11 haffi96

@haffi96 Cool, thanks! The UI looks good.

I think passing just the row IDs should be OK.

dantownsend avatar Nov 20 '22 20:11 dantownsend

@sinisaos What do you think about this? I think it could be a really useful feature.

dantownsend avatar Nov 21 '22 22:11 dantownsend

The tests are just failing because it relies on new features in Piccolo API, which haven't been published to PyPI - we need to prioritise getting them released.

dantownsend avatar Nov 21 '22 22:11 dantownsend

@dantownsend I also think it's useful and similar to Django admin actions. Can you also merge this as our main bulk actions so that we have proper bulk update and delete via orm method instead of iterating through a single method like we do now. After that I can change the Piccolo Admin code.

sinisaos avatar Nov 22 '22 06:11 sinisaos

This PR has been marked as stale because it has been open for 30 days with no activity. Are there any blockers, or should this be closed?

github-actions[bot] avatar Dec 25 '22 02:12 github-actions[bot]

Still valid.

dantownsend avatar Dec 30 '22 06:12 dantownsend

Hey @dantownsend sorry about the delay. I've done some more work on this last week and implemented your suggestions on the piccolo api PR. Just have some unit tests left to do.

But quick question, Is this PR dependent on https://github.com/piccolo-orm/piccolo_api/pull/145 being merged first?

haffi96 avatar Dec 30 '22 10:12 haffi96

But quick question, Is this PR dependent on piccolo-orm/piccolo_api#145 being merged first?

@haffi96 No, this PR is not dependent on piccolo-orm/piccolo_api#145 . It's a completely different PR that provides a solution for propper bulk actions (delete and update) for Piccolo API and Piccolo Admin.

sinisaos avatar Jan 01 '23 18:01 sinisaos

This PR has been marked as stale because it has been open for 30 days with no activity. Are there any blockers, or should this be closed?

github-actions[bot] avatar Feb 01 '23 02:02 github-actions[bot]