jenkins-bot has submitted this change and it was merged.
Change subject: Don't drop zones table before writing, improve success message
......................................................................
Don't drop zones table before writing, improve success message
Change-Id: Id72398c091859d923c2ffb79b944d98e7cb942f1
---
M AdManager.class.php
M AdManager.php
M AdManagerZones.class.php
M i18n/en.json
M i18n/qqq.json
M specials/SpecialAdManager.php
M specials/SpecialAdManagerZones.php
7 files changed, 198 insertions(+), 83 deletions(-)
Approvals:
tosfos: Looks good to me, approved
Siebrand: Looks good to me, but someone else must approve
jenkins-bot: Verified
diff --git a/AdManager.class.php b/AdManager.class.php
index dc23d6b..3ae0ca8 100644
--- a/AdManager.class.php
+++ b/AdManager.class.php
@@ -4,13 +4,13 @@
* Layer on top of the ad table with some helper functions
*/
class AdManager {
+ const AD_TABLE = 'ad';
private static $catList = array();
/**
* @var array $ads Ads to be added to db
*/
private $ads = array();
- const AD_TABLE = 'ad';
public function __construct( array $ads ) {
$this->setAds( $ads );
@@ -30,7 +30,7 @@
* @return boolean
*/
public static function tableExists() {
- $dbr = wfGetDB( DB_SLAVE );
+ $dbr = self::getReadDbConnection();
return $dbr->tableExists( self::getTableName() );
}
@@ -77,7 +77,7 @@
*/
public static function getSomeAdsfromDB( $type ) {
$blank = AdManagerZones::getBlankZoneID();
- $dbr = wfGetDB( DB_SLAVE );
+ $dbr = self::getReadDbConnection();
$current = $dbr->select(
self::getTableName(), array(
'ad_id', 'ad_page_id', 'ad_zone', 'ad_page_is_category'
@@ -103,7 +103,7 @@
* @return type
*/
public function execute() {
- $dbw = wfGetDB( DB_MASTER );
+ $dbw = $this->getWriteDbConnection();
/** @todo Not such a good idea. What if insert fails? */
$dbw->delete( self::getTableName(), '*', __METHOD__ );
@@ -162,7 +162,7 @@
* @return type
*/
protected function addAd( $type, $adZoneID, $ad ) {
- $dbw = wfGetDB( DB_MASTER );
+ $dbw = $this->getReadDbConnection();
// Depending on fields being processed, lookup either the
// text's Page ID or Category ID
@@ -256,7 +256,7 @@
*/
public static function getCategoryAdZonesFor( $title ) {
$fullTableName = AdManager::getTableName();
- $dbr = wfGetDB( DB_SLAVE );
+ $dbr = self::getReadDbConnection();
$thisPageAdZones = array();
// check if an ad zone was set for any of this page's categories
$allCategories = $dbr->select(
@@ -315,4 +315,18 @@
return $wgAdManagerCode;
}
}
+
+ /**
+ * @return DatabaseBase Read-only db connection
+ */
+ public static function getReadDbConnection() {
+ return wfGetDB( DB_SLAVE );
+ }
+
+ /**
+ * @return DatabaseBase Writable db connection
+ */
+ public static function getWriteDbConnection() {
+ return wfGetDB( DB_MASTER );
+ }
}
diff --git a/AdManager.php b/AdManager.php
index fc91278..149a240 100644
--- a/AdManager.php
+++ b/AdManager.php
@@ -21,7 +21,7 @@
$wgExtensionCredits['specialpage'][] = array(
'path' => __FILE__,
'name' => 'AdManager',
- 'version' => '0.4.0',
+ 'version' => '0.5.0',
'author' => 'Ike Hecht for WikiWorks',
'url' => 'https://www.mediawiki.org/wiki/Extension:AdManager',
'descriptionmsg' => 'admanager-desc',
diff --git a/AdManagerZones.class.php b/AdManagerZones.class.php
index b472c3b..b14710a 100644
--- a/AdManagerZones.class.php
+++ b/AdManagerZones.class.php
@@ -1,16 +1,71 @@
<?php
+
/**
* Layer on top of the ad zones table
*/
class AdManagerZones {
+ const AD_ZONES_TABLE = 'adzones';
/**
* @var array $zones Zones to be added to db
*/
- private $zones = array();
- const AD_ZONES_TABLE = 'adzones';
+ private $zonesToAdd = array();
- public function __construct( array $zones ) {
- $this->setZones( $zones );
+ /**
+ * @var array $zones Zones to be removed from db
+ */
+ private $zonesToRemove = array();
+
+ /**
+ * @var boolean $zonesAddedSuccessfully
+ */
+ private $zonesAddedSuccessfully;
+
+ /**
+ * @var boolean $zonesRemovedSuccessfully
+ */
+ private $zonesRemovedSuccessfully;
+
+ /**
+ * @return array
+ */
+ public function getZonesToAdd() {
+ return $this->zonesToAdd;
+ }
+
+
+ public function getZonesToRemove() {
+ return $this->zonesToRemove;
+ }
+
+ public function getZonesAddedSuccessfully() {
+ return $this->zonesAddedSuccessfully;
+ }
+
+ public function getZonesRemovedSuccessfully() {
+ return $this->zonesRemovedSuccessfully;
+ }
+
+ public function setZonesToAdd( array $zones ) {
+ return wfSetVar( $this->zonesToAdd, $zones );
+ }
+
+ public function setZonesToRemove( $zonesToRemove ) {
+ return wfSetVar( $this->zonesToRemove, $zonesToRemove );
+ }
+
+ public function setZonesAddedSuccessfully( $zonesAddedSuccessfully ) {
+ return wfSetVar( $this->zonesAddedSuccessfully,
$zonesAddedSuccessfully );
+ }
+
+ public function setZonesRemovedSuccessfully( $zonesRemovedSuccessfully
) {
+ return wfSetVar( $this->zonesRemovedSuccessfully,
$zonesRemovedSuccessfully );
+ }
+
+ public function __construct( array $wantedZones ) {
+ $currentZones = $this->getZonesFromDB();
+
+ $this->zonesToAdd = array_diff( $wantedZones, $currentZones );
+ $this->zonesToRemove = array_diff( $currentZones, $wantedZones
);
}
public static function getTableName() {
@@ -22,7 +77,7 @@
}
/**
- * Check if this zone is valid
+ * Check if the zone is valid
*
* @param string $zone
* @return boolean
@@ -71,15 +126,8 @@
* @return boolean
*/
public static function tableExists() {
- $dbr = wfGetDB( DB_SLAVE );
+ $dbr = self::getReadDbConnection();
return $dbr->tableExists( self::getTableName() );
- }
-
- /**
- * @return array
- */
- public function getZones() {
- return $this->zones;
}
/**
@@ -88,7 +136,7 @@
* @return array current zones in db
*/
public static function getZonesFromDB() {
- $dbr = wfGetDB( DB_SLAVE );
+ $dbr = self::getReadDbConnection();
$current = $dbr->select(
self::getTableName(), array( '*' ), array(), __METHOD__
);
@@ -103,58 +151,69 @@
}
/**
- * Purge the db and insert new zones
+ * Add new zones to db and remove the old zones
*
- * @return boolean
+ * @return boolean Successful if added and removed successfully
*/
public function execute() {
- $dbw = wfGetDB( DB_MASTER );
- /** @todo Not such a good idea. What if insert fails? */
- $dbw->delete( self::getTableName(), '*', __METHOD__ );
+ $this->zonesAdded = $this->doAddZones();
+ $this->zonesRemoved = $this->doRemoveZones();
- return $this->addZones( $this->zones );
+ return $this->zonesAdded && $this->zonesRemoved;
}
/**
* Insert an array of zones into the db
*
- * @param array $zones
* @return boolean
*/
- protected function addZones( $zones = null ) {
- if ( !$zones ) {
- if ( isset( $this->zones ) ) {
- $zones = $this->zones;
- } else {
- return false;
- }
+ protected function doAddZones() {
+ $dbw = $this->getWriteDbConnection();
+ $rows = array();
+ foreach ( $this->getZonesToAdd() as $zone ) {
+ $rows[] = array( 'ad_zone_id' => $zone );
}
+ $success = $dbw->insert( self::getTableName(), $rows,
__METHOD__, 'IGNORE' );
- foreach ( $zones as $zone ) {
- if ( !self::addZone( $zone ) ) {
- return false;
- }
- }
- return true;
+ // DatabaseBase::insert does not always return true for success
as documented...
+ return $success !== false;
}
/**
- * Insert a single zone into the db
+ * Remove zones from the db
+ *
+ * return Boolean True if all zones were removed
+ */
+ protected function doRemoveZones() {
+ $successAll = true;
+ foreach ( $this->getZonesToRemove() as $zone ) {
+ $successAll &= (bool) $this->removeZone( $zone );
+ }
+ return $successAll;
+ }
+
+ /**
+ * Remove a zone from the db
*
* @param string $zone
- * @return boolean
+ * @return bool|ResultWrapper
*/
- protected function addZone( $zone ) {
- $dbw = wfGetDB( DB_MASTER );
- return $dbw->insert(
- self::getTableName(), array( 'ad_zone_id' =>
$zone ), __METHOD__
- );
+ protected function removeZone( $zone ) {
+ $dbw = $this->getWriteDbConnection();
+ return $dbw->delete( self::getTableName(), array( 'ad_zone_id'
=> $zone ), __METHOD__ );
}
/**
- * @param array $zones
+ * @return DatabaseBase Read-only db connection
*/
- public function setZones( array $zones ) {
- $this->zones = $zones;
+ public static function getReadDbConnection() {
+ return wfGetDB( DB_SLAVE );
+ }
+
+ /**
+ * @return DatabaseBase Writable db connection
+ */
+ public static function getWriteDbConnection() {
+ return wfGetDB( DB_MASTER );
}
}
diff --git a/i18n/en.json b/i18n/en.json
index e0f4a84..d74f420 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -1,30 +1,33 @@
{
- "@metadata": {
- "authors": [
- "Ike Hecht"
- ]
- },
- "admanager": "Ad Manager",
- "admanagerzones": "Ad Manager Zones",
- "admanager-desc": "Provides a [[Special:AdManager|special page]] which
allows sysops to set the zone for pages or categories",
- "admanager_docu": "To add or remove the ad zone of a page or entire
category, add or remove its title below.",
- "admanagerzones_docu": "Enter each ad zone number on its own line.",
- "admanager_invalidtargetpage": "No page found with title \"$1\".",
- "admanager_invalidtargetcategory": "No category found with title \"$1\".",
- "admanager_notable": "Error! A required database table was not found! Run
update.php first.",
- "admanager_noAdManagerZones": "Error! You must add some ad zones. Enter
them at [[Special:AdManagerZones|Ad Manager Zones]].",
- "admanager_labelPage": "Page titles",
- "admanager_labelCategory": "Category names",
- "admanager_submit": "Submit",
- "admanager_noads": "Display no ads",
- "admanager_Page": "Pages",
- "admanager_Category": "Categories",
- "admanager_added": "Your changes have been saved",
- "admanager_addedzone": "Added zone $1",
- "admanager_zonenum": "Zone #: $1",
- "admanager_zonenotnumber": "Error! $1 is not a number.",
- "admanager_return": "Return to [[Special:AdManager|Ad Manager]]",
- "admanager_gotoads": "[[Special:AdManager|Edit ad placement]]",
- "admanager_gotozones": "[[Special:AdManagerZones|Edit ad zones]]",
- "right-admanager": "[[Special:AdManager|Manage advertising configuration]]"
+ "@metadata": {
+ "authors": [
+ "Ike Hecht"
+ ]
+ },
+ "admanager": "Ad Manager",
+ "admanagerzones": "Ad Manager zones",
+ "admanager-desc": "Provides a [[Special:AdManager|special page]] which
allows sysops to set the zone for pages or categories",
+ "admanager_docu": "To add or remove the ad zone of a page or entire
category, add or remove its title below.",
+ "admanagerzones_docu": "Enter each ad zone number on its own line.",
+ "admanager_invalidtargetpage": "No page found with title \"$1\".",
+ "admanager_invalidtargetcategory": "No category found with title
\"$1\".",
+ "admanager_notable": "Error! A required database table was not found!
Run update.php first.",
+ "admanager_noAdManagerZones": "Error! You must add some ad zones. Enter
them at [[Special:AdManagerZones|Ad Manager zones]].",
+ "admanager_labelPage": "Page titles",
+ "admanager_labelCategory": "Category names",
+ "admanager_submit": "Submit",
+ "admanager_noads": "Display no ads",
+ "admanager_Page": "Pages",
+ "admanager_Category": "Categories",
+ "admanager_added": "Your changes have been saved.",
+ "admanager_addedzone": "Added zone $1.",
+ "admanager_removedzone": "Removed zone $1.",
+ "admanager_nozonesadded": "No zones were added.",
+ "admanager_nozonesremoved": "No zones were removed.",
+ "admanager_zonenum": "Zone #: $1",
+ "admanager_zonenotnumber": "Error! $1 is not a number.",
+ "admanager_return": "Return to [[Special:AdManager|Ad Manager]]",
+ "admanager_gotoads": "[[Special:AdManager|Edit ad placement]]",
+ "admanager_gotozones": "[[Special:AdManagerZones|Edit ad zones]]",
+ "right-admanager": "[[Special:AdManager|Manage advertising
configuration]]"
}
\ No newline at end of file
diff --git a/i18n/qqq.json b/i18n/qqq.json
index fc75fcb..f21b508 100644
--- a/i18n/qqq.json
+++ b/i18n/qqq.json
@@ -24,6 +24,9 @@
"admanager_Category": "{{Identical|Category}}",
"admanager_added": "Used as success message in
Special:AdManager.\n\nThis message is followed by the message
{{msg-mw|Admanager return}}.",
"admanager_addedzone": " Parameters:\n* $1 - the Zone number.\n\nUsage
example of this message (123 is the Zone number):\n\n*Added zone 123",
+ "admanager_removedzone": " Parameters:\n* $1 - the Zone
number.\n\nUsage example of this message (123 is the Zone number):\n\n*Removed
zone 123",
+ "admanager_nozonesadded": "Used as success message when no zones were
added",
+ "admanager_nozonesremoved": "Used as success message when no zones were
removed",
"admanager_zonenum": "\"#\" stands for \"number\" (number sign).
Parameters:\n* $1 - the Zone number",
"admanager_zonenotnumber": "Error message\n\n* $1 is the invalid Zone
number. $1 may contain HTML tags.",
"admanager_return": "Used in Special:AdManager.\n\nThis message follows
the message {{msg-mw|Admanager added}}.",
diff --git a/specials/SpecialAdManager.php b/specials/SpecialAdManager.php
index e3244f9..3fcd856 100644
--- a/specials/SpecialAdManager.php
+++ b/specials/SpecialAdManager.php
@@ -194,6 +194,7 @@
private function getAdPageTitleText( $adPage, $type ) {
if ( $type == 'Page' ) {
$pageObject = Title::newFromText( trim( $adPage ) );
+ /** @todo disallows Specials */
if ( !$pageObject->exists() ) {
throw new ErrorPageError( 'internalerror',
'admanager_invalidtargetpage',
$adPage );
diff --git a/specials/SpecialAdManagerZones.php
b/specials/SpecialAdManagerZones.php
index b8e499d..6e65e9a 100644
--- a/specials/SpecialAdManagerZones.php
+++ b/specials/SpecialAdManagerZones.php
@@ -67,7 +67,6 @@
* @return bool|string|array|Status As documented for
HTMLForm::trySubmit.
*/
public function onSubmit( array $data ) {
-
$zones = explode( "\n", $data['zones'] );
$this->adManagerZones = new AdManagerZones( array_filter(
$zones ) ); //remove blanks
return $this->adManagerZones->execute();
@@ -78,9 +77,8 @@
*/
public function onSuccess() {
$text = Html::openElement( 'div', array( 'class' =>
'successbox' ) );
- foreach ( $this->adManagerZones->getZones() as $zone ) {
- $text .= "\n* " . $this->msg( 'admanager_addedzone',
$zone )->text();
- }
+ $text .= $this->getZonesAddedMessage();
+ $text .= $this->getZonesRemovedMessage();
$text .= Html::closeElement( 'div' ) . Html::element( 'br',
array( 'clear' => 'both' ) );
$this->getOutput()->addWikiText( $text );
@@ -119,4 +117,41 @@
$zones = explode( "\n", $zonesText );
return AdManagerZones::validateZones( $zones );
}
+
+ /**
+ * Get the complete message detailing which zones were added
+ *
+ * @return string Message
+ */
+ public function getZonesAddedMessage() {
+ $zonesToAdd = $this->adManagerZones->getZonesToAdd();
+ if ( empty( $zonesToAdd ) ) {
+ return "\n* " . $this->msg( 'admanager_nozonesadded'
)->escaped();
+ }
+
+ $text = '';
+ foreach ( $zonesToAdd as $zone ) {
+ $text .= "\n* " . $this->msg( 'admanager_addedzone',
$zone )->escaped();
+ }
+ return $text;
+ }
+
+ /**
+ * Get the complete message detailing which zones were removed
+ *
+ * @return string Message
+ */
+ public function getZonesRemovedMessage() {
+ $zonesToRemove = $this->adManagerZones->getZonesToRemove();
+ if ( empty( $zonesToRemove ) ) {
+ return "\n* " . $this->msg( 'admanager_nozonesremoved'
)->escaped();
+ }
+
+ $text = '';
+ foreach ( $zonesToRemove as $zone ) {
+ $text .= "\n* " . $this->msg( 'admanager_removedzone',
$zone )->escaped();
+ }
+
+ return $text;
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/156989
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id72398c091859d923c2ffb79b944d98e7cb942f1
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/AdManager
Gerrit-Branch: master
Gerrit-Owner: tosfos <[email protected]>
Gerrit-Reviewer: Jack Phoenix <[email protected]>
Gerrit-Reviewer: Siebrand <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
Gerrit-Reviewer: tosfos <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
