Lucas Werkmeister (WMDE) has uploaded a new change for review. (
https://gerrit.wikimedia.org/r/366582 )
Change subject: Distinguish between non-mandatory and mandatory constraints
......................................................................
Distinguish between non-mandatory and mandatory constraints
This adds a new constraint status, “warning”, for all violations of
constraints that are not mandatory. “violation” is only used for
violations of constraints that are explicitly marked mandatory, using
the “constraint status: mandatory constraint” constraint parameter
(which ConstraintParameterParser is taught to parse).
To avoid having to change all checkers and duplicating the constraint
status parsing into them, DelegatingConstraintChecker downgrades
“violation” to “warning” if appropriate, so that the individual checkers
remain unchanged. A few tests that include DelegatingConstraintChecker
are adjusted to make their constraints mandatory so that the result is
still the expected “violation”.
On the special page, warnings are displayed in yellow; the status
“exception”, which previously used this color, is changed to a slightly
darker green than “compliance”. The gadget is slightly updated to
include warnings in the report, but the full update for the gadget
(different texts, different icon, …) will happen in a separate commit.
Bug: T164254
Change-Id: I6fa0af86dd7d3d00abc4cd004417667028599428
---
M extension.json
M i18n/en.json
M i18n/qqq.json
M includes/ConstraintCheck/DelegatingConstraintChecker.php
M includes/ConstraintCheck/Helper/ConstraintParameterParser.php
M includes/ConstraintCheck/Result/CheckResult.php
M modules/SpecialConstraintReportPage.css
M modules/gadget.js
M tests/phpunit/Api/CheckConstraintsTest.php
M tests/phpunit/DelegatingConstraintCheckerTest.php
M tests/phpunit/Helper/ConstraintParameterParserTest.php
M tests/phpunit/Specials/SpecialConstraintReportTest.php
12 files changed, 202 insertions(+), 5 deletions(-)
git pull
ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/WikibaseQualityConstraints
refs/changes/82/366582/1
diff --git a/extension.json b/extension.json
index 3318802..eca145f 100644
--- a/extension.json
+++ b/extension.json
@@ -136,6 +136,16 @@
"description": "The property ID of the 'exception to
constraint' property (data type: item), which specifies the exceptions of a
constraint.",
"public": true
},
+ "WBQualityConstraintsConstraintStatusId": {
+ "value": "P2316",
+ "description": "The property ID of the 'constraint
status' property (data type: item), which specifies the constraint status of a
constraint statement. Currently, only one constraint status is known (see
WBQualityConstraintsMandatoryConstraintId), and the default status is signified
by the absence of a 'constraint status' qualifier.",
+ "public": true
+ },
+ "WBQualityConstraintsMandatoryConstraintId": {
+ "value": "Q21502408",
+ "description": "The item ID of the 'mandatory
constraint' item, which, when used in a 'constraint status' qualifier of a
'property constraint' statement on a property, indicates that the constraint is
mandatory and should have no violations except for the known exceptions.",
+ "public": true
+ },
"WBQualityConstraintsDistinctValuesConstraintId": {
"value": "Q21502410",
"description": "The item ID of the 'distinct values
constraint' item, which, when used in a 'property constraint' statement on a
property, indicates that all values for this property should differ from each
other, or, equivalently, that each value for this property should be unique to
one item.",
diff --git a/i18n/en.json b/i18n/en.json
index 6f104b7..99a5ced 100644
--- a/i18n/en.json
+++ b/i18n/en.json
@@ -23,6 +23,7 @@
"wbqc-constraintreport-status-todo": "Todo",
"wbqc-constraintreport-status-bad-parameters": "Bad parameters",
"wbqc-constraintreport-status-deprecated": "Deprecated",
+ "wbqc-constraintreport-status-warning": "Warning",
"wbqc-constraintreport-result-table-header-status": "Status",
"wbqc-constraintreport-result-table-header-claim": "Claim",
"wbqc-constraintreport-result-table-header-constraint": "Constraint",
diff --git a/i18n/qqq.json b/i18n/qqq.json
index 071ea19..ff4ff75 100644
--- a/i18n/qqq.json
+++ b/i18n/qqq.json
@@ -25,6 +25,7 @@
"wbqc-constraintreport-status-todo": "Status for constraints which
cannot be checked yet.\n{{Identical|Todo}}",
"wbqc-constraintreport-status-bad-parameters": "Status for constraints
that have missing or invalid constraint parameters.",
"wbqc-constraintreport-status-deprecated": "Status for constraint
checks that have been skipped because the statement is deprecated.",
+ "wbqc-constraintreport-status-warning": "Status for statements that
violate a non-mandatory constraint.",
"wbqc-constraintreport-result-table-header-status": "Header of the
column that tells whether the check found a violation or something
else.\n{{Identical|Status}}",
"wbqc-constraintreport-result-table-header-claim": "Header of the
column that displays a link to the claim, the used property and its
value.\n{{Identical|Claim}}",
"wbqc-constraintreport-result-table-header-constraint": "Header of the
column that gives information about which constraint was
checked.\n{{Identical|Constraint}}",
diff --git a/includes/ConstraintCheck/DelegatingConstraintChecker.php
b/includes/ConstraintCheck/DelegatingConstraintChecker.php
index d5b0a7c..15d55cd 100644
--- a/includes/ConstraintCheck/DelegatingConstraintChecker.php
+++ b/includes/ConstraintCheck/DelegatingConstraintChecker.php
@@ -3,6 +3,7 @@
namespace WikibaseQuality\ConstraintReport\ConstraintCheck;
use InvalidArgumentException;
+use LogicException;
use MediaWiki\MediaWikiServices;
use Wikibase\DataModel\Entity\EntityDocument;
use Wikibase\DataModel\Entity\PropertyId;
@@ -328,6 +329,35 @@
( microtime( true ) - $startTime ) * 1000
);
+ try {
+ $constraintStatus =
$this->constraintParameterParser
+ ->parseConstraintStatusParameter(
$constraint->getConstraintParameters() );
+ } catch ( ConstraintParameterException $e ) {
+ $result = new CheckResult(
+ $entity->getId(),
+ $statement,
+ $constraint,
+ [],
+ CheckResult::STATUS_BAD_PARAMETERS,
+ $e->getMessage()
+ );
+ $constraintStatus = null;
+ }
+ if ( $constraintStatus === null ) {
+ // downgrade violation to warning
+ if ( $result->getStatus() ===
CheckResult::STATUS_VIOLATION ) {
+ $result->setStatus(
CheckResult::STATUS_WARNING );
+ }
+ } else {
+ if ( $constraintStatus !== 'mandatory' ) {
+ throw new LogicException(
+ "Unknown constraint status
'$constraintStatus', " .
+ "only known status is
'mandatory'"
+ );
+ }
+ $result->getParameters['constraint_status'] =
$constraintStatus;
+ }
+
return $result;
} else {
return new CheckResult( $entity->getId(), $statement,
$constraint, [], CheckResult::STATUS_TODO, null );
@@ -349,6 +379,7 @@
$order = [
'bad-parameters' => $orderNum++,
'violation' => $orderNum++,
+ 'warning' => $orderNum++,
'exception' => $orderNum++,
'compliance' => $orderNum++,
'deprecated' => $orderNum++,
diff --git a/includes/ConstraintCheck/Helper/ConstraintParameterParser.php
b/includes/ConstraintCheck/Helper/ConstraintParameterParser.php
index d1423d7..f95f82c 100644
--- a/includes/ConstraintCheck/Helper/ConstraintParameterParser.php
+++ b/includes/ConstraintCheck/Helper/ConstraintParameterParser.php
@@ -708,4 +708,55 @@
}
}
+ private function parseConstraintStatusParameterFromStatement( array
$constraintParameters ) {
+ $constraintStatusId = $this->config->get(
'WBQualityConstraintsConstraintStatusId' );
+ $mandatoryId = $this->config->get(
'WBQualityConstraintsMandatoryConstraintId' );
+ $this->requireSingleParameter( $constraintParameters,
$constraintStatusId );
+ $snak = $this->snakDeserializer->deserialize(
$constraintParameters[$constraintStatusId][0] );
+ $this->requireValueParameter( $snak, $constraintStatusId );
+ $statusId =
$snak->getDataValue()->getEntityId()->getSerialization();
+
+ if ( $statusId === $mandatoryId ) {
+ return 'mandatory';
+ } else {
+ throw new ConstraintParameterException(
+ wfMessage(
'wbqc-violation-message-parameter-oneof' )
+ ->rawParams(
$this->constraintParameterRenderer->formatPropertyId( $constraintStatusId,
Role::CONSTRAINT_PARAMETER_PROPERTY ) )
+ ->numParams( 1 )
+ ->rawParams(
$this->constraintParameterRenderer->formatItemIdList( [ $mandatoryId ],
Role::CONSTRAINT_PARAMETER_VALUE ) )
+ ->escaped()
+ );
+ }
+ }
+
+ private function parseConstraintStatusParameterFromTemplate( array
$constraintParameters ) {
+ if ( $constraintParameters['constraint_status'] === 'mandatory'
) {
+ return 'mandatory';
+ } else {
+ throw new ConstraintParameterException(
+ wfMessage(
'wbqc-violation-message-parameter-oneof' )
+ ->rawParams(
$this->constraintParameterRenderer->formatPropertyId( 'constraint_status',
Role::CONSTRAINT_PARAMETER_PROPERTY ) )
+ ->numParams( 1 )
+ ->rawParams(
$this->constraintParameterRenderer->formatItemIdList( [ 'mandatory' ],
Role::CONSTRAINT_PARAMETER_VALUE ) )
+ ->escaped()
+ );
+ }
+ }
+
+ /**
+ * @param array $constraintParameters see {@link
\WikibaseQuality\Constraint::getConstraintParameters()}
+ * @throws ConstraintParameterException if the parameter is invalid
+ * @return string|null 'mandatory' or null
+ */
+ public function parseConstraintStatusParameter( array
$constraintParameters ) {
+ $constraintStatusId = $this->config->get(
'WBQualityConstraintsConstraintStatusId' );
+ if ( array_key_exists( $constraintStatusId,
$constraintParameters ) ) {
+ return
$this->parseConstraintStatusParameterFromStatement( $constraintParameters );
+ } elseif ( array_key_exists( 'constraint_status',
$constraintParameters ) ) {
+ return
$this->parseConstraintStatusParameterFromTemplate( $constraintParameters );
+ } else {
+ return null;
+ }
+ }
+
}
diff --git a/includes/ConstraintCheck/Result/CheckResult.php
b/includes/ConstraintCheck/Result/CheckResult.php
index 355354f..d1bbbff 100644
--- a/includes/ConstraintCheck/Result/CheckResult.php
+++ b/includes/ConstraintCheck/Result/CheckResult.php
@@ -44,6 +44,13 @@
* The constraint has not been checked because the statement is
deprecated.
*/
const STATUS_DEPRECATED = 'deprecated';
+ /**
+ * The statement violates the constraint, but the constraint is not
mandatory.
+ *
+ * DelegatingConstraintChecker downgrades violations to warnings
automatically based on the constraint parameters;
+ * constraint checkers should not assign this status directly.
+ */
+ const STATUS_WARNING = 'warning';
/**
* @var EntityId
@@ -164,6 +171,13 @@
}
/**
+ * @param string $status
+ */
+ public function setStatus( $status ) {
+ $this->status = $status;
+ }
+
+ /**
* @return string (sanitized HTML)
*/
public function getMessage() {
diff --git a/modules/SpecialConstraintReportPage.css
b/modules/SpecialConstraintReportPage.css
index 7dcc63d..deb2ff2 100644
--- a/modules/SpecialConstraintReportPage.css
+++ b/modules/SpecialConstraintReportPage.css
@@ -25,7 +25,7 @@
}
.wbqc-status-exception {
- color: #ac6600; /* Yellow30 */
+ color: #14866d; /* Green30 */
}
.wbqc-status-violation {
@@ -44,6 +44,10 @@
color: #72777d; /* Base30 */
}
+.wbqc-status-warning {
+ color: #ac6600; /* Yellow30 */
+}
+
/* Tooltip */
.wbqc-indicator {
color: #72777d; /* Base30 */
diff --git a/modules/gadget.js b/modules/gadget.js
index d10b811..e3e3742 100644
--- a/modules/gadget.js
+++ b/modules/gadget.js
@@ -84,6 +84,10 @@
label: mw.message(
'wbqc-potentialissues-short' ).text()
},
{
+ status: 'warning',
+ label: mw.message(
'wbqc-potentialissues-short' ).text()
+ },
+ {
status: 'bad-parameters',
label: mw.message(
'wbqc-parameterissues-short' ).text(),
subheading: mw.message(
'wbqc-parameterissues-long' ).text(),
diff --git a/tests/phpunit/Api/CheckConstraintsTest.php
b/tests/phpunit/Api/CheckConstraintsTest.php
index d30d45a..5d1e399 100644
--- a/tests/phpunit/Api/CheckConstraintsTest.php
+++ b/tests/phpunit/Api/CheckConstraintsTest.php
@@ -102,6 +102,7 @@
$config = new HashConfig( [
'WBQualityConstraintsPropertyConstraintId' =>
'P1',
'WBQualityConstraintsExceptionToConstraintId'
=> 'P2',
+ 'WBQualityConstraintsConstraintStatusId' =>
'P3',
] );
$entityIdParser = new ItemIdParser();
$constraintParameterRenderer = new
ConstraintParameterRenderer( $entityIdFormatter, $valueFormatter );
@@ -233,7 +234,7 @@
'some guid',
$propertyId,
'violationConstraint',
- []
+ [ 'constraint_status' => 'mandatory' ]
);
}
diff --git a/tests/phpunit/DelegatingConstraintCheckerTest.php
b/tests/phpunit/DelegatingConstraintCheckerTest.php
index 0347b4e..aeb8efa 100644
--- a/tests/phpunit/DelegatingConstraintCheckerTest.php
+++ b/tests/phpunit/DelegatingConstraintCheckerTest.php
@@ -8,6 +8,8 @@
use Wikibase\DataModel\Services\Lookup\EntityRetrievingDataTypeLookup;
use Wikibase\DataModel\Services\Statement\StatementGuidParser;
use Wikibase\Rdf\RdfVocabulary;
+use Wikibase\Repo\Tests\NewItem;
+use Wikibase\Repo\Tests\NewStatement;
use
WikibaseQuality\ConstraintReport\ConstraintCheck\DelegatingConstraintChecker;
use WikibaseQuality\ConstraintReport\ConstraintReportFactory;
use WikibaseQuality\ConstraintReport\Tests\ConstraintParameters;
@@ -262,7 +264,22 @@
'pid' => 3,
'constraint_type_qid' => 'Is not
inside',
'constraint_parameters' => '{}'
- ]
+ ],
+ [
+ 'constraint_guid' =>
'P6$ad792000-6a12-413d-9fe5-11d2467b7a92',
+ 'pid' => 6,
+ 'constraint_type_qid' => 'Qualifier',
+ 'constraint_parameters' => json_encode(
+ [
+ 'constraint_status' =>
'mandatory'
+ ] )
+ ],
+ [
+ 'constraint_guid' =>
'P7$a3f746e7-66a0-46fd-96ab-6ff6638332a4',
+ 'pid' => 7,
+ 'constraint_type_qid' => 'Qualifier',
+ 'constraint_parameters' => '{}'
+ ],
]
);
}
@@ -303,6 +320,22 @@
$this->assertEquals( 'exception', $result[ 0 ]->getStatus(),
'Should be an exception' );
}
+ public function testCheckAgainstConstraintsWithMandatoryConstraint() {
+ $entity = NewItem::withId( 'Q6' )
+ ->andStatement( NewStatement::noValueFor( 'P6' ) )
+ ->build();
+ $result = $this->constraintChecker->checkAgainstConstraints(
$entity );
+ $this->assertEquals( 'violation', $result[ 0 ]->getStatus(),
'Should be a violation' );
+ }
+
+ public function testCheckAgainstConstraintsWithNonMandatoryConstraint()
{
+ $entity = NewItem::withId( 'Q7' )
+ ->andStatement( NewStatement::noValueFor( 'P7' ) )
+ ->build();
+ $result = $this->constraintChecker->checkAgainstConstraints(
$entity );
+ $this->assertEquals( 'warning', $result[ 0 ]->getStatus(),
'Should be a warning' );
+ }
+
public function testCheckAgainstConstraints_ByClaims() {
$result =
$this->constraintChecker->checkAgainstConstraintsOnClaimId(
'Q1$c0f25a6f-9e33-41c8-be34-c86a730ff30b' );
diff --git a/tests/phpunit/Helper/ConstraintParameterParserTest.php
b/tests/phpunit/Helper/ConstraintParameterParserTest.php
index 6105589..e0e77f6 100644
--- a/tests/phpunit/Helper/ConstraintParameterParserTest.php
+++ b/tests/phpunit/Helper/ConstraintParameterParserTest.php
@@ -898,4 +898,51 @@
);
}
+ public function testParseConstraintStatusParameter() {
+ $constraintStatusId = $this->getDefaultConfig()->get(
'WBQualityConstraintsConstraintStatusId' );
+ $mandatoryId = $this->getDefaultConfig()->get(
'WBQualityConstraintsMandatoryConstraintId' );
+ $snak = new PropertyValueSnak( new PropertyId(
$constraintStatusId ), new EntityIdValue( new ItemId( $mandatoryId ) ) );
+
+ $parsed =
$this->getConstraintParameterParser()->parseConstraintStatusParameter(
+ [ $constraintStatusId => [
$this->snakSerializer->serialize( $snak ) ] ]
+ );
+
+ $this->assertEquals( 'mandatory', $parsed );
+ }
+
+ public function testParseConstraintStatusParameterMissing() {
+ $parsed =
$this->getConstraintParameterParser()->parseConstraintStatusParameter(
+ []
+ );
+
+ $this->assertNull( $parsed );
+ }
+
+ public function testParseConstraintStatusParameterFromTemplate() {
+ $parsed =
$this->getConstraintParameterParser()->parseConstraintStatusParameter(
+ [ 'constraint_status' => 'mandatory' ]
+ );
+
+ $this->assertEquals( 'mandatory', $parsed );
+ }
+
+ public function testParseConstraintStatusParameterInvalid() {
+ $constraintStatusId = $this->getDefaultConfig()->get(
'WBQualityConstraintsConstraintStatusId' );
+ $snak = new PropertyValueSnak( new PropertyId(
$constraintStatusId ), new EntityIdValue( new ItemId( 'Q1' ) ) );
+
+ $this->assertThrowsConstraintParameterException(
+ 'parseConstraintStatusParameter',
+ [ [ $constraintStatusId => [
$this->snakSerializer->serialize( $snak ) ] ] ],
+ 'wbqc-violation-message-parameter-oneof'
+ );
+ }
+
+ public function testParseConstraintStatusParameterFromTemplateInvalid()
{
+ $this->assertThrowsConstraintParameterException(
+ 'parseConstraintStatusParameter',
+ [ [ 'constraint_status' => 'other' ] ],
+ 'wbqc-violation-message-parameter-oneof'
+ );
+ }
+
}
diff --git a/tests/phpunit/Specials/SpecialConstraintReportTest.php
b/tests/phpunit/Specials/SpecialConstraintReportTest.php
index 49e74f1..4b793bd 100644
--- a/tests/phpunit/Specials/SpecialConstraintReportTest.php
+++ b/tests/phpunit/Specials/SpecialConstraintReportTest.php
@@ -127,13 +127,13 @@
'constraint_guid' => '1',
'pid' => self::$idMap[ 'P1'
]->getNumericId(),
'constraint_type_qid' => 'Multi value',
- 'constraint_parameters' => '{}'
+ 'constraint_parameters' =>
'{"constraint_status":"mandatory"}'
],
[
'constraint_guid' => '3',
'pid' => self::$idMap[ 'P1'
]->getNumericId(),
'constraint_type_qid' => 'Single value',
- 'constraint_parameters' => '{}'
+ 'constraint_parameters' =>
'{"constraint_status":"mandatory"}'
]
]
);
--
To view, visit https://gerrit.wikimedia.org/r/366582
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6fa0af86dd7d3d00abc4cd004417667028599428
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/WikibaseQualityConstraints
Gerrit-Branch: master
Gerrit-Owner: Lucas Werkmeister (WMDE) <lucas.werkmeis...@wikimedia.de>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
