Hi. Is there some reason why the autoreview group (aka autochecked users) is implicit? It can neither be given to nor taken away from a user by a sysop/bureaucrat unlike the editor privilege.
Handling the autopromotion to an autochecked user in the same way as to an editor would IMHO give the project administration more control over it. For example, someone makes more harm than good - take autoreview away from them. Someone has been blocked once but their edits are OK - give it to them. And so on.
And actually I can't see any drawbacks of it but I don't know much about technical details. Are there some any?
I'm talking with regard to a discussion on Polish wiki, which I have begun, on enabling autoreview group. The implicitness of it has become an obstacle.
Regards lampak
On Wed, Aug 25, 2010 at 3:53 PM, lampak llampak@gmail.com wrote:
Is there some reason why the autoreview group (aka autochecked users) is implicit? It can neither be given to nor taken away from a user by a sysop/bureaucrat unlike the editor privilege.
All automatically granted groups are implicit. If you want to be able to revoke them, you can add a new group like "autoreview-revoked" and have it revoke the permissions and/or the implicit group itself. A general mechanism to manually add or remove any autopromoted group would be useful, but this workaround will do the trick for now.
On 25/08/10 22:16, Aryeh Gregor wrote:
On Wed, Aug 25, 2010 at 3:53 PM, lampakllampak@gmail.com wrote:
Is there some reason why the autoreview group (aka autochecked users) is implicit? It can neither be given to nor taken away from a user by a sysop/bureaucrat unlike the editor privilege.
All automatically granted groups are implicit. If you want to be able to revoke them, you can add a new group like "autoreview-revoked" and have it revoke the permissions and/or the implicit group itself. A general mechanism to manually add or remove any autopromoted group would be useful, but this workaround will do the trick for now.
All automatically granted groups are implicit.
Editors aren't. This is because they are autopromoted in ArticleSaveComplete hook instead of GetAutoPromoteGroups. What I'm asking for is moving autoreview autopromotion to the same hook so both groups would behave in the same way.
lampak
On Wed, Aug 25, 2010 at 4:36 PM, lampak llampak@gmail.com wrote:
Editors aren't. This is because they are autopromoted in ArticleSaveComplete hook instead of GetAutoPromoteGroups. What I'm asking for is moving autoreview autopromotion to the same hook so both groups would behave in the same way.
Oh, blech, FlaggedRevs reinvented the wheel and made up its own autopromote system. At a glance, it looks like it's hardcoded to only work for the editor group, so no, it can't be reused for other groups without some refactoring. It should really be refactored to work for all groups, but some of the checks are pretty expensive -- looks like maybeMakeEditor() in FlaggedRevs.hooks.php can run several database queries in some cases.
On 25/08/10 22:45, Aryeh Gregor wrote:
On Wed, Aug 25, 2010 at 4:36 PM, lampakllampak@gmail.com wrote:
Editors aren't. This is because they are autopromoted in ArticleSaveComplete hook instead of GetAutoPromoteGroups. What I'm asking for is moving autoreview autopromotion to the same hook so both groups would behave in the same way.
Oh, blech, FlaggedRevs reinvented the wheel and made up its own autopromote system. At a glance, it looks like it's hardcoded to only work for the editor group, so no, it can't be reused for other groups without some refactoring. It should really be refactored to work for all groups, but some of the checks are pretty expensive -- looks like maybeMakeEditor() in FlaggedRevs.hooks.php can run several database queries in some cases.
I'm not looking for code to autopromote to any arbitrary groups - just the two: editor and autoreview. Can't it be re-hardcoded then?
Speaking of database queries: currently (unless there's some hidden caching I can't see), the same queries, like the test for totalCheckedEdits, are done twice - in maybeMakeEditor and checkAutoPromote. Merging both functions could only decrease their number.
BTW, maybe they have reinvented the wheel, but the new wheel clearly works better.
lampak
On Wed, Aug 25, 2010 at 5:12 PM, lampak llampak@gmail.com wrote:
I'm not looking for code to autopromote to any arbitrary groups - just the two: editor and autoreview. Can't it be re-hardcoded then?
Sure, if someone wants to do it. It'd be pretty easy. But it would require code changes, not just configuration.
BTW, maybe they have reinvented the wheel, but the new wheel clearly works better.
All the more reason that it should have been implemented as improvements to the preexisting system, so that all groups could take advantage of it. The core system has advantages too, mainly being much more flexible -- arbitrary anding/oring/xoring/nesting of conditions, and easy extensibility. Plus, well, it can actually be used for groups other than 'editor'. :)
On Wed, Aug 25, 2010 at 2:39 PM, Aryeh Gregor Simetrical+wikilist@gmail.com wrote:
On Wed, Aug 25, 2010 at 5:12 PM, lampak llampak@gmail.com wrote:
On 25/08/10 22:45, Aryeh Gregor wrote:
Oh, blech, FlaggedRevs reinvented the wheel and made up its own autopromote system. At a glance, it looks like it's hardcoded to only work for the editor group, so no, it can't be reused for other groups without some refactoring. It should really be refactored to work for all groups, but some of the checks are pretty expensive -- looks like maybeMakeEditor() in FlaggedRevs.hooks.php can run several database queries in some cases.
BTW, maybe they have reinvented the wheel, but the new wheel clearly works better.
All the more reason that it should have been implemented as improvements to the preexisting system, so that all groups could take advantage of it. The core system has advantages too, mainly being much more flexible -- arbitrary anding/oring/xoring/nesting of conditions, and easy extensibility. Plus, well, it can actually be used for groups other than 'editor'. :)
Could someone(s) write up a design doc and put something in Bugzilla requesting this? It seems like a sensible thing to do, but not likely to happen without prodding.
Rob
On Wed, Aug 25, 2010 at 6:35 PM, Rob Lanphier robla@wikimedia.org wrote:
Could someone(s) write up a design doc and put something in Bugzilla requesting this? It seems like a sensible thing to do, but not likely to happen without prodding.
I filed a bug:
https://bugzilla.wikimedia.org/show_bug.cgi?id=24948
The one fixing the bug would design the fix, presumably. That's generally easier than fixing someone else's design anyway.
On 26/08/10 21:53, Aryeh Gregor wrote:
On Wed, Aug 25, 2010 at 6:35 PM, Rob Lanphierrobla@wikimedia.org wrote:
Could someone(s) write up a design doc and put something in Bugzilla requesting this? It seems like a sensible thing to do, but not likely to happen without prodding.
I filed a bug:
https://bugzilla.wikimedia.org/show_bug.cgi?id=24948
The one fixing the bug would design the fix, presumably. That's generally easier than fixing someone else's design anyway.
I have started to do it (or at least try). The main problem is: how to check if the user has ever belonged to the group? Should I (a) search the log or (b) create a new database table for former user groups or (c) add a column to the user_groups table so that if a user is kicked off some group the row is not deleted but the value is changed?
I'm not sure if the goal is not too trivial for (b) and (c). But (a) doesn't seem very elegant to me. (Was the log meant to be read by other parts of the code?)
From all these options (c) seems the neatest. But that would mean searching for all places where this table is used... And I don't really feel like altering core tables. (b) would be simpler but produces some redundancy...
Any advice?
lampak
On Sun, Aug 29, 2010 at 8:08 AM, lampak llampak@gmail.com wrote:
I have started to do it (or at least try). The main problem is: how to check if the user has ever belonged to the group? Should I (a) search the log or (b) create a new database table for former user groups or (c) add a column to the user_groups table so that if a user is kicked off some group the row is not deleted but the value is changed?
I'm not sure if the goal is not too trivial for (b) and (c). But (a) doesn't seem very elegant to me. (Was the log meant to be read by other parts of the code?)
From all these options (c) seems the neatest. But that would mean searching for all places where this table is used... And I don't really feel like altering core tables. (b) would be simpler but produces some redundancy...
(b) seems like the best idea to me. It doesn't create any redundancy that I can see -- at least not in the database-normalization sense -- and it's the most compatible with existing code. (a) is unreliable, and (c) will break old users of the table in a pretty terrible way (group removal won't work).
lampak wrote:
I have started to do it (or at least try). The main problem is: how to check if the user has ever belonged to the group? Should I (a) search the log or (b) create a new database table for former user groups or (c) add a column to the user_groups table so that if a user is kicked off some group the row is not deleted but the value is changed?
I'm not sure if the goal is not too trivial for (b) and (c). But (a) doesn't seem very elegant to me. (Was the log meant to be read by other parts of the code?)
From all these options (c) seems the neatest. But that would mean searching for all places where this table is used... And I don't really feel like altering core tables. (b) would be simpler but produces some redundancy...
Any advice?
lampak
You seem to be using it just to not promet him again when he has been removed. Why not change the user_groups entry to something like "~editor" ? (ie. "not editor")
Only User.php and UserRightsProxy.php should need to be changed (the api should format them differently, too, but that's no funcitonality change). Most uses in core are just to check if he's a bot, and if an extension accessed directly to the db (which shouldn't), would just see an unknown group.
On Mon, Aug 30, 2010 at 8:00 AM, Platonides Platonides@gmail.com wrote:
You seem to be using it just to not promet him again when he has been removed. Why not change the user_groups entry to something like "~editor" ? (ie. "not editor")
That seems a lot less obvious. Everything accessing user_groups will have to special-case it. Making a separate table is cleaner, IMO.
Most uses in core are just to check if he's a bot, and if an extension accessed directly to the db (which shouldn't), would just see an unknown group.
There are legitimate reasons for extensions or maintenance scripts to directly access the DB, like to do batch queries. In trunk alone, it looks like CentralAuth, EmailPage, Farmer, FlaggedRevs, Maintenance, PageBy, SemanticNotifyMe, SpecialFileList, StrategyWiki, UserMerge, and WhiteListEdit access the table directly. Some maybe wrongly, but they do. There are at least a dozen different places in phase3 that access it too.
I'm not against making breaking changes to the semantics of tables when necessary (see categorylinks), but I don't think it's necessary here -- especially since a separate table is a cleaner design to begin with (no magic values).
Aryeh Gregor wrote:
There are legitimate reasons for extensions or maintenance scripts to directly access the DB, like to do batch queries. In trunk alone, it looks like CentralAuth, EmailPage, Farmer, FlaggedRevs, Maintenance, PageBy, SemanticNotifyMe, SpecialFileList, StrategyWiki, UserMerge, and WhiteListEdit access the table directly. Some maybe wrongly, but they do. There are at least a dozen different places in phase3 that access it too.
I agree. There are too many direct accesses to that table. But what do they use it too? Looking at what they do (mostly bot filtering) only two extensions would be affected:
*CentralAuth is fetching the data by itself and incorporates its own global_user_groups. *GroupPermissionsManager in the RemoveUserGroups Special page
I'm not against making breaking changes to the semantics of tables when necessary (see categorylinks), but I don't think it's necessary here -- especially since a separate table is a cleaner design to begin with (no magic values).
We already have a painfully long list of tables, and new tables also mean more problems for deployment.
On Mon, Aug 30, 2010 at 2:46 PM, Platonides Platonides@gmail.com wrote:
We already have a painfully long list of tables, and new tables also mean more problems for deployment.
It's fine to have lots of tables, that's usual in complicated relational databases. New tables are trivial to deploy, it's alterations to existing large tables that are hard. (Altering user_groups would probably be easy to deploy too, since an alter should only take a second or two, so you likely wouldn't have to bother taking slaves out of rotation to do it.)
wikitech-l@lists.wikimedia.org