r90749 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:r90748‎ | r90749 | r90750 >
Date:02:52, 25 June 2011
Author:aaron
Status:resolved (Comments)
Tags:
Comment:
Added one-time promote support via Autopromote::autopromoteOnceHook function. This is still a bit rough on the edges. This uses a hook since extension may want to control when it's called for performance reasons. Patch by lampak. (for bug 24948)
Modified paths:
  • /trunk/phase3/includes/Autopromote.php (modified) (history)
  • /trunk/phase3/includes/DefaultSettings.php (modified) (history)
  • /trunk/phase3/includes/User.php (modified) (history)
  • /trunk/phase3/includes/installer/MysqlUpdater.php (modified) (history)
  • /trunk/phase3/includes/installer/SqliteUpdater.php (modified) (history)
  • /trunk/phase3/maintenance/archives/patch-user_former_groups.sql (added) (history)
  • /trunk/phase3/maintenance/postgres/archives/patch-user_former_groups.sql (added) (history)
  • /trunk/phase3/maintenance/postgres/tables.sql (modified) (history)
  • /trunk/phase3/maintenance/tables.sql (modified) (history)

Diff [purge]

Index: trunk/phase3/maintenance/archives/patch-user_former_groups.sql
@@ -0,0 +1,10 @@
 2+-- Stores the groups the user has once belonged to.
 3+-- The user may still belong these groups. Check user_groups.
 4+CREATE TABLE /*_*/user_former_groups (
 5+ -- Key to user_id
 6+ ufg_user int unsigned NOT NULL default 0,
 7+ ufg_group varbinary(16) NOT NULL default '',
 8+
 9+ PRIMARY KEY (ufg_user,ufg_group),
 10+ KEY (ufg_group)
 11+) /*$wgDBTableOptions*/;
Property changes on: trunk/phase3/maintenance/archives/patch-user_former_groups.sql
___________________________________________________________________
Added: svn:eol-style
112 + native
Index: trunk/phase3/maintenance/postgres/archives/patch-user_former_groups.sql
@@ -0,0 +1,5 @@
 2+CREATE TABLE user_former_groups (
 3+ ufg_user INTEGER NULL REFERENCES mwuser(user_id) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
 4+ ufg_group TEXT NOT NULL
 5+);
 6+CREATE UNIQUE INDEX user_former_groups_unique ON user_former_groups (ufg_user, ufg_group);
Property changes on: trunk/phase3/maintenance/postgres/archives/patch-user_former_groups.sql
___________________________________________________________________
Added: svn:eol-style
17 + native
Index: trunk/phase3/maintenance/postgres/tables.sql
@@ -54,6 +54,12 @@
5555 );
5656 CREATE UNIQUE INDEX user_groups_unique ON user_groups (ug_user, ug_group);
5757
 58+CREATE TABLE user_former_groups (
 59+ ufg_user INTEGER NULL REFERENCES mwuser(user_id) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
 60+ ufg_group TEXT NOT NULL
 61+);
 62+CREATE UNIQUE INDEX user_former_groups_unique ON user_former_groups (ufg_user, ufg_group);
 63+
5864 CREATE TABLE user_newtalk (
5965 user_id INTEGER NOT NULL REFERENCES mwuser(user_id) ON DELETE CASCADE DEFERRABLE INITIALLY DEFERRED,
6066 user_ip TEXT NULL,
Index: trunk/phase3/maintenance/tables.sql
@@ -164,7 +164,17 @@
165165 CREATE UNIQUE INDEX /*i*/ug_user_group ON /*_*/user_groups (ug_user,ug_group);
166166 CREATE INDEX /*i*/ug_group ON /*_*/user_groups (ug_group);
167167
 168+-- Stores the groups the user has once belonged to.
 169+-- The user may still belong these groups. Check user_groups.
 170+CREATE TABLE /*_*/user_former_groups (
 171+ -- Key to user_id
 172+ ufg_user int unsigned NOT NULL default 0,
 173+ ufg_group varbinary(16) NOT NULL default ''
 174+) /*$wgDBTableOptions*/;
168175
 176+CREATE UNIQUE INDEX /*i*/ufg_user_group ON /*_*/user_former_groups (ufg_user,ufg_group);
 177+CREATE INDEX /*i*/ufg_group ON /*_*/user_former_groups (ufg_group);
 178+
169179 --
170180 -- Stores notifications of user talk page changes, for the display
171181 -- of the "you have new messages" box
Index: trunk/phase3/includes/User.php
@@ -71,7 +71,7 @@
7272 'mEmailTokenExpires',
7373 'mRegistration',
7474 'mEditCount',
75 - // user_group table
 75+ // user_groups table
7676 'mGroups',
7777 // user_properties table
7878 'mOptionOverrides',
@@ -182,7 +182,7 @@
183183 * Lazy-initialized variables, invalidated with clearInstanceCache
184184 */
185185 var $mNewtalk, $mDatePreference, $mBlockedby, $mHash, $mRights,
186 - $mBlockreason, $mEffectiveGroups, $mBlockedGlobally,
 186+ $mBlockreason, $mEffectiveGroups, $mFormerGroups, $mBlockedGlobally,
187187 $mLocked, $mHideName, $mOptions;
188188
189189 /**
@@ -1103,6 +1103,33 @@
11041104 }
11051105
11061106 /**
 1107+ * Add the user to the group if he/she meets given criteria.
 1108+ *
 1109+ * Contrary to autopromotion by \ref $wgAutopromote, the group will be
 1110+ * possible to remove manually via Special:UserRights. In such case it
 1111+ * will not be re-added autmoatically. The user will also not lose the
 1112+ * group if they no longer meet the criteria.
 1113+ *
 1114+ * @param $criteria array Groups and conditions the user must meet in order
 1115+ * to be promoted to these groups. Array of the same format as
 1116+ * \ref $wgAutopromote.
 1117+ *
 1118+ * @return array Array of groups the user has been promoted to.
 1119+ *
 1120+ * @see $wgAutopromote
 1121+ * @see Autopromote::autopromoteOnceHook()
 1122+ */
 1123+ public function autopromoteOnce( $criteria ) {
 1124+ if ($this->getId()) {
 1125+ $toPromote = Autopromote::getAutopromoteOnceGroups($this, $criteria);
 1126+ foreach($toPromote as $group)
 1127+ $this->addGroup($group);
 1128+ return $toPromote;
 1129+ }
 1130+ return array();
 1131+ }
 1132+
 1133+ /**
11071134 * Clear various cached data stored in this object.
11081135 * @param $reloadFrom String Reload user and user_groups table data from a
11091136 * given source. May be "name", "id", "defaults", "session", or false for
@@ -1116,7 +1143,7 @@
11171144 $this->mSkin = null;
11181145 $this->mRights = null;
11191146 $this->mEffectiveGroups = null;
1120 - $this->mOptions = null;
 1147+ $this->mOptions = null;
11211148
11221149 if ( $reloadFrom ) {
11231150 $this->mLoadedItems = array();
@@ -2240,8 +2267,32 @@
22412268 }
22422269 return $this->mEffectiveGroups;
22432270 }
2244 -
 2271+
22452272 /**
 2273+ * Returns the groups the user has belonged to.
 2274+ *
 2275+ * The user may still belong to the returned groups. Compare with getGroups().
 2276+ *
 2277+ * The function will not return groups the user had belonged to before MW 1.17
 2278+ *
 2279+ * @return array Names of the groups the user has belonged to.
 2280+ */
 2281+ function getFormerGroups() {
 2282+ if(is_null($this->mFormerGroups)) {
 2283+ $dbr = wfGetDB( DB_MASTER );
 2284+ $res = $dbr->select( 'user_former_groups',
 2285+ array( 'ufg_group' ),
 2286+ array( 'ufg_user' => $this->mId ),
 2287+ __METHOD__ );
 2288+ $this->mFormerGroups = array();
 2289+ while( $row = $dbr->fetchObject( $res ) ) {
 2290+ $this->mFormerGroups[] = $row->ufg_group;
 2291+ }
 2292+ }
 2293+ return $this->mFormerGroups;
 2294+ }
 2295+
 2296+ /**
22462297 * Get the user's edit count.
22472298 * @return Int
22482299 */
@@ -2297,6 +2348,14 @@
22982349 'ug_user' => $this->getID(),
22992350 'ug_group' => $group,
23002351 ), __METHOD__ );
 2352+ //remember that the user has had this group
 2353+ $dbw->insert( 'user_former_groups',
 2354+ array(
 2355+ 'ufg_user' => $this->getID(),
 2356+ 'ufg_group' => $group,
 2357+ ),
 2358+ __METHOD__,
 2359+ array( 'IGNORE' ) );
23012360 }
23022361 $this->loadGroups();
23032362 $this->mGroups = array_diff( $this->mGroups, array( $group ) );
Index: trunk/phase3/includes/installer/MysqlUpdater.php
@@ -162,6 +162,7 @@
163163
164164 // 1.17
165165 array( 'addTable', 'iwlinks', 'patch-iwlinks.sql' ),
 166+ array( 'addTable', 'user_former_groups', 'patch-user_former_groups.sql'),
166167 array( 'addIndex', 'iwlinks', 'iwl_prefix_title_from', 'patch-rename-iwl_prefix.sql' ),
167168 array( 'addField', 'updatelog', 'ul_value', 'patch-ul_value.sql' ),
168169 array( 'addField', 'interwiki', 'iw_api', 'patch-iw_api_and_wikiid.sql' ),
Index: trunk/phase3/includes/installer/SqliteUpdater.php
@@ -43,6 +43,7 @@
4444
4545 // 1.17
4646 array( 'addTable', 'iwlinks', 'patch-iwlinks.sql' ),
 47+ array( 'addTable', 'user_former_groups', 'patch-user_former_groups.sql'),
4748 array( 'addIndex', 'iwlinks', 'iwl_prefix_title_from', 'patch-rename-iwl_prefix.sql' ),
4849 array( 'addField', 'updatelog', 'ul_value', 'patch-ul_value.sql' ),
4950 array( 'addField', 'interwiki', 'iw_api', 'patch-iw_api_and_wikiid.sql' ),
Index: trunk/phase3/includes/DefaultSettings.php
@@ -3513,6 +3513,12 @@
35143514 *
35153515 * If $wgEmailAuthentication is off, APCOND_EMAILCONFIRMED will be true for any
35163516 * user who has provided an e-mail address.
 3517+ *
 3518+ * If the groups should be removable, consider using
 3519+ * Autopromote::autopromoteOnceHook() instead.
 3520+ *
 3521+ * @see Autopromote::autopromoteOnceHook()
 3522+ * @see User::autopromoteOnce()
35173523 */
35183524 $wgAutopromote = array(
35193525 'autoconfirmed' => array( '&',
Index: trunk/phase3/includes/Autopromote.php
@@ -6,6 +6,41 @@
77
88 class Autopromote {
99 /**
 10+ * A function which may be assigned to a hook in order to check
 11+ * autopromotion of the current user (\ref $wgUser) to the specified
 12+ * group.
 13+ *
 14+ * Contrary to autopromotion by \ref $wgAutopromote, the group will be
 15+ * possible to remove manually via Special:UserRights. In such case it
 16+ * will not be re-added autmoatically. The user will also not lose the
 17+ * group if they no longer meet the criteria.
 18+ *
 19+ * Example configuration:
 20+ * \code $wgHooks['ArticleSaveComplete'][] = array (
 21+ * 'Autopromote::autopromoteOnceHook',
 22+ * array( 'somegroup' => array(APCOND_EDITCOUNT, 200) )
 23+ * ); \endcode
 24+ *
 25+ * The second array should be of the same format as \ref $wgAutopromote.
 26+ *
 27+ * This funciton simply runs User::autopromoteOnce() on $wgUser. You may
 28+ * run this method from your custom function if you wish.
 29+ *
 30+ * @param $criteria array Groups and conditions which must be met in order to
 31+ * aquire these groups. Array of the same format as \ref $wgAutopromote.
 32+ *
 33+ * @return Always true.
 34+ *
 35+ * @see User::autopromoteOnce()
 36+ * @see $wgAutopromote
 37+ */
 38+ public static function autopromoteOnceHook($criteria) {
 39+ global $wgUser;
 40+ $wgUser->autopromoteOnce($criteria);
 41+ return true;
 42+ }
 43+
 44+ /**
1045 * Get the groups for the given user based on $wgAutopromote.
1146 *
1247 * @param $user User The user to get the groups for
@@ -26,6 +61,41 @@
2762
2863 return $promote;
2964 }
 65+
 66+ /**
 67+ * Get the groups for the given user based on the given criteria.
 68+ *
 69+ * Does not return groups the user already belongs to or has once belonged.
 70+ *
 71+ * @param $user The user to get the groups for
 72+ * @param $criteria array Groups and conditions the user must meet in order
 73+ * to be promoted to these groups. Array of the same format as
 74+ * \ref $wgAutopromote.
 75+ *
 76+ * @return array Groups the user should be promoted to.
 77+ */
 78+ public static function getAutopromoteOnceGroups( User $user, $criteria ) {
 79+ $promote = array();
 80+
 81+ //get the current groups
 82+ $currentGroups = $user->getGroups();
 83+
 84+ foreach( $criteria as $group => $cond ) {
 85+ //do not check if the user's already a member
 86+ if ( in_array($group, $currentGroups))
 87+ continue;
 88+
 89+ //do not autopromote if the user has belonged to the group
 90+ $formerGroups = $user->getFormerGroups();
 91+ if ( in_array($group, $formerGroups) )
 92+ continue;
 93+
 94+ //finally - check the conditions
 95+ if ( self::recCheckCondition($cond, $user) )
 96+ $promote[] = $group;
 97+ }
 98+ return $promote;
 99+ }
30100
31101 /**
32102 * Recursively check a condition. Conditions are in the form

Follow-up revisions

RevisionCommit summaryAuthorDate
r90753Follow-up r90749:...aaron04:11, 25 June 2011
r90755Follow-up r90749:...aaron04:58, 25 June 2011
r90756(bug 25390) autoreview privilege should be removeable like editor (uses r90749)aaron05:05, 25 June 2011
r90757Follow-up r90749: pushed down accessing of $wgAutopromoteOnce to getAutopromo...aaron05:21, 25 June 2011
r90776* Follow-up r90749...aaron17:59, 25 June 2011
r90816* Core:...aaron04:12, 26 June 2011
r90855Follow-up r90749: use a new 'autopromote' action for the autopromote rights l...aaron21:44, 26 June 2011
r97625Followup r90749, beat Aaron...reedy13:36, 20 September 2011

Comments

#Comment by Aaron Schulz (talk | contribs)   03:14, 25 June 2011

autopromoteOnceHook() could probably be renamed and the User should be passed in. I see a few doc typos too.

#Comment by Aaron Schulz (talk | contribs)   03:30, 25 June 2011

Gah, disregard the first bit.

#Comment by Bawolff (talk | contribs)   03:54, 25 June 2011

[21:50] <codurr> Something broke. See <http://ci.tesla.usability.wikimedia.org/cruisecontrol/>. Possible culprits: aaron/r90749 Reedy/r90750

The failing test was:

error DatabaseSqliteTest::testUpgrades() DatabaseSqliteTest

#Comment by Aaron Schulz (talk | contribs)   04:47, 25 June 2011
#Comment by Reedy (talk | contribs)   16:53, 18 July 2011

Created the barebones of this documentation at Manual:User_former_groups_table

#Comment by Happy-melon (talk | contribs)   21:46, 4 August 2011

Although it seems to be fashionable to add endless extra tables for things like this, adding a ug_current field to the user_groups table would be nicer, IMO. It's not a particularly massive table, shouldn't be too painful to alter it even on enwiki.

#Comment by 😂 (talk | contribs)   16:02, 16 August 2011

I agree. I'm not a huge fan of adding a new table for this.

#Comment by Aaron Schulz (talk | contribs)   20:09, 20 August 2011

Why is this fixme?

#Comment by 😂 (talk | contribs)   20:18, 20 August 2011

I'm guessing Sam marked it fixme for the objections I and Happy-Melon raised above. I'm not a fan of adding an additional table here.

#Comment by Aaron Schulz (talk | contribs)   21:09, 20 August 2011

I'm not sure how well ug_current would work. Any existing queries would need to be updated for that work. The PK would need to change and the new field would probably need to be part of an indexed. I don't really think it would have a good separation of concern either, which is why I ended up using the new table as the patch had.

#Comment by 😂 (talk | contribs)   17:14, 6 September 2011

You're right, marking resolved.

#Comment by MZMcBride (talk | contribs)   23:16, 9 February 2012

Adding a database table is really something that ought to be mentioned in a commit summary.

I don't really like this table as I don't think it will ever be accurate for nearly any user.

Status & tagging log