jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/346055 )
Change subject: Sync DB tables manually on a newsletter edit over API
......................................................................
Sync DB tables manually on a newsletter edit over API
Checked the following on API edit:
1) API edit to create a new newsletter with valid new name, mainpage -
the page gets created and the data gets added to the db as well.
2) API edit to edit an existing newsletter (in db) with valid name,
mainpage - the page gets created, and teh edit gets added to the db as well.
3) API edit to create a new newsletter with existing main_page -
currently this action gets aborted from the hook, and the API edit fails.
* Shifted truncating of description text to DatabaseUpdate
On a normal edit - every case works normal as before
Bug: T160854
Change-Id: I2f631e336b0219958c917242078d88ec94a95ae5
---
M Newsletter.hooks.php
M extension.json
M includes/NewsletterDb.php
M includes/NewsletterEditPage.php
M includes/content/NewsletterContent.php
A includes/content/NewsletterDataUpdate.php
M includes/content/NewsletterDiffEngine.php
M includes/specials/SpecialNewsletterCreate.php
8 files changed, 277 insertions(+), 65 deletions(-)
Approvals:
Brian Wolff: Looks good to me, approved
jenkins-bot: Verified
diff --git a/Newsletter.hooks.php b/Newsletter.hooks.php
index 7790b2a..8427b51 100755
--- a/Newsletter.hooks.php
+++ b/Newsletter.hooks.php
@@ -187,7 +187,6 @@
if ( !$article->getTitle()->inNamespace( NS_NEWSLETTER ) ) {
return true;
}
-
$newsletter = Newsletter::newFromName(
$article->getTitle()->getText() );
if ( $newsletter ) {
// A newsletter exists in that title, lets redirect to
manage page
@@ -304,4 +303,51 @@
}
return true;
}
+
+ /**
+ * @param RequestContext $context object implementing the
IContextSource interface.
+ * @param Content $content content of the edit box, as a Content object.
+ * @param Status $status Status object to represent errors, etc.
+ * @param $summary string Edit summary for page
+ * @param User $user the User object representing the user whois
performing the edit.
+ * @param $minoredit bool whether the edit was marked as minor by the
user.
+ * @return bool
+ */
+ public static function onEditFilterMergedContent( IContextSource
$context, Content $content,
+ Status $status,
$summary, User $user, $minoredit ) {
+ if ( !$context->getTitle()->inNamespace( NS_NEWSLETTER ) ) {
+ return;
+ }
+ if ( !$context->getTitle()->hasContentModel(
'NewsletterContent' ) ||
+ ( !$content instanceof NewsletterContent ) ) {
+ return;
+ }
+ $newsletter = Newsletter::newFromName(
$context->getTitle()->getText() );
+
+ // Validate API Edit parameters
+ $formData = array(
+ 'Name' => $context->getTitle()->getText(),
+ 'Description' => $content->getDescription(),
+ 'MainPage' => $content->getMainPage(),
+ );
+ $validator = new NewsletterValidator( $formData );
+ $validation = $validator->validate( !$newsletter );
+ if ( !$validation->isGood() ) {
+ // Invalid input was entered
+ return false;
+ }
+ $mainPageId = $content->getMainPage()->getArticleID();
+ $store = NewsletterStore::getDefaultInstance();
+ if ( !$newsletter || ( $newsletter && $newsletter->getPageId()
!== $mainPageId ) ) {
+ $rows = $store->newsletterExistsForMainPage(
$mainPageId );
+ foreach ( $rows as $row ) {
+ if ( (int)$row->nl_main_page_id === $mainPageId
&& (int)$row->nl_active === 1 ) {
+ $status->newFatal(
'newsletter-mainpage-in-use' );
+ return false;
+ }
+ }
+ }
+
+ return true;
+ }
}
diff --git a/extension.json b/extension.json
index 0ee39e9..34de892 100644
--- a/extension.json
+++ b/extension.json
@@ -82,6 +82,7 @@
"ApiNewsletterSubscribe":
"includes/api/ApiNewsletterSubscribe.php",
"NewsletterContent": "includes/content/NewsletterContent.php",
"NewsletterContentHandler":
"includes/content/NewsletterContentHandler.php",
+ "NewsletterDataUpdate":
"includes/content/NewsletterDataUpdate.php",
"NewsletterDiffEngine":
"includes/content/NewsletterDiffEngine.php",
"EchoNewsletterUserLocator":
"includes/Echo/EchoNewsletterUserLocator.php",
"BaseNewsletterPresentationModel":
"includes/Echo/BaseNewsletterPresentationModel.php",
@@ -156,6 +157,9 @@
],
"ContentModelCanBeUsedOn": [
"NewsletterHooks::onContentModelCanBeUsedOn"
+ ],
+ "EditFilterMergedContent": [
+ "NewsletterHooks::onEditFilterMergedContent"
]
},
"namespaces": [
diff --git a/includes/NewsletterDb.php b/includes/NewsletterDb.php
index 9cf1494..3548cf8 100644
--- a/includes/NewsletterDb.php
+++ b/includes/NewsletterDb.php
@@ -108,9 +108,12 @@
* @return bool|int the id of the newsletter added, false on failure
*/
public function addNewsletter( Newsletter $newsletter ) {
+ global $wgContLang;
$rowData = [
'nl_name' => $newsletter->getName(),
- 'nl_desc' => $newsletter->getDescription(),
+ // nl_newsletters.nl_desc is a blob but put some limit
+ // here which is less than the max size for blobs
+ 'nl_desc' => $wgContLang->truncate(
$newsletter->getDescription(), 600000 ),
'nl_main_page_id' => $newsletter->getPageId(),
];
@@ -137,11 +140,15 @@
* @return bool success of the action
*/
public function updateDescription( $id, $description ) {
+ global $wgContLang;
+
Assert::parameterType( 'integer', $id, '$id' );
Assert::parameterType( 'string', $description, '$description' );
$rowData = [
- 'nl_desc' => $description,
+ // nl_newsletters.nl_desc is a blob but put some limit
+ // here which is less than the max size for blobs
+ 'nl_desc' => $wgContLang->truncate( $description,
600000 ),
];
$conds = [
diff --git a/includes/NewsletterEditPage.php b/includes/NewsletterEditPage.php
index 2ba5727..c629a17 100644
--- a/includes/NewsletterEditPage.php
+++ b/includes/NewsletterEditPage.php
@@ -190,8 +190,6 @@
* @return Status
*/
public function attemptSave( array $input ) {
- global $wgContLang;
-
$data = [
'Name' => trim( $input['name'] ),
'Description' => trim( $input['description'] ),
@@ -240,12 +238,12 @@
$data['Name'],
// nl_newsletters.nl_desc is a blob but put some limit
// here which is less than the max size for blobs
- $wgContLang->truncate( $data['Description'], 600000 ),
+ $data['Description'],
$mainPageId
);
$newsletterCreated = $store->addNewsletter( $this->newsletter );
if ( $newsletterCreated ) {
- $title = Title::makeTitleSafe( NS_NEWSLETTER, trim(
$data['Name'] ) );
+ $title = Title::makeTitleSafe( NS_NEWSLETTER,
$data['Name'] );
$editSummaryMsg = $this->context->msg(
'newsletter-create-editsummary' );
$result = NewsletterContentHandler::edit(
$title,
diff --git a/includes/content/NewsletterContent.php
b/includes/content/NewsletterContent.php
index b4ca299..4039492 100644
--- a/includes/content/NewsletterContent.php
+++ b/includes/content/NewsletterContent.php
@@ -20,7 +20,7 @@
private $description;
/**
- * @var string|null
+ * @var Title
*/
private $mainPage;
@@ -53,7 +53,8 @@
public function isValid() {
$this->decode();
- if ( !is_string( $this->description ) || !is_string(
$this->mainPage ) || !is_array( $this->publishers ) ) {
+ if ( !is_string( $this->description ) || !( $this->mainPage
instanceof Title ) ||
+ !is_array( $this->publishers ) ) {
return false;
}
@@ -78,7 +79,8 @@
if ( $data ) {
$this->description = isset( $data->description ) ?
$data->description : null;
- $this->mainPage = isset( $data->mainpage ) ?
$data->mainpage : null;
+ $this->mainPage = !empty( $data->mainpage ) ?
Title::newFromText( $data->mainpage ) :
+ Title::makeTitle( NS_SPECIAL, 'Badtitle' );
if ( isset( $data->publishers ) && is_array(
$data->publishers ) ) {
$this->publishers = [];
foreach ( $data->publishers as $publisher ) {
@@ -89,7 +91,7 @@
$this->publishers[] = $publisher;
}
} else {
- $data->publishers = null;
+ $this->publishers = null;
}
}
$this->decoded = true;
@@ -125,14 +127,12 @@
protected function fillParserOutput( Title $title, $revId,
ParserOptions $options, $generateHtml, ParserOutput &$output ) {
if ( $generateHtml ) {
$this->newsletter = Newsletter::newFromName(
$title->getText() );
- if ( !$this->newsletter ) {
- throw new MWException( 'Cannot find newsletter
with name \"' . $title->getText() . '\"' );
- }
- // Make sure things are decoded at this point
+ //Make sure things are decoded at this point
$this->decode();
- $newsletterActionButtons =
$this->getNewsletterActionButtons( $options );
- $mainTitle = Title::newFromText( $this->mainPage );
+ $newsletterActionButtons = !$this->newsletter ? '' :
$this->getNewsletterActionButtons(
+ $options );
+ $mainTitle = $this->mainPage;
$fields = [
'mainpage' => [
@@ -158,7 +158,8 @@
'subscribers' => [
'type' => 'info',
'label-message' =>
'newsletter-view-subscriber-count',
- 'default' =>
$options->getUserLangObj()->formatNum( $this->newsletter->getSubscriberCount()
),
+ 'default' => !$this->newsletter ? 0 :
$options->getUserLangObj()->formatNum(
+
$this->newsletter->getSubscriberCount() ),
],
];
$publishersArray = $this->getPublishersFromJSONData(
$this->publishers );
@@ -173,29 +174,28 @@
->inLanguage(
$options->getUserLangObj() )
->escaped();
}
- // Show the 10 most recent issues if there have been
announcements
- $logs = '';
- $logCount = LogEventsList::showLogExtract(
- $logs, // by reference
- 'newsletter',
- SpecialPage::getTitleFor( 'Newsletter',
$this->newsletter->getId() ),
- '',
- [
- 'lim' => 10,
- 'showIfEmpty' => false,
- 'conds' => [ 'log_action' =>
'issue-added' ],
- 'extraUrlParams' => [ 'subtype' =>
'issue-added' ],
- ]
- );
- if ( $logCount !== 0 ) {
- $fields['issues'] = [
- 'type' => 'info',
- 'raw' => true,
- 'default' => $logs,
- 'label' => wfMessage(
'newsletter-view-issues-log' )
- ->inLanguage(
$options->getUserLangObj() )
- ->numParams( $logCount
)->text(),
- ];
+ if ( $this->newsletter ) {
+ // Show the 10 most recent issues if there have
been announcements
+ $logs = '';
+ $logCount = LogEventsList::showLogExtract(
$logs, // by reference
+ 'newsletter',
+ SpecialPage::getTitleFor( 'Newsletter',
$this->newsletter->getId() ), '', array(
+ 'lim' => 10,
+ 'showIfEmpty' => false,
+ 'conds' => array( 'log_action'
=> 'issue-added' ),
+ 'extraUrlParams' => array(
'subtype' => 'issue-added' ),
+ ) );
+ if ( $logCount !== 0 ) {
+ $fields['issues'] = array(
+ 'type' => 'info',
+ 'raw' => true,
+ 'default' => $logs,
+ 'label' => wfMessage(
'newsletter-view-issues-log' )
+ ->inLanguage(
$options->getUserLangObj() )
+ ->numParams( $logCount )
+ ->text(),
+ );
+ }
}
$form = $this->getHTMLForm(
$fields,
@@ -207,9 +207,13 @@
$form->suppressDefaultSubmit();
$form->prepareForm();
- $output->setText( $this->getNavigationLinks( $options )
. $newsletterActionButtons .
- "<br><br>" . $form->getBody() );
+ !$this->newsletter ? $output->setText( $form->getBody()
) : $output->setText(
+ $this->getNavigationLinks( $options ) .
$newsletterActionButtons . "<br><br>" .
+ $form->getBody()
+ );
return $output;
+ } else {
+ $output->setText( '' );
}
}
@@ -253,7 +257,7 @@
'label' => $wgOut->msg(
'newsletter-subscribe-button' )->text(),
'flags' => [ 'constructive' ],
'href' => SpecialPage::getTitleFor(
'Newsletter', $id. '/' .
- self::NEWSLETTER_SUBSCRIBE
)->getFullURL()
+
self::NEWSLETTER_SUBSCRIBE )->getFullURL()
]
);
@@ -263,7 +267,7 @@
'label' => $wgOut->msg(
'newsletter-unsubscribe-button' )->text(),
'flags' => [ 'destructive' ],
'href' => SpecialPage::getTitleFor(
'Newsletter', $id. '/' .
- self::NEWSLETTER_UNSUBSCRIBE
)->getFullURL()
+
self::NEWSLETTER_UNSUBSCRIBE )->getFullURL()
]
);
@@ -282,7 +286,7 @@
'label' => $wgOut->msg(
'newsletter-subscribers-button' )->text(),
'icon' => 'info',
'href' => SpecialPage::getTitleFor(
'Newsletter', $id. '/' .
- self::NEWSLETTER_SUBSCRIBERS
)->getFullURL()
+
self::NEWSLETTER_SUBSCRIBERS )->getFullURL()
]
);
@@ -293,7 +297,7 @@
'label' => $wgOut->msg(
'newsletter-announce-button' )->text(),
'icon' => 'comment',
'href' => SpecialPage::getTitleFor(
'Newsletter', $id. '/' .
- self::NEWSLETTER_ANNOUNCE
)->getFullURL()
+
self::NEWSLETTER_ANNOUNCE )->getFullURL()
]
);
}
@@ -381,8 +385,8 @@
$links[] = $link;
}
$newsletterLinks = Linker::makeSelfLinkObj(
- SpecialPage::getTitleFor( 'Newsletter',
$this->newsletter->getId() ), $this->getEscapedName()
- ) . ' ' . wfMessage( 'parentheses' )->rawParams(
$options->getUserLangObj()->pipeList( $links ) )->escaped();
+ SpecialPage::getTitleFor( 'Newsletter',
$this->newsletter->getId() ), $this->getEscapedName()
+ ) . ' ' . wfMessage( 'parentheses' )->rawParams(
$options->getUserLangObj()->pipeList( $links ) )->escaped();
return $wgOut->setSubtitle(
$options->getUserLangObj()->pipeList( [ $listLink, $newsletterLinks ] ) );
}
@@ -396,7 +400,7 @@
}
/**
- * @return string
+ * @return Title
*/
public function getMainPage() {
$this->decode();
@@ -435,4 +439,27 @@
return $truncatedtext;
}
+
+ /**
+ * @param Title $title Title of the page that is being edited.
+ * @param Content $old Content object representing the page's content
before the edit.
+ * @param bool $recursive bool indicating whether DataUpdates should
trigger recursive
+ * updates (relevant mostly for LinksUpdate).
+ * @param ParserOutput $parserOutput ParserOutput representing the
rendered version of the page after the edit.
+ * @return DataUpdate[]
+ *
+ * @see Content::getSecondaryDataUpdates()
+ */
+ public function getSecondaryDataUpdates( Title $title, Content $old =
null, $recursive = true,
+ ParserOutput $parserOutput =
null
+ ) {
+ global $wgUser;
+ // @todo This user object might not be the right one in some
cases.
+ // but that should be pretty rare in the context of newsletters.
+ $mwUpdate = new NewsletterDataUpdate( $this, $title, $wgUser );
+ return array_merge(
+ parent::getSecondaryDataUpdates( $title, $old,
$recursive, $parserOutput ),
+ [ $mwUpdate ]
+ );
+ }
}
diff --git a/includes/content/NewsletterDataUpdate.php
b/includes/content/NewsletterDataUpdate.php
new file mode 100644
index 0000000..1d157d7
--- /dev/null
+++ b/includes/content/NewsletterDataUpdate.php
@@ -0,0 +1,134 @@
+<?php
+/**
+ * @license GNU GPL v2+
+ * @author tonythomas
+ */
+use \MediaWiki\Logger\LoggerFactory;
+
+class NewsletterDataUpdate extends DataUpdate {
+
+ private $content; /** NewsletterContent */
+ private $user; /** @var User Triggering user */
+ private $title; /** @var Title */
+ protected $newsletter; /** @var Newsletter */
+ private $name; /** @var string */
+
+ /**
+ * @param NewsletterContent $content
+ * @param Title $title
+ * @param User $user
+ *
+ * @todo User might be wrong if triggered from template edits etc.
+ */
+ function __construct( NewsletterContent $content, Title $title, User
$user ) {
+ $this->content = $content;
+ $this->user = $user;
+ $this->title = $title;
+ }
+
+ function doUpdate() {
+ $logger = LoggerFactory::getInstance( 'newsletter' );
+
+ $store = NewsletterStore::getDefaultInstance();
+ // We might have a situation when the newsletter is not created
yet. Hence, we should add
+ // that to the database, and exit.
+ $newsletter = Newsletter::newFromName( $this->title->getText()
);
+
+ if ( !$newsletter ) {
+ // Possible API edit to create a new newsletter, and
the newsletter is not in the
+ // database yet.
+ $this->name = $this->title->getText();
+ $dbr = wfGetDB( DB_SLAVE );
+ $rows = $dbr->select(
+ 'nl_newsletters',
+ [ 'nl_name', 'nl_main_page_id', 'nl_active' ],
+ $dbr->makeList([
+ 'nl_name' => $this->name,
+ $dbr->makeList(
+ [
+ 'nl_main_page_id' =>
$this->content->getMainPage()->getArticleID(),
+ 'nl_active' => 1
+ ], LIST_AND )
+ ], LIST_OR )
+ );
+
+ // Check whether another existing newsletter has the
same name or main page
+ foreach ( $rows as $row ) {
+ if ( $row->nl_name === $this->name ) {
+ $logger->warning(
'newsletter-exist-error', $this->name );
+ return;
+ } elseif ( (int)$row->nl_main_page_id ===
$this->content->getMainPage()->getArticleID()
+ && (int)$row->nl_active === 1 ) {
+ $logger->warning(
'newsletter-mainpage-in-use' );
+ return;
+ }
+ }
+ if ( $this->user->pingLimiter( 'newsletter' ) ) {
+ // Default user access level for creating a
newsletter is quite low
+ // so add a throttle here to prevent abuse (eg.
mass vandalism spree)
+ throw new ThrottledError;
+ }
+ $title = Title::makeTitleSafe( NS_NEWSLETTER,
$this->name );
+ $newsletter = new Newsletter( 0,
+ $title->getText(),
+ $this->content->getDescription(),
+ $this->content->getMainPage()->getArticleID()
+ );
+ $newsletterCreated = $store->addNewsletter( $newsletter
);
+ if ( $newsletterCreated ) {
+ $newsletter->subscribe( $this->user );
+ $store->addPublisher( $newsletter, $this->user
);
+ return Status::newGood();
+ } else {
+ // Couldn't insert to the DB..
+ return Status::newFatal(
'newsletter-create-error' );
+ }
+ }
+
+ // This was a possible edit to an existing newsletter.
+ $newsletterId = $newsletter->getId();
+ if ( $this->content->getDescription() !=
$newsletter->getDescription() ) {
+ $store->updateDescription( $newsletterId,
$this->content->getDescription() );
+ }
+
+ $updatedMainPageId =
$this->content->getMainPage()->getArticleID();
+ if ( $updatedMainPageId != $newsletter->getPageId() ) {
+ $store->updateMainPage($newsletterId,
$updatedMainPageId );
+ }
+
+ $updatedPublishers = array_map( 'User::newFromName',
$this->content->getPublishers() );
+ $oldPublishersIds = $newsletter->getPublishers();
+ $updatedPublishersIds = array();
+
+ foreach ( $updatedPublishers as $user ) {
+ if ( $user instanceof User )
+ $updatedPublishersIds[] = $user->getId();
+ }
+
+ // Do the actual modifications now
+ $added = array_diff( $updatedPublishersIds, $oldPublishersIds );
+ $removed = array_diff( $oldPublishersIds, $updatedPublishersIds
);
+
+ // @todo Do this in a batch..
+ foreach ( $added as $auId ) {
+ $store->addPublisher( $newsletter, User::newFromId(
$auId ) );
+ }
+
+ if ( $added ) {
+ EchoEvent::create(
+ array(
+ 'type' => 'newsletter-newpublisher',
+ 'extra' => array(
+ 'newsletter-name' =>
$newsletter->getName(),
+ 'new-publishers-id' => $added,
+ 'newsletter-id' => $newsletterId
+ ),
+ 'agent' => $this->user
+ )
+ );
+ }
+ foreach ( $removed as $ruId ) {
+ $store->removePublisher( $newsletter, User::newFromId(
$ruId ) );
+ }
+ }
+}
diff --git a/includes/content/NewsletterDiffEngine.php
b/includes/content/NewsletterDiffEngine.php
index a371069..c3f54bc 100644
--- a/includes/content/NewsletterDiffEngine.php
+++ b/includes/content/NewsletterDiffEngine.php
@@ -27,7 +27,7 @@
}
$mainPageDiff = $this->generateTextDiffBody(
- $old->getMainPage(), $new->getMainPage()
+ $old->getMainPage()->getText(),
$new->getMainPage()->getText()
);
if ( $mainPageDiff ) {
diff --git a/includes/specials/SpecialNewsletterCreate.php
b/includes/specials/SpecialNewsletterCreate.php
index ba87302..6e8a201 100644
--- a/includes/specials/SpecialNewsletterCreate.php
+++ b/includes/specials/SpecialNewsletterCreate.php
@@ -70,8 +70,6 @@
* @return Status
*/
public function onSubmit( array $input ) {
- global $wgContLang;
-
$data = [
'Name' => trim( $input['name'] ),
'Description' => trim( $input['description'] ),
@@ -91,14 +89,14 @@
$rows = $dbr->select(
'nl_newsletters',
[ 'nl_name', 'nl_main_page_id', 'nl_active' ],
- $dbr->makeList(
- [
- 'nl_name' => $data['Name'],
- 'nl_main_page_id' => $mainPageId,
- 'nl_active' => 1
- ],
- LIST_OR
- )
+ $dbr->makeList([
+ 'nl_name' => $data['Name'],
+ $dbr->makeList(
+ [
+ 'nl_main_page_id' =>
$mainPageId,
+ 'nl_active' => 1
+ ], LIST_AND )
+ ], LIST_OR )
);
// Check whether another existing newsletter has the same name
or main page
foreach ( $rows as $row ) {
@@ -116,14 +114,12 @@
throw new ThrottledError;
}
- $title = Title::makeTitleSafe( NS_NEWSLETTER, trim(
$data['Name'] ) );
+ $title = Title::makeTitleSafe( NS_NEWSLETTER, $data['Name'] );
$store = NewsletterStore::getDefaultInstance();
$this->newsletter = new Newsletter( 0,
$title->getText(),
- // nl_newsletters.nl_desc is a blob but put some limit
- // here which is less than the max size for blobs
- $wgContLang->truncate( $data['Description'], 600000 ),
+ $data['Description'],
$mainPageId
);
$newsletterCreated = $store->addNewsletter( $this->newsletter );
--
To view, visit https://gerrit.wikimedia.org/r/346055
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I2f631e336b0219958c917242078d88ec94a95ae5
Gerrit-PatchSet: 26
Gerrit-Project: mediawiki/extensions/Newsletter
Gerrit-Branch: master
Gerrit-Owner: 01tonythomas <[email protected]>
Gerrit-Reviewer: 01tonythomas <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Brian Wolff <[email protected]>
Gerrit-Reviewer: Florianschmidtwelzow <[email protected]>
Gerrit-Reviewer: Glaisher <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
