Theoretically this wouldn't be too hard (I think without looking at the code) since we already join against the block table to check to see if user is revdeleted.

ashishmittal.mail wrote:

Hi,

I would like to take this as my first bug and attempt to fix it. Kindly give some more guidelines as to what needs to be done.

Thanks,
Aashish

(In reply to comment #2)

Hi,

I would like to take this as my first bug and attempt to fix it. Kindly give
some more guidelines as to what needs to be done.

Thanks,
Aashish

Awesome!

I'm going to assume you already have a trunk copy of MediaWiki downloaded, and some knowledge of SQL. (If you don't know anything about SQL this might not be a good choice for a first bug).

The file you would have to modify is includes/specials/SpecialListusers.php
You would have to modify UserPager::getPageHeader to add some sort of interface to trigger the filtering, UserPager::__construct to set some flag to indicate the filtering is in use, and UserPager::getQueryInfo to actually filter via blocked status. The last part will probably be the hardest part - you'll have to change the outer join condition to still join on non-"deleted" blocks (at least when doing this filtering), change the ipb_deleted condition to be ok with either 0 or null (since you'd sometimes have it joined on non-deleted blocks after the change) and add some conds for the actual filtering based on being blocked.

ashishmittal.mail wrote:

Hello,

Thanks for the guidance.

As per your guidelines, I have modified the functions getPageHeader() and __construct() to get the checkbox for hiding temporary blocks.

Mainly, I modified the getQueryInfo to change the join and the conds.

Firstly I removed the 'ipb_deleted=1' from the join condition to consider the non-deleted blocks. So the join returns the non-blocked (ipb_d=null), the non-deleted blocks (ipb_d=0) and the deleted blocks (ipb_d=1).

Secondly I modified the cond for users without 'hideuser' rights from 'ipb_deleted IS NULL' to 'ipb_deleted = 0 OR ipb_deleted IS NULL'. As a result of this, the non-blocked (ipb_d=null) and the non-deleted blocked users (ipb_d=0) are visible to the users without hideuser rights and the deleted blocks are not visible.

Next, I added a cond 'ipb_deleted IS NULL' for filtering the blocks if the checkbox is checked since the blocked users will have a not-null ipb_deleted.

On testing, I am getting the expected results. Kindly mention if this method is fine and should I go ahead and create a patch.

Thanks,
Aashish

That sounds like you're going in the right direction. Please post patch to this bug (in unified diff format).

ashishmittal.mail wrote:

Added an option in Special:ListUsers to filter users based on their blocked status

Attached the patch containing the mentioned modifications. Kindly give your review.

Thanks,
Aashish

Attached:

Adding relavent keywords to denote there is a patch.

In general one should create new i18n messages rather than re-using existing ones ( blocklist-tempblocks ), but that's a minor issue. I'll take a more fuller look at the patch tomorrow.

(In reply to comment #7)

Adding relavent keywords to denote there is a patch.

In general one should create new i18n messages rather than re-using existing
ones ( blocklist-tempblocks ), but that's a minor issue. I'll take a more
fuller look at the patch tomorrow.

It's been 3 days! where are you???

Sorry things got busy in real life. I promise to look at this tomorrow (Saturday)

Sorry for the delay. Some comments:

*It'd probably be a good idea to run svn up before submitting a patch, just because people make changes to code base as time goes on (In this case somebody migrated ListUser's to use RequestContext which changed some variable names from $wgRequest to $request).

*As I mentioned before, a new message should be used for the checkbox label (instead of doing wfMsg( 'blocklist-tempblocks') ). You can add a new message by adding an entry to the giant array at /includes/langauges/messages/MessagesEn.php (Any new message should have a quick documentation entry in includes/langauges/messages/MessagesQqq.php to help the translators. You don't have to worry about the other message files, the translators will translate the message in due time).

*This currently would also get people who have temp blocked that are now expired (such entries eventually get removed from the ipblocks table, but not immediatly)

It will need some sort of condition in the conds[] array, as an OR to the is NULL condition, that's something like 'ipb_deleted = 0 and ipb_expiry < ' . $dbr->addQuotes( $dbr->timestamp( wfTimestampNow() ) ) in order to catch already expired blocks

(which should work for infinite blocks since infinite supposedly sorts after all timestamps).

Otherwise, it looks like a very good start.

Other possibilities with this feature include the possibility of adding a css class to blocked users (when not filtering by blocked) so people could style them greyed out if they wanted.

Another thing to consider for this, do we want separate check boxes for hiding people blocked indefinitly vs people just blocked (temporarily) or should we just have one checkbox for all blocks (since more broad), or should we only allow filtering out indef blocked users (on the notion that temp blocked folks are being punished, but indef folks are permenantly kicked out). Having two boxes seems a little clutery in my opinion, but I'm not really sure.

zsaigol wrote:

Enhanced patch to allow drop-down for type of blocked user to show

Created an enhanced patch that

• Shows a drop-down for selecting whether to hide all, temp or permanent blocks
• Uses messages from the MessagesEn.php file

Attached:

zsaigol: Thanks for your patch!

You are welcome to use Developer access

https://www.mediawiki.org/wiki/Developer_access

to submit this as a Git branch directly into Gerrit:

https://www.mediawiki.org/wiki/Git/Tutorial

Putting your branch in Git makes it easier to review it quickly.
Thanks again! We appreciate your contribution.

zsaigol wrote:

Hi, I've got a reviewable changeset in Gerrit:
https://gerrit.wikimedia.org/r/#/c/79041/

Have already added some potential reviewers - hope that's OK.

sumanah wrote:

Asking Jared whether his team could take a look at this from a design perspective, and asking Steve Zhang for his perspective as well.

cro0016 wrote:

This looks good to me, I can't really see any changes I'd make or any bugs I can see with the code.

zsaigol wrote:

Any progress on reviewing the patch I submitted for this?
Thanks!

Patch received reviews now, needs some rework...

zsaigol wrote:

I've looked at the problems with CentralAuth -- my suggestion is that these can be fixed under a new bug-report for CentralAuth page Special:GlobalUsers. Then the patchset I submitted would be fine as it is.

Is this on your radar James?

Looks like that an existing patch "just" needs to be fluffed for current master. I believe that I am not the only one who would like this become reality. Keeping fingers crossed.