Currently the banned IP-addresses can only be removed one by one in the UI: /admin/config/people/ban
It would be much better to be able to remove them in bulk.
Additionally, (or as simple first step) it would also be helpful to have a "Clear all" option like in dblog (/admin/reports/dblog/confirm)
The current workaround is to delete the record in the database.
Update summary
Change record
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #48 | Bildschirmfoto vom 2024-11-05 11-21-36.png | 26.83 KB | lrwebks |
| #48 | Bildschirmfoto vom 2024-11-05 11-19-23.png | 34.02 KB | lrwebks |
| #45 | 3427545-nr-bot.txt | 2.57 KB | needs-review-queue-bot |
| #38 | 3427545-nr-bot.txt | 2.57 KB | needs-review-queue-bot |
| #32 | Bildschirmfoto vom 2024-09-25 14-48-22.png | 31.64 KB | lrwebks |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
anybodyComment #3
anybodyComment #6
lrwebks commentedComment #7
lrwebks commentedComment #8
lrwebks commentedComment #9
lrwebks commentedI'm currently still writing the appropriate tests for the new behaviors, but @Anybody could already take a look at what's implemented!
Comment #10
anybodyThanks @LRWebks looks great! Just some textual changes.
Comment #11
anybodyComment #12
lrwebks commentedAdded 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...
Comment #13
anybody@LRWebks makes sense to learn it then :)
I think this is hopefully simple: GitLab reports:
for
core/modules/ban/tests/src/Functional/IpAddressBlockingTest.phpSee 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.
Comment #14
anybodyReason can be found here: #3468435: Convert IpAddressBlockingTest to a Unit and Kernel test and improve
The tests were moved 3 days ago!
Comment #15
anybody@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.
Comment #16
lrwebks commentedWell, 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.
Comment #17
lrwebks commentedThe pipeline is currently failing, unfortunately, this seems to be due to the new tests established in #3468435: Convert IpAddressBlockingTest to a Unit and Kernel test and improve... My test is fine, what now?
Comment #18
anybodyphpstan fails
Comment #19
anybody@LRWebks you're right, I reported that at #3468435: Convert IpAddressBlockingTest to a Unit and Kernel test and improve
Comment #20
anybody@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.
Comment #21
lrwebks commentedOkay, I've updated the
BanAdminTestto work with our changes. Back to review!Comment #22
smustgrave commentedLeft some comments on MR.
Since we are changing UI text may need usability sign off also.
Comment #23
lrwebks commentedI'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.
Comment #24
lrwebks commentedWell, that was really it! So what's left now is:
Comment #25
grevil commentedI 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).Edit: Talked with @Anybody about this internally and he thinks using states here might be confusing.
Comment #26
grevil commentedGreat 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"?
Comment #27
grevil commentedMade a few more comments.
Comment #28
lrwebks commentedThe 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.
Comment #29
smustgrave commentedGave 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.
Comment #30
lrwebks commentedComment #31
grevil commented@lrwebks can you add screenshots like @smustgrave asked for?
Comment #32
lrwebks commentedHere 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.
Comment #33
grevil commentedNice! Added 3 more comments!
Comment #34
lrwebks commentedComment #35
grevil commentedBack to "Needs work" based on @anybody's last comment.
Comment #36
lrwebks commentedComment #37
grevil commentedLGTM! Pipeline green! 😁👍
Comment #38
needs-review-queue-bot commentedThe 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.
Comment #39
anybody@LRWebks: The bot reports:
Is that correct and can you fix it?
Comment #41
mrinalini9 commentedHi,
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
Comment #42
anybody@mrinalini9 thanks, but the class needs to be final then, read https://phpstan.org/blog/solving-phpstan-error-unsafe-usage-of-new-static
Comment #43
mrinalini9 commentedHi,
I have made the class final, please review it.
Thanks!
Comment #44
anybodyThanks, back to RTBC.
Screenshots were provided in #32, so I'm removing that tag.
Comment #45
needs-review-queue-bot commentedThe 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.
Comment #46
mrinalini9 commentedFixed PHPStan issues reported by bot in #45, please review it.
Thanks!
Comment #47
smustgrave commentedScreenshots 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.
Comment #48
lrwebks commentedComment #49
grevil commented@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...
Comment #50
lrwebks commentedAdded the CR! Now we are left with the Usability Signoff.
Comment #51
anybodyThanks @LRWebks! Needs usability review left now.
Comment #52
lrwebks commentedComment #53
quietone commentedThe Ban Module was approved for removal in #1570102: [Policy] Deprecate Ban module.
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 #3482198: [meta] Tasks to deprecate the Ban module and the removal work in #3488827: [meta] Tasks to remove Ban module.
Ban will be moved to a contributed project after the Drupal 12.x branch is open.
Comment #54
anybodyMoving this over to the Ban module. I'm unsure if we should finish this here or instead wait for a possible merge of the Ban and the Advban module (that already may have this functionality)?
Comment #55
anybodySorry for moving this over too early, my mistake -.-
Reverting #54.
We'll prepate separate MRs against core and the contrib ban module to be safe.
Comment #57
mstrelan commentedThis is good to move now
Comment #58
anybodyLet's also finish this one!
Comment #62
grevil commentedApplied the core patch to the contrib module.
I'll test and finalize this once #2972332: Introduce the ability to use CIDR Notation to ban IP-Ranges is merged. Since there will be a few merge conflicts.
Comment #63
anybodyComment #64
grevil commentedSince #2972332: Introduce the ability to use CIDR Notation to ban IP-Ranges is postponed for now, I'll finish this now.
Comment #65
grevil commentedDid some final adjustments, should be good to go.
Comment #66
anybodyThank you all for the implementation and especially the tests! This LGTM, I just added two minor questions / phrasing suggestions.
Maybe @smustgrave and @mstrelan could check this for final RTBC and leave their feedback?
Very helpful UX addition, thanks! Let's get this finished.
Comment #69
grevil commentedWaiting for feedback from others regarding https://git.drupalcode.org/project/ban/-/merge_requests/9#note_718632.
Comment #70
anybodyCopying my comment from the MR here for reference regarding logging:
Comment #71
anybodyTests are present, manually testing also was fine, so RTBC! Thank you all!
Comment #73
grevil commented