Remove the SkinMinervaReplaceNotificationsBadge hook
Closed, ResolvedPublic3 Estimated Story Points
Actions

Assigned To

Authored By

	Jdlrobson
	Feb 8 2022, 5:12 PM

Description

The SkinMinervaReplaceNotificationsBadge hook should no longer be needed with the changes to the core skin architecture to recognize notifications.

TODO

Update Echo to work without the SkinMinervaReplaceNotificationsBadge hook
Make Minerva use core notifications data

QA

Confirm there are no UI regressions in the Echo group on pixel.wmcloud.org/ and that all the scenario screenshots show evidence of a logged in user (these scenarios should never show screenshots for anonymous users)
Exploratory testing: Please login on mobile and explore the Echo notification feature, recording any behaviour or change in visual design that looks unexpected.

Sign off steps

Next step will be: T309748

QA Results - Beta

AC	Status	Details
1	✅	T301263#7981819
2	✅	T301263#7981819

QA Results - Prod

AC	Status	Details
1	✅	T301263#8010501
2	✅	T301263#8010501

Details

	Subject	Repo	Branch	Lines +/-
	Do not use the SkinMinervaReplaceNotificationsBadge hook to add Echo	mediawiki/skins/MinervaNeue	master	+106 -29
	Prepare for removal of SkinMinervaReplaceNotificationsBadge hook	mediawiki/extensions/Echo	master	+15 -7

Customize query in gerrit

Related Objects

Mentioned In: T310537: [Regression] Minerva notification count is not localized to current language
T310358: [Unplanned] Notification icon should not be red when there all notifications have been seen
T309748: SkinMinervaReplaceNotificationsBadge (phase 2)
T307134: [Bug] Notices and Alerts icons should not move around in the header
Mentioned Here: T309748: SkinMinervaReplaceNotificationsBadge (phase 2)

Event Timeline

Jdlrobson created this task.Feb 8 2022, 5:12 PM

Restricted Application added a project: Growth-Team. · View Herald TranscriptFeb 8 2022, 5:12 PM

Restricted Application added subscribers: Masumrezarock100, Aklapper. · View Herald Transcript

Jdlrobson added a project: Web-Team-Backlog (Needs Prioritization (Tech)).Feb 8 2022, 5:15 PM

Change 761003 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Prepare for removal of SkinMinervaReplaceNotificationsBadge hook

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

Change 761004 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Echo@master] Drop SkinMinervaReplaceNotificationsBadge hook

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

Change 761005 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/MinervaNeue@master] Do not use the SkinMinervaReplaceNotificationsBadge hook to add Echo

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

Jdlrobson moved this task from Backlog to Menus on the Web-Team-Backlog (Needs Prioritization (Tech)) board.Feb 8 2022, 5:16 PM

• mewoph edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.Feb 8 2022, 11:16 PM

• mewoph moved this task from Incoming to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.

kostajh changed the subtype of this task from "Bug Report" to "Task".Feb 16 2022, 12:47 PM

Jdlrobson triaged this task as Medium priority.Feb 22 2022, 4:19 PM

This item is more bandwidth than the growth team has at the moment. We'll circle back in 2-3 weeks.

Hi growth team, just checking in. Do you think you'd have bandwidth to review https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/761003/ in the next 2 weeks? Since the 1.38 release branch has been cut, we're looking to do our routine maintenance technical task and I'd like this to be one of the things that gets completed.

Jdlrobson edited projects, added Web-Team-Backlog (Kanbanana-FY-2021-22); removed Web-Team-Backlog (Needs Prioritization (Tech)).Mar 22 2022, 11:24 PM

Jdlrobson moved this task from Needs Analysis to Blocked on Others on the Web-Team-Backlog (Kanbanana-FY-2021-22) board.

In T301263#7796934, @Jdlrobson wrote:

Hi growth team, just checking in. Do you think you'd have bandwidth to review https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/761003/ in the next 2 weeks? Since the 1.38 release branch has been cut, we're looking to do our routine maintenance technical task and I'd like this to be one of the things that gets completed.

We can try to get to it, yes (cc @mewoph, @Sgs); we have a lot going on at the moment though (cc @DMburugu, @MShilova_WMF).

Jdlrobson assigned this task to kostajh.Mar 28 2022, 9:44 PM

Jdlrobson claimed this task.Apr 4 2022, 3:46 PM

Jdlrobson moved this task from Blocked on Others to Doing on the Web-Team-Backlog (Kanbanana-FY-2021-22) board.

Jdlrobson moved this task from Doing to Blocked on Others on the Web-Team-Backlog (Kanbanana-FY-2021-22) board.Apr 4 2022, 8:49 PM

Lens0021 subscribed.Apr 7 2022, 12:38 PM

@kostaj @Tgr just checking in on this one. Would it help if I setup 1 hr in the calendar this week?

In T301263#7882171, @Jdlrobson wrote:

@kostaj @Tgr just checking in on this one. Would it help if I setup 1 hr in the calendar this week?

Sorry, I was confused because I thought the MinervaNeue patch was going through web team workflows per the latest comment on that patch and the Drop SkinMinervaReplaceNotificationsBadge hook patch has a merge conflict and is marked as work in progress. Would you like for us to review those again in their current state?

In T301263#7883575, @kostajh wrote:

In T301263#7882171, @Jdlrobson wrote:

@kostaj @Tgr just checking in on this one. Would it help if I setup 1 hr in the calendar this week?

Sorry, I was confused because I thought the MinervaNeue patch was going through web team workflows per the latest comment on that patch and the Drop SkinMinervaReplaceNotificationsBadge hook patch has a merge conflict and is marked as work in progress. Would you like for us to review those again in their current state?

I re-reviewed Prepare for removal of SkinMinervaReplaceNotificationsBadge hook just now.

I re-reviewed Prepare for removal of SkinMinervaReplaceNotificationsBadge hook just now.

Hopefully ready for final review. To confirm, we'll review the MinervaNeue patch. Just need to prepare our UI regression suite before doing that.

Jdlrobson mentioned this in T307134: [Bug] Notices and Alerts icons should not move around in the header.May 3 2022, 11:08 PM

In T301263#7888481, @Jdlrobson wrote:

I re-reviewed Prepare for removal of SkinMinervaReplaceNotificationsBadge hook just now.

Hopefully ready for final review. To confirm, we'll review the MinervaNeue patch. Just need to prepare our UI regression suite before doing that.

Just +2'ed. @Etonkovidova could you please QA this before reaching group1 wikis next week?

Change 761003 merged by jenkins-bot:

[mediawiki/extensions/Echo@master] Prepare for removal of SkinMinervaReplaceNotificationsBadge hook

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

ReleaseTaggerBot added a project: MW-1.39-notes (1.39.0-wmf.12; 2022-05-16).May 6 2022, 5:00 PM

Thanks growth team! Now https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/761003/ is merged, we're unblocked and this is now ready to be taken on by web team.

The next step would be updating Minerva to use the same elements as Vector and this is now ready for estimation. We'll probably want to setup some visual regression tests to aid this one. Growth team, how involved do you want to be with this work?

In T301263#7910440, @Jdlrobson wrote:

Thanks growth team! Now https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Echo/+/761003/ is merged, we're unblocked and this is now ready to be taken on by web team.

\o/ happy to hear it

The next step would be updating Minerva to use the same elements as Vector and this is now ready for estimation. We'll probably want to setup some visual regression tests to aid this one. Growth team, how involved do you want to be with this work?

If you'd like us to look at the patches, please let us know, with the caveat that our code review backlog is already long so it may take a while to get to.

Jdlrobson removed Jdlrobson as the assignee of this task.May 13 2022, 7:08 PM

alistair3149 subscribed.May 13 2022, 9:04 PM

Jdlrobson set the point value for this task to 3.May 24 2022, 5:29 PM

Jdlrobson moved this task from Upcoming to Ready for Development on the Web-Team-Backlog (Kanbanana-FY-2021-22) board.

Jdlrobson claimed this task.May 24 2022, 6:06 PM

Jdlrobson moved this task from Ready for Development to Doing on the Web-Team-Backlog (Kanbanana-FY-2021-22) board.

@Jdlrobson when you'd like our review again, please move it back to the code review column on our current sprint board and/or leave a comment here. thanks!

Okay, patch is now ready for review. To aid reviewing this I setup a test group in our UI regression https://github.com/nicholasray/pixel/pull/34

I am seeing zero UI differences before and after the change:

./pixel.js reference -b latest-release --group echo && ./pixel.js test -c 761005 --group echo

@kostajh I don't think we need Growth review here at this point, unless you'd prefer it, otherwise I'll just assume that I should keep you informed on what's happening.

Jdlrobson updated the task description. (Show Details)May 26 2022, 10:14 PM

Jdlrobson removed Jdlrobson as the assignee of this task.May 31 2022, 5:09 PM

LGoto assigned this task to • nray.May 31 2022, 5:10 PM

Change 761005 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Do not use the SkinMinervaReplaceNotificationsBadge hook to add Echo

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

Jdlrobson updated the task description. (Show Details)Jun 1 2022, 11:48 PM

Jdlrobson mentioned this in T309748: SkinMinervaReplaceNotificationsBadge (phase 2).Jun 1 2022, 11:51 PM

Jdlrobson updated the task description. (Show Details)Jun 1 2022, 11:58 PM

ReleaseTaggerBot edited projects, added MW-1.39-notes (1.39.0-wmf.15; 2022-06-06); removed MW-1.39-notes (1.39.0-wmf.12; 2022-05-16).Jun 2 2022, 12:00 AM

@kostajh heads up this is riding next week's train. I've split out phase 2 which is less urgent to T309748 so that we can focus now on QA for the new version (the code removed in T309748 should be a dead code path now). Web team will do QA, but since the Growth team knows more about this feature, having additional QA support would be ideal.

Jdlrobson updated the task description. (Show Details)Jun 2 2022, 12:21 AM

In T301263#7975335, @Jdlrobson wrote:

@kostajh heads up this is riding next week's train. I've split out phase 2 which is less urgent to T309748 so that we can focus now on QA for the new version (the code removed in T309748 should be a dead code path now). Web team will do QA, but since the Growth team knows more about this feature, having additional QA support would be ideal.

Thanks, moved to our QA column as well.

• nray reassigned this task from • nray to Edtadros.Jun 2 2022, 5:06 PM

• nray subscribed.

Etonkovidova moved this task from QA to Test in Production | Watching on the Growth-Team (Sprint 0 (Growth Team)) board.Jun 3 2022, 12:59 AM

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Monterey
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

Screen Shot 2022-06-05 at 7.17.24 PM.png (1×1 px, 386 KB)

✅ AC2: Exploratory testing: Please login on mobile and explore the Echo notification feature, recording any behaviour or change in visual design that looks unexpected.

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/77fe56a465/w/

Jdlrobson mentioned this in T310358: [Unplanned] Notification icon should not be red when there all notifications have been seen.Jun 10 2022, 3:14 PM

Jdlrobson mentioned this in T310537: [Regression] Minerva notification count is not localized to current language.Jun 13 2022, 11:18 PM

Test wiki created on Patch demo by Jdlrobson using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/9225eb4c81/w/

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

~~https://patchdemo.wmflabs.org/wikis/9225eb4c81/w/~~

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Monterey
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

✅ AC1: Confirm there are no UI regressions in the Echo group on pixel.wmcloud.org/ and that all the scenario screenshots show evidence of a logged in user (these scenarios should never show screenshots for anonymous users)
This isn't testable in Prod specifically. The image in T301263#7981819 shows the comparison of the master to the new code. This should be sufficient.
✅ AC2: Exploratory testing: Please login on mobile and explore the Echo notification feature, recording any behaviour or change in visual design that looks unexpected.

Edtadros updated the task description. (Show Details)Jun 19 2022, 1:11 AM

Jdlrobson closed this task as Resolved.Jun 21 2022, 5:19 PM

Test wiki on Patch demo by Jdlrobson using patch(es) linked to this task was deleted:

~~https://patchdemo.wmflabs.org/wikis/77fe56a465/w/~~

Urbanecm_WMF removed a subscriber: MShilova_WMF.Jul 31 2023, 6:04 PM

	F35247824: IMG_5758.jpg
	Jun 16 2022, 10:54 PM

	F35247823: IMG_5759.jpg
	Jun 16 2022, 10:54 PM

	F35247822: IMG_5760.jpg
	Jun 16 2022, 10:54 PM

	F35247821: IMG_5757.jpg
	Jun 16 2022, 10:54 PM

Remove the SkinMinervaReplaceNotificationsBadge hookClosed, ResolvedPublic3 Estimated Story PointsActions

Description

TODO

QA

Sign off steps

QA Results - Beta

QA Results - Prod

Details

Related Objects

Event Timeline

Test Result - Beta

QA Steps

Test Result - Prod

QA Steps

Remove the SkinMinervaReplaceNotificationsBadge hook
Closed, ResolvedPublic3 Estimated Story Points
Actions