Page MenuHomePhabricator

Add thank link to the end of log entries which have associated revisions
Closed, ResolvedPublic5 Story Points

Description

Repurposed this ticket (see comments below). This ticket is for adding a thank link for log entries on Special:Log - only those which have an associated revision and which are enabled (opted-in) for this feature. It will work similar to how revision-thank does.

Event Timeline

Niharika triaged this task as Normal priority.Feb 7 2018, 11:08 PM
Niharika created this task.
Restricted Application removed a project: Patch-For-Review. · View Herald TranscriptFeb 7 2018, 11:08 PM
Niharika moved this task from Untriaged to To be estimated/discussed on the Community-Tech board.EditedFeb 7 2018, 11:08 PM

@Samwilson has an in-progress patch at https://gerrit.wikimedia.org/r/#/c/408500/

Sam, if you have ideas about how to break this ticket into smaller chunks, I'd be happy to do that.

Did that list of log types happen to come from somewhere from where it's easy to copy and paste the matching values of logging.log_type? (I know I can look em up but just in case...)

Did that list of log types happen to come from somewhere from where it's easy to copy and paste the matching values of logging.log_type? (I know I can look em up but just in case...)

Unfortunately no, the list came from https://phabricator.wikimedia.org/T60485#2270744 originally, I believe. I copied it from the Epic task.

Quiddity added a comment.EditedFeb 8 2018, 3:15 AM

There were some missing items. I've updated the other task's description, and commented with explanation. @Samwilson you should be able to use the links I put in the description there to get a simple list. I hope there's a more canonical (guaranteed thorough) way to do it though!

Thanks @Quiddity, that's great.

The Thanks extension should only by default include logs types that are in core, Thanks, and Echo. That is to say:

$wgThanksLogTypeWhitelist = [
    'contentmodel',
    'delete',
    'import',
    'massmessage',
    'merge',
    'patrol',
    'protect',
    'tag',
    'managetags',
    'renameuser',
    'rights',
]

The others can be added at installation time (and any others can be added at any time after that of course):

'abusefilter'
'gwtoolset',
'gblblock',
'globalauth',
'gblblock',
'gblrename ',
'gblrights',
'notifytranslators',
'translationreview',
'usermerge',
'mwoauthconsumer',
'pagetranslation',

Does that sound right?

massmessage and renameuser are not core log types, they are from those two extensions.

Niharika updated the task description. (Show Details)Feb 8 2018, 7:34 PM

I took mass message and user rename logs out of the list.

Samwilson added a comment.EditedFeb 9 2018, 2:31 AM

Sam, if you have ideas about how to break this ticket into smaller chunks, I'd be happy to do that.

Maybe roughly:

  1. Add the thanks link to the end of log entries on Special:Log, but only for log entries that have an associated revision and make this like just do a normal revision-thank.
  2. Add log-thanks support to the API (action=thanks, type=(rev|log), id=123, source=blah; with the current rev=123 being deprecated). T186855: Add support for log-thanks to the Thanks API
  3. Update Special:Thanks to support log-thanks.
  4. Update ext.thanks.revthank module to support log-thanks.

Does that sound okay?

Change 408500 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/Thanks@master] [WiP don't review]

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

Sam, if you have ideas about how to break this ticket into smaller chunks, I'd be happy to do that.

Maybe roughly:

  1. Add the thanks link to the end of log entries on Special:Log, but only for log entries that have an associated revision and make this like just do a normal revision-thank.
  2. Add log-thanks support to the API (action=thanks, type=(rev|log), id=123, source=blah; with the current rev=123 being deprecated). T186855: Add support for log-thanks to the Thanks API
  3. Update Special:Thanks to support log-thanks.
  4. Update ext.thanks.revthank module to support log-thanks.

Does that sound okay?

That makes sense. I'll repurpose this ticket and we'll keep the Epic one at T60485: [Epic] Allow thanks of log entry.

Niharika renamed this task from Write a patch to allow thanks for specific log entries to Add the thanks link to the end of log entries on Special:Log.Feb 9 2018, 8:06 PM
Niharika updated the task description. (Show Details)
Niharika renamed this task from Add the thanks link to the end of log entries on Special:Log to Add thank link to the end of log entries on Special:Log.Feb 9 2018, 8:09 PM
Niharika updated the task description. (Show Details)

Maybe roughly:

  1. Add the thanks link to the end of log entries on Special:Log, but only for log entries that have an associated revision and make this like just do a normal revision-thank.
  2. Add log-thanks support to the API (action=thanks, type=(rev|log), id=123, source=blah; with the current rev=123 being deprecated). T186855: Add support for log-thanks to the Thanks API
  3. Update Special:Thanks to support log-thanks.
  4. Update ext.thanks.revthank module to support log-thanks.

Shouldn't there be:

  1. Add thanks links to log entries on Special:Log that are supported by the new log-thanks API

Good point @kaldari! That should definitely be on the list. :-)

Maybe roughly:

  1. Add the thanks link to the end of log entries on Special:Log, but only for log entries that have an associated revision and make this like just do a normal revision-thank.
  2. Add log-thanks support to the API (action=thanks, type=(rev|log), id=123, source=blah; with the current rev=123 being deprecated). T186855: Add support for log-thanks to the Thanks API
  3. Update Special:Thanks to support log-thanks.
  4. Update ext.thanks.revthank module to support log-thanks.

Shouldn't there be:

  1. Add thanks links to log entries on Special:Log that are supported by the new log-thanks API

I thought #5 was basically same as #1. Shouldn't #1 come after we have done #2 then, because we're refactoring that API? Also, do we have a list of log entries that have associated revisions?

Niharika renamed this task from Add thank link to the end of log entries on Special:Log to Add thank link to the end of log entries which have associated revisions .Feb 12 2018, 6:56 PM

No, #1 was just the easy addition of the existing thanks function to a new spot, and means that in #5 we're just adding the log-thanking and don't have to do much with the JS module. Could be done differently of course! :-)

Niharika set the point value for this task to 5.Feb 14 2018, 12:32 AM
Niharika removed Niharika as the assignee of this task.Feb 14 2018, 12:36 AM
TBolliger moved this task from Estimated to Archive on the Community-Tech board.Feb 14 2018, 12:58 AM
TBolliger moved this task from Archive to Estimated on the Community-Tech board.Feb 14 2018, 1:04 AM

Do 'protect' log entries need thanking? Are all protect actions accompanied by a revision, and therefore thankable with the current revision-thanks system?

JJMC89 added a subscriber: JJMC89.Feb 14 2018, 6:38 AM

Do 'protect' log entries need thanking? Are all protect actions accompanied by a revision, and therefore thankable with the current revision-thanks system?

I wold say yes. Creation protection doesn't have an associated revision.

For logs that have an associated revision (move, protection), will the logged action still be thankable if the page is deleted? I think it should be.

Do 'protect' log entries need thanking? Are all protect actions accompanied by a revision, and therefore thankable with the current revision-thanks system?

T60485: [Epic] Allow thanks of log entry has the canonical list for what logs should be thankable.

For logs that have an associated revision (move, protection), will the logged action still be thankable if the page is deleted? I think it should be.

I would agree with you, because the log items are still available on the log itself. But I leave it to @Samwilson and @Niharika to understand the nuances.

@Samwilson I agree with above. We should allow protect log entries to be thanked. And I don't see why the page being deleted will have any effect on it. It should be thinkable irrespective of the existence of the page.

Good point. All protect log entries should be thankable. Many are already thankable via the revisions that are created, so will be done as part of this task. The other, non-revision-related ones will be done separately.

A page being deleted doesn't create a revision, so will also not be thankable as part of this task.

And am I right in thinking that we still don't need to add move to wgThanksLogTypeWhitelist? That in fact the whitelist doesn't need to exist for this task because we can just check for the existence of a revision to thank?

Good point. All protect log entries should be thankable. Many are already thankable via the revisions that are created, so will be done as part of this task. The other, non-revision-related ones will be done separately.
A page being deleted doesn't create a revision, so will also not be thankable as part of this task.
And am I right in thinking that we still don't need to add move to wgThanksLogTypeWhitelist? That in fact the whitelist doesn't need to exist for this task because we can just check for the existence of a revision to thank?

Why should be not add 'move' to $wgThanksLogTypeWhitelist? Moving pages seems to be an activity deserving a thanks to me.

If all log entries which have corresponding revisions fall under the permitted thankable entries defined in T60485, then yes, we can skip the whitelist creation in this task.
Does that make sense?

Maybe roughly:

  1. Add the thanks link to the end of log entries on Special:Log, but only for log entries that have an associated revision and make this like just do a normal revision-thank.
  2. Add log-thanks support to the API (action=thanks, type=(rev|log), id=123, source=blah; with the current rev=123 being deprecated). T186855: Add support for log-thanks to the Thanks API
  3. Update Special:Thanks to support log-thanks.
  4. Update ext.thanks.revthank module to support log-thanks.

Shouldn't there be:

  1. Add thanks links to log entries on Special:Log that are supported by the new log-thanks API

Now at T187485: Add thanks link to log entries on Special:Log that are supported by the new log-thank API

matmarex removed a subscriber: matmarex.Feb 15 2018, 7:30 PM

Change 408500 abandoned by Samwilson:
[WiP don't review]

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

Samwilson claimed this task.Mar 2 2018, 4:50 AM
Samwilson edited projects, added Community-Tech-Sprint; removed Community-Tech.

Change 415803 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/Thanks@master] Add revision-thanking links to some log entries

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

Max brought up a good point: should 'move' log entries also be thankable from Special:Log? They're rather a lot like (most) 'protect' ones in that they're thankable already from the history of a page, so it makes sense that they be thankable from other places they're seen. (Just to be clear, this would simply be thanking the revision of the move, not the log entry itself, and so is separate from T186855.)

Max brought up a good point: should 'move' log entries also be thankable from Special:Log? They're rather a lot like (most) 'protect' ones in that they're thankable already from the history of a page, so it makes sense that they be thankable from other places they're seen. (Just to be clear, this would simply be thanking the revision of the move, not the log entry itself, and so is separate from T186855.)

Are there any arguments against them being thankable? If not, we should allow them to be thanked like we do for other logs with revisions.

I agree with Niharika. Anything can be controversial but moving a page is a pretty common activity.

Added 'move' to the whitelist.

Change 415803 merged by jenkins-bot:
[mediawiki/extensions/Thanks@master] Add revision-thanking links to some log entries

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

Niharika closed this task as Resolved.Mar 13 2018, 6:37 PM
Niharika moved this task from In Development to Q1 2018-19 on the Community-Tech-Sprint board.

\o/