AndyRussG has uploaded a new change for review.
https://gerrit.wikimedia.org/r/277792
Change subject: [WIP] Admin UI: Optimize handling of changes to campaigns via
list
......................................................................
[WIP] Admin UI: Optimize handling of changes to campaigns via list
Instead of submitting the list of campaigns as one long form, use JS
detect which controls the user has changed, and only modify those
rows.
Bug: T128869
Change-Id: Ib3dd8611c4c118982ed9c4ba2c2ed85026afd0bd
---
M includes/CNCampaignPager.php
M resources/infrastructure/ext.centralNotice.adminUi.campaignPager.js
M special/SpecialCentralNotice.php
3 files changed, 287 insertions(+), 126 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/CentralNotice
refs/changes/92/277792/1
diff --git a/includes/CNCampaignPager.php b/includes/CNCampaignPager.php
index 1f12dd4..5c1f9cf 100644
--- a/includes/CNCampaignPager.php
+++ b/includes/CNCampaignPager.php
@@ -145,13 +145,14 @@
$htmlOut = '';
- $htmlOut .= Xml::openElement( 'fieldset', array( 'class' =>
'prefsection' ) );
-
- // If this is editable, make it a form
- if ( $this->editable ) {
- $htmlOut .= Xml::openElement( 'form', array( 'method'
=> 'post' ) );
- $htmlOut .= Html::hidden( 'authtoken',
$this->getUser()->getEditToken() );
- }
+ $htmlOut .= Xml::openElement(
+ 'fieldset',
+ array(
+ 'class' => 'prefsection',
+ 'id' => 'cn-campaign-pager',
+ 'data-editable' => $this->editable ? 1 : 0
+ )
+ );
// Filters
$htmlOut .= Xml::openElement( 'div', array( 'class' =>
'cn-formsection-emphasis' ) );
@@ -209,15 +210,18 @@
case 'not_end':
return date( '<\b>Y-m-d</\b> H:i', wfTimestamp(
TS_UNIX, $value ) );
+ // Note: Names of controls and data attributes must
coordinate with
+ // ext.centralNotice.adminUi.campaignPager.js
+
case 'not_enabled':
return Xml::check(
- 'enabled[]',
+ 'enabled',
$rowIsEnabled,
array_replace(
( !$this->editable ||
$rowIsLocked || $rowIsArchived )
? $readonly : array(),
array(
- 'value' => $name,
+ 'data-campaign-name' =>
$name,
'class' =>
'noshiftselect mw-cn-input-check-sort'
)
)
@@ -239,13 +243,17 @@
case 'not_locked':
return Xml::check(
- 'locked[]',
+ 'locked',
$rowIsLocked,
array_replace(
- ( !$this->editable ||
$rowIsArchived )
+ // Note: Lockability should
always be modifiable
+ // regardless of whether the
camapgin is archived.
+ // Otherwise we create a
dead-end state of locked and
+ // archived.
+ ( !$this->editable )
? $readonly : array(),
array(
- 'value' => $name,
+ 'data-campaign-name' =>
$name,
'class' =>
'noshiftselect mw-cn-input-check-sort'
)
)
@@ -253,13 +261,13 @@
case 'not_archived':
return Xml::check(
- 'archiveCampaigns[]',
+ 'archived',
$rowIsArchived,
array_replace(
( !$this->editable ||
$rowIsLocked || $rowIsEnabled )
? $readonly : array(),
array(
- 'value' => $name,
+ 'data-campaign-name' =>
$name,
'class' =>
'noshiftselect mw-cn-input-check-sort'
)
)
@@ -337,16 +345,17 @@
$htmlOut .= $this->onSpecialCN->makeSummaryField();
- $htmlOut .=
- Xml::submitButton( $this->msg(
'centralnotice-modify' )->text(),
+ $htmlOut .= Xml::input(
+ 'centralnoticesubmit',
+ false,
+ $this->msg( 'centralnotice-modify' )->text(),
array(
- 'id' => 'centralnoticesubmit',
- 'name' => 'centralnoticesubmit'
+ 'type' => 'button',
+ 'id' => 'cn-campaign-pager-submit'
)
);
$htmlOut .= Xml::closeElement( 'div' );
- $htmlOut .= Xml::closeElement( 'form' );
}
$htmlOut .= Xml::closeElement( 'fieldset' );
@@ -402,9 +411,8 @@
parent::extractResultInfo( $isFirst, $limit, $res );
- // Due to the way CentralNotice processes changes, editing and
- // paging don't work together, causing data corruption. So we
disable
- // editing if there's more than one page of results.
+ // Disable editing if there's more than one page. (This is a
legacy
+ // requirement; it might work even with paging now.)
if ( !$this->isWithinLimit() ) {
$this->editable = false;
}
diff --git
a/resources/infrastructure/ext.centralNotice.adminUi.campaignPager.js
b/resources/infrastructure/ext.centralNotice.adminUi.campaignPager.js
index 619d70d..fd8bb60 100644
--- a/resources/infrastructure/ext.centralNotice.adminUi.campaignPager.js
+++ b/resources/infrastructure/ext.centralNotice.adminUi.campaignPager.js
@@ -3,17 +3,72 @@
*/
( function( mw, $ ) {
+ var changes = {};
+
+ /**
+ * Helper function that provides the appropriate changes object for a
+ * specific campaign, from jQuery control element
+ * @param {jQuery} jQuery object for the control that has changed
+ * @returns {Object} The object for setting changes on
+ */
+ function getChangesEntry( $control ) {
+ var campaignName = $control.data( 'campaignName' );
+
+ if ( changes[campaignName] === undefined ) {
+ changes[campaignName] = {};
+ }
+ return changes[campaignName];
+ }
+
+ /**
+ * Handler for changes to checkboxes
+ * @param {Object} event As provided by jQuery
+ */
+ function updateCheckboxChanges( event ) {
+ var $this = $( this );
+ getChangesEntry( $this )[event.data.checkboxName] = $this.prop(
'checked' );
+ }
+
+ /**
+ * Handler for changes to priority dropdowns
+ */
+ function updatePriorityChanges() {
+ var $this = $( this );
+ getChangesEntry( $this )['priority'] = $this.val();
+ }
+
+ /**
+ * Click handler for faux submit button, to submit just what's changed
+ */
+ function submitChanges() {
+ var $form = $( '<form method="post"></form>'),
+
+ $authtokenField =
+ $( '<input type="hidden" name="authtoken"
value="' +
+ mw.user.tokens.get( 'editToken' ) +
+ '"></input>' ),
+
+ $summaryField =
+ $( '<input type="hidden" name="changeSummary"
value="' +
+ $( '#cn-campaign-pager
input.cn-change-summary-input' ).val() +
+ '"></input>' ),
+
+ $changesField =
+ $( '<input type="hidden" name="changes"
></input>' );
+
+ $changesField.val( JSON.stringify( changes ) );
+ $form.append( $authtokenField, $summaryField, $changesField );
+ $( document.body ).append( $form );
+ $form.submit();
+ }
+
jQuery(document).ready( function ( $ ) {
- // Keep data-sort-value attributes for jquery.tablesorter in
sync
- $( '.mw-cn-input-check-sort' ).on( 'change click blur',
function () {
- $(this).parent( 'td' )
- .data( 'sortValue', Number( this.checked ) );
- } );
-
- // Show or hide archived campaigns
- var $showArchived = $( '#centralnotice-showarchived' );
+ var CHECKBOX_NAMES = [ 'enabled', 'locked', 'archived' ],
+ i, checkboxName, selector,
+ $showArchived = $( '#centralnotice-showarchived' );
+ // Show or hide archived campaigns
if ( $showArchived.length > 0 ) {
$showArchived.click( function () {
@@ -24,5 +79,38 @@
}
} );
}
+
+ // If the table is editable, attach handlers to controls
+ if ( $( '#cn-campaign-pager' ).data( 'editable' ) ) {
+
+ // Go through all the fields with checkbox controls
+ for ( i = 0; i < CHECKBOX_NAMES.length; i++ ) {
+ checkboxName = CHECKBOX_NAMES[i];
+
+ // Select enabled checkboxes with this name
+ // See CNCampaignPager::formatValue()
+ selector = '#cn-campaign-pager input[name="' +
checkboxName +
+ '"]:not([disabled])';
+
+ // When checked or unchecked, update changes to
send
+ $( selector ).change(
+ { checkboxName: checkboxName },
updateCheckboxChanges );
+ }
+
+ // Attach handler to priority dropdowns
+ // See CentralNotice::prioritySelector()
+ $( '#cn-campaign-pager
select[name="priority"]:not([disabled])' )
+ .change( updatePriorityChanges );
+
+ // Attach handler to "Submit" button
+ // See CNCampaignPager::getEndBody()
+ $( '#cn-campaign-pager-submit' ).click( submitChanges );
+ }
+
+ // Keep data-sort-value attributes for jquery.tablesorter in
sync
+ $( '.mw-cn-input-check-sort' ).on( 'change click blur',
function () {
+ $(this).parent( 'td' )
+ .data( 'sortValue', Number( this.checked ) );
+ } );
} );
} )( mediaWiki, jQuery );
\ No newline at end of file
diff --git a/special/SpecialCentralNotice.php b/special/SpecialCentralNotice.php
index d5cada0..85f430c 100644
--- a/special/SpecialCentralNotice.php
+++ b/special/SpecialCentralNotice.php
@@ -123,123 +123,184 @@
}
protected function handleNoticePostFromList() {
+
$request = $this->getRequest();
+ $changes = json_decode( $request->getText( 'changes' ), true );
+ $summary = $this->getSummaryFromRequest( $request );
- // Get all the initial campaign settings for logging
- $allCampaignNames = Campaign::getAllCampaignNames();
- $allInitialCampaignSettings = array();
- foreach ( $allCampaignNames as $campaignName ) {
- $settings = Campaign::getCampaignSettings(
$campaignName );
- $allInitialCampaignSettings[ $campaignName ] =
$settings;
- }
+ // Make the changes requested
+ foreach ( $changes as $campaignName => $campaignChanges ) {
- // FIXME The following three blocks of code are similar.
- // They might be refactored as part of a bigger refactoring
- // of code for changing campaign settings via the list of
- // campaigns. If not, refactor this following the current
- // logic.
+ $initialSettings = Campaign::getCampaignSettings(
$campaignName );
- // Handle archiving/unarchiving campaigns
- $archived = $request->getArray( 'archiveCampaigns' );
- if ( $archived ) {
- // Build list of campaigns to archive
- $notArchived = array_diff(
Campaign::getAllCampaignNames(), $archived );
+ // Next campaign if somehow this one doesn't exist
+ if ( !$initialSettings ) {
+ continue;
+ }
- // Set archived/not archived flag accordingly
- foreach ( $archived as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'archived', 1 );
- }
- foreach ( $notArchived as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'archived', 0 );
- }
- // Handle updates if no post content came through (all
checkboxes unchecked)
- } else {
- $allNotices = Campaign::getAllCampaignNames();
- foreach ( $allNotices as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'archived', 0 );
- }
- }
+ // Set values as per $changes
+ if ( isset( $campaignChanges['archived'] ) ) {
- // Handle locking/unlocking campaigns
- $lockedNotices = $request->getArray( 'locked' );
- if ( $lockedNotices ) {
- // Build list of campaigns to lock
- $unlockedNotices = array_diff(
Campaign::getAllCampaignNames(), $lockedNotices );
+ Campaign::setBooleanCampaignSetting(
$campaignName, 'archived',
+ $campaignChanges['archived'] ? 1 : 0 );
+ }
- // Set locked/unlocked flag accordingly
- foreach ( $lockedNotices as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'locked', 1 );
- }
- foreach ( $unlockedNotices as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'locked', 0 );
- }
- // Handle updates if no post content came through (all
checkboxes unchecked)
- } else {
- $allNotices = Campaign::getAllCampaignNames();
- foreach ( $allNotices as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'locked', 0 );
- }
- }
+ if ( isset( $campaignChanges['locked'] ) ) {
- // Handle enabling/disabling campaigns
- $enabledNotices = $request->getArray( 'enabled' );
- if ( $enabledNotices ) {
- // Build list of campaigns to disable
- $disabledNotices = array_diff(
Campaign::getAllCampaignNames(), $enabledNotices );
+ Campaign::setBooleanCampaignSetting(
$campaignName, 'locked',
+ $campaignChanges['locked'] ? 1
: 0 );
+ }
- // Set enabled/disabled flag accordingly
- foreach ( $enabledNotices as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'enabled', 1 );
- }
- foreach ( $disabledNotices as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'enabled', 0 );
- }
- // Handle updates if no post content came through (all
checkboxes unchecked)
- } else {
- $allNotices = Campaign::getAllCampaignNames();
- foreach ( $allNotices as $notice ) {
- Campaign::setBooleanCampaignSetting( $notice,
'enabled', 0 );
- }
- }
+ if ( isset( $campaignChanges['enabled'] ) ) {
- // Handle setting priority on campaigns
- $preferredNotices = $request->getArray( 'priority' );
- if ( $preferredNotices ) {
- foreach ( $preferredNotices as $notice => $value ) {
+ Campaign::setBooleanCampaignSetting(
$campaignName, 'enabled',
+ $campaignChanges['enabled'] ? 1
: 0 );
+ }
+
+ if ( isset( $campaignChanges['priority'] ) ) {
+
Campaign::setNumericCampaignSetting(
- $notice,
+ $campaignName,
'preferred',
- $value,
+ intval( $campaignChanges['priority'] ),
CentralNotice::EMERGENCY_PRIORITY,
CentralNotice::LOW_PRIORITY
);
}
- }
- // Get all the final campaign settings for potential logging
- foreach ( $allCampaignNames as $campaignName ) {
- $finalCampaignSettings = Campaign::getCampaignSettings(
$campaignName );
- if ( !$allInitialCampaignSettings[ $campaignName ] ||
!$finalCampaignSettings ) {
- // Condition where the campaign has apparently
disappeared mid operations
- // -- possibly a delete call
- $diffs = false;
- } else {
- $diffs = array_diff_assoc(
$allInitialCampaignSettings[ $campaignName ], $finalCampaignSettings );
- }
- // If there are changes, log them
+ // Log any differences in settings
+ $newSettings = Campaign::getCampaignSettings(
$campaignName );
+ $diffs = array_diff_assoc( $initialSettings,
$newSettings );
+
if ( $diffs ) {
$campaignId = Campaign::getNoticeId(
$campaignName );
Campaign::logCampaignChange(
'modified',
$campaignId,
$this->getUser(),
- $allInitialCampaignSettings[
$campaignName ],
- $finalCampaignSettings,
+ $initialSettings,
+ $newSettings,
array(), array(),
- $this->getSummaryFromRequest( $request )
+ $summary
);
}
}
+
+// // Get all the initial campaign settings for logging
+// $allCampaignNames = Campaign::getAllCampaignNames();
+// $allInitialCampaignSettings = array();
+// foreach ( $allCampaignNames as $campaignName ) {
+// $settings = Campaign::getCampaignSettings(
$campaignName );
+// $allInitialCampaignSettings[ $campaignName ] =
$settings;
+// }
+
+// // FIXME The following three blocks of code are similar.
+// // They might be refactored as part of a bigger refactoring
+// // of code for changing campaign settings via the list of
+// // campaigns. If not, refactor this following the current
+// // logic.
+
+// // Handle archiving/unarchiving campaigns
+// $archived = $request->getArray( 'archiveCampaigns' );
+// if ( $archived ) {
+// // Build list of campaigns to archive
+// $notArchived = array_diff(
Campaign::getAllCampaignNames(), $archived );
+
+// // Set archived/not archived flag accordingly
+// foreach ( $archived as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'archived', 1 );
+// }
+// foreach ( $notArchived as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'archived', 0 );
+// }
+// // Handle updates if no post content came through (all
checkboxes unchecked)
+// } else {
+// $allNotices = Campaign::getAllCampaignNames();
+// foreach ( $allNotices as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'archived', 0 );
+// }
+// }
+
+// // Handle locking/unlocking campaigns
+// $lockedNotices = $request->getArray( 'locked' );
+// if ( $lockedNotices ) {
+// // Build list of campaigns to lock
+// $unlockedNotices = array_diff(
Campaign::getAllCampaignNames(), $lockedNotices );
+
+// // Set locked/unlocked flag accordingly
+// foreach ( $lockedNotices as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'locked', 1 );
+// }
+// foreach ( $unlockedNotices as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'locked', 0 );
+// }
+// // Handle updates if no post content came through (all
checkboxes unchecked)
+// } else {
+// $allNotices = Campaign::getAllCampaignNames();
+// foreach ( $allNotices as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'locked', 0 );
+// }
+// }
+
+// // Handle enabling/disabling campaigns
+// $enabledNotices = $request->getArray( 'enabled' );
+// if ( $enabledNotices ) {
+// // Build list of campaigns to disable
+// $disabledNotices = array_diff(
Campaign::getAllCampaignNames(), $enabledNotices );
+
+// // Set enabled/disabled flag accordingly
+// foreach ( $enabledNotices as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'enabled', 1 );
+// }
+// foreach ( $disabledNotices as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'enabled', 0 );
+// }
+// // Handle updates if no post content came through (all
checkboxes unchecked)
+// } else {
+// $allNotices = Campaign::getAllCampaignNames();
+// foreach ( $allNotices as $notice ) {
+// Campaign::setBooleanCampaignSetting( $notice,
'enabled', 0 );
+// }
+// }
+
+// // Handle setting priority on campaigns
+// $preferredNotices = $request->getArray( 'priority' );
+// if ( $preferredNotices ) {
+// foreach ( $preferredNotices as $notice => $value ) {
+// Campaign::setNumericCampaignSetting(
+// $notice,
+// 'preferred',
+// $value,
+// CentralNotice::EMERGENCY_PRIORITY,
+// CentralNotice::LOW_PRIORITY
+// );
+// }
+// }
+
+// // Get all the final campaign settings for potential logging
+// foreach ( $allCampaignNames as $campaignName ) {
+// $finalCampaignSettings = Campaign::getCampaignSettings(
$campaignName );
+// if ( !$allInitialCampaignSettings[ $campaignName ] ||
!$finalCampaignSettings ) {
+// // Condition where the campaign has apparently
disappeared mid operations
+// // -- possibly a delete call
+// $diffs = false;
+// } else {
+// $diffs = array_diff_assoc(
$allInitialCampaignSettings[ $campaignName ], $finalCampaignSettings );
+// }
+// // If there are changes, log them
+// if ( $diffs ) {
+// $campaignId = Campaign::getNoticeId(
$campaignName );
+// Campaign::logCampaignChange(
+// 'modified',
+// $campaignId,
+// $this->getUser(),
+// $allInitialCampaignSettings[
$campaignName ],
+// $finalCampaignSettings,
+// array(), array(),
+// $this->getSummaryFromRequest( $request )
+// );
+// }
+// }
}
/**
@@ -325,10 +386,6 @@
);
if ( $editable ) {
- $itemName = 'priority';
- if ( $index ) {
- $itemName .= "[$index]";
- }
$options = ''; // The HTML for the select list options
foreach ( $priorities as $key => $label ) {
@@ -336,10 +393,18 @@
}
$selectAttribs = array(
- 'id' => $itemName,
- 'name' => $itemName,
+ 'name' => 'priority',
+ // FIXME From legacy; shouldn't have more than
one element with
+ // the same id. Is this used?
+ 'id' => 'priority',
);
+ if ( $index ) {
+ // 'data-campaign-name' must coordinate with
CNCampaignPager
+ // and
ext.centralNotice.adminUi.campaignPager.js
+ $selectAttribs['data-campaign-name'] = $index;
+ }
+
return Xml::openElement( 'select', $selectAttribs )
. "\n"
. $options
--
To view, visit https://gerrit.wikimedia.org/r/277792
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ib3dd8611c4c118982ed9c4ba2c2ed85026afd0bd
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/CentralNotice
Gerrit-Branch: master
Gerrit-Owner: AndyRussG <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
