Page MenuHomePhabricator

sectionNumbers and issuesSeverity should always be consistent in length
Closed, ResolvedPublic5 Story Points

Description

There are 2 remaining cases where sectionNumbers and issuesSeverity are of different length for pageLoaded and editClicked events.

Two issues inside the lead (no multiple issues)

Examples

https://reading-web-staging.wmflabs.org/w/index.php?title=Four_Horsemen_(drink)
https://en.wikipedia.org/wiki/Endurance_Kart_Championship

Visual

Current behaviour

sectionNumbers: [0] issuesSeverity: [HIGH, HIGH]

Expected behavior

we should log

sectionNumbers: [0, 0] issuesSeverity: [HIGH, HIGH]

Multiple issues template

Examples

Can be found here: https://en.wikipedia.org/w/index.php?title=Special%3AWhatLinksHere&target=Template%3AMultiple+issues&namespace=0
e.g.
https://reading-web-staging.wmflabs.org/w/index.php?title=Politics_of_Cyprus

Visual

Current behaviour

{ sectionNumbers: [0], issuesSeverity: [MEDIUM, MEDIUM] }

Expected behavior

we should log

{ sectionNumbers: [0], issuesSeverity: [MEDIUM] }

where MEDIUM is the maximum severity level.

combining both above

Example

https://reading-web-staging.wmflabs.org/w/index.php?title=Altitude_latitude_theory

Visual

Current behaviour

{ sectionNumbers: [0], issuesSeverity: ["HIGH", "MEDIUM", "LOW", "MEDIUM"]  }

Expected behavior

we should log

{ sectionNumbers: [0], issuesSeverity: [HIGH, MEDIUM] }

where MEDIUM is the maximum severity level of the multiple issues template and HIGH relates to the other box

Acceptance criteria

  • Behaviour is consistent between group A and group B
  • Clicking edit should log sectionNumbers and issuesSeverity per definition above
  • pageLoaded event should log sectionNumbers and issuesSeverity per definition above

Developer notes

getAllIssuesSections is the problematic function

/**
 * Returns an array of all the page sections that have issues.
 * @return {array}
 */
function getAllIssuesSections() {
        return Object.keys( allIssues ).filter( function ( section ) {
                return allIssues[ section ].length;
        } );
}

This will need to be modified. If allIssues[section] is length 2, we should return 2 section ids.
However, allIssues is also used to track all issues collapsed/not-collapsed.
We'll probably have to update the IssueSummary typedef to include a hidden field which will be set for issues hidden under multiple issues tag.

This is not going to be a trivial change.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 29 2018, 12:04 AM

And for reference, this implements the following part of the schema description: "This array [sectionNumbers] should be consistent with severities, except on modalClose". Basically the n-th element in each array should correspond to the same issues template on the page, with special treatment for multiple issues templates as detailed in the schema description.

Jdlrobson updated the task description. (Show Details)Aug 29 2018, 12:37 AM
ovasileva triaged this task as High priority.Aug 29 2018, 7:42 AM

Change 456104 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] Consistent sectionNumbers and issuesSeverity for page-issues.

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

The first patch above addresses the first scenario, multiple issues without the "multiple issues" template.
Tested on "List_of_residents_of_10_Downing_Street", "Four_Horsemen_(drink)" and "Roman_Catholic_Diocese_of_Darwin" articles. For groups A and B,
the pageLoaded and editClicked events return the following data:

Four_Horsemen_(drink) - pageLoaded and editClicked, groups A and B

"issuesSeverity":["HIGH","MEDIUM"],"sectionNumbers":[0,0]

List_of_residents_of_10_Downing_Street - pageLoaded and editClicked, groups A and B

"issuesSeverity":["LOW","LOW","MEDIUM"],"sectionNumbers":[0,0,0]

Roman_Catholic_Diocese_of_Darwin - pageLoaded and editClicked, groups B (new) - not group A (old)!*

"issuesSeverity":["MEDIUM","MEDIUM"],"sectionNumbers":[1,4]

*the reason group A doesn't return the same values as group B on the last article is that the old page-issues banner doesn't show up if issues aren't in the lead section :( (maybe a separate task for that?)

For scenario A
Two issues inside the lead (no multiple issues)
issue #1 high severity
issue #2 high severity

sectionNumbers: [0, 0] issuesSeverity: [HIGH, HIGH]

To be compatible with scenario C mixed 'multi-template' and non-multi-template as in "Altitude_latitude_theory"
issue #1 high severity
issue #2 (multiple) medium severity, low severity, low severity
I think the output has to be

sectionNumbers: [0, 0] issuesSeverity: [HIGH, MEDIUM]

instead of

sectionNumbers: [0], issuesSeverity: [HIGH, MEDIUM]

as in the description.

Change 456132 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] For page-issues pageLoaded and editClicked events, treat "multiple issues" templates as one issue.

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

ovasileva set the point value for this task to 5.Aug 29 2018, 4:54 PM

Change 456104 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Consistent sectionNumbers and issuesSeverity for page-issues.

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

Change 456132 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] For page-issues pageLoaded and editClicked events, treat "multiple issues" templates as one issue.

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

After reviewing with @Tbayer today, we noticed one error regarding the multiple issues template.
On the page Politics_of_Cyprus, which has a multiple issues template, the modalClosed event sends

"issuesSeverity":["MEDIUM","MEDIUM"],"sectionNumbers":[0]

Instead, there should be a section number corresponding to each issue in the modal.

"issuesSeverity":["MEDIUM","MEDIUM"],"sectionNumbers":[0,0]

Although it's kind of a given that the issues in modal are in the same section, we should still be explicit about this in the data.

Change 456764 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/skins/MinervaNeue@master] page-issues sectionNumbers consistency on modalClose event

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

Change 458281 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Correct the link for page issues in the old treatment.

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

Change 456764 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] page-issues sectionNumbers consistency on modalClose event

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

Change 458281 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Correct the link for page issues in the old treatment.

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

Jdlrobson added a comment.EditedSep 7 2018, 12:15 AM

In the old treatment, on https://reading-web-staging.wmflabs.org/w/index.php?title=Rover_(lawnmower)

Rover has 1 multiple issues box (containing 2 issues) and 1 deletion notice.
The pageLoaded event correctly identifies this to be the case.

However clicking "Page issues" triggers an event "issuesClicked" which has 2 sectionNumbers and 1 severity. When clicking the length should only be 1.

The modalClosed event works correctly showing the severities and sectionNumbers of all 3 issues in the page

Change 458619 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Issues instrumentation: issuesClicked events should always send 1 sectionNumber

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

Change 458619 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Issues instrumentation: issuesClicked events should always send 1 sectionNumber

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

Jdlrobson closed this task as Resolved.Sep 10 2018, 8:00 PM

I ran through this with @Tbayer @Jdrewniak and @Niedzielski and this appears to be implemented as expected
I've added some exploratory QA steps to T191532 to cover this scenario further.