Skip to content

Conversation

@agoujot
Copy link
Member

@agoujot agoujot commented Apr 14, 2025

  1. (re)blocks actually doesn't count reblocks. One of the commits changes blocksOnly to include reblocks (and so exclude only unblocks), as it's only used for this purpose.
  2. Currently, for infinite reblocks of non-infinite blocks, the code is made so that the longest block duration will be recorded as (time between start of block and reblock)-1 seconds, instead of infinite. One of these commits fixes that part of getLongestBlockSeconds so that it does return infinite for these cases
  3. ipblocks is deprecated since MW 1.42 (June 2024); eventually it'll cause issues. The last commit switches the query to the new recommended tables. This may or may not be related to an issue with "current block" listed as "-" for many users who are blocked. If it is, then good; if it isn't, using deprecated stuff is never a good idea.

Note:

  • Although I've read a fair bit of PHP, I very rarely write some, so please check I haven't made syntax errors or other stuff like that;
  • And I'm not even remotely close to having a working testing environment, so I'd appreciate if someone else can test what I made.

Bug: T391824

agoujot added 3 commits April 14, 2025 18:03
previously, they were counted as reducing of 1 second the duration of the previous block, instead of setting it to infinite. See https://phabricator.wikimedia.org/T391824#10739130
ipblocks was deprecated in MW 1.42 (WMF sites are at 1.44), and I think that this query is the cause of Xtools not counting some reblocks for "current block". see https://phabricator.wikimedia.org/T391824#10739714
else the count of "re-blocks" actually only includes blocks, which, you know, is a bit weird. I'd say it's this or changing the interface message. see https://phabricator.wikimedia.org/T391824#10739502
@MusikAnimal
Copy link
Member

Thanks for this! I will review this soon. Looks like I need to fix CI, too.

block and block_target, and not blocks and blocks (what was I thinking!), per mw
@agoujot
Copy link
Member Author

agoujot commented Apr 14, 2025

For the table switch, specifically, I just corrected some mistakes I'd made (in the actual names), but I'm not sure what should be put in the last parameter of getTableName.

agoujot added 2 commits April 14, 2025 22:58
I really need to get used to PHP
forgot to do that after tests at quarry with fixed value (the number of things I managed to forget is incredible)
@MusikAnimal
Copy link
Member

MusikAnimal commented Apr 16, 2025

Can you rebase (or merge in main) and push again, for all of your PRs? I just fixed CI finally :)

I hope to everything reviewed before this Friday.

Thanks for contributing! 😄

@codecov
Copy link

codecov bot commented Apr 17, 2025

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 71.00%. Comparing base (4b8eb28) to head (9adb3df).
Report is 64 commits behind head on main.

Files with missing lines Patch % Lines
src/Model/EditCounter.php 60.00% 2 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main     #495      +/-   ##
============================================
- Coverage     71.30%   71.00%   -0.31%     
- Complexity     1275     1295      +20     
============================================
  Files            46       46              
  Lines          3792     3900     +108     
============================================
+ Hits           2704     2769      +65     
- Misses         1088     1131      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@MusikAnimal MusikAnimal left a comment

Choose a reason for hiding this comment

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

💯 Thank you!!! I'm surprised these bugs have gone unnoticed for this long!

Don't worry about the tests, since you aren't able to set up a dev environment. I can add coverage in a follow-up.

@MusikAnimal MusikAnimal merged commit 666e06a into x-tools:main Apr 17, 2025
1 of 3 checks passed
@MusikAnimal
Copy link
Member

Also FYI, multiblocks is coming soon (hence the schema change), so we'll need to rethink how we display the data. Only two wikis have it enabled right now, so it's not urgent.

@agoujot
Copy link
Member Author

agoujot commented Apr 17, 2025

Also, I was wondering: with what frequency is xtools.wmcloud.org updated? To see the effect notably if the table change had something to do with the issue with current blocks issue.

@agoujot agoujot deleted the fixreblocks branch April 17, 2025 10:58
@agoujot agoujot restored the fixreblocks branch April 17, 2025 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants