jenkins-bot has submitted this change and it was merged.

Change subject: Improve Special:BookSources validation and error messages
......................................................................


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
---
M includes/specials/SpecialBooksources.php
M tests/phpunit/includes/specials/SpecialBooksourcesTest.php
2 files changed, 37 insertions(+), 20 deletions(-)

Approvals:
  Florianschmidtwelzow: Looks good to me, approved
  jenkins-bot: Verified



diff --git a/includes/specials/SpecialBooksources.php 
b/includes/specials/SpecialBooksources.php
index 11faa28..2fef725 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 @@
                $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 @@
 
        /**
         * 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 @@
         * 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 @@
                                // 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 @@
                $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 @@
        /**
         * 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 3310d02..074045d 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 @@
        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 );
+       }
 }

-- 
To view, visit https://gerrit.wikimedia.org/r/316047
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I40b703eace956ebbcdc0a2c2986b2c10474dd1fd
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/core
Gerrit-Branch: master
Gerrit-Owner: Legoktm <legoktm.wikipe...@gmail.com>
Gerrit-Reviewer: Bartosz DziewoƄski <matma....@gmail.com>
Gerrit-Reviewer: Florianschmidtwelzow <florian.schmidt.stargatewis...@gmail.com>
Gerrit-Reviewer: TTO <at.li...@live.com.au>
Gerrit-Reviewer: Tim Starling <tstarl...@wikimedia.org>
Gerrit-Reviewer: jenkins-bot <>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to