Page MenuHomePhabricator

Remove the SkinMinervaReplaceNotificationsBadge hook
Closed, ResolvedPublic3 Estimated Story Points

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

  1. 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)
  2. 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

ACStatusDetails
1T301263#7981819
2T301263#7981819

QA Results - Prod

ACStatusDetails
1T301263#8010501
2T301263#8010501

Event Timeline

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

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

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.

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).

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

@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?

@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.

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

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?

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 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.

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

@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.

@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.

Test Result - Beta

Status: ✅ PASS
Environment: beta
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)

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.

IMG_5626.PNG (2×1 px, 905 KB)
IMG_5625.PNG (2×1 px, 841 KB)
IMG_5624.PNG (2×1 px, 1 MB)
IMG_5627.PNG (2×1 px, 861 KB)
IMG_5628.PNG (2×1 px, 1 MB)

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

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/

Edtadros subscribed.

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.

IMG_5761.jpg (2×1 px, 703 KB)
IMG_5757.jpg (2×1 px, 747 KB)
IMG_5760.jpg (2×1 px, 730 KB)
IMG_5759.jpg (2×1 px, 814 KB)
IMG_5758.jpg (2×1 px, 818 KB)

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

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