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

343310 - Bulk action on Servers #298

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

MBausson
Copy link
Contributor

@MBausson MBausson commented Dec 11, 2024

  • Add tests
  • Fix flash message not good font + checkboxes unchecked
  • Clean bulk action button
  • Do we need a BulkDataTableComponent?

@MBausson
Copy link
Contributor Author

MBausson commented Dec 11, 2024

About futures implementations of bulk actions on other resources:

  • The current bulk_destroy action could be moved to a concern
  • delete :bulk_destroy should be added to every resource's collection routes
  • DataLists should specify the bulk: true constructor parameter
  • Datalists should be wrapped within a <form> (see this file). We can maybe move this wrapper directly inside the DataList component ?

@MBausson MBausson marked this pull request as ready for review December 11, 2024 16:40
Copy link
Collaborator

@B-Rass B-Rass left a comment

Choose a reason for hiding this comment

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

We should have ask user confirmation before deleting, with a text informing the user how many items is going to delete

app/views/servers/index.html.erb Outdated Show resolved Hide resolved
@nicolas-brousse
Copy link
Collaborator

We should have to thing a bit more of the future actions that may be implemented.
What if we have other actions, like edit or change server category?

I think at least the stimulus controller may be declared inside the datatable to make it easier than add it in every form.
Form may be inside datatable too but how to make it works with more than one action.

May be slot could be use to declare actions.

c.with_bulk_action("Destroy all", bulk_destroy_servers_path, method: :delete)

We also could imagine that bulk is automatically enabled when at least one "slot" is added.

config/routes.rb Outdated
@@ -55,6 +55,7 @@
post :sort
get :import_csv
post :import
post :bulk_manage
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prefer patch

Comment on lines 7 to 15
def bulk_manage
@items = params[:item_ids].map { |id| resource_class.find(id) }

if params[:bulk_destroy]
bulk_destroy
else
redirect_back fallback_location: root_path
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may imagine having this just as a proxy, and having bulk_destroy in the ServerController.

And if we have bulk_disable button later, then we may just need to have a bulk disable action in the controller.

app/components/list/data_table_component.rb Outdated Show resolved Hide resolved
@nicolas-brousse nicolas-brousse force-pushed the 343310-add-bulk-actions-on-servers branch from 0f16a27 to 939f553 Compare January 15, 2025 10:49
@nicolas-brousse nicolas-brousse self-assigned this Jan 15, 2025
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

Successfully merging this pull request may close these issues.

3 participants