Page MenuHomePhabricator

Tag Thanks actions with AMC tag
Closed, ResolvedPublic8 Estimated Story Points

Description

All actions performed by a user with Advanced Mobile Contributions mode enabled should be tagged with advanced mobile edit tag introduced in T212959.

If possible, all thanks actions in the thanks log should also be marked with mobile web edit.

Acceptance criteria

  • If MobileFrontend is not installed, the code to check for AMC and add the tag dies silently
  • If MobileFrontend is installed and the user is not opted into AMC, no tags are added
  • If MobileFrontend is installed and AMC is enabled and I thank a user, the edit tag is added
  • Any click on the thanks button on MOBILE creates a log entry on https://en.wikipedia.org/wiki/Special:Log which is tagged "mobile edit"
  • Any click on the thanks button on MOBILE AMC creates a log entry on https://en.wikipedia.org/wiki/Special:Log which is tagged "mobile edit" and "advanced mobile contribution".
  • Thanks entries are tagged "mobile edit" and "advanced mobile contribution" as appropriate.

Developer notes

There is no need to introduce new Hook. The only required change is to publish Thanks Log entries both to RecentChange and UDP (rcandudp). The associated RecentChange save will trigger RecentChangeSave hook. MobileFrontend already knows how to add tags to the RecentChangeSave.

from Piotr

EDIT: Does not apply any more
I think (Sam/Jon, please correct me if I'm wrong) that the easiest way to implement that in a nice way is to split change into two separate patches (for example I'll use Thanks extension)
1). First, check if the extension, or the MWCore part has a responsible hook for that action, if not, please create a new hook Hooks::run('HOOKNAME', $tags). The {HOOKNAME} should be very descriptive, something like "ExtensionThanksLogEntryTagCreate".
2). in the MobileFrontend add a new hook listener. Add it to extension.json,. method should be located in includes/amc/Hooks.php. When hook is executed and current user has amc mode enabled, please add amc tag to the $tags array (it should be passed as reference).

Done, thanks to that, you do not create a hard dependency between Thanks and MobileFrontend, and you tackle the tagging.
There is one more thing, I'd love to pass the whole LogEntry object, not only $tags array, but looks like the common way is to pass references to arrays, I have one more day, I'll check is it ok to push objects (I'm concerned that those objects somehow/somewhere are serialized and thats why we pass scalar types/arrays)

from Jon

It doesn't look like thanks on mobile are tagged with "mobile edit"
See:
https://en.wikipedia.org/wiki/Special:Log?type=&user=Jdlrobson&page=&wpdate=&tagfilter=&wpfilters%5B%5D=thanks
(I never thank on desktop)

In the Thanks extension the log entry is created at ApiThank::logThanks
This is where the new hook must be added and run.

QA steps

Test on https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/390159

e.g. https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/390159

QA Results

StatusDetails
✅ PassedT215477#5039143

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@ovasileva @Tbayer clarification question so we are all on the same page. When should we tag "thanks" action with advanced mobile edit? I see two possibilities - when user who performs the action has amc mode enabled (when performing action), or do we want to tag thanks action when the revision thank is sent for was made with amc mode enabled?.

@ovasileva @Tbayer clarification question so we are all on the same page. When should we tag "thanks" action with advanced mobile edit? I see two possibilities - when user who performs the action has amc mode enabled (when performing action), or do we want to tag thanks action when the revision thank is sent for was made with amc mode enabled?.

The former - it's about measuring how often the Thanks feature is being used from the AMC interface.

(Recording a note from another conversation today:)
We may want to keep in mind the recent changes that were made as part of T60485: [Epic] Allow thanks of log entry, depending on how/whether we add the corresponding log pages into the AMC interface.

T215675 is a blocker, we need to decide on how to dynamically tag log entries before we can do this task.

Jdlrobson updated the task description. (Show Details)
Jdlrobson subscribed.

T215675 is a blocker, we need to decide on how to dynamically tag log entries before we can do this task.

I disagree and would say it's the other way round. Let's make a bespoke solution for Thanks and see what's involved. We can then think about how we might improve this approach to apply to other workflows.

onExtensionThanksLogEntryTagCreate

Sorry, coming into this discussion a little late. Can this be generalized to any tag and be a change in Core instead? Something like onExtensionLogEntryTagCreate()?

onExtensionThanksLogEntryTagCreate

Sorry, coming into this discussion a little late. Can this be generalized to any tag and be a change in Core instead? Something like onExtensionLogEntryTagCreate()?

I figured it might make sense to make a bespoke solution for Thanks first.
I guess such a generic hook for log events would need to run inside $logEntry->insert() ? We could start with that approach but I'm not sure who we'd need to talk about that and it might be easier to demonstrate the need for it with various examples first. Open to different approaches here!

onExtensionThanksLogEntryTagCreate

Sorry, coming into this discussion a little late. Can this be generalized to any tag and be a change in Core instead? Something like onExtensionLogEntryTagCreate()?

I figured it might make sense to make a bespoke solution for Thanks first.
I guess such a generic hook for log events would need to run inside $logEntry->insert() ? We could start with that approach but I'm not sure who we'd need to talk about that and it might be easier to demonstrate the need for it with various examples first. Open to different approaches here!

In terms of need - we're interested in being able to track all of the moderation actions from the beginning with the motivation that while we might not be working directly on the pages that include these actions, we will still be making changes that make performing these actions smoother for users indirectly. Meaning - it's possible that even having the talk tab on the page, for example, might affect these numbers and we'd like to be able to track that from the beginning (and any changes to it as we improve the functionality)

I guess such a generic hook for log events would need to run inside $logEntry->insert()?

Quoting myself from 5 days ago to a private email thread:

Rather than creating a hook per log entry creation path, we could create the BeforeLogEntryInsert hook and run it in ManualLogEntry::insert.

There's a bunch of refactoring that should be done to LogEntry and ManualLogEntry (as well as DatabaseLogEntry) that will be deftly avoided by the introduction of such a hook. I can't help but feel that doing so would be contributing to the technical debt of the codebase though.

I think it'd be worth exploring refactoring the LogEntry-related code, drafting a factoring that would allow extensions to modify log entries as they need, and then estimate that work. This way, Olga would be able to make an informed decision as to whether the product goal of having all AMC edits tagged is worth it.


We could start with that approach but I'm not sure who we'd need to talk about that and it might be easier to demonstrate the need for it with various examples first. Open to different approaches here!

According to https://www.mediawiki.org/wiki/Developers/Maintainers, Umherirrender is a maintainer for Special:Log. You might want to ask them for guidance. Otherwise, you could ask on wikitech.

I would strongly recommend against introducing technical debt in several areas when you can limit it to one.

we're interested in being able to track all of the moderation actions

I understand the end goal. I think Sam+stephen and I are just disagreeing on how to get there.

I would strongly recommend against introducing technical debt in several areas when you can limit it to one.

Do we know enough about log entries to be sure that they are always run inside a user session and that mobile edit / amc tags would be applicable in all cases?
If seems risky to me to run this hook in every single call to insert (there are over a 110 usages). Validating it does work on the single use case of Thanks still seems like a good idea to me before proceeding to generalise this.
Note calling setTags:

$logEntry->setTags( [ 'mobile edit' ] )

according to the code would wipe out all existing tags. So if we did that, that would be quite a costly mistake. There may be other behaviours we are not aware of that would suggest starting with a Thanks-only solution then generalising would be a good idea.

We talked through this in estimation, and there were definitely some wires crossed. I felt more reassured that we would still be able to scope log entries to Thanks while adding this hook in core.

Change 493740 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Thanks@master] Publish Thanks log actions to rcandudp

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

Change 493743 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Allow all RecentChanges to be tagged with mobile tags

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

Change 493743 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Allow all RecentChanges to be tagged with mobile tags

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

The mobile edit and mobile web edit tags were applied only to article edits and file uploads. The acceptance criteria say:

Any click on the thanks button on MOBILE creates a log entry on https://en.wikipedia.org/wiki/Special:Log which is tagged "mobile edit"

I also know that we want to properly tag all moderation actions. Instead of tackling the Thanks log only (and applying the tag only to Thanks), In rEMFR86aa0dc4b84fecc3f124aab2bac1202a63d6b0a5 I removed the restriction Tag only log edits and uploads.
From now on, all actions performed on the mobile web and visible in RecentChanges log is will be tagged with mobile web edit and mobile edit accordingly.

Change 493740 abandoned by Pmiazga:
Publish Thanks log actions to rcandudp

Reason:
We don't want to make Thanks action visible in RecentChanges log.

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

Leaving this here since we were talking about it earlier today in this context:

Here is the announcement of the depreciation of the tag_summarytable
https://lists.wikimedia.org/pipermail/wikitech-l/2018-November/091170.html (see also the general context of the change tags refactoring there).
I have also added this to https://www.mediawiki.org/wiki/Manual:Tag_summary_table .

This doesn't appear to be working after following the new QA steps. This is not ready for QA.

Change 496339 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Tag all log items with 'advanced mobile edit'

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

Blocked on T215675 - There are 2 patches in core that have to be merged first:

Change 497432 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Tag all log entries when user uses mobile mode.

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

Change 496339 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Tag all log items with 'advanced mobile edit'

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

Change 497432 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Tag all log entries when user uses mobile mode.

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

Edtadros subscribed.

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome DevTools Device Emulator (iPhone X)

Test Artifact(s):

QA steps

Test on https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/390159

Visit https://en.m.wikipedia.beta.wmflabs.org/w/index.php?title=Special:MobileOptions&returnto=Special%3AMobileDiff%2F390159 and opt into advanced mobile mode
Visit a diff and click "Thanks" in the bottom right corner
e.g. https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:MobileDiff/390159

Verify you get a message "Username was thanked"
❌ FAIL: the messages are both different.
✅ PASS : see T215477#5039353

Screen Shot 2019-03-19 at 11.06.34 PM.png (2×1 px, 705 KB)
Screen Shot 2019-03-19 at 11.06.17 PM.png (2×1 px, 705 KB)

Verify the log message was tagged correctly at https://en.m.wikipedia.beta.wmflabs.org/wiki/Special:Log?type=&user=&page=&wpdate=&tagfilter=&wpfilters%5B%5D=thanks
❓ Not sure what "correctly" means. This looks correct but I want to be sure.
✅ PASS : see T215477#5039353

T215477-1.png (1×750 px, 200 KB)

Verify that Thanks action do not appear on the RecentChanges page - https://en.wikipedia.beta.wmflabs.org/wiki/Special:RecentChanges?hidebots=1&hidecategorization=1&hideWikibase=1&limit=50&days=7&urlversion=2
✅ PASS

T215477-2.png (1×750 px, 217 KB)

@Jdlrobson I suspect in the failed step the message is okay, but I just wanted to confirm because it wasn't an exact match. For the ?, i'm not sure what is "correct" but if you look a the screenshot and you're good with what was returned I can pass that step.

@Edtadros - The failed test is ok, the texts are bit different. If you want I can update the QA steps and fix the expected messages.
The second step "tagged correctly at" => it means that it has all three tags (Advanced mobile edit, mobile edit, mobile web edit), and those tags are applied.

IMHO all tests pass. Thank you for being so detail-oriented!

@pmiazga Thanks for checking this out. I've updated the test results per your comment.

@ovasileva This is passed per T215477#5039353

This task is blocked by https://phabricator.wikimedia.org/T218940 - we need to solve logspam with Logging system, otherwise, our tagging system will be reverted, therefore tagging thanks action will stop working.

Test Result:Production

Status: ✅ PASS
OS: iOS
Browser: Chrome browser on (iPhone XS Max)

Test Artifact(s):

QA steps

Visit a diff and click "Thanks" in the bottom right corner

Verify you get a message "Username was thanked"
✅ PASS

IMG_2154.PNG (2×1 px, 543 KB)

IMG_2155.PNG (2×1 px, 544 KB)

Verify the log message was tagged correctly
✅ PASS

IMG_FB3E8E52515A-1.jpeg (2×1 px, 1 MB)

Verify that Thanks action do not appear on the RecentChanges page
✅ PASS

IMG_FFD577DC5D93-1.jpeg (2×1 px, 1 MB)