Page MenuHomePhabricator

markasread param doesn't work for log entry thanks
Closed, ResolvedPublic5 Story Points

Description

Steps to reproduce:

  • Get thanked for a log entry (ie. page deletion).
  • Click notifications badge to see new notices.
  • Click the notification body, which redirects you to the log entry (the link includes markasread=123456789).

Expected result:

  • The notification is marked as read.

Actual result:

  • The notification is not marked as read, only as seen (indicated by the grey number).

Event Timeline

Restricted Application added a project: Collaboration-Team-Triage. · View Herald TranscriptApr 6 2018, 9:12 AM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
TBolliger set the point value for this task to 5.
Etonkovidova added a subscriber: Etonkovidova.

Confirmed in beta - clicking on the notification body for Thank notifications that came from Logs pages won't make them markasread - the counter won't go down.

There is another issue with Logs 'Thank' notifications - T192041: Logs 'Thank' notifications appear to bundle on different actions

Samwilson moved this task from Ready to In Development on the Community-Tech-Sprint board.

This seems to be because we're using the Special:Redirect/logid/123 method to link to a log entry, and the markasread parameter isn't forwarded with the redirect.

The problem is that there's no way to link directly to one log entry (it'd be nicer to just do that). The redirection only sort-of does it by limiting the Special:Log list to a specific user, offset, and log type (which in most cases resolves to just a single entry).

Echo marks things as read via the PersonalUrls hook, which is never called for Special:Redirect. Perhaps this there is a better hook... what is the most appropriate hook that's called both for Special:Redirect and action=diff? Is it BeforeInitialize? Or should we just do it specifically for Special:Redirect, perhaps in SpecialPageBeforeFormDisplay?

Or should we look at adding a single-log-entry view to core?

Or should we look at adding a single-log-entry view to core?

The below would be my suggestion if it would help here.

Actually, it might have been wiser to add id-based paging to Special:Log so as to allow permanent links to lists of entries.

Yeah, I agree with this.

Change 428265 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/core@master] Add 'logid' parameter to Special:Log

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

Change 428266 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/Thanks@master] Use new Special:Log?logid=xxx URL for notification link

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

This is sorted now: no /123 ID parameter, because it clashes with the existing user parameter in that position (and usernames can be numeric). Ready for re-review: https://gerrit.wikimedia.org/r/#/c/428265/ (and then https://gerrit.wikimedia.org/r/#/c/428266/ ).

Change 428265 merged by jenkins-bot:
[mediawiki/core@master] Add 'logid' parameter to Special:Log

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

Change 428266 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] Use new Special:Log?logid=xxx URL for notification link

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

Niharika added a subscriber: Niharika.EditedMay 5 2018, 1:15 AM

Fixed. Rolling out the week of May 7.

Niharika closed this task as Resolved.Jun 6 2018, 11:07 PM
Niharika moved this task from Needs Review/Feedback to Q1 2018-19 on the Community-Tech-Sprint board.

Change 444351 had a related patch set uploaded (by Reedy; owner: Samwilson):
[mediawiki/core@REL1_31] Add 'logid' parameter to Special:Log

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

Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 7 2018, 10:46 AM

Change 444356 had a related patch set uploaded (by Reedy; owner: Samwilson):
[mediawiki/core@REL1_30] Add 'logid' parameter to Special:Log

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

Change 444357 had a related patch set uploaded (by Reedy; owner: Samwilson):
[mediawiki/core@REL1_27] Add 'logid' parameter to Special:Log

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

Change 444358 had a related patch set uploaded (by Reedy; owner: Samwilson):
[mediawiki/core@REL1_29] Add 'logid' parameter to Special:Log

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

Change 444351 merged by jenkins-bot:
[mediawiki/core@REL1_31] Add 'logid' parameter to Special:Log

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

Change 444356 merged by jenkins-bot:
[mediawiki/core@REL1_30] Add 'logid' parameter to Special:Log

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

Change 444357 merged by jenkins-bot:
[mediawiki/core@REL1_27] Add 'logid' parameter to Special:Log

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

Change 444358 merged by jenkins-bot:
[mediawiki/core@REL1_29] Add 'logid' parameter to Special:Log

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