jenkins-bot has submitted this change and it was merged.
Change subject: Add NewsletterValidator and changes to Special:CreateNewsletter
......................................................................
Add NewsletterValidator and changes to Special:CreateNewsletter
Add a new class NewsletterValidator which can also be used when
newsletter fields are edited.
Changes to Special:CreateNewsletter
- Separate error messages shown for main page and newsletter name key collisions
- Rename autoSubscribe() to onPostCreation() as it doesn't only do subscription
- Use Title's naming restrictions for newsletter names as well
- Don't rely on the web form's browser validation as this can easily be bypassed
- Don't allow whitespace characters on required input
- Increase newsletter name maxlength's field - currently it's a bit low
Bug: T110491
Change-Id: Ica686bc1eb26bb482c07dff9e0982bbcdad3b210
---
M extension.json
M i18n/en.json
M i18n/qqq.json
A includes/NewsletterValidator.php
M includes/specials/SpecialNewsletterCreate.php
5 files changed, 135 insertions(+), 56 deletions(-)
Approvals:
01tonythomas: Looks good to me, approved
jenkins-bot: Verified
diff --git a/extension.json b/extension.json
index 88b3245..b90ba12 100644
--- a/extension.json
+++ b/extension.json
@@ -50,6 +50,7 @@
"Newsletter": "includes/Newsletter.php",
"NewsletterDb": "includes/NewsletterDb.php",
"NewsletterHooks": "Newsletter.hooks.php",
+ "NewsletterValidator": "includes/NewsletterValidator.php",
"NewsletterLinksGenerator":
"includes/specials/NewsletterLinksGenerator.php",
"SpecialNewsletter": "includes/specials/SpecialNewsletter.php",
"SpecialNewsletters":
"includes/specials/SpecialNewsletters.php",
diff --git a/i18n/en.json b/i18n/en.json
index 9c4bd79..88c1ad1 100755
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -10,12 +10,17 @@
"newsletter-name": "Name of newsletter",
"newsletter-desc": "Description",
"newsletter-title": "Title of Main Page",
- "newsletter-exist-error": "A newsletter with the same name already
exists. Try again with another name",
+ "newsletter-exist-error": "A newsletter with the name \"$1\" already
exists. Please try again with another name.",
+ "newsletter-create-error": "An error occured while trying to create a
new newsletter. Please try again.",
+ "newsletter-input-required": "Required input was not entered. Please
try again.",
+ "newsletter-invalid-name": "The name you entered for the newsletter is
invalid. Please try again.",
"newslettercreate": "Create newsletter",
"newslettercreate-text": "This page allows you to create a new
newsletter. You will be added as a publisher and subscribed to the newsletter
on its creation. All fields are required.",
"newsletter-create-submit": "Create newsletter",
"newsletter-create-confirmation": "A new newsletter has been
successfully created.",
"newsletter-create-mainpage-error": "Invalid newsletter main page
entered. Please try again.",
+ "newsletter-mainpage-non-existent": "The newsletter main page does not
exist. Please enter a valid existing page.",
+ "newsletter-mainpage-in-use": "An existing newsletter has the same main
page. Please enter another title.",
"newsletter-create-short-description-error": "The description is too
short (less than 30 characters). Please try again.",
"newsletter-subtitlelinks-list": "List of newsletters",
"newsletter-subtitlelinks-create": "Create a new newsletter",
diff --git a/i18n/qqq.json b/i18n/qqq.json
index 99c2b63..d77be7f 100644
--- a/i18n/qqq.json
+++ b/i18n/qqq.json
@@ -14,11 +14,16 @@
"newsletter-name": "Label of the field which takes the name of
newsletter as input in [[Special:CreateNewsletter]]",
"newsletter-desc": "Label of the field which takes a short description
of newsletter as input in
[[Special:CreateNewsletter]]\n{{Identical|Description}}",
"newsletter-title": "Label of the field which takes the title of Main
Page of newsletter as input in [[Special:CreateNewsletter]]",
- "newsletter-exist-error": "Error message to be displayed in
[[Special:CreateNewsletter]] when trying to create a newsletter with same name
as an existing newsletter",
+ "newsletter-exist-error": "Error message to be displayed in
[[Special:CreateNewsletter]] when trying to create a newsletter with same name
as an existing newsletter. $1- Name of newsletter",
+ "newsletter-create-error": "Error message shown on
[[Special:CreateNewsletter]] if newsletter creation failed.",
+ "newsletter-input-required": "Error message shown on
[[Special:CreateNewsletter]] if required input is missing..",
+ "newsletter-invalid-name": "Error message shown on
[[Special:CreateNewsletter]] if the newsletter name is invalid.",
"newslettercreate": "Name of [[Special:CreateNewsletter]]. This is used
as the header on this page and shown in links to the page.",
"newslettercreate-text": "Introductory message shown on top of
Special:CreateNewsletter.",
"newsletter-create-submit": "Label of submit button on
[[Special:CreateNewsletter]]",
"newsletter-create-confirmation": "Confirmation message displayed after
creation of a newsletter",
+ "newsletter-mainpage-non-existent": "Error message shown if the
newsletter main page doesn't exist.",
+ "newsletter-mainpage-in-use": "Error message shown if the newsletter's
main page is already in use by another newsletter.",
"newsletter-create-mainpage-error": "Error message shown on
[[Special:CreateNewsletter]] if the page entered on main page field does not
exist.",
"newsletter-create-short-description-error": "Error message shown on
[[Special:CreateNewsletter]] if the text entered in the description field is
too short.",
"newsletter-subtitlelinks-list": "Label for link to
[[Special:Newsletters]] shown under the header on Newsletter special
pages.\n\nSee also:\n* {{msg-mw|newsletter-subtitlelinks-create}}",
diff --git a/includes/NewsletterValidator.php b/includes/NewsletterValidator.php
new file mode 100644
index 0000000..1837d88
--- /dev/null
+++ b/includes/NewsletterValidator.php
@@ -0,0 +1,61 @@
+<?php
+/**
+ * Handles validation for newsletters
+ */
+
+class NewsletterValidator {
+
+ private static $requiredData = array(
+ 'Name',
+ 'Description',
+ 'MainPage',
+ );
+
+ public function __construct( $fields ) {
+ $this->data = $fields;
+ }
+
+ /**
+ * Check whether all input have proper values
+ *
+ * @return Status fatal if invalid, good otherwise
+ */
+ public function validate() {
+ // Check whether required fields are not empty
+ foreach( self::$requiredData as $field ) {
+ if ( trim( $this->data[ $field ] ) === '' ) {
+ return Status::newFatal(
'newsletter-input-required' );
+ }
+ }
+
+ // Prevents random nonsensical characters in newsletter names
+ // and also adds a length limit
+ // (uses Title's rules now - maybe use our own?)
+ $name = Title::makeTitleSafe( NS_MAIN, $this->data['Name'] );
+ if ( !$name ) {
+ return Status::newFatal( 'newsletter-invalid-name' );
+ }
+
+ if ( strlen ( $this->data['Description'] ) < 30 ) {
+ // Should this limit be lowered?
+ return Status::newFatal(
'newsletter-create-short-description-error' );
+ }
+
+ $mainTitle = $this->data['MainPage'];
+ if ( !$mainTitle ) {
+ return Status::newFatal(
'newsletter-create-mainpage-error' );
+ }
+
+ if ( !$mainTitle->canExist() ) {
+ return Status::newFatal(
'newsletter-create-mainpage-error' );
+ }
+
+ if ( !$mainTitle->exists() ) {
+ return Status::newFatal(
'newsletter-mainpage-non-existent' );
+ }
+
+
+
+ return Status::newGood();
+ }
+}
diff --git a/includes/specials/SpecialNewsletterCreate.php
b/includes/specials/SpecialNewsletterCreate.php
index bfb0848..fe8521f 100644
--- a/includes/specials/SpecialNewsletterCreate.php
+++ b/includes/specials/SpecialNewsletterCreate.php
@@ -36,7 +36,7 @@
'type' => 'text',
'required' => true,
'label-message' => 'newsletter-name',
- 'maxlength' => 50
+ 'maxlength' => 120
),
'description' => array(
'type' => 'textarea',
@@ -47,7 +47,6 @@
),
'mainpage' => array(
'type' => 'title',
- 'exists' => true,
'required' => true,
'label-message' => 'newsletter-title',
),
@@ -57,17 +56,47 @@
/**
* Do input validation, error handling and create a new newletter.
*
- * @param array $data The data entered by user in the form
- * @return bool|array true on success, array on error
+ * @param array $input The data entered by user in the form
+ * @throws ThrottledError
+ * @return Status
*/
- public function onSubmit( array $data ) {
+ public function onSubmit( array $input ) {
global $wgContLang;
- $mainTitle = Title::newFromText( $data['mainpage'] );
- if ( !$mainTitle ) {
- // HTMLTitleTextField should do validation but we can't
be sure about
- // it so let's check again here - otherwise this may
throw fatals below
- return array( 'newsletter-create-mainpage-error' );
+ $data = array(
+ 'Name' => trim( $input['name'] ),
+ 'Description' => trim( $input['description'] ),
+ 'MainPage' => Title::newFromText( $input['mainpage'] ),
+ );
+
+ $validator = new NewsletterValidator( $data );
+ $validation = $validator->validate();
+ if ( !$validation->isGood() ) {
+ // Invalid input was entered
+ return $validation;
+ }
+
+ $mainPageId = $data['MainPage']->getArticleID();
+
+ $dbr = wfGetDB( DB_SLAVE );
+ $rows = $dbr->select(
+ 'nl_newsletters',
+ array( 'nl_name', 'nl_main_page_id' ),
+ $dbr->makeList(
+ array(
+ 'nl_name' => $data['Name'],
+ 'nl_main_page_id' => $mainPageId,
+ ),
+ LIST_OR
+ )
+ );
+ // Check whether another existing newsletter has the same name
or main page
+ foreach( $rows as $row ) {
+ if ( $row->nl_name === $data['Name'] ) {
+ return Status::newFatal(
'newsletter-exist-error', $data['Name'] );
+ } elseif ( (int)$row->nl_main_page_id === $mainPageId )
{
+ return Status::newFatal(
'newsletter-mainpage-in-use' );
+ }
}
$user = $this->getUser();
@@ -77,55 +106,25 @@
throw new ThrottledError;
}
- $articleId = $mainTitle->getArticleId();
-
- if ( isset( $data['name'] ) &&
- isset( $data['description'] ) &&
- ( $articleId !== 0 ) &&
- isset( $data['mainpage'] )
- ) {
- if ( strlen ( $data['description'] ) < 30 ) {
- return array(
'newsletter-create-short-description-error' );
- }
-
- $db = NewsletterDb::newFromGlobalState();
-
+ $ndb = NewsletterDb::newFromGlobalState();
+ $newsletterCreated = $ndb->addNewsletter(
+ $data['Name'],
// nl_newsletters.nl_desc is a blob but put some limit
- // here which is less than max size for blobs
- $description = $wgContLang->truncate( trim(
$data['description'] ), 600000 );
+ // here which is less than the max size for blobs
+ $wgContLang->truncate( $data['Description'], 600000 ),
+ $mainPageId
+ );
- $newsletterAdded = $db->addNewsletter(
- trim( $data['name'] ),
- $description,
- $articleId
- );
+ if ( $newsletterCreated ) {
+ $newsletter = $ndb->getNewsletterForPageId( $mainPageId
);
+ $this->onPostCreation( $newsletter->getId(),
$user->getId() );
- if ( !$newsletterAdded ) {
- // @todo FIXME: This shouldn't be thrown for
main page key collisions
- return array( 'newsletter-exist-error' );
- }
-
- $newsletter = $db->getNewsletterForPageId( $articleId );
-
- $this->autoSubscribe( $newsletter->getId(),
$user->getId() );
-
- return true;
+ return Status::newGood();
}
- // Could not insert - newsletter by this name already exists
- return array( 'newsletter-exist-error' );
-
+ // Couldn't insert to the DB..
+ return Status::newFatal( 'newsletter-create-error' );
}
-
- protected function getDisplayFormat() {
- return 'ooui';
- }
-
- public function onSuccess() {
- // @todo Link to corresponding Special:Newsletter page
- $this->getOutput()->addWikiMsg(
'newsletter-create-confirmation' );
- }
-
/**
* Automatically subscribe and add creator as publisher of the
newsletter
@@ -133,10 +132,18 @@
* @param int $newsletterId Id of the newsletter
* @param int $userID User Id of the publisher
*/
- private function autoSubscribe( $newsletterId, $userID ) {
+ private function onPostCreation( $newsletterId, $userID ) {
$db = NewsletterDb::newFromGlobalState();
$db->addPublisher( $userID, $newsletterId );
$db->addSubscription( $userID, $newsletterId );
}
+ public function onSuccess() {
+ // @todo Link to corresponding Special:Newsletter page
+ $this->getOutput()->addWikiMsg(
'newsletter-create-confirmation' );
+ }
+
+ protected function getDisplayFormat() {
+ return 'ooui';
+ }
}
--
To view, visit https://gerrit.wikimedia.org/r/270496
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Ica686bc1eb26bb482c07dff9e0982bbcdad3b210
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/Newsletter
Gerrit-Branch: master
Gerrit-Owner: Glaisher <[email protected]>
Gerrit-Reviewer: 01tonythomas <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Siebrand <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
