Allow bulk unbanning / removal and clearing ip addresses

I'm currently still writing the appropriate tests for the new behaviors, but @Anybody could already take a look at what's implemented!

Thanks @LRWebks looks great! Just some textual changes.

Added the required test for the bulk unbanning and corrected all occurrences of "unblock" to "unban"! Unfortunately the test file seems to have been modified since we started with this issue - @Anybody, could you try to resolve this as I am not very experienced with resolving core merge conflicts...

@LRWebks makes sense to learn it then :)

I think this is hopefully simple: GitLab reports:

Conflict: This file was modified in the source branch, but removed in the target branch. Ask someone with write access to resolve it.

for
core/modules/ban/tests/src/Functional/IpAddressBlockingTest.php
See https://git.drupalcode.org/project/drupal/-/merge_requests/9095/diffs#di...

So maybe you should first check that. Typically "Update fork" is often enough, but the thing above seems to cause the conflict.

Changes LGTM.

This is a good chance to understand how Git & forking works and how to update the issue fork to the target branch.

Safe hint: Often it's helpful to save the patch (Save target from "plain diff" link above)
So if things go wrong, you can easily restore the status by applying the patch using 3-way-merge.

Reason can be found here: 📌 Convert IpAddressBlockingTest to a Unit and Kernel test and improve Fixed

The tests were moved 3 days ago!

@LRWebks: The learning: Before proceeding work on an (highly active) project (especially core) it makes sense to first update the fork (here in GitLab UI), then pull and then continue working. Then the risk is lower for things like this to happen.

Well, everything should be in order now - I rebased everything and since the old test file was split into a Kernel and a Unit test, the UI test got its own Functional test class now.

The pipeline is currently failing, unfortunately, this is due to the new tests established in 📌 Convert IpAddressBlockingTest to a Unit and Kernel test and improve Fixed ... My test is fine, what now?

phpstan fails

@LRWebks you're right, I reported that at 📌 Convert IpAddressBlockingTest to a Unit and Kernel test and improve Fixed

@LRWebks no I was wrong and that was misleading. You need to update the tests, as you introduced a new parameter to the constructor!

That's why it fails.

Okay, I've updated the BanAdminTest to work with our changes. Back to review!

Left some comments on MR.

Since we are changing UI text may need usability sign off also.

I've tried for a while now to figure out what is causing the test failures, and I must say that I'm really not sure. I have run the failed tests locally, and they work for me. The test failures also seem unrelated again. It could be because I updated the fork and the new commits are causing this - I'll try updating again and see if that changes anything.

Well, that was really it! So what's left now is:

• @smustgrave, please respond to my comment so that I can quickly fix the problem
• Usability Review?

I am not really a big fan of having the "Unban selected" button in the "actions" section:

I think having the button under the table feels "more organically" and more similiar to other drupal lists:

Furthermore, it would be nice if the button only appears if any ips are selected (using the states api).

Great work @lrwebks! I added a few more comments, which should be resolved.

@smustgrave here you said, that changing "unblock" to "unban" might need a "usability sign off" should we instead just use "unblock" instead of "unban" everywhere else?

Instead of a mixture of "unban", "unblock" and "delete"?

Made a few more comments.

The pipeline failure seems unrelated, which happened in this issue before already, so the best solution to that is to wait for another commit to 11.x that fixes the issue and then update the fork.
Until then, this issue can go back to review as all current issues have been resolved.

Gave another round of reviews.

To get this ready for usability tagging for screenshots to be added to the issue summary so they can easily see the change.

@lrwebks can you add screenshots like @smustgrave asked for?

Here is a screenshot of the new ban admin UI:

And here is the "Delete Multiple" confirmation form:

For easy viewing I have also added the screenshots to the issue summary.

Nice! Added 3 more comments!

Back to "Needs work" based on @anybody's last comment.

LGTM! Pipeline green! 😁👍

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@LRWebks: The bot reports:

Running PHPStan on changed files.
------ -----------------------------------------------------------------------
Line src/Form/BanDeleteMultiple.php
------ -----------------------------------------------------------------------
45 Method Drupal\ban\Form\BanDeleteMultiple::create() has no return type
specified.
------ -----------------------------------------------------------------------

[ERROR] Found 1 error

Is that correct and can you fix it?

Hi,

I was getting below error while ran PHPStan on file (core/modules/ban/src/Form/BanDeleteMultiple.php) instead of the error mentioned on #39:

vendor/bin/phpstan analyse core/modules/ban/src/Form/BanDeleteMultiple.php
1/1 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

------ -----------------------------------------------------------------------------------
Line BanDeleteMultiple.php
------ -----------------------------------------------------------------------------------
46 Unsafe usage of new static().
💡 See: https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static
------ -----------------------------------------------------------------------------------

[ERROR] Found 1 error

I have fixed this error and have updated MR, please review it.

Thanks & Regards,
Mrinalini

@mrinalini9 thanks, but the class needs to be final then, read https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static

Hi,

I have made the class final, please review it.

Thanks!

Thanks, back to RTBC.
Screenshots were provided in #32, so I'm removing that tag.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Fixed PHPStan issues reported by bot in #45, please review it.

Thanks!

Screenshots of the current UI isn't an actual section of the summary. Before/after screenshots should go under User interface changes to help the usability team.

Since that section seems to be current UI there appears to be no after in the summary.

Also taking for a CR for the change in workflow/new service.

Will still need usability sign off before RTBC.

@lrwebks can you create a CR (change record) as well, as @smustgrave suggested? There is a link in the sidebar, which leads you to the CR creation, or you can go there directly, through https://www.drupal.org/node/add/changenotice?field_project=3060&field_is... →

Added the CR! Now we are left with the Usability Signoff.

Thanks @LRWebks! Needs usability review left now.

The Ban Module was approved for removal in 📌 [Policy] Deprecate Ban module in Drupal 10 and move it to contrib for Drupal 11 Active .

This remains Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project → and the Extensions approved for removal → policies.

The deprecation work is in 🌱 Deprecate Ban module Active and the removal work in 📌 [12.x] [Meta] Tasks to remove Ban module Postponed .

Ban will be moved to a contributed project after the Drupal 12.x branch is open.