Ladsgroup has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/391900 )
Change subject: Change handling of sitelinks in EntityDiffChangedAspects
......................................................................
Change handling of sitelinks in EntityDiffChangedAspects
Per discussions in I0ac579e2063dfe92a974841330586423e5e7f393
Bug: T113468
Change-Id: Id185a573c5e0dab5b2cae6e389d5b1018cac2a04
---
M lib/includes/Changes/EntityDiffChangedAspects.php
M lib/includes/Changes/EntityDiffChangedAspectsFactory.php
M lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
M lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
4 files changed, 65 insertions(+), 17 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Wikibase
refs/changes/00/391900/1
diff --git a/lib/includes/Changes/EntityDiffChangedAspects.php
b/lib/includes/Changes/EntityDiffChangedAspects.php
index 20b8615..f84ddcf 100644
--- a/lib/includes/Changes/EntityDiffChangedAspects.php
+++ b/lib/includes/Changes/EntityDiffChangedAspects.php
@@ -44,8 +44,8 @@
private $statementChanges;
/**
- * Map of site ids to bool. The bool indicates whether only the badge
has changed (false)
- * or the actual value of the sitelink changed (true).
+ * Map of site ids to array of old value, new value and boolean value
determining if badge
+ * has changed or not
*
* @var bool[]
*/
@@ -78,7 +78,6 @@
Assert::parameterElementType( 'string', $descriptionChanges,
'$descriptionChanges' );
Assert::parameterElementType( 'string', $statementChanges,
'$statementChanges' );
Assert::parameterElementType( 'string', array_keys(
$siteLinkChanges ), 'array_keys( $siteLinkChanges )' );
- Assert::parameterElementType( 'boolean', $siteLinkChanges,
'$siteLinkChanges' );
Assert::parameterType( 'boolean', $otherChanges,
'$otherChanges' );
$this->labelChanges = $labelChanges;
diff --git a/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
b/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
index 0d7b25a..d7e6dd6 100644
--- a/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
+++ b/lib/includes/Changes/EntityDiffChangedAspectsFactory.php
@@ -83,7 +83,7 @@
$siteLinkChanges = [];
foreach ( $siteLinkDiff as $siteId => $diffPerSite ) {
- $siteLinkChanges[$siteId] = !$this->isBadgesOnlyChange(
$diffPerSite );
+ $siteLinkChanges[$siteId] =
$this->getSiteLinkChangePerSite( $diffPerSite );
}
return $siteLinkChanges;
@@ -92,10 +92,24 @@
/**
* @param DiffOp $siteLinkDiffOp
*
- * @return bool
+ * @return string|bool[]
*/
- private function isBadgesOnlyChange( DiffOp $siteLinkDiffOp ) {
- return $siteLinkDiffOp instanceof Diff && !array_key_exists(
'name', $siteLinkDiffOp );
+ private function getSiteLinkChangePerSite( DiffOp $siteLinkDiffOp ) {
+ if ( !$siteLinkDiffOp instanceof Diff ) {
+ return [ null, null, false ];
+ }
+ $oldValue = ( array_key_exists( 'name',
$siteLinkDiffOp->getRemovedValues() ) ) ?
+ $siteLinkDiffOp->getRemovedValues()['name'] : null;
+
+ $newValue = ( array_key_exists( 'name',
$siteLinkDiffOp->getAddedValues() ) ) ?
+ $siteLinkDiffOp->getAddedValues()['name'] : null;
+
+ if ( array_key_exists( 'name', $siteLinkDiffOp->getChanges() )
) {
+ $oldValue =
$siteLinkDiffOp->getChanges()['name']->getOldValue();
+ $newValue =
$siteLinkDiffOp->getChanges()['name']->getNewValue();
+ }
+
+ return [ $oldValue, $newValue, array_key_exists( 'badges',
$siteLinkDiffOp ) ];
}
/**
diff --git a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
index ca19dd3..08c9346 100644
--- a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
+++ b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsFactoryTest.php
@@ -56,17 +56,23 @@
$statementP1P2Diff['statementChanges'] = [ 'P1', 'P2' ];
$siteLinkDiff = $emptyDiff;
- $siteLinkDiff['siteLinkChanges'] = [ 'enwiki' => true ];
+ $siteLinkDiff['siteLinkChanges'] = [ 'enwiki' => [ null, 'PHP',
false ] ];
+
+ $siteLinkWithBadgeDiff = $emptyDiff;
+ $siteLinkWithBadgeDiff['siteLinkChanges'] = [ 'enwiki' => [
null, 'PHP', true ] ];
$siteLinkBadgeOnlyDiff = $emptyDiff;
- $siteLinkBadgeOnlyDiff['siteLinkChanges'] = [ 'enwiki' => false
];
+ $siteLinkBadgeOnlyDiff['siteLinkChanges'] = [ 'enwiki' => [
null, null, true ] ];
$berlinEmptyDiff = [
'arrayFormatVersion' =>
EntityDiffChangedAspects::ARRAYFORMATVERSION,
'labelChanges' => [ 'de', 'ru' ],
'descriptionChanges' => [ 'de', 'es' ],
'statementChanges' => [ 'P2' ],
- 'siteLinkChanges' => [ 'dewiki' => true, 'enwiki' =>
true ],
+ 'siteLinkChanges' => [
+ 'dewiki' => [ null, 'Berlin', true ],
+ 'enwiki' => [ null, 'Berlin', false ]
+ ],
'otherChanges' => false,
];
@@ -75,7 +81,11 @@
'labelChanges' => [ 'fr', 'ru' ],
'descriptionChanges' => [ 'de', 'es', 'pl' ],
'statementChanges' => [ 'P2' ],
- 'siteLinkChanges' => [ 'dewiki' => true, 'enwiki' =>
true, 'ruwiki' => true ],
+ 'siteLinkChanges' => [
+ 'dewiki' => [ null, 'Paris', true ],
+ 'enwiki' => [ null, 'Paris', false ],
+ 'ruwiki' => [ null, 'Paris', false ]
+ ],
'otherChanges' => false,
];
@@ -84,7 +94,11 @@
'labelChanges' => [ 'de', 'fr', 'ru' ],
'descriptionChanges' => [ 'pl' ],
'statementChanges' => [ 'P2' ],
- 'siteLinkChanges' => [ 'dewiki' => true, 'enwiki' =>
true, 'ruwiki' => true ],
+ 'siteLinkChanges' => [
+ 'dewiki' => [ 'Berlin', 'Paris', false ],
+ 'enwiki' => [ 'Berlin', 'Paris', false ],
+ 'ruwiki' => [ null, 'Paris', false ]
+ ],
'otherChanges' => false,
];
@@ -250,7 +264,7 @@
$siteLinkItem
],
'sitelink change with badge' => [
- $siteLinkDiff,
+ $siteLinkWithBadgeDiff,
$emptyItem,
$siteLinkBadgeItem
],
@@ -280,7 +294,7 @@
$reverseTests = [];
foreach ( $cases as $testDescription => $case ) {
$reverseTests[$testDescription . ' (reversed)'] = [
- $case[0],
+ $this->reverseDiff( $case[0] ),
$case[2],
$case[1]
];
@@ -289,6 +303,27 @@
return array_merge( $cases, $reverseTests );
}
+ private function reverseDiff( array $diffs ) {
+ $newDiff = [];
+ foreach ( $diffs as $diffType => $diff ) {
+ if ( $diffType === 'siteLinkChanges' ) {
+ $newDiff[$diffType] =
$this->reverseSiteLinkDiff( $diff );
+ continue;
+ }
+
+ $newDiff[$diffType] = $diff;
+ }
+
+ return $newDiff;
+ }
+
+ private function reverseSiteLinkDiff( $diff ) {
+ $newSiteLinkDiff = [];
+ foreach ( $diff as $site => $siteDiff ) {
+ $newSiteLinkDiff[$site] = [ $siteDiff[1], $siteDiff[0],
$siteDiff[2] ];
+ }
+ return $newSiteLinkDiff;
+ }
/**
* @dataProvider provideNewFromEntityDiff
*/
diff --git a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
index bd1bd23..62c233f 100644
--- a/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
+++ b/lib/tests/phpunit/Changes/EntityDiffChangedAspectsTest.php
@@ -23,7 +23,7 @@
'labelChanges' => [ 'a', '1' ],
'descriptionChanges' => [ 'b', '2' ],
'statementChanges' => [ 'c', '3' ],
- 'siteLinkChanges' => [ 'd' => true ],
+ 'siteLinkChanges' => [ 'd' => [ null, null, true ] ],
'otherChanges' => true,
];
@@ -37,7 +37,7 @@
$invalidStatementChanges['statementChanges'] = [ 'c', 3 ];
$invalidSiteLinkChangesKeys = $validParams;
- $invalidSiteLinkChangesKeys['siteLinkChanges'] = [ 1 => true ];
+ $invalidSiteLinkChangesKeys['siteLinkChanges'] = [ 1 => [ null,
null, true ] ];
$invalidSiteLinkChangesValues = $validParams;
$invalidSiteLinkChangesValues['siteLinkChanges'] = [ 'd' => 12
];
@@ -121,7 +121,7 @@
'labelChanges' => [ 'a', '1' ],
'descriptionChanges' => [ 'b', '2' ],
'statementChanges' => [ 'c', '3' ],
- 'siteLinkChanges' => [ 'd' => true ],
+ 'siteLinkChanges' => [ 'd' => [ null, null, true ] ],
'otherChanges' => true,
];
--
To view, visit https://gerrit.wikimedia.org/r/391900
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id185a573c5e0dab5b2cae6e389d5b1018cac2a04
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Wikibase
Gerrit-Branch: master
Gerrit-Owner: Ladsgroup <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
