Adamw has submitted this change and it was merged.
Change subject: fixes a bug in GlobalAllocation: don't include expired campaigns
......................................................................
fixes a bug in GlobalAllocation: don't include expired campaigns
Change-Id: If60e87df61b323c0081eae7907d07a72e54b7036
---
M includes/Campaign.php
M special/SpecialCentralNotice.php
M tests/CentralNoticeTestFixtures.php
A tests/HistoryTest.php
4 files changed, 72 insertions(+), 7 deletions(-)
Approvals:
Adamw: Verified; Looks good to me, approved
diff --git a/includes/Campaign.php b/includes/Campaign.php
index 55ae22a..e7cc85e 100644
--- a/includes/Campaign.php
+++ b/includes/Campaign.php
@@ -455,8 +455,6 @@
),
array(
"notlog_timestamp <= $ts",
- "notlog_end_start <= $ts",
- "notlog_end_end >= $ts",
),
__METHOD__,
array(
@@ -483,12 +481,14 @@
),
array(
"notlog_id = {$row->log_id}",
+ "notlog_end_start <= $ts",
+ "notlog_end_end >= $ts",
),
__METHOD__
);
$campaign = $singleRes->fetchRow();
- if ( $campaign['enabled'] !== "1" ) {
+ if ( !$campaign or $campaign['enabled'] !== "1" ) {
continue;
}
$campaign['projects'] = explode( ", ",
$campaign['projects'] );
@@ -571,7 +571,7 @@
* @param $user User adding the campaign
*
* @throws MWException
- * @return bool|string True on success, string with message key for
error
+ * @return int|string noticeId on success, or message key for error
*/
static function addCampaign( $noticeName, $enabled, $startTs,
$projects, $project_languages,
$geotargeted, $geo_countries, $throttle, $priority, $user
@@ -653,7 +653,7 @@
Campaign::logCampaignChange( 'created', $not_id, $user,
$beginSettings, $endSettings );
- return true;
+ return $not_id;
}
throw new MWException( 'insertId() did not return a value.' );
diff --git a/special/SpecialCentralNotice.php b/special/SpecialCentralNotice.php
index 303a84a..befcdac 100644
--- a/special/SpecialCentralNotice.php
+++ b/special/SpecialCentralNotice.php
@@ -64,7 +64,7 @@
$result =
Campaign::addCampaign( $noticeName, '0', $start, $projects,
$project_languages,
$geotargeted, $geo_countries,
100,
CentralNotice::NORMAL_PRIORITY, $this->getUser() );
- if( $result !== true ) {
+ if ( is_string( $result ) ) {
$this->showError(
$result );
}
}
diff --git a/tests/CentralNoticeTestFixtures.php
b/tests/CentralNoticeTestFixtures.php
index 919c92f..77ace7a 100644
--- a/tests/CentralNoticeTestFixtures.php
+++ b/tests/CentralNoticeTestFixtures.php
@@ -43,7 +43,7 @@
'name' => 'TestCampaign_' . rand(),
);
- Campaign::addCampaign(
+ $campaign['id'] = Campaign::addCampaign(
$campaign['name'],
$campaign['enabled'],
$campaign['startTs'],
diff --git a/tests/HistoryTest.php b/tests/HistoryTest.php
new file mode 100644
index 0000000..504e9a6
--- /dev/null
+++ b/tests/HistoryTest.php
@@ -0,0 +1,65 @@
+<?php
+
+require_once 'CentralNoticeTestFixtures.php';
+
+/**
+ * @group CentralNotice
+ * @group medium
+ * @group Database
+ */
+class HistoryTest extends CentralNoticeTest {
+ /** @var CentralNoticeTestFixtures */
+ protected $cnFixtures;
+
+ protected function setUp() {
+ parent::setUp();
+
+ $this->cnFixtures = new CentralNoticeTestFixtures();
+ }
+
+ protected function tearDown() {
+ $this->cnFixtures->removeFixtures();
+ parent::tearDown();
+ }
+
+ public function testStaleHistoricalCampaigns() {
+ // Bug was that expired campaigns would still be included in the
+ // history, as long as they were enabled.
+ $this->cnFixtures->addFixtures( array(
+ 'campaigns' => array(
+ array(
+ 'banners' => array(
+ array()
+ ),
+ ),
+ ),
+ ) );
+
+ $made_by_ts = wfTimestamp( TS_MW );
+ $this->assertEquals( 1, count(
Campaign::getHistoricalCampaigns( $made_by_ts ) ) );
+
+ $initialSettings = Campaign::getCampaignSettings(
$this->cnFixtures->spec['campaigns'][0]['name'] );
+
+ sleep( 2 );
+
+ // FIXME: Campaign::updateSettings
+ $newSettings = array(
+ 'end' => $made_by_ts,
+ ) + $initialSettings;
+ Campaign::updateNoticeDate(
+ $this->cnFixtures->spec['campaigns'][0]['name'],
+ $newSettings['start'],
+ $newSettings['end']
+ );
+ Campaign::logCampaignChange(
+ 'modified',
+ $this->cnFixtures->spec['campaigns'][0]['id'],
+ $this->userUser,
+ $initialSettings,
+ $newSettings
+ );
+
+ $modified_ts = wfTimestamp( TS_MW );
+ $this->assertEquals( 0, count(
Campaign::getHistoricalCampaigns( $modified_ts ) ) );
+ }
+}
--
To view, visit https://gerrit.wikimedia.org/r/91649
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: If60e87df61b323c0081eae7907d07a72e54b7036
Gerrit-PatchSet: 4
Gerrit-Project: mediawiki/extensions/CentralNotice
Gerrit-Branch: master
Gerrit-Owner: Adamw <[email protected]>
Gerrit-Reviewer: Adamw <[email protected]>
Gerrit-Reviewer: Mwalker <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
