Implement fast track deletion for unused wikis#605
Implement fast track deletion for unused wikis#605pixDeVl wants to merge 18 commits intomiraheze:mainfrom
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis pull request restructures the configuration for wiki state days by nesting the previous flat structure into two objects: Changes
Sequence Diagram(s)sequenceDiagram
participant Cron as Cron Job
participant MIW as ManageInactiveWikis
participant Config as Configuration
participant Wiki as Wiki Data
Cron->>MIW: execute()
MIW->>MIW: checkLastActivity()
MIW->>Config: Retrieve 'default' state config
alt 'default' config exists
MIW->>MIW: Determine config (default/no-edits) based on last activity
else 'default' missing
MIW->>MIW: Throw ConfigException
end
MIW->>Wiki: Update inactivity, closure, and removal statuses
sequenceDiagram
participant User as User Action
participant WMF as WikiManagerFactory
participant Config as Configuration
participant Wiki as Wiki Data
User->>WMF: Request deletion
WMF->>Config: Retrieve deletion grace period from 'no-edits' config
WMF->>WMF: Evaluate deletion conditions
alt Conditions met
WMF->>Wiki: Perform deletion
else Conditions not met
WMF->>User: Return error/skip deletion
end
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
BlankEclair
left a comment
BlankEclair
left a comment
There was a problem hiding this comment.
Apologies for the delay--was busy setting up my new phone, and you know how that is... I have to copy everything, then configure all my settings, then reverse engineer one of my banking apps to figure out how it detects root. Y'know, all those typical things
maintenance/manageInactiveWikis.php
Outdated
| $dbName = $row->wiki_dbname; | ||
| $remoteWiki = $remoteWikiFactory->newInstance( $dbName ); | ||
| $inactiveDays = (int)$this->getConfig()->get( ConfigNames::StateDays )['inactive']; | ||
| $inactiveDays = (int)$this->getConfig()->get( ConfigNames::StateDays )['default']['inactive']; |
There was a problem hiding this comment.
I wonder if this should be no-edits?
There was a problem hiding this comment.
I think so? I'll do some thinking, but also need to look to make sure we're not breaking any previously implemented assumptions about the state from when there was only one track.
There was a problem hiding this comment.
If the no_edits_inactivity < default_inactivity, but we only do checking for when it's default, then the no edits stuff won't trigger, right?
There was a problem hiding this comment.
Updated to no-edits, but we should take a look to see if that doesn't break and assumptions and cause it to early close any wikis it shouldn't. A bit late for me to be doing a thoughtful look right now though.
|
Looks ~~gay~~ good to me
|
Universal-Omega
left a comment
Universal-Omega
left a comment
There was a problem hiding this comment.
Mostly very minor nitpick stuff, but one critical issue as well.
maintenance/manageInactiveWikis.php
Outdated
|
|
||
| $lastActivityTimestamp = $activity->getTimestamp(); | ||
|
|
||
| $track = ( $lastActivityTimestamp !== 0 ) ? 'default': 'no-edits' |
There was a problem hiding this comment.
Unnecessary parentheses and syntax error. (missing semicolon)
$track = $lastActivityTimestamp ? 'default' : 'no-edits';
Also we need some validation here if configured incorrectly. If no-edits is not set at all, always use default, if neither is not set use the old behavior and add null checks with ??. Maybe this should be split to seperate method? I guess we could just throw ConfigException if configured incorrectly also...
There was a problem hiding this comment.
@Universal-Omega configured incorrectly being not defining no edits or default or just not defining no edits?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #605 +/- ##
============================================
- Coverage 26.55% 23.47% -3.08%
- Complexity 819 820 +1
============================================
Files 50 50
Lines 4135 4140 +5
============================================
- Hits 1098 972 -126
- Misses 3037 3168 +131 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| public function getTimestamp(): int { | ||
| $defaultActor = $this->getServiceContainer()->getUserFactory() |
There was a problem hiding this comment.
Probably should have a test case in CheckLastWikiActivityTest to make sure properly excludes MediaWiki default edits.
| $this->fatalError( 'Enable $wgCreateWikiEnableManageInactiveWikis to run this script.' ); | ||
| } | ||
|
|
||
| $inactiveDays = (int)$this->getConfig()->get( ConfigNames::StateDays )['no-edits']['inactive']; |
There was a problem hiding this comment.
Is there a reason we hardcode no-edits here? I'm uncertain about that.
There was a problem hiding this comment.
Performance wise, the check this is used in is to tell if it's worth doing more processing. Using the function to get the proper check would mean that code(querying the database) runs for every wiki even if they're active, while if we leave it here, checklastActivity() would run on every wiki that hasn't been edited for at least 7 days under default config. So I guess it depends on which performance hit we think will hurt less? It would probably be activity->getTimestamp() I feel.. in which case we should change it.
There was a problem hiding this comment.
This also needs tests in ManageInactiveWikisTest but I can probably put some work into doing that at some point.
Universal-Omega
left a comment
Universal-Omega
left a comment
There was a problem hiding this comment.
Also needs updates in https://github.com/miraheze/CreateWiki/blob/master/includes/Services/WikiManagerFactory.php#L315 to support new format
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
extension.json(1 hunks)includes/Services/WikiManagerFactory.php(1 hunks)maintenance/CheckLastWikiActivity.php(2 hunks)maintenance/ManageInactiveWikis.php(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.php`: Follow MediaWiki PHP Coding Standards:
Indentat...
**/*.php: Follow MediaWiki PHP Coding Standards:Indentation:
- Use tabs for indentation, not spaces.
Spaces:
- Use spaces on either side of binary operators.
Example: $a = $b + $c; (Correct) $a=$b+$c; (Incorrect)- Add spaces inside parentheses, except where they are empty.
Example: $a = getFoo( $b ); (Correct) $a = getFoo($b); (Incorrect)- Do not add a space after a function name.
Example: $c = getBar(); (Correct) $c = getBar (); (Incorrect)Function Return Type Hint:
- Add a space after the colon in the function return type hint.
Example: function square( int $x ): int { return $x * $x; }Arrays:
- Use spaces in brackets when declaring an array, except where the array is empty.
Example: $a = [ 'foo', 'bar' ]; (Correct) $a = ['foo', 'bar']; (Incorrect)- Do not put spaces in brackets when accessing array elements.
Example: $c = $a[0]; (Correct) $c = $a[ 0 ]; (Incorrect)Control Structures:
- Add a space after control structure keywords.
Example: if ( isFoo() ) { ... } (Correct) if(isFoo()){ ... } (Incorrect)Type Casting:
- Do not use a space within or after the cast operator.
Example: (int)$foo; (Correct) (int) $bar; (Incorrect)Comments:
- In comments, there should be one space between the # or // and the comment text.
Example: // This is a comment. (Correct) //This is a comment. (Incorrect)Ternary Operator:
- Keep ternary operator expressions short and obvious.
- For multi-line ternary expressions, place the '?' and ':' at the beginning of the new lines.
File Formatting:
- Avoid trailing whitespace on lines.
maintenance/CheckLastWikiActivity.phpmaintenance/ManageInactiveWikis.phpincludes/Services/WikiManagerFactory.php
🔇 Additional comments (7)
maintenance/CheckLastWikiActivity.php (1)
44-44: Good implementation of additional filtering conditionThe new condition successfully extends the existing filters to exclude the default actor's actions from activity calculations.
includes/Services/WikiManagerFactory.php (1)
314-314:Details
❓ Verification inconclusive
Updated to use the new configuration structure for deletion grace period
This change properly aligns the deletion logic with the restructured configuration by accessing the 'deleted' value from the 'no-edits' section instead of from the root level.
Let's verify that the configuration access is consistent throughout the codebase:
🏁 Script executed:
#!/bin/bash # Check for any other direct references to StateDays that might need updating rg -t php "StateDays\)\[\"" --include="*.php" -A 1 -B 1Length of output: 498
Configuration access verification for deletion grace period update
File: includes/Services/WikiManagerFactory.php (Line 314)
The deletion grace period is now calculated using the new configuration structure. The code:
$deletionGracePeriod = (int)$this->options->get(ConfigNames::StateDays)["no-edits"]['deleted'] * 86400;correctly retrieves the ‘deleted’ value from the nested 'no-edits' section, aligning with the updated configuration.
Action Required:
Please verify that this revised configuration access pattern is applied consistently throughout the codebase. To assist with this, run the following command which uses the corrected glob flag to scan PHP files for additional references:#!/bin/bash # Check for any other direct references to StateDays that might need updating rg -t php "StateDays\)\[\"" --glob "*.php" -A 1 -B 1extension.json (1)
385-396: Restructured configuration to support fast-track deletionThe configuration has been refactored from a flat structure to a nested one with 'default' and 'no-edits' objects. This allows for different inactivity/deletion timelines based on whether a wiki has been edited.
The 'no-edits' configuration accelerates the process:
- Inactive after 15 days (vs 45 days in default)
- Closed after 30 days (vs 15 days after inactivity in default)
- Immediate removal and deletion (0 days) once closed
This implementation aligns with the PR objective to implement fast-track deletion for unused wikis.
maintenance/ManageInactiveWikis.php (4)
6-6: Added ConfigException import for error handlingGood addition of the ConfigException import to properly handle configuration errors.
36-37: Optimized inactiveDays retrievalMoving the inactiveDays retrieval outside the loop is a good optimization, as it avoids repeatedly retrieving the same configuration value for each wiki.
92-92: Logical track selection based on activityGood implementation of determining which configuration track to use based on last activity timestamp. This is a key part of the fast-track deletion logic.
94-97: Updated configuration access to use track-specific valuesThe code now properly retrieves configuration values from the appropriate track (default or no-edits), ensuring the correct timing rules are applied based on wiki activity.
| $defaultActor = $this->getServiceContainer()->getUserFactory() | ||
| ->newFromName( 'MediaWiki default' ) | ||
| ->getActorId(); |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Improvement to exclude MediaWiki default actor from activity checks
This change adds logic to filter out log entries from the MediaWiki default actor, ensuring that only meaningful user activities are counted when determining wiki activity.
A test case in CheckLastWikiActivityTest would be valuable to verify that this properly excludes MediaWiki default edits, as mentioned in a previous review.
🏁 Script executed:
#!/bin/bash
# Check if any test exists for this functionality
rg -t php "MediaWiki default" tests/phpunitLength of output: 45
Action Required: Add a test to verify MediaWiki default actor exclusion
The logic in maintenance/CheckLastWikiActivity.php (lines 22–24) correctly retrieves the actor ID for "MediaWiki default" to later exclude its log entries from wiki activity checks. However, our search in the test suite didn’t find any test case covering this functionality. To fully validate that log entries from the MediaWiki default actor are excluded, please add a test case (preferably in CheckLastWikiActivityTest.php) that explicitly checks for this behavior.
Co-authored-by: Claire <blankeclair@disroot.org>
Co-authored-by: Claire <blankeclair@disroot.org>
3 participants
This was requested and approved by the community in https://meta.miraheze.org/wiki/Requests_for_Comment/Content_and_Dormancy_Policies back in March(over half a year ago, so long overdue and then some).
Since many, many, many wiki requests unfortunately never really get into editing and just give up on or forget about making entirely, leaving swarms of ghost wikis on the site taking up space.
This pull request implements the 'fast-track' deletion, where wikis that have no edits(besides MediaWiki default) are marked as inactive after 15 day, and deleted after 30.
Rest in peace our 10k badge.
Summary by CodeRabbit