jenkins-bot has submitted this change and it was merged.
Change subject: Fixes of current logging, and cleanup of out of date code
......................................................................
Fixes of current logging, and cleanup of out of date code
* Remove incorrect (out of date) logging, and some support code for it
* Various client-side logging
* onEditPageAttemptSave/page-save-attempt
* Fix logging for "click to close".
* Clarify addReturnToModules name and documentation
* Update to schema ID 5944134 (only difference to schema is docs)
* Various TODOs. Some of the code can still be simplified more
now that the logging is no longer needed
Change-Id: Id90418a6058deba030cb0af7d9660167d49fa3ab
---
M GettingStarted.hooks.php
M GettingStarted.php
M resources/ext.gettingstarted.js
M resources/ext.gettingstarted.logging.js
M resources/ext.gettingstarted.openTask.js
M resources/ext.gettingstarted.return.js
M resources/ext.gettingstarted.showSeparatePage.accountCreation.js
M resources/ext.gettingstarted.specialPage.js
M resources/ext.gettingstarted.taskToolbar.js
9 files changed, 50 insertions(+), 109 deletions(-)
Approvals:
Spage: Looks good to me, approved
jenkins-bot: Verified
diff --git a/GettingStarted.hooks.php b/GettingStarted.hooks.php
index 1765651..afe4437 100644
--- a/GettingStarted.hooks.php
+++ b/GettingStarted.hooks.php
@@ -24,7 +24,7 @@
// There is used unprefixed for legacy reasons.
const COOKIE_NAME = 'openTask';
- const SCHEMA_REV_ID = 5928325;
+ const SCHEMA_REV_ID = 5944134;
// Keep following two lines in sync with ext.gettingstarted.logging.js
// These are for the primary schema. There is a secondary schema,
@@ -195,10 +195,12 @@
/**
+ * Adds the returnTo module to the page the user returned to upon
signup.
*
- * Add CTA to page the user returned to upon signup
+ * Depending on the page, this may do nothing (except log), or add a
CTA with
+ * one or two buttons.
*/
- protected static function offerReturnToCTA( &$out, &$skin ) {
+ protected static function addReturnToModules( &$out, &$skin ) {
// OB6: 4 kinds of pages to test, different outcomes for each.
// put up popup
// load messages
@@ -318,7 +320,14 @@
}
if ( self::isPostCreateReturn( $out ) ) {
- self::offerReturnToCTA( $out, $skin );
+ // TODO (mattflaschen, 2013-10-05): If we're not going
to show
+ // anything, we probably shouldn't add this module for
performance
+ // reasons.
+ //
+ // We could move the "no show" logic to
isPostCreateReturn (with a
+ // suitable name), then decide what to do about
+ // redirect-page-impression (maybe log on the server,
or get rid of it?)
+ self::addReturnToModules( $out, $skin );
}
return true;
@@ -481,30 +490,6 @@
),
) );
}
- }
-
- return true;
- }
-
- /**
- * Called from the beginning of EditPage::internalAttemptSave
- *
- * @param EditPage $editPage page being edited
- */
- public static function onEditPageAttemptSave( EditPage $editPage ) {
- global $wgRequest, $wgUser;
-
- $title = $editPage->getTitle();
- $fullTask = self::getPageTask( $wgRequest, $title );
- if ( $fullTask !== null ) {
- self::logEvent( array(
- 'action' => 'page-save-attempt',
- 'funnel' => $fullTask,
- 'bucket' => self::getBucket( $wgUser ),
- 'pageId' => $title->getArticleID(),
- 'revId' => $title->getLatestRevID(),
- 'isEditable' => $title->quickUserCan( 'edit',
$wgUser ),
- ) );
}
return true;
diff --git a/GettingStarted.php b/GettingStarted.php
index 33be294..afacbd5 100644
--- a/GettingStarted.php
+++ b/GettingStarted.php
@@ -314,7 +314,6 @@
$wgHooks[ 'BeforeCreateEchoEvent' ][] =
'GettingStartedHooks::onBeforeCreateEchoEvent';
$wgHooks[ 'EchoGetDefaultNotifiedUsers' ][] =
'GettingStartedHooks::onEchoGetDefaultNotifiedUsers';
$wgHooks[ 'ConfirmEmailComplete' ][] =
'GettingStartedHooks::onConfirmEmailComplete';
-$wgHooks[ 'EditPage::attemptSave' ][] =
'GettingStartedHooks::onEditPageAttemptSave';
$wgHooks[ 'GetPreferences' ][] = 'GettingStartedHooks::onGetPreferences';
// Extension:CentralAuth's hook
$wgHooks[ 'CentralAuthPostLoginRedirect' ][] =
'GettingStartedHooks::onCentralAuthPostLoginRedirect';
diff --git a/resources/ext.gettingstarted.js b/resources/ext.gettingstarted.js
index dd8f2c0..d7bd200 100644
--- a/resources/ext.gettingstarted.js
+++ b/resources/ext.gettingstarted.js
@@ -1,5 +1,8 @@
// Loaded on Getting Started task selection page, regardless of whether it is
// the actual special page, or the post-account creation version.
+//
+// TODO (mattflaschen, 2013-10-07): This distinction no longer exists.
+// Cleanup the code further.
( function ( $, mw ) {
'use strict';
diff --git a/resources/ext.gettingstarted.logging.js
b/resources/ext.gettingstarted.logging.js
index 463c0a3..33447e8 100644
--- a/resources/ext.gettingstarted.logging.js
+++ b/resources/ext.gettingstarted.logging.js
@@ -38,28 +38,22 @@
/**
* Gets the applicable client-side page action (for logging purposes),
or null if there is
- * none.
+ * none. Actions that were logged before October 2013 are no longer,
so the only possible
+ * log action is 'page-impression'.
*
- * @return {string} 'page-impression', 'page-edit-impression', or
'page-save-success', or
- * null
+ * @return {string} 'page-impression' or null
*/
function getPageSchemaAction() {
- var wgAction, wgPostEdit, loggedActions, schemaAction;
+ var wgAction, loggedActions, schemaAction;
wgAction = mw.config.get( 'wgAction' );
- wgPostEdit = mw.config.get( 'wgPostEdit' );
loggedActions = {
- view : 'page-impression',
- edit : 'page-edit-impression'
+ view: 'page-impression'
};
- if ( wgPostEdit ) {
- schemaAction = 'page-save-success';
- } else {
- schemaAction = loggedActions[ wgAction ];
- }
+ schemaAction = loggedActions[wgAction];
- // We ignore history, submit, etc.
+ // We ignore edit, history, submit, etc.
if ( !schemaAction ) {
schemaAction = null;
}
@@ -134,7 +128,7 @@
* Remembers a task the user has chosen by adding it to a session
cookie.
* @param {string} article The article title (possibly including a
namespace), in prefixed
* text format. You can get this from mw.Title.getPrefixedText()
- * @param {string|null} task The kind of task, such as 'returnto' or
'gettingstarted-copyedit',
+ * @param {string|null} task The kind of task, such as 'redirect' or
'gettingstarted-copyedit',
* or null to clear the task for the article.
* @return {void}
*/
@@ -172,8 +166,6 @@
/**
* Logs a page impression.
*
- * As part of the data, it logs whether the toolbar is visible.
- *
* This is called when the user is shown a page for fixing. But with
OB6, it
* is also called from the initial redirect-page-impression (before the
user
* has responded to the CTA).
@@ -186,7 +178,7 @@
* or null for invalid schema
*/
function logImpression( fullTask, schemaAction) {
- var event,
+ var event, source,
cfg = mw.config.get( [ 'wgArticleId', 'wgRevisionId',
'wgIsProbablyEditable' ] );
event = {
@@ -201,8 +193,12 @@
if ( schemaAction === 'page-impression' ) {
// EventLogging will still log it, but mark the event
clientValidated false
- // if the source is invalid.
- event.source = mw.util.getParamValue( 'source' );
+ // if the source is invalid. However, we don't want
events with missing
+ // source to be invalid.
+ source = mw.util.getParamValue( 'source' );
+ if ( source !== null ) {
+ event.source = source;
+ }
}
return logEvent( event );
diff --git a/resources/ext.gettingstarted.openTask.js
b/resources/ext.gettingstarted.openTask.js
index 8656732..4f0aa5a 100644
--- a/resources/ext.gettingstarted.openTask.js
+++ b/resources/ext.gettingstarted.openTask.js
@@ -16,11 +16,12 @@
return;
}
- // Leave gettingstarted page-impression and page-save-success to
taskToolbar, since we need
+ // Leave gettingstarted page-impression to taskToolbar, since we need
// to wait until the toolbar is loaded. NOTE: This needs adjustment if
we do another split
// test.
- if ( task.indexOf( 'gettingstarted-' ) === 0 &&
- ( schemaAction === 'page-impression' || schemaAction ===
'page-save-success' ) ) {
+ // TODO (mattflaschen, 2013-10-05): This can be simplfied, since the
original motivation
+ // (isNavbarVisible) is no longer logged.
+ if ( task.indexOf( 'gettingstarted-' ) === 0 && schemaAction ===
'page-impression' ) {
return;
}
diff --git a/resources/ext.gettingstarted.return.js
b/resources/ext.gettingstarted.return.js
index 1423b6d..5b49d11 100644
--- a/resources/ext.gettingstarted.return.js
+++ b/resources/ext.gettingstarted.return.js
@@ -254,8 +254,16 @@
// Background overlay like GuidedTour
// TODO: Click outside or ESC to close
- function closeDialog( evt ) {
+ function closeDialogByClick( evt ) {
evt.preventDefault();
+
+ // Note: Some users might dismiss the CTA by
this click or
+ // other techniques, yet go ahead and edit this
page anyway; we
+ // _don't_ set up a funnel for this.
+ logging.logEvent( {
+ action: 'redirect-invite-click'
+ } );
+
$dialog.remove();
}
@@ -284,7 +292,7 @@
'aria-label': closeText
} )
.text( '×' )
- .click( closeDialog );
+ .click( closeDialogByClick );
$heading = $( '<div>' )
.attr( 'class', 'mw-gettingstarted-cta-heading'
)
@@ -317,25 +325,13 @@
href: '#'
} )
.text( mw.message( 'gettingstarted-cta-leave'
).text() )
- .click( closeDialog );
+ .click( closeDialogByClick );
return $dialog.append( $leaveLink );
},
showCTA: function ( kind ) {
var $cta = self.buildCTA( kind );
-
- // Hook up the leave link.
- // XXX should also log clicks on the dismiss button (if
we keep it).
- $cta.find( '#mw-gettingstarted-cta-leave-link') .on(
'click', function ( /* evt */ ) {
- $cta.dialog( 'close' );
- // Note: Some users might dismiss the CTA by
this click or
- // other techniques, yet go ahead and edit this
page anyway; we
- // _don't_ set up a funnel for this.
- logging.logEvent( {
- action: 'redirect-invite-click'
- } );
- } );
$cta.show();
$( document.body ).append( $cta );
diff --git a/resources/ext.gettingstarted.showSeparatePage.accountCreation.js
b/resources/ext.gettingstarted.showSeparatePage.accountCreation.js
index a12ab2a..ebb6af7 100644
--- a/resources/ext.gettingstarted.showSeparatePage.accountCreation.js
+++ b/resources/ext.gettingstarted.showSeparatePage.accountCreation.js
@@ -2,14 +2,6 @@
( function ( $, mw ) {
'use strict';
- var logging = mw.gettingStarted.logging;
-
- logging.setDefaults( { bucket: 'control' } );
- logging.logEvent( {
- action: 'welcomepage-impression'
- } );
-
-
$( document ).ready( function () {
var
state,
diff --git a/resources/ext.gettingstarted.specialPage.js
b/resources/ext.gettingstarted.specialPage.js
index adff8ed..eb3f6ae 100644
--- a/resources/ext.gettingstarted.specialPage.js
+++ b/resources/ext.gettingstarted.specialPage.js
@@ -7,10 +7,6 @@
action: 'gettingstarted-specialpage-impression'
};
- if ( mw.util.getParamValue( 'source' ) === 'navbar-return' ) {
- event.source = 'navbar-return';
- }
-
mw.gettingStarted.logging.logEvent( event );
} )( mediaWiki );
diff --git a/resources/ext.gettingstarted.taskToolbar.js
b/resources/ext.gettingstarted.taskToolbar.js
index 3484f50..781a8b4 100644
--- a/resources/ext.gettingstarted.taskToolbar.js
+++ b/resources/ext.gettingstarted.taskToolbar.js
@@ -17,27 +17,13 @@
toolbarInfo = cfg.wgGettingStarted.toolbar;
fullTask = 'gettingstarted-' + toolbarInfo.taskName;
- returnToListUri = new mw.Uri( mw.util.wikiGetlink(
'Special:GettingStarted' ) )
- .extend( {source: 'navbar-return'} );
+ returnToListUri = mw.util.wikiGetlink( 'Special:GettingStarted'
);
$returnToList = $( '<a>' ).attr( {
href: returnToListUri.toString(),
title: mw.message(
'gettingstarted-task-toolbar-return-to-list-title' ).text()
} ).text( mw.message(
'gettingstarted-task-toolbar-return-to-list-text' ).text() )
- .addClass( 'mw-gettingstarted-toolbar-link' ).click(
function ( evt ) {
- var $this = $( this );
-
- logging.logUnlessTimeout( {
- action: 'navbar-return-click',
- funnel: fullTask,
- pageId: cfg.wgArticleId,
- revId : cfg.wgRevisionId
- }, 500 ).always( function () {
- location.href = $this.attr( 'href' );
- } );
-
- evt.preventDefault();
- } );
+ .addClass( 'mw-gettingstarted-toolbar-link' );
$left = $( '<div>' ).attr( {
'class': 'mw-gettingstarted-toolbar-left'
@@ -75,27 +61,14 @@
tryAnotherUri = new mw.Uri(
mw.util.wikiGetlink( 'Special:GettingStarted/task/' +
toolbarInfo.taskName )
).extend( {
- exclude: cfg.wgPageName,
- source: 'navbar-next'
+ exclude: cfg.wgPageName
} );
$tryAnother = $( '<a>' ).attr( {
href: tryAnotherUri.toString(),
title: mw.message( toolbarInfo.tryAnotherTitle ).text()
} ).text( mw.message(
'gettingstarted-task-toolbar-try-another-text' ).text() )
- .addClass( 'mw-gettingstarted-toolbar-link' ).click(
function ( evt ) {
- var $this = $( this );
- logging.logUnlessTimeout( {
- action: 'navbar-next-click',
- funnel: fullTask,
- pageId: cfg.wgArticleId,
- revId : cfg.wgRevisionId
- }, 500 ).always( function () {
- location.href = $this.attr( 'href' );
- } );
-
- evt.preventDefault();
- } );
+ .addClass( 'mw-gettingstarted-toolbar-link' );
$close = $( '<a>' ).attr( {
'class': 'mw-gettingstarted-toolbar-dismiss',
--
To view, visit https://gerrit.wikimedia.org/r/87665
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id90418a6058deba030cb0af7d9660167d49fa3ab
Gerrit-PatchSet: 2
Gerrit-Project: mediawiki/extensions/GettingStarted
Gerrit-Branch: master
Gerrit-Owner: Mattflaschen <[email protected]>
Gerrit-Reviewer: Halfak <[email protected]>
Gerrit-Reviewer: Mattflaschen <[email protected]>
Gerrit-Reviewer: Ori.livneh <[email protected]>
Gerrit-Reviewer: Spage <[email protected]>
Gerrit-Reviewer: Swalling <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
