Page MenuHomePhabricator

Add newsletter description to log messages
Closed, ResolvedPublic

Description

As of now titles of newsletters are logged, but descriptions are not. Logging both could allow easier monitoring and reverting of abusive changes.

Related Objects

StatusSubtypeAssignedTask
DuplicateQgil
ResolvedQgil
ResolvedQgil
DeclinedNone
ResolvedAddshore
DuplicateNone
Resolvedori
ResolvedBawolff
ResolvedGlaisher
Resolved01tonythomas
Resolved01tonythomas
Resolvednikitavbv
InvalidNone
ResolvedFilip
DuplicateNone
Resolved01tonythomas
ResolvedFilip
ResolvedMtDu
ResolvedMtDu
ResolvedGeorggi199
ResolvedFilip
ResolvedPppery
ResolvedFilip
ResolvedPppery
ResolvedPppery

Event Timeline

Qgil triaged this task as Medium priority.Apr 15 2016, 2:57 PM

Change 284137 had a related patch set uploaded (by 01tonythomas):
Add newsletter description to log messages

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

Descriptions can be pretty long though right?

A character limit would be fine. These descriptions are supposed to be one sentence long (or so).

Descriptions can be pretty long though right?

14:07 <addshore> well, either tim the description and put it in the params
14:07 <addshore> or a trimmed current description could be magically added to the log line by a formatter
14:08 <tonythomas> addshore: okey.! any preference ?
14:08 → DanielK_WMDE joined (daniel@wikipedia/duesentrieb)
14:08 <addshore> I dont know what is best / what has been done before
14:09 <addshore> going to formatter approach would require a db call to format the log line
14:09 <tonythomas> addshore: yeah. slowy
14:09 <addshore> so just trimming and putting it in params may be better
14:09 <addshore> also are descriptions required?
14:09 <addshore> if not you should make sure you account for if there is none

So Trim ? I am wondering, by what length ?

My initial thinking was that the description field was supposed to not be a short field. But it seems that it has now also been changed to a 'text' field (from 'textarea'). Maybe the short one is better considering that we don't even parse it. If we want to make it a short field, the DB constraint/type should also be changed. I had changed it to a blob sometime ago.

This comment was removed by Qgil.
Qgil lowered the priority of this task from Medium to Low.May 12 2016, 10:51 AM
Qgil raised the priority of this task from Low to High.

Sorry, wrong task.

My initial thinking was that the description field was supposed to not be a short field. But it seems that it has now also been changed to a 'text' field (from 'textarea').

If you are talking about the description field that shows up while creating a new newsletter -- its still a textarea, and we are storing it as a blob. Looks like we are not storing the summary of the newsletter issue anywhere.

Maybe the short one is better considering that we don't even parse it. If we want to make it a short field, the DB constraint/type should also be changed. I had changed it to a blob sometime ago.

This one is for the Main newsletter description ? We will need it as blob ?ref: https://github.com/wikimedia/mediawiki-extensions-Newsletter/blob/master/includes/specials/SpecialNewsletterCreate.php#L54

If you are talking about the description field that shows up while creating a new newsletter -- its still a textarea, and we are storing it as a blob.

I meant the description on Special:Newsletter. Since my assumption at the time when I implemented it was that it would be a long field, it was a readonly textarea (since no parsing at that time). It has since been changed to a normal text HTMLForm field.

Looks like we are not storing the summary of the newsletter issue anywhere.

This is a separate issue (not related to this task) but we should add it to the announce log entry. When I added the new announce page form, logging hasn't been implemented so we couldn't show it anywhere except in the notification back then. But now it should be pretty easy to fix it.


The patch linked is adding a truncated description to the log message as a log parameter. That has several issues:

  • full text of descripton cannot be properly attributed to users
  • no easy way to find the diff of the changes
  • vandalism/abuse also cannot be easily detected (by whom and when)

I think we can implement something similar to AbuseFilter's (and even Phabricator tasks!) history system. It should add a log entry linking to a page showing the diff of the full description. This would allow us to show the complete description (with diff) and would make the currently unresolved issues for T132016 significantly easier.
Example:
https://meta.wikimedia.org/wiki/Special:AbuseFilter/history/106/diff/prev/1187
https://meta.wikimedia.org/w/index.php?title=Special%3ALog&limit=1&offset=20160430174915&type=abusefilter&user=Glaisher

@01tonythomas can you reply to @Glaisher please? Let's try to agree on a plan to address this task, which is blocking others.

17:33 <Glaisher> We'll probably have to introduce a history table
17:33 <Glaisher> It would have fields like user, timestamp, content(?) etc.
17:34 <Glaisher> the content field would have the description stored in a json serialized blob
17:34 <Glaisher> so that we can store other stuff in the future if we need to
17:35 <Glaisher> tonythomas: We can use the diff engines in core

Change 295503 had a related patch set uploaded (by 01tonythomas):
Log changes to Newsletter description

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

Change 295503 abandoned by 01tonythomas:
Log changes to Newsletter description

Reason:
In favor of https://gerrit.wikimedia.org/r/#/c/295670

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

Change 289720 abandoned by 01tonythomas:
Setup a Newsletter history page to track changes

Reason:
In favor of https://gerrit.wikimedia.org/r/#/c/295670

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

Change 284137 abandoned by 01tonythomas:
Add newsletter description to log messages

Reason:
Thank you @siebrand, @Glaisher for the reviews, but this patch just became obsolete after https://gerrit.wikimedia.org/r/#/c/295670/. Abandoning it now.

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

Obselete after the contenthandler change!