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

WIP✨(backend) add bulk delete route #346

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

samy-aitouakli
Copy link
Contributor

Add route to admin api to bulk delete objects

@samy-aitouakli samy-aitouakli force-pushed the saitouakli/bulk_delete branch 3 times, most recently from c4fbcc0 to 8a81d16 Compare July 21, 2023 07:42
@samy-aitouakli samy-aitouakli force-pushed the saitouakli/bulk_delete branch 5 times, most recently from 2cd1f31 to b825de9 Compare July 24, 2023 16:59
@samy-aitouakli samy-aitouakli changed the title ✨(backend) add bulk delete route WIP✨(backend) add bulk delete route Jul 25, 2023
@samy-aitouakli samy-aitouakli removed the request for review from sampaccoud July 25, 2023 15:18
@sampaccoud
Copy link
Contributor

  • I would have found it more coherent with our other endpoints to pass the ids to delete as a required query string filter.
  • you do not check that the admin user has delete rights on this type of object. This is something your tests should take care of!

Add route to admin api to bulk delete objects
@samy-aitouakli samy-aitouakli removed their assignment Aug 1, 2023
@jbpenrath
Copy link
Member

This PR has been adjourned as bulk delete is not trivial at all as :

  • We should check user rights to delete or not the resource
  • It implies to write a custom mixin with a non standard response

At first, frontend admin could use a cascade of unique delete requests. Then we could imagine a soft delete logic.

@jbpenrath jbpenrath marked this pull request as draft August 16, 2023 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants