From: Kunal Mehta <legoktm@member.fsf.org>
Date: Sat, 15 Oct 2016 07:34:26 +0000 (-0700)
Subject: Improve Special:BookSources validation and error messages
X-Git-Tag: 1.31.0-rc.0~5097^2
X-Git-Url: http://git.heureux-cyclage.org/?a=commitdiff_plain;h=56c1f67c75daa8fdda7f07e2ec223331fd2da567;p=lhc%2Fweb%2Fwiklou.git

Improve Special:BookSources validation and error messages

Visiting Special:BookSources/invalid would not show any error message
because "invalid" would get stripped away by cleanIsbn(), and then
appear as the empty string.

Instead, first validate the provided ISBN (if any), and display the
error message if invalid. Then show the form and book details.

Tests included.

Also remove an unused variable.

Change-Id: I40b703eace956ebbcdc0a2c2986b2c10474dd1fd
---

diff --git a/includes/specials/SpecialBooksources.php b/includes/specials/SpecialBooksources.php
index 11faa2803d..2fef72520a 100644
--- a/includes/specials/SpecialBooksources.php
+++ b/includes/specials/SpecialBooksources.php
@@ -29,11 +29,6 @@
  * @ingroup SpecialPage
  */
 class SpecialBookSources extends SpecialPage {
-	/**
-	 * ISBN passed to the page, if any
-	 */
-	protected $isbn = '';
-
 	public function __construct() {
 		parent::__construct( 'Booksources' );
 	}
@@ -49,19 +44,21 @@ class SpecialBookSources extends SpecialPage {
 		$this->setHeaders();
 		$this->outputHeader();
 
-		$this->isbn = self::cleanIsbn( $isbn ?: $this->getRequest()->getText( 'isbn' ) );
+		// User provided ISBN
+		$isbn = $isbn ?: $this->getRequest()->getText( 'isbn' );
+		$isbn = trim( $isbn );
 
-		$this->buildForm();
+		$this->buildForm( $isbn );
 
-		if ( $this->isbn !== '' ) {
-			if ( !self::isValidISBN( $this->isbn ) ) {
+		if ( $isbn !== '' ) {
+			if ( !self::isValidISBN( $isbn ) ) {
 				$out->wrapWikiMsg(
 					"<div class=\"error\">\n$1\n</div>",
 					'booksources-invalid-isbn'
 				);
 			}
 
-			$this->showList();
+			$this->showList( $isbn );
 		}
 	}
 
@@ -121,20 +118,22 @@ class SpecialBookSources extends SpecialPage {
 
 	/**
 	 * Generate a form to allow users to enter an ISBN
+	 *
+	 * @param string $isbn
 	 */
-	private function buildForm() {
+	private function buildForm( $isbn ) {
 		$formDescriptor = [
 			'isbn' => [
 				'type' => 'text',
 				'name' => 'isbn',
 				'label-message' => 'booksources-isbn',
-				'default' => $this->isbn,
+				'default' => $isbn,
 				'autofocus' => true,
 				'required' => true,
 			],
 		];
 
-		$htmlForm = HTMLForm::factory( 'ooui', $formDescriptor, $this->getContext() )
+		HTMLForm::factory( 'ooui', $formDescriptor, $this->getContext() )
 			->setWrapperLegendMsg( 'booksources-search-legend' )
 			->setSubmitTextMsg( 'booksources-search' )
 			->setMethod( 'get' )
@@ -146,17 +145,19 @@ class SpecialBookSources extends SpecialPage {
 	 * Determine where to get the list of book sources from,
 	 * format and output them
 	 *
+	 * @param string $isbn
 	 * @throws MWException
 	 * @return bool
 	 */
-	private function showList() {
+	private function showList( $isbn ) {
 		$out = $this->getOutput();
 
 		global $wgContLang;
 
+		$isbn = self::cleanIsbn( $isbn );
 		# Hook to allow extensions to insert additional HTML,
 		# e.g. for API-interacting plugins and so on
-		Hooks::run( 'BookInformation', [ $this->isbn, $out ] );
+		Hooks::run( 'BookInformation', [ $isbn, $out ] );
 
 		# Check for a local page such as Project:Book_sources and use that if available
 		$page = $this->msg( 'booksources' )->inContentLanguage()->text();
@@ -169,7 +170,7 @@ class SpecialBookSources extends SpecialPage {
 				// XXX: in the future, this could be stored as structured data, defining a list of book sources
 
 				$text = $content->getNativeData();
-				$out->addWikiText( str_replace( 'MAGICNUMBER', $this->isbn, $text ) );
+				$out->addWikiText( str_replace( 'MAGICNUMBER', $isbn, $text ) );
 
 				return true;
 			} else {
@@ -182,7 +183,7 @@ class SpecialBookSources extends SpecialPage {
 		$out->addHTML( '<ul>' );
 		$items = $wgContLang->getBookstoreList();
 		foreach ( $items as $label => $url ) {
-			$out->addHTML( $this->makeListItem( $label, $url ) );
+			$out->addHTML( $this->makeListItem( $isbn, $label, $url ) );
 		}
 		$out->addHTML( '</ul>' );
 
@@ -192,12 +193,13 @@ class SpecialBookSources extends SpecialPage {
 	/**
 	 * Format a book source list item
 	 *
+	 * @param string $isbn
 	 * @param string $label Book source label
 	 * @param string $url Book source URL
 	 * @return string
 	 */
-	private function makeListItem( $label, $url ) {
-		$url = str_replace( '$1', $this->isbn, $url );
+	private function makeListItem( $isbn, $label, $url ) {
+		$url = str_replace( '$1', $isbn, $url );
 
 		return Html::rawElement( 'li', [],
 			Html::element( 'a', [ 'href' => $url, 'class' => 'external' ], $label )
diff --git a/tests/phpunit/includes/specials/SpecialBooksourcesTest.php b/tests/phpunit/includes/specials/SpecialBooksourcesTest.php
index 3310d0217a..074045d79f 100644
--- a/tests/phpunit/includes/specials/SpecialBooksourcesTest.php
+++ b/tests/phpunit/includes/specials/SpecialBooksourcesTest.php
@@ -1,5 +1,5 @@
 <?php
-class SpecialBooksourcesTest extends MediaWikiTestCase {
+class SpecialBooksourcesTest extends SpecialPageTestBase {
 	public static function provideISBNs() {
 		return [
 			[ '978-0-300-14424-6', true ],
@@ -33,4 +33,19 @@ class SpecialBooksourcesTest extends MediaWikiTestCase {
 	public function testIsValidISBN( $isbn, $isValid ) {
 		$this->assertSame( $isValid, SpecialBookSources::isValidISBN( $isbn ) );
 	}
+
+	protected function newSpecialPage() {
+		return new SpecialBookSources();
+	}
+
+	/**
+	 * @covers SpecialBookSources::execute
+	 */
+	public function testExecute() {
+		list( $html, ) = $this->executeSpecialPage( 'Invalid', null, 'qqx' );
+		$this->assertContains( '(booksources-invalid-isbn)', $html );
+		list( $html, ) = $this->executeSpecialPage( '0-7475-3269-9', null, 'qqx' );
+		$this->assertNotContains( '(booksources-invalid-isbn)', $html );
+		$this->assertContains( '(booksources-text)', $html );
+	}
 }