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

[UI] List toolbar - select all to perform batch action #14392

Open
wants to merge 41 commits into
base: 6.x
Choose a base branch
from

Conversation

laurielim
Copy link
Member

@laurielim laurielim commented Dec 18, 2024

Q A
Bug fix? (use the a.b branch) 🔴
New feature/enhancement? (use the a.x branch) 🟢
Deprecations? 🔴
BC breaks? (use the c.x branch) 🔴
Automated tests included? 🟢
Related user documentation PR URL mautic/user-documentation#...
Related developer documentation PR URL mautic/developer-documentation-new#...
Issue(s) addressed Fixes #...

Description

  • This branch is based off [UI] Toolbar batch actions #14306
  • Created a select all button that when clicked, also "selects" paginated items (as opposed to the checkall checkbox which only selects items on the rendered page)
  • Created batchDeleteService which perform batch delete for batchDeleteActions in various entity controllers
  • Created CannotBeDeletedInterface for models where entity cannot be deleted if there are dependencies or in the case of custom fields, the entity is "fixed".

📋 Steps to test this PR:

  1. Open this PR on Gitpod or pull down for testing locally (see docs on testing PRs here)
  2. Generate dummy data for testing with ddev exec bin/console doctrine:fixtures:load
  3. Go to the different entity lists and see that batch actions still work when selecting individual checkboxes and when clicking on "select all".
  4. See that the select all button shows how many items there are in total
  5. See that the # of selected items match the total items when you click on select all
  • image
  • image
  1. See that Segments that segments used in other segments cannot be deleted unless you're deleting all segments
  • Same for roles with users
  • See that you can't delete your own user account
  • See that you can't delete fixed custom fields
  1. Install custom plugins Clearbit and FullContact and see that they still work for both person and company lookup

app/bundles/CoreBundle/Config/config.php Outdated Show resolved Hide resolved
app/bundles/CoreBundle/Service/BatchDeleteService.php Outdated Show resolved Hide resolved
$postActionVars,
$this->getSessionBase(),
$this->getModelName(),
[$this, 'isLocked'],
Copy link
Member

Choose a reason for hiding this comment

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

I would create another service for this isLocked method but that can be done in the second step.

@andersonjeccel
Copy link
Contributor

On the current state, while we can see the button for selecting all, only contacts from the page you're on receive the batch action, right?

Screencast.From.2024-12-18.10-02-36.mp4

@laurielim laurielim marked this pull request as draft December 18, 2024 17:54
@andersonjeccel andersonjeccel added T1 Low difficulty to fix (issue) or test (PR) user-experience Anything related to related to workflows, feedback, and navigation enhancement Any improvement to an existing feature or functionality refactoring The change does not change behavior but improves the code labels Dec 18, 2024
@andersonjeccel andersonjeccel added this to the 6.0 milestone Dec 18, 2024
@laurielim
Copy link
Member Author

laurielim commented Dec 19, 2024

On the current state, while we can see the button for selecting all, only contacts from the page you're on receive the batch action, right?

@andersonjeccel The select all should work for contacts batch actions already. I reckon you might need to clear browser cache as there's JS changes.

@andersonjeccel
Copy link
Contributor

@laurielim Definitely working great! I'm happy to see this feature ready :)

When checking the branch locally, the command ddev exec composer install was needed as I was working on 6.x branches before
it downgraded some packages (which makes me think that you changed the target on GitHub but missed rebasing it)

You can try rebasing the branch using the last version of #14275 to avoid dealing with conflicts, or invite me to your fork so I can help you if needed

@andersonjeccel andersonjeccel added needs-rebase PR's that need to be rebased feature A new feature for inclusion in minor or major releases and removed enhancement Any improvement to an existing feature or functionality labels Dec 19, 2024
@laurielim
Copy link
Member Author

laurielim commented Dec 27, 2024

@andersonjeccel yep the branch was outdated, based my branch off #14306 which i've now rebased so should have latest packages now. Also this is now ready for review. I did add you as collaborator though in case.

@laurielim laurielim marked this pull request as ready for review December 27, 2024 09:43
@laurielim laurielim changed the title WIP [UI] List toolbar - select all to perform batch action [UI] List toolbar - select all to perform batch action Dec 27, 2024
@laurielim laurielim added ready-to-test PR's that are ready to test code-review-needed PR's that require a code review before merging and removed needs-rebase PR's that need to be rebased labels Dec 27, 2024
Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

It works at overal. I did some tests.
If I am trying change tags on all selected, it return no ids

image

Copy link
Member

@kuzmany kuzmany left a comment

Choose a reason for hiding this comment

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

It works at overal. I did some tests.
If I am trying change tags on all selected, it return no ids

image

@laurielim
Copy link
Member Author

@kuzmany could you provide a step by step please? I am not able to reproduce.

@shinde-rahul
Copy link
Contributor

@laurielim, please resolve the conflicts.

@shinde-rahul shinde-rahul added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Jan 3, 2025
Copy link
Contributor

@jos0405 jos0405 left a comment

Choose a reason for hiding this comment

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

I tested with multiple lists, and worked really well!
Also in tags. Couldn't reproduce the previously mentioned error.

image

image

image

@laurielim laurielim force-pushed the ui-toolbar-select-all branch from 0f1379b to 2ff08db Compare January 21, 2025 08:18
@andersonjeccel
Copy link
Contributor

@laurielim Hm, would you say that rebasing was successful?

I'm wondering if something broke along the way to result on the failing tests

@andersonjeccel andersonjeccel removed the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Jan 21, 2025
@laurielim laurielim force-pushed the ui-toolbar-select-all branch from 05de5da to bb9e1fc Compare January 26, 2025 11:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-needed PR's that require a code review before merging feature A new feature for inclusion in minor or major releases pending-test-confirmation PR's that require one test before they can be merged refactoring The change does not change behavior but improves the code T1 Low difficulty to fix (issue) or test (PR) user-experience Anything related to related to workflows, feedback, and navigation
Projects
Status: 🚨 Developer revision needed
Development

Successfully merging this pull request may close these issues.

6 participants