Page MenuHomePhabricator

Double-logging of dialog close events
Closed, ResolvedPublicBUG REPORT

Description

While researching an answer for T259308 I noticed that dialog close events (dialog-insert, dialog-done, dialog-abort, etc) are all getting logged twice.

See:

Testing instructions

  1. Make a sandbox page with wikitext containing various nodes [i]
  2. Edit it using the trackdebug=1 URL parameter
  3. Cause the edit-dialog for these nodes to be opened by focusing them and clicking the "Edit" button in their inspector
  4. Close the edit-dialog either with the X or by applying changes
  5. ✅Observe that the dialog close event (dialog-abort/dialog-done) only appears in the console once:

Can also try adding a new node via the "insert" menu, but it doesn't exercise any particularly different code-paths.

(Everything but the link in the sample wikitext is a node. The link is there just to prove there aren't any regressions for non-node dialogs.)

Done

  • @Ryasmeen: verify the events are "firing" as expected
  • [x ] @MNeisler: verify events are landing in the database as expected

i. Sample wikitext:

[[File:Foo Fighters Live 21.jpg|thumb|Foo]]
[[foo]] {{Foo}} <ref>Foo.</ref><!-- Foo -->

Event Timeline

DLynch created this task.Oct 5 2020, 11:39 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 5 2020, 11:39 PM
DLynch added a comment.Oct 6 2020, 4:41 PM

We have a doubled inheritance via NodeDialog, it seems.

  1. NodeDialog inherits FragmentDialog and mixes in NodeWindow.
  2. NodeWindow inherits FragmentWindow, and FragmentDialog mixes in FragmentWindow.
  3. NodeDialog's getTeardownProcess then calls the parent's (FragmentDialog) and mixin's (NodeWindow) getTeardownProcess methods
  4. These both call FragmentWindow's getTeardownProcess.

Same applies to getSetupProcess.

The doubling is a side-effect of 202adf904fe2d7559f1a9857d55eb24ae65581b4, I suppose, but that predates the logging code getting moved to FragmentWindow in f57c6344c392a7307e14a1992d1040b25a5e6452, so I should really have caught it then.

I think this double inheritance also applies to other methods (e.g. the constructor).

Here is the visualisation for reference.

FragmentWindow    <--mixes--    FragmentDialog
      ^                                ^
      |                                |
      |                                |
  inherits                         inherits
      |                                |
      |                                |
  NodeWindow      <--mixes--      NodeDialog

We still want all corners of the square to be called when NodeDialog calls a method, so I think we need to remove the inheritance call between NodeWindow and FragmentWindow.

Change 632566 had a related patch set uploaded (by Esanders; owner: Esanders):
[VisualEditor/VisualEditor@master] Make NodeWindow a standalone mixin

https://gerrit.wikimedia.org/r/632566

Esanders moved this task from To Triage to Triaged on the VisualEditor board.Oct 6 2020, 8:28 PM

I did a quick test of the above patch with MWGalleryDialog and both the constructor and getTeardownProcess ended up calling each corner of the square once.

This also affects Node/FragmentInspector, and the comment node inspector seemed to work fine.

I didn't test the double logging specifically.

Change 632566 merged by jenkins-bot:
[VisualEditor/VisualEditor@master] Make NodeWindow a standalone mixin

https://gerrit.wikimedia.org/r/632566

@DLynch can you add QA instructions?

JTannerWMF added a subscriber: JTannerWMF.

Once @DLynch adds testing instructions to this task move it to the QA Column and add the Editing QA tag

DLynch added a comment.EditedOct 7 2020, 5:25 PM

Sample wikitext:

[[File:Foo Fighters Live 21.jpg|thumb|Foo]]
[[foo]] {{Foo}} <ref>Foo.</ref><!-- Foo -->
  • Make a sandbox page with wikitext containing various nodes
  • Edit it using the trackdebug=1 URL parameter
  • Cause the edit-dialog for these nodes to be opened by focusing them and clicking the "Edit" button in their inspector
  • Close the edit-dialog either with the X or by applying changes
  • Observe that the dialog close event (dialog-abort/dialog-done) only appears in the console once

Can also try adding a new node via the "insert" menu, but it doesn't exercise any particularly different code-paths.

(Everything but the link in the sample wikitext is a node. The link is there just to prove there aren't any regressions for non-node dialogs.)

Change 632936 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (68c5f1a3c)

https://gerrit.wikimedia.org/r/632936

Change 632936 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Update VE core submodule to master (68c5f1a3c)

https://gerrit.wikimedia.org/r/632936

ppelberg moved this task from Inbox to High Priority on the Editing QA board.Oct 8 2020, 10:08 PM
ppelberg updated the task description. (Show Details)Oct 8 2020, 10:11 PM
ppelberg updated the task description. (Show Details)

Checked the events for both action: "dialog-abort" and action: "dialog-close" on all kinds of nodes. There is now only one event getting logged for each.

Ryasmeen edited projects, added Verified; removed Editing QA.Oct 14 2020, 8:01 PM
ppelberg reassigned this task from Esanders to MNeisler.Oct 16 2020, 12:34 AM
ppelberg updated the task description. (Show Details)
ppelberg moved this task from Backlog to Analytics on the Editing-team (Tracking) board.
ppelberg added a subscriber: MNeisler.

I'm assigning this over to @MNeisler to verify the dialog-abort and dialog-done events are being logged in the database as we expect.

MNeisler triaged this task as Medium priority.Oct 19 2020, 2:11 PM
MNeisler added a comment.EditedThu, Oct 29, 3:03 PM

I reviewed events recorded in VisualEditorFeatureUse and confirmed that the dialog close events are now only logging once. See details below.

The number of dialog close events (event.action IN ('dialog-done', 'dialog-insert', 'dialog-abort', 'dialog-remove') recorded for these node features (event.feature IN ('citoid', 'transclusion', 'media', 'reference') decreases by half on 16 October 2020.


I also looked specifically at instances of duplicate close events and confirmed that we stop recording these around 16 October 2020 aligning with the same drop off in the number of daily dialog close events shown in the graph above

@ppelberg - Reassigning to you for sign-off. Let me know if you have any questions.

MNeisler reassigned this task from MNeisler to ppelberg.Thu, Oct 29, 3:04 PM
MNeisler moved this task from Doing to Needs Sign-off on the Product-Analytics (Kanban) board.
MNeisler updated the task description. (Show Details)
ppelberg closed this task as Resolved.Fri, Nov 6, 3:22 AM

@ppelberg - Reassigning to you for sign-off. Let me know if you have any questions.

Looks good. Thank you, Megan.

Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptFri, Nov 6, 3:22 AM