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.

Sign up for GitHub

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

Jump to bottom

Global bans and ban editing #854

Merged
merged 5 commits into from
Feb 24, 2024
Merged

Global bans and ban editing #854

merged 5 commits into from
Feb 24, 2024

Conversation

stwalkerster
Copy link
Member

@stwalkerster stwalkerster commented Sep 21, 2023 •
edited
Loading

Implements global bans and ban editing

@stwalkerster stwalkerster changed the base branch from master to db-cleanup September 21, 2023 18:18
This was linked to issues Sep 21, 2023
Ban modification interface #641
Closed
Implement global bans #853
Closed
@stwalkerster stwalkerster changed the title Multiproject/globalbans Global bans and ban editing Sep 21, 2023
@stwalkerster stwalkerster marked this pull request as ready for review September 21, 2023 22:55
@stwalkerster stwalkerster added the blocked-by-other This task is blocked from progressing by another issue label Sep 21, 2023
@stwalkerster
Copy link
Member Author

stwalkerster commented Sep 21, 2023 •
edited
Loading

Blocked on #852 being merged. Once that's good-to-go, this will be too.

@stwalkerster stwalkerster added the schema changes This task requires database schema changes label Sep 21, 2023
@stwalkerster stwalkerster force-pushed the multiproject/globalbans branch 2 times, most recently from 81c0121 to 2edc797 Compare September 25, 2023 17:21
@FastLizard4 FastLizard4 mentioned this pull request Nov 13, 2023
Open
Base automatically changed from db-cleanup to master November 22, 2023 23:14
FastLizard4
FastLizard4 reviewed Nov 23, 2023
View reviewed changes
Copy link
Member

@FastLizard4 FastLizard4 left a comment

Choose a reason for hiding this comment

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

One question and in-line review comment. But I also went to test this locally and /internal.php/bans/set just spat out several pages of PHP warnings; may be worth looking into? Otherwise things LGTM.

includes/DataObjects/Ban.php Show resolved Hide resolved
includes/DataObjects/Log.php Show resolved Hide resolved
@FastLizard4
Copy link
Member

FastLizard4 commented Nov 23, 2023 •
edited
Loading

Here's what I have after some grepping:

( ! ) Warning: Undefined array key "replaceBanId" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 63
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 63
( ! ) Warning: Undefined array key "banName" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 89
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 89
( ! ) Warning: Undefined array key "banIP" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 102
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 102
( ! ) Warning: Undefined array key "banEmail" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 120
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 120
( ! ) Warning: Undefined array key "banReason" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 150
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 150
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 170
( ! ) Warning: Undefined array key "banDuration" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 175
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 175
( ! ) Warning: Undefined array key "banAction" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 187
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 206
( ! ) Warning: Undefined array key "banQueue" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 219
( ! ) Warning: Undefined array key "banVisibility" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 235
( ! ) Warning: Undefined array key "banGlobal" in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 250
( ! ) Warning: Attempt to read property "value" on null in /var/www/html/templates_c/38d864d996b15129e862ec0740be547b4de7d8c1_0.file.banform.tpl.php on line 250

This is from trying to create a new ban.

@stwalkerster stwalkerster removed the blocked-by-other This task is blocked from progressing by another issue label Dec 2, 2023
@stwalkerster
Implement global bans
58af654 
Adds a "global" flag to the ban UI, though in practice this is just UI
sugar for a nullable domain column. If domain is null, then the ban
applies globally. If it's a domain ID, the ban applies in that domain
only.

Also adds a new ban type flag which is granted to tool roots only to
allow setting global bans.

Fixes #853
@stwalkerster
Documentation and typehinting on PageBan
503f421
@stwalkerster
Implement ban "editing"
446b2cf 
This is done as UI sugar around an unban followed by an immediate ban.
This helps maintain the ban history for investigation of what happened
to certain requests.

Fixes #641
@stwalkerster
Run CI on all pull requests
458042e
@stwalkerster
Fix PHP notices, typehint
2084e16
FastLizard4
FastLizard4 approved these changes Feb 21, 2024
View reviewed changes
Copy link
Member

@FastLizard4 FastLizard4 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code LGTM and local testing revealed only a minor issue which I think can be deferred to a follow-up PR: The log for replacing a ban is not sent to IRC, so when a ban is replaced, it appears to IRC that a new ban has been set without affecting the old ban at all. The in-tool logs correctly reflect the replacement, however. I assume this is because \Waca\Pages\PageBan::replace() doesn't issue a notification for the replacement after logging it. Also as a follow-up, I wonder if it would be beneficial to include the new ban ID as a comment for the ban replaced log entry?

image

(Note that message 3 got included twice in my screenshot below, somehow.)

CleanShot 2024-02-20 at 21 29 01@2x

@stwalkerster stwalkerster mentioned this pull request Feb 22, 2024
Open
@stwalkerster
Copy link
Member Author

Code LGTM and local testing revealed only a minor issue which I think can be deferred to a follow-up PR: The log for replacing a ban is not sent to IRC, so when a ban is replaced, it appears to IRC that a new ban has been set without affecting the old ban at all.
#908 opened to capture this

Also as a follow-up, I wonder if it would be beneficial to include the new ban ID as a comment for the ban replaced log entry?

Log entries which touch multiple objects are currently represented as multiple log entries in other places in the tool. A good example of this is sending a reservation.

I don't like that approach, but it's also a larger change. #607 is related here.

@stwalkerster stwalkerster merged commit 2084e16 into master Feb 24, 2024
4 checks passed
@stwalkerster stwalkerster deleted the multiproject/globalbans branch February 24, 2024 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FastLizard4 FastLizard4 FastLizard4 approved these changes

Assignees
No one assigned
Labels
schema changes This task requires database schema changes
Projects
Multi-project support
Status: Done
Review queue
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement global bans Ban modification interface
2 participants
@stwalkerster @FastLizard4