wikimedia/mediawiki-extensions-AbuseFilter
1
0
Fork 0
mirror of https://gerrit.wikimedia.org/r/mediawiki/extensions/AbuseFilter.git synced 2024-09-23 10:18:22 +00:00
Code Issues Packages Projects Releases Wiki Activity

Simplify ViewEdit, last round

Browse source 
This deals with data inconsistencies in buildFilterEditor. Every
property of $row was tested in all 5 scenarios (also using Selenium) to
check when it's set. The result is in the normalizeRow method, which
aims to remove any inconsistencies, so that buildFilterEditor always
receives a "complete" row with all defaults set.

The code in buildFilterEditor is now cleaner (because there are no
isset() checks), and it gives us a unique place where we can set
defaults (rather than partly doing that in
loadRequest/loadFilterData/loadImport, and partly relying on isset).

This will be especially useful when introducing value objects to
represent filters, because now you just have to look at normalizeRow()
to tell which properties are allowed to be missing, and thus what "kind"
of filter object you need (see
I5f33227887c035e301313bbe24d1c1fefb75bc6a).

Additionally, reduce the properties that get passed around during
export/import, and make the selenium test try a roundtrip, rather than
relying on hardcoded data that may get outdated. A future patch will
refactor the import/export code.

Change-Id: Id52c466baaf6da18e2981f27a81ffdad3a509e78
This commit is contained in:
Daimona Eaytoy 2020-09-27 17:15:46 +02:00
parent 711f949b95
commit 916234598d
4 changed files with 106 additions and 75 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

7
includes/AbuseFilter.php
View file

@ -898,8 +898,11 @@ class AbuseFilter {
$is_new = $filter === null;
$new_id = $filter;
// Reset throttled marker, if we're re-enabling it.
$newRow['af_throttled'] = $newRow['af_throttled'] && !$newRow['af_enabled'];
// Preserve the old throttled status (if any) only if disabling the filter.
// TODO: It might make more sense to check what was actually changed
$newRow['af_throttled'] = ( $newRow['af_throttled'] ?? false ) && !$newRow['af_enabled'];
// This is null when creating a new filter, but the DB field is NOT NULL
$newRow['af_hit_count'] = $newRow['af_hit_count'] ?? 0;
$newRow['af_id'] = $new_id;
// T67807: integer 1's & 0's might be better understood than booleans

120
includes/Views/AbuseFilterViewEdit.php
View file

@ -5,7 +5,10 @@ use MediaWiki\Linker\LinkRenderer;
use MediaWiki\MediaWikiServices;
class AbuseFilterViewEdit extends AbuseFilterView {
// Temporary hack
private const EXPORT_EXCLUDED_PROPS = [
'af_id', 'af_timestamp', 'af_user', 'af_user_text', 'af_hit_count', 'af_throttled'
];
/**
* @var int|null The history ID of the current filter
*/
@ -175,15 +178,38 @@ class AbuseFilterViewEdit extends AbuseFilterView {
}
/**
* Builds the full form for edit filters, adding it to the OutputPage. $row and $actions can be
* passed in (for instance if there was a failure during save) to avoid searching the DB.
* Normalize an abuse_filter row given to buildFilterEditor, to avoid isset-like checks later.
*
* @param stdClass $row
* @return stdClass
*/
private function normalizeRow( stdClass $row ) : stdClass {
// These fields are unset when creating a new filter
$row->af_public_comments = $row->af_public_comments ?? '';
$row->af_group = $row->af_group ?? 'default';
$row->af_comments = $row->af_comments ?? '';
// These are also unset when creating a new filter, and also when viewing an old version of a filter,
// saving the filter failed (and the filter wasn't current), or importing a filter.
$row->af_hit_count = $row->af_hit_count ?? null;
$row->af_throttled = $row->af_throttled ?? null;
return $row;
}
/**
* Builds the full form for edit filters, adding it to the OutputPage. This method can be called in 5 different
* situations, for a total of 5 different data sources for $row and $actions:
* 1 - View the result of importing a filter
* 2 - Create a new filter
* 3 - Load the current version of an existing filter
* 4 - Load an old version of an existing filter
* 5 - Show the user input again if saving fails after one of the steps above
*
* @param string|null $error An error message to show above the filter box (HTML).
* @param stdClass $row abuse_filter row representing this filter
* @param array $actions Actions enabled and their parameters
* @param int|null $filter The filter ID, or null for a new filter
* @param int|null $history_id The history ID of the filter, if applicable. Otherwise null
* @todo There are a lot of isset/empty/?? checks to account for many different scenarios, which is hard to follow
*/
protected function buildFilterEditor(
$error,
@ -197,6 +223,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
$lang = $this->getLanguage();
$user = $this->getUser();
$afPermManager = AbuseFilterServices::getPermissionManager();
$row = $this->normalizeRow( $row );
$out->addSubtitle( $this->msg(
$filter === null ? 'abusefilter-edit-subtitle-new' : 'abusefilter-edit-subtitle',
@ -204,7 +231,8 @@ class AbuseFilterViewEdit extends AbuseFilterView {
$history_id
)->parse() );
// Hide hidden filters.
// We use filterHidden() to ensure that if a public filter is made private, the public
// revision is also hidden.
if (
( $row->af_hidden || ( $filter !== null && AbuseFilter::filterHidden( $filter ) ) ) &&
!$afPermManager->canViewPrivateFilters( $user )
@ -235,7 +263,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
$fields['abusefilter-edit-description'] =
new OOUI\TextInputWidget( [
'name' => 'wpFilterDescription',
'value' => $row->af_public_comments ?? '',
'value' => $row->af_public_comments,
'readOnly' => $readOnly
]
);
@ -246,15 +274,11 @@ class AbuseFilterViewEdit extends AbuseFilterView {
new OOUI\DropdownInputWidget( [
'name' => 'wpFilterGroup',
'id' => 'mw-abusefilter-edit-group-input',
'value' => 'default',
'value' => $row->af_group,
'disabled' => $readOnly
] );
$options = [];
if ( isset( $row->af_group ) && $row->af_group ) {
$groupSelector->setValue( $row->af_group );
}
foreach ( $validGroups as $group ) {
$options += [ AbuseFilter::nameGroup( $group ) => $group ];
}
@ -266,7 +290,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
}
// Hit count display
if ( !empty( $row->af_hit_count ) && $afPermManager->canSeeLogDetails( $user ) ) {
if ( $row->af_hit_count !== null && $afPermManager->canSeeLogDetails( $user ) ) {
$count_display = $this->msg( 'abusefilter-hitcount' )
->numParams( (int)$row->af_hit_count )->text();
$hitCount = $this->linkRenderer->makeKnownLink(
@ -299,7 +323,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
$fields['abusefilter-edit-notes'] =
new OOUI\MultilineTextInputWidget( [
'name' => 'wpFilterNotes',
'value' => isset( $row->af_comments ) ? $row->af_comments . "\n" : "\n",
'value' => $row->af_comments . "\n",
'rows' => 15,
'readOnly' => $readOnly,
'id' => 'mw-abusefilter-notes-editor'
@ -313,7 +337,7 @@ class AbuseFilterViewEdit extends AbuseFilterView {
$checkboxes[] = 'global';
}
if ( isset( $row->af_throttled ) && $row->af_throttled ) {
if ( $row->af_throttled ) {
$filterActions = explode( ',', $row->af_actions );
$throttledActions = array_intersect(
$filterActions,
@ -436,7 +460,11 @@ class AbuseFilterViewEdit extends AbuseFilterView {
$this->linkRenderer->makeKnownLink( $this->getTitle( 'history/' . $filter ), $history_display );
// Add export
$exportText = FormatJson::encode( [ 'row' => $row, 'actions' => $actions ] );
$exportRow = (object)array_diff_key(
get_object_vars( $row ),
array_fill_keys( self::EXPORT_EXCLUDED_PROPS, 1 )
);
$exportText = FormatJson::encode( [ 'row' => $exportRow, 'actions' => $actions ] );
$tools .= Xml::tags( 'a', [ 'href' => '#', 'id' => 'mw-abusefilter-export-link' ],
$this->msg( 'abusefilter-edit-export' )->parse() );
$tools .=
@ -677,10 +705,8 @@ class AbuseFilterViewEdit extends AbuseFilterView {
if ( $set && isset( $parameters[0] ) ) {
$msg = $parameters[0];
} elseif (
isset( $row->af_group ) && (
( $action === 'warn' && isset( $defaultWarnMsg[$row->af_group] ) ) ||
( $action === 'disallow' && isset( $defaultDisallowMsg[$row->af_group] ) )
)
( $action === 'warn' && isset( $defaultWarnMsg[$row->af_group] ) ) ||
( $action === 'disallow' && isset( $defaultDisallowMsg[$row->af_group] ) )
) {
$msg = $action === 'warn' ? $defaultWarnMsg[$row->af_group] :
$defaultDisallowMsg[$row->af_group];
@ -1104,28 +1130,9 @@ class AbuseFilterViewEdit extends AbuseFilterView {
// a schema change adding a new field caused that extra field to be selected.
// Since the selected row may be inserted back into the database, this will cause
// an SQL error if, say, one server has the updated schema but another does not.
$loadFields = [
'af_id',
'af_pattern',
'af_user',
'af_user_text',
'af_timestamp',
'af_enabled',
'af_comments',
'af_public_comments',
'af_hidden',
'af_hit_count',
'af_throttled',
'af_deleted',
'af_actions',
'af_global',
'af_group',
];
$row = $dbr->selectRow( 'abuse_filter', AbuseFilter::ALL_ABUSE_FILTER_FIELDS, [ 'af_id' => $id ], __METHOD__ );
// Load the main row
$row = $dbr->selectRow( 'abuse_filter', $loadFields, [ 'af_id' => $id ], __METHOD__ );
if ( !isset( $row ) || !isset( $row->af_id ) || !$row->af_id ) {
if ( !$row ) {
return null;
}
@ -1176,14 +1183,13 @@ class AbuseFilterViewEdit extends AbuseFilterView {
list( $origRow, $origActions ) = $this->loadFilterData( $filter );
// Unchangeable values
$row = (object)[
'af_throttled' => $filter === null ? 0 : $origRow->af_throttled,
'af_hit_count' => $filter === null ? 0 : $origRow->af_hit_count,
];
$row = new stdClass();
if ( $filter !== null ) {
// Needed if the save fails
// Unchangeable values
$row->af_throttled = $origRow->af_throttled;
$row->af_hit_count = $origRow->af_hit_count;
// These are needed if the save fails and the filter is not new
$row->af_id = $origRow->af_id;
$row->af_user = $origRow->af_user;
$row->af_user_text = $origRow->af_user_text;
@ -1213,6 +1219,8 @@ class AbuseFilterViewEdit extends AbuseFilterView {
* @return array|null
*/
private function loadImportRequest() : ?array {
$validGroups = $this->getConfig()->get( 'AbuseFilterValidGroups' );
$globalFiltersEnabled = $this->getConfig()->get( 'AbuseFilterIsCentral' );
$request = $this->getRequest();
if ( !$request->wasPosted() ) {
// Sanity
@ -1224,17 +1232,15 @@ class AbuseFilterViewEdit extends AbuseFilterView {
return null;
}
// Default values
$row = (object)[
'af_throttled' => 0,
'af_hit_count' => 0,
'af_group' => 'default',
'af_global' => 0
];
$importRow = $importData->row;
$actions = wfObjectToArray( $importData->actions );
$row = new stdClass();
// Keep the group only if it exists on this wiki
$row->af_group = in_array( $importRow->af_group, $validGroups, true ) ? $importRow->af_group : 'default';
// And also make it global only if global filters are enabled here
$row->af_global = intval( $importRow->af_global && $globalFiltersEnabled );
$row->af_public_comments = $importRow->af_public_comments;
$row->af_pattern = $importRow->af_pattern;
$row->af_comments = $importRow->af_comments;
@ -1385,7 +1391,11 @@ class AbuseFilterViewEdit extends AbuseFilterView {
}
}
if ( !AbuseFilter::isFullAbuseFilterRow( $arr['row'] ) ) {
$completeRow = (object)array_merge(
get_object_vars( $arr['row'] ),
array_fill_keys( self::EXPORT_EXCLUDED_PROPS, 1 )
);
if ( !AbuseFilter::isFullAbuseFilterRow( $completeRow ) ) {
return false;
}

2
tests/selenium/pageobjects/viewedit.page.js
View file

@ -16,6 +16,8 @@ class ViewEditPage extends Page {
get warnCheckbox() { return $( 'input[name="wpFilterActionWarn"]' ); }
get warnOtherMessage() { return $( 'input[name="wpFilterWarnMessageOther"]' ); }
get exportData() { return $( '#mw-abusefilter-export textarea' ).getValue(); }
get submitButton() { return $( '#mw-abusefilter-editing-form input[type="submit"]' ); }
get error() { return $( '.errorbox' ); }

52
tests/selenium/specs/importingFilters.js
View file

@ -8,25 +8,41 @@ const assert = require( 'assert' ),
describe( 'When importing a filter', function () {
const filterSpecs = {
name: 'My filter name',
comments: 'Notes go here.',
rules: 'true === false',
enabled: 1,
hidden: 1,
deleted: 0
},
warnMessage = 'abusefilter-warning-foobar';
function getImportData() {
return `{"row":{"af_id":"242","af_pattern":"${filterSpecs.rules}","af_user":"1","af_user_text":\
"Daimona Eaytoy","af_timestamp":"20200924132008","af_enabled":"${filterSpecs.enabled}","af_comments":"\
${filterSpecs.comments}","af_public_comments":"${filterSpecs.name}","af_hidden":"${filterSpecs.hidden}",\
"af_hit_count":"0","af_throttled":"0","af_deleted":"${filterSpecs.deleted}","af_actions":"warn","af_global":\
"0","af_group":"default"},"actions":{"warn":["${warnMessage}"]}}`;
}
name: 'My filter name',
comments: 'Notes go here.',
rules: 'true === false',
enabled: true,
hidden: true,
deleted: false,
warnMessage: 'abusefilter-warning-foobar'
};
let importData;
before( function () {
LoginPage.loginAdmin();
ViewEditPage.open( 'new' );
ViewEditPage.switchEditor();
ViewEditPage.name.setValue( filterSpecs.name );
ViewEditPage.rules.setValue( filterSpecs.rules );
ViewEditPage.comments.setValue( filterSpecs.comments );
if ( !filterSpecs.enabled ) {
ViewEditPage.enabled.click();
}
if ( filterSpecs.hidden ) {
ViewEditPage.hidden.click();
}
if ( filterSpecs.deleted ) {
ViewEditPage.deleted.click();
}
ViewEditPage.warnCheckbox.click();
ViewEditPage.setWarningMessage( filterSpecs.warnMessage );
ViewEditPage.submit();
assert( ViewListPage.filterSavedNotice.isDisplayed() );
const filterID = ViewListPage.savedFilterID;
ViewEditPage.open( filterID );
importData = ViewEditPage.exportData;
} );
it( 'the interface should be visible', function () {
@ -45,7 +61,7 @@ ${filterSpecs.comments}","af_public_comments":"${filterSpecs.name}","af_hidden":
it( 'valid data shows the editing interface', function () {
ViewImportPage.open();
ViewImportPage.importText( getImportData() );
ViewImportPage.importText( importData );
assert( ViewEditPage.name.isDisplayed() );
} );
@ -62,7 +78,7 @@ ${filterSpecs.comments}","af_public_comments":"${filterSpecs.name}","af_hidden":
} );
it( 'filter actions are copied', function () {
assert.strictEqual( ViewEditPage.warnCheckbox.isSelected(), true );
assert.strictEqual( ViewEditPage.warnOtherMessage.getValue(), warnMessage );
assert.strictEqual( ViewEditPage.warnOtherMessage.getValue(), filterSpecs.warnMessage );
} );
it( 'the imported data can be saved', function () {