jenkins-bot has submitted this change and it was merged. Change subject: Adding and removing claims now shows a full diff ......................................................................
Adding and removing claims now shows a full diff Until now if you added a new claim or removed a claim the diff would only show the mainsnak. This change means the whole claim appears in the diff when it is removed. This also adds test for the ClaimDifferenceVisualizer There were no tests actually running in this class before as they were commented out... We also remove a functions that is no longer used Bug: 53142 Change-Id: Ib8709a7b5a1cc3e2ba32bc9057154ced1ff2d334 --- M lib/includes/ClaimDiffer.php M lib/includes/ClaimDifferenceVisualizer.php M lib/tests/phpunit/claim/ClaimDifferTest.php M lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php 4 files changed, 272 insertions(+), 149 deletions(-) Approvals: Aude: Looks good to me, approved jenkins-bot: Verified diff --git a/lib/includes/ClaimDiffer.php b/lib/includes/ClaimDiffer.php index 9a23c38..5b42066 100644 --- a/lib/includes/ClaimDiffer.php +++ b/lib/includes/ClaimDiffer.php @@ -13,6 +13,7 @@ * * @licence GNU GPL v2+ * @author Tobias Gritschacher < [email protected] > + * @author Adam Shorland */ class ClaimDiffer { @@ -39,41 +40,84 @@ * * @since 0.4 * - * @param Claim $oldClaim - * @param Claim $newClaim + * @param Claim|null $oldClaim + * @param Claim|null $newClaim * * @return ClaimDifference */ - public function diffClaims( Claim $oldClaim, Claim $newClaim ) { - $mainSnakChange = null; - $rankChange = null; - $referenceChanges = null; + public function diffClaims( $oldClaim, $newClaim ) { + $mainSnakChange = $this->diffMainSnaks( $oldClaim, $newClaim ); + $qualifierChanges = $this->diffQualifiers( $oldClaim, $newClaim ); - if ( !$oldClaim->getMainSnak()->equals( $newClaim->getMainSnak() ) ) { - $mainSnakChange = new DiffOpChange( $oldClaim->getMainSnak(), $newClaim->getMainSnak() ); - } - - $oldQualifiers = iterator_to_array( $oldClaim->getQualifiers() ); - $newQualifiers = iterator_to_array( $newClaim->getQualifiers() ); - - $qualifierChanges = new Diff( $this->listDiffer->doDiff( - $oldQualifiers, - $newQualifiers - ), false ); - - if ( $oldClaim instanceof Statement && $newClaim instanceof Statement ) { - - if ( $oldClaim->getRank() !== $newClaim->getRank() ) { - $rankChange = new DiffOpChange( $oldClaim->getRank(), $newClaim->getRank() ); - } - - $referenceChanges = new Diff( $this->listDiffer->doDiff( - iterator_to_array( $oldClaim->getReferences() ), - iterator_to_array( $newClaim->getReferences() ) - ), false ); + if ( $oldClaim instanceof Statement || $newClaim instanceof Statement ) { + $rankChange = $this->diffRank( $oldClaim, $newClaim ); + $referenceChanges = $this->diffReferences( $oldClaim, $newClaim ); + } else { + $rankChange = null; + $referenceChanges = null; } return new ClaimDifference( $mainSnakChange, $qualifierChanges, $referenceChanges, $rankChange ); } + /** + * @param Claim $oldClaim + * @param Claim $newClaim + * + * @return DiffOpChange|null + */ + private function diffMainSnaks( $oldClaim, $newClaim ) { + $oldClaimMainSnak = $oldClaim === null ? null : $oldClaim->getMainSnak(); + $newClaimMainSnak = $newClaim === null ? null : $newClaim->getMainSnak(); + if( $oldClaimMainSnak !== $newClaimMainSnak ){ + return new DiffOpChange( $oldClaimMainSnak, $newClaimMainSnak ); + } + return null; + } + + /** + * @param Claim $oldClaim + * @param Claim $newClaim + * + * @return DiffOpChange|null + */ + private function diffQualifiers( $oldClaim, $newClaim ) { + $oldQualifiers = $oldClaim === null ? array() : iterator_to_array( $oldClaim->getQualifiers() ); + $newQualifiers = $newClaim === null ? array() : iterator_to_array( $newClaim->getQualifiers() ); + if( $oldQualifiers !== $newQualifiers ){ + return new Diff( $this->listDiffer->doDiff( $oldQualifiers, $newQualifiers ), false ); + } + return null; + } + + /** + * @param Statement $oldClaim + * @param Statement $newClaim + * + * @return DiffOpChange|null + */ + private function diffRank( $oldClaim, $newClaim ) { + $oldRank = $oldClaim === null ? null : $oldClaim->getRank(); + $newRank = $newClaim === null ? null : $newClaim->getRank(); + if( $oldRank !== $newRank ){ + return new DiffOpChange( $oldRank, $newRank ); + } + return null; + } + + /** + * @param Statement $oldClaim + * @param Statement $newClaim + * + * @return DiffOpChange|null + */ + private function diffReferences( $oldClaim, $newClaim ) { + $oldReferences = $oldClaim === null ? array() : iterator_to_array( $oldClaim->getReferences() ); + $newReferences = $newClaim === null ? array() : iterator_to_array( $newClaim->getReferences() ); + if( $oldReferences !== $newReferences ){ + return new Diff( $this->listDiffer->doDiff( $oldReferences, $newReferences ), false ); + } + return null; + } + } diff --git a/lib/includes/ClaimDifferenceVisualizer.php b/lib/includes/ClaimDifferenceVisualizer.php index f37a0da..b17cb81 100644 --- a/lib/includes/ClaimDifferenceVisualizer.php +++ b/lib/includes/ClaimDifferenceVisualizer.php @@ -1,12 +1,12 @@ <?php namespace Wikibase; -use DataValues\TimeValue; use Diff\DiffOpAdd; use Diff\DiffOpChange; use Diff\DiffOpRemove; -use Html; +use Diff\ListDiffer; use Diff\Diff; +use InvalidArgumentException; use RuntimeException; use ValueParsers\FormattingException; use Wikibase\Lib\EntityIdLabelFormatter; @@ -17,12 +17,10 @@ * * @since 0.4 * - * @file - * @ingroup WikibaseLib - * * @licence GNU GPL v2+ * @author Tobias Gritschacher < [email protected] > * @author Katie Filbert < [email protected] > + * @author Adam Shorland */ class ClaimDifferenceVisualizer { @@ -48,11 +46,11 @@ * @param EntityIdLabelFormatter $propertyIdFormatter * @param SnakFormatter $snakFormatter * - * @throws \InvalidArgumentException + * @throws InvalidArgumentException */ public function __construct( EntityIdLabelFormatter $propertyIdFormatter, SnakFormatter $snakFormatter ) { if ( $snakFormatter->getFormat() !== SnakFormatter::FORMAT_PLAIN ) { - throw new \InvalidArgumentException( + throw new InvalidArgumentException( 'Expected $snakFormatter to generate plain text, not ' . $snakFormatter->getFormat() ); } @@ -109,16 +107,9 @@ * @return string */ public function visualizeNewClaim( Claim $claim ) { - $mainSnak = $claim->getMainSnak(); - - $html = ''; - - $html .= $this->getSnakHtml( - null, - $mainSnak - ); - - return $html; + $claimDiffer = new ClaimDiffer( new ListDiffer() ); + $claimDifference = $claimDiffer->diffClaims( null, $claim ); + return $this->visualizeClaimChange( $claimDifference, $claim ); } /** @@ -131,16 +122,9 @@ * @return string */ public function visualizeRemovedClaim( Claim $claim ) { - $mainSnak = $claim->getMainSnak(); - - $html = ''; - - $html .= $this->getSnakHtml( - $mainSnak, - null - ); - - return $html; + $claimDiffer = new ClaimDiffer( new ListDiffer() ); + $claimDifference = $claimDiffer->diffClaims( $claim, null ); + return $this->visualizeClaimChange( $claimDifference, $claim ); } /** @@ -155,7 +139,7 @@ protected function visualizeMainSnakChange( DiffOpChange $mainSnakChange ) { $valueFormatter = new DiffOpValueFormatter( // todo: should show specific headers for both columns - $this->getSnakHeader( $mainSnakChange->getNewValue() ), + $this->getSnakHeaderFromDiffOp( $mainSnakChange ), $this->formatSnak( $mainSnakChange->getOldValue() ), $this->formatSnak( $mainSnakChange->getNewValue() ) ); @@ -182,54 +166,14 @@ } /** - * Format a snak - * - * @since 0.4 - * - * @param Snak|null $oldSnak - * @param Snak|null $newSnak - * @param string|null $prependHeader - * - * @throws \MWException - * @return string - */ - public function getSnakHtml( $oldSnak, $newSnak, $prependHeader = null ) { - $snakHeader = ''; - // @todo fix ugly cruft! - if ( $prependHeader !== null ) { - $snakHeader = $prependHeader; - } - - if ( $newSnak instanceof Snak || $oldSnak instanceof Snak ) { - $headerSnak = $newSnak instanceof Snak ? $newSnak : $oldSnak; - $snakHeader .= $this->getSnakHeader( $headerSnak ); - } else { - // something went wrong - throw new \MWException( 'Snak parameters not provided.' ); - } - - $oldValue = null; - $newValue = null; - - if ( $oldSnak instanceof Snak ) { - $oldValue = $this->formatSnak( $oldSnak ); - } - - if ( $newSnak instanceof Snak ) { - $newValue = $this->formatSnak( $newSnak ); - } - - $valueFormatter = new DiffOpValueFormatter( $snakHeader, $oldValue, $newValue ); - - return $valueFormatter->generateHtml(); - } - - /** - * @param Snak $snak + * @param Snak|null $snak * * @return string */ - protected function formatSnak( Snak $snak ) { + protected function formatSnak( $snak ) { + if( $snak === null ){ + return null; + } try { return $this->snakFormatter->formatSnak( $snak ); } catch ( FormattingException $ex ) { @@ -263,6 +207,7 @@ $values = array(); foreach ( $snakList as $snak ) { + /** @var $snak Snak */ // TODO: change hardcoded ": " so something like wfMessage( 'colon-separator' ), // but this will require further refactoring as it would add HTML which gets escaped $values[] = @@ -272,6 +217,14 @@ } return $values; + } + + protected function getSnakHeaderFromDiffOp( DiffOpChange $snakChange ){ + if( $snakChange->getNewValue() === null ){ + return $this->getSnakHeader( $snakChange->getOldValue() ); + } else { + return $this->getSnakHeader( $snakChange->getNewValue() ); + } } /** @@ -296,7 +249,7 @@ * * @since 0.4 * - * @param Diff[] $changes + * @param Diff $changes * @param Claim $claim * @param string $breadCrumb * diff --git a/lib/tests/phpunit/claim/ClaimDifferTest.php b/lib/tests/phpunit/claim/ClaimDifferTest.php index 2bb71dd..ba42032 100644 --- a/lib/tests/phpunit/claim/ClaimDifferTest.php +++ b/lib/tests/phpunit/claim/ClaimDifferTest.php @@ -122,7 +122,7 @@ $this->assertInstanceOf( 'Wikibase\ClaimDifference', $actual ); if ( !$expected->equals( $actual ) ) { - q($expected, $actual); + $this->assertEquals($expected, $actual); } $this->assertTrue( diff --git a/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php b/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php index ef7aabb..73b2e2a 100644 --- a/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php +++ b/lib/tests/phpunit/claim/ClaimDifferenceVisualizerTest.php @@ -7,79 +7,205 @@ use Diff\DiffOpAdd; use Diff\DiffOpChange; use Diff\DiffOpRemove; +use Wikibase\Claim; use Wikibase\ClaimDifference; +use Wikibase\ClaimDifferenceVisualizer; +use Wikibase\DataModel\Entity\PropertyId; +use Wikibase\Lib\EntityIdLabelFormatter; +use Wikibase\Lib\SnakFormatter; use Wikibase\PropertyNoValueSnak; use Wikibase\PropertySomeValueSnak; use Wikibase\PropertyValueSnak; use Wikibase\Reference; +use Wikibase\ReferenceList; use Wikibase\SnakList; use Wikibase\Statement; /** * @covers Wikibase\ClaimDifferenceVisualizer * - * @since 0.4 - * * @group Wikibase * @group WikibaseLib * @group WikibaseClaim * * @licence GNU GPL v2+ - * @author Jeroen De Dauw < [email protected] > + * @author Adam Shorland */ class ClaimDifferenceVisualizerTest extends \MediaWikiTestCase { - public function visualizeDiffProvider() { - $differences = array(); + public function newSnakFormatter( $format = SnakFormatter::FORMAT_PLAIN ){ + $instance = $this->getMock( 'Wikibase\Lib\SnakFormatter' ); + $instance->expects( $this->atLeastOnce() ) + ->method( 'getFormat' ) + ->will( $this->returnValue( $format ) ); + $instance->expects( $this->any() ) + ->method( 'canFormatSnak' ) + ->will( $this->returnValue( true ) ); + $instance->expects( $this->any() ) + ->method( 'formatSnak' ) + ->will( $this->returnValue( 'SNAK' ) ); + return $instance; + } - $differences[] = new ClaimDifference(); + public function newEntityIdLabelFormatter(){ + $instance = $this + ->getMockBuilder( 'Wikibase\Lib\EntityIdLabelFormatter' ) + ->disableOriginalConstructor() + ->getMock(); - $differences[] = new ClaimDifference( - new DiffOpChange( - new PropertyNoValueSnak( 42 ), - new PropertyNoValueSnak( 43 ) + $instance->expects( $this->any() ) + ->method( 'format' ) + ->will( $this->returnValue( 'PID' ) ); + + return $instance; + } + + public function newClaimDifferenceVisualizer(){ + return new ClaimDifferenceVisualizer( + $this->newEntityIdLabelFormatter(), + $this->newSnakFormatter() + ); + } + + public function testConstruction(){ + $instance = $this->newClaimDifferenceVisualizer(); + $this->assertInstanceOf( 'Wikibase\ClaimDifferenceVisualizer', $instance ); + } + + public function testConstructionWithBadFormatter(){ + $this->setExpectedException( 'InvalidArgumentException' ); + new ClaimDifferenceVisualizer( + $this->newEntityIdLabelFormatter(), + $this->newSnakFormatter( 'qwertyuiop' ) + ); + } + + //TODO come up with a better way of testing this.... EWW at all the html... + public function provideDifferenceAndClaim(){ + return array( + //0 no change + array( + new ClaimDifference(), + new Claim( new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'foo' ) ) ), + '' ), - new Diff( array( - new DiffOpAdd( new PropertySomeValueSnak( 44 ) ), - new DiffOpRemove( new PropertyValueSnak( 45, new StringValue( 'foo' ) ) ), - new DiffOpChange( new PropertySomeValueSnak( 46 ), new PropertySomeValueSnak( 47 ) ), - ) ) - ); - - $differences[] = new ClaimDifference( - new DiffOpChange( - new PropertyNoValueSnak( 42 ), - new PropertyNoValueSnak( 43 ) + //1 mainsnak + array( + new ClaimDifference( + new DiffOpChange( + new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'bar' ) ), + new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'foo' ) ) + ) + ), + new Claim( new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'foo' ) ) ), + '<tr><td colspan="2" class="diff-lineno">property / PID</td><td colspan="2" class="diff-lineno">property / PID</td></tr>'. + '<tr><td class="diff-marker">-</td><td class="diff-deletedline">'. + '<div><del class="diffchange diffchange-inline"><span>SNAK</span></del></div></td>'. + '<td class="diff-marker">+</td><td class="diff-addedline">'. + '<div><ins class="diffchange diffchange-inline"><span>SNAK</span></ins></div></td></tr></tr>' ), - null, - new Diff( array( - new DiffOpAdd( new Reference() ), - new DiffOpRemove( new Reference( new SnakList( array( new PropertyNoValueSnak( 50 ) ) ) ) ), - ) ) + //2 +qualifiers + array( + new ClaimDifference( + null, + new Diff( array( + new DiffOpAdd( new PropertySomeValueSnak( 44 ) ), + ) ) + ), + new Claim( new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'foo' ) ) ), + '<tr><td colspan="2" class="diff-lineno"></td><td colspan="2" class="diff-lineno">property / PID / qualifier</td></tr>'. + '<tr><td colspan="2"> </td><td class="diff-marker">+</td><td class="diff-addedline">'. + '<div><ins class="diffchange diffchange-inline"><span>PID: SNAK</span></ins></div></td></tr>' + ), + //3 +references + array( + new ClaimDifference( + null, + null, + new Diff( array( + new DiffOpRemove( new Reference( new SnakList( array( new PropertyNoValueSnak( 50 ) ) ) ) ), + ) ) + ), + new Claim( new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'foo' ) ) ), + '<tr><td colspan="2" class="diff-lineno">property / PID / reference</td><td colspan="2" class="diff-lineno"></td></tr>'. + '<tr><td class="diff-marker">-</td><td class="diff-deletedline">'. + '<div><del class="diffchange diffchange-inline"><span>PID: SNAK</span></del></div></td><td colspan="2"> </td></tr>' + ), + //4 ranks + //TODO no diff is currently created for RANKS, Implement this! + array( + new ClaimDifference( + null, + null, + null, + new DiffOpChange( Statement::RANK_NORMAL, Statement::RANK_PREFERRED ) + ), + new Statement( new PropertyValueSnak( new PropertyId( 'P1' ), new StringValue( 'foo' ) ) ), + '' + ), ); - - $differences[] = new ClaimDifference( - null, - null, - null, - new DiffOpChange( Statement::RANK_DEPRECATED, Statement::RANK_PREFERRED ) - ); - - return $this->arrayWrap( $differences ); } /** - * @dataProvider visualizeDiffProvider - * - * @param ClaimDifference $claimDifference + * @dataProvider provideDifferenceAndClaim */ -// @todo provide a second parameter for the function -/* public function testVisualizeDiff( ClaimDifference $claimDifference ) { - $differenceVisualizer = new ClaimDifferenceVisualizer( new \Wikibase\CachingEntityLoader(), 'en' ); - - $visualization = $differenceVisualizer->visualizeDiff( $claimDifference ); - - $this->assertInternalType( 'string', $visualization ); + public function testVisualizeClaimChange( $difference, $baseClaim, $expectedHtml ){ + $visualizer = $this->newClaimDifferenceVisualizer(); + $html = $visualizer->visualizeClaimChange( $difference, $baseClaim ); + $this->assertEquals( $expectedHtml, $html ); } -*/ + + public function testVisualizeNewClaim(){ + $expect = '<tr><td colspan="2" class="diff-lineno"></td><td colspan="2" class="diff-lineno">property / PID</td></tr>'. + '<tr><td colspan="2"> </td><td class="diff-marker">+</td><td class="diff-addedline">'. + '<div><ins class="diffchange diffchange-inline"><span>SNAK</span></ins></div>'. + '</td></tr><tr><td colspan="2" class="diff-lineno"></td><td colspan="2" class="diff-lineno">property / PID / qualifier</td></tr>'. + '<tr><td colspan="2"> </td><td class="diff-marker">+</td><td class="diff-addedline">'. + '<div><ins class="diffchange diffchange-inline"><span>PID: SNAK</span></ins></div>'. + '</td></tr><tr><td colspan="2" class="diff-lineno"></td><td colspan="2" class="diff-lineno">property / PID / reference</td></tr>'. + '<tr><td colspan="2"> </td><td class="diff-marker">+</td><td class="diff-addedline">'. + '<div><ins class="diffchange diffchange-inline"><span>PID: SNAK</span></ins></div></td></tr>'; + + $visualizer = $this->newClaimDifferenceVisualizer(); + $claim = new Statement( + new PropertyValueSnak( new PropertyId( 'P12' ), new StringValue( 'foo' ) ), + new SnakList( array( new PropertyNoValueSnak( 50 ) ) ), + new ReferenceList( array( + new Reference( + new SnakList( array( + new PropertyValueSnak( new PropertyId( 'P44' ), new StringValue( 'referencevalue' ) ) + ) ) ) ) ) ); + $html = $visualizer->visualizeNewClaim( $claim ); + + $this->assertEquals( $expect, $html ); + } + + public function testVisualizeRemovedClaim(){ + $expect = '<tr><td colspan="2" class="diff-lineno">property / PID</td><td colspan="2" class="diff-lineno"></td></tr>'. + '<tr><td class="diff-marker">-</td><td class="diff-deletedline">'. + '<div><del class="diffchange diffchange-inline"><span>SNAK</span></del></div>'. + '</td><td colspan="2"> </td></tr><tr><td colspan="2" class="diff-lineno">property / PID / qualifier</td>'. + '<td colspan="2" class="diff-lineno"></td></tr><tr><td class="diff-marker">-</td><td class="diff-deletedline">'. + '<div><del class="diffchange diffchange-inline"><span>PID: SNAK</span></del></div>'. + '</td><td colspan="2"> </td></tr><tr><td colspan="2" class="diff-lineno">property / PID / reference</td>'. + '<td colspan="2" class="diff-lineno"></td></tr><tr><td class="diff-marker">-</td><td class="diff-deletedline">'. + '<div><del class="diffchange diffchange-inline"><span>PID: SNAK</span></del></div></td><td colspan="2"> </td></tr>'; + + $visualizer = $this->newClaimDifferenceVisualizer(); + $claim = new Statement( + new PropertyValueSnak( new PropertyId( 'P12' ), new StringValue( 'foo' ) ), + new SnakList( array( new PropertyNoValueSnak( 50 ) ) ), + new ReferenceList( array( + new Reference( + new SnakList( array( + new PropertyValueSnak( new PropertyId( 'P44' ), new StringValue( 'referencevalue' ) ) + ) ) ) ) ) ); + $html = $visualizer->visualizeRemovedClaim( $claim ); + + $this->assertEquals( $expect, $html ); + } + + + + } -- To view, visit https://gerrit.wikimedia.org/r/90876 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: Ib8709a7b5a1cc3e2ba32bc9057154ced1ff2d334 Gerrit-PatchSet: 6 Gerrit-Project: mediawiki/extensions/Wikibase Gerrit-Branch: master Gerrit-Owner: Addshore <[email protected]> Gerrit-Reviewer: Addshore <[email protected]> Gerrit-Reviewer: Aude <[email protected]> Gerrit-Reviewer: Daniel Kinzler <[email protected]> Gerrit-Reviewer: Hashar <[email protected]> Gerrit-Reviewer: Jeroen De Dauw <[email protected]> Gerrit-Reviewer: Tobias Gritschacher <[email protected]> Gerrit-Reviewer: Umherirrender <[email protected]> Gerrit-Reviewer: jenkins-bot _______________________________________________ MediaWiki-commits mailing list [email protected] https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
