r105641 MediaWiki - Code Review archive

+	/**
+	 * Wait for all the slave database servers to sync up with the master
+	 */	
+	public function syncDBs() {
+		// FIXME: Copied from populateAFRevisions.php, which coppied from updateCollation.php, should be centralized somewhere
+		$lb = wfGetLB();
+		// [https://bugzilla.wikimedia.org/show_bug.cgi?id=27975 bug 27975] - Don't try to wait for slaves if there are none
+		// Prevents permission error when getting master position
+		if ( $lb->getServerCount() > 1 ) {
+			$dbw = $lb->getConnection( DB_MASTER );
+			$pos = $dbw->getMasterPos();
+			$lb->waitForAll( $pos );
+		}
+	}

You don't need this anymore since 1.18 was deployed. You can now simply use wfWaitForSlaves() .

+			'converted_amount >= ' . $egFundraiserStatisticsMinimum,
+			'converted_amount <= ' . $egFundraiserStatisticsMaximum

I would feel safer if you ran these through intval() before injecting them into SQL.

+			date_default_timezone_set( 'UTC' );

This looks like a hack. Why is this done here, rather than further up where strtotime() is first used? Why not avoid using strtotime() altogether by specifying the start and end dates in a time format recognized by wfTimestamp()?

+CREATE UNIQUE INDEX /*i*/prf_id ON /*$wgDBprefix*/public_reporting_fundraisers (prf_id);

You can just make this the primary key.

+CREATE UNIQUE INDEX /*i*/prf_id ON /*$wgDBprefix*/public_reporting_days (prd_date);

Misnamed index.

OK otherwise, this is looking good.

Fixed in r106234.

I've left the config/strtotime stuff how it is for now, but I think you have a point and we should look into changing these configs from simple dates into standard timestamp formats.

I should've caught this the first time around but it just occurred to me that this is not going to work as expected the way things are currently set up:

+		$this->dbr = wfGetDB( DB_SLAVE );
+		$this->dbw = wfGetDB( DB_MASTER );
<snip>
+		// Get the data for a fundraiser
+		$result = $this->dbr->select( 'public_reporting',
+			array(
+				"DATE_FORMAT( FROM_UNIXTIME( received ),'%Y-%m-%d' ) AS date",
+				'sum( converted_amount ) AS total',
+				'count( * ) AS number',
+				'avg( converted_amount ) AS average',
+				'max( converted_amount ) AS maximum',
+			),
+			$conditions,
+			__METHOD__,
+			array(
+				'ORDER BY' => 'received',
+				'GROUP BY' => "DATE_FORMAT( FROM_UNIXTIME( received ),'%Y-%m-%d' )"
+			)
+		);

The public_reporting table is NOT in the wiki's db. There is a function in ContributionReporting.php that will build you a db connection to the db that holds the public reporting table - efContributionReportingConnection(). We'll need to decide whether or not adding the summary tables to the wiki's db is the appropriate thing to do, or if it makes more sense to leave in the same place as public_reporting. If you want to keep the summary tables in the wiki's db you could just create another db object to fetch data from public_reporting and leave the dbr/dbw as they are. IMHO it's probably better to keep the summary tables in the wiki's db - if that's the road we go down, you could revise this to be something like:

+		$this->dbr = wfGetDB( DB_SLAVE );
+		$this->dbw = wfGetDB( DB_MASTER );
                $this->dbpr = efContributionReportingConnection();
<snip>
+		// Get the data for a fundraiser
+		$result = $this->dbpr->select( 'public_reporting',
+			array(
+				"DATE_FORMAT( FROM_UNIXTIME( received ),'%Y-%m-%d' ) AS date",
+				'sum( converted_amount ) AS total',
+				'count( * ) AS number',
+				'avg( converted_amount ) AS average',
+				'max( converted_amount ) AS maximum',
+			),
+			$conditions,
+			__METHOD__,
+			array(
+				'ORDER BY' => 'received',
+				'GROUP BY' => "DATE_FORMAT( FROM_UNIXTIME( received ),'%Y-%m-%d' )"
+			)
+		);

Repeat replacing $this->dbr with $this->dbpr everywhere you're fetching data from public reporting and leave the rest as-is.

Fixed in r106362.