Page MenuHomePhabricator

Phabricator should only notify changes to the "Security" field if it is indeed changed
Closed, ResolvedPublic

Description

Generic problem reported upstream: https://secure.phabricator.com/T7661

When a task is edited, the (new/"new") status of the "Security" field is notified regardless of it having been changed:

scfc triaged this task as "Normal" priority.
scfc claimed this task.
scfc set Security to none.

The first few times I feared I might have accidentally changed the setting.

Changes of that setting should only be notified if it has indeed been changed from the previous value.

Related Objects

Event Timeline

scfc created this task.Jan 18 2015, 7:06 AM
scfc raised the priority of this task from to Needs Triage.
scfc updated the task description. (Show Details)
scfc added a project: Phabricator.
scfc added a subscriber: scfc.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 18 2015, 7:06 AM
scfc reopened this task as Open.Jan 18 2015, 9:20 AM
scfc set Security to None.

AFAIUI, T479 is about the initial notification when a task is created (which apparently has been fixed). This task is about notifications that occur when an existing task is edited; cf. T87117 for a current example.

This only happens in tasks imported from bugzilla, the first time someone use the "edit this task".

Another example: T20871#983927

scfc added a comment.Jan 18 2015, 6:25 PM

T87117 was not imported from Bugzilla, but freshly created yesterday.

Aklapper triaged this task as Low priority.Jan 18 2015, 6:33 PM

I'm not really sure what could be the cause of this but it's likely an upstream issue. :-/

He7d3r added a subscriber: He7d3r.Feb 13 2015, 6:48 PM

I'm not really sure what could be the cause of this but it's likely an upstream issue. :-/

The last change (by @chasemp) to that line (post T-479), was at https://phabricator.wikimedia.org/rOPUP269b542f567d76359c37d54bc960fb679fd0d1dd but the bug was a problem before that patch (see third example below), so I don't think the issue originates there.


I've drafted this... If it's correct, could someone push it upstream, please? Thanks :)


Using "Edit Task" can send a confusing "Security changed to none" message

We have the message "Security changed to none" appearing if someone edits using the "Edit Task" button. Examples:

How can we fix this? Thanks!

(Possibly related: There was an older bug about the message "changed Security from none to none." always appearing during Task Creation, which MModel fixed with https://phabricator.wikimedia.org/rOPUP26192d415b593eb754a73370dd74bfd117d6a412
The current version is at https://phabricator.wikimedia.org/diffusion/OPUP/browse/production/modules/phabricator/data/fixed_settings.yaml )

Qgil added a subscriber: Qgil.

The Security field is a "custom field" in Phabricator language, and the report should request something like "Custom fields not changing their 'none' value shouldn't report 'changed to none'", or something along these lines.

Ltrlg added a subscriber: Ltrlg.Feb 20 2015, 10:14 AM
Qgil added a comment.Feb 23 2015, 9:27 AM

This only happens in tasks imported from bugzilla, the first time someone use the "edit this task".

... and it happens in relation to a custom field and a template that we have created. I really wonder if this is acceptable upstream, unless @mmodell can identify which piece of Phabricator is causing this problem. With my little knowledge of the several pieces at play here, I don't dare submitting this problem upstream.

Qgil added a comment.Feb 23 2015, 6:50 PM

This only happens in tasks imported from bugzilla, the first time someone use the "edit this task".

Actually it happens just in any task, apparently. I created T89473 in Phabricator, updated several times from the actions dialog, but when I edited the task... T89473#1059766

it might be a regression...I thought it had been resolved.

Qgil added a comment.Feb 23 2015, 6:58 PM

I don't think it is a regression. When T479 was fixed we got this new type of message.

If rOPUP26192d415b59 was about setting default: default instead of default: none when creating a task, can it be that editing a task still sets default: none? Bare with me, I'm speaking blindly without looking at the code.

Qgil added a comment.Mar 2 2015, 9:40 AM

@mmodell, does this ring a bell, perhaps? ^^^^

The Security field is a "custom field" in Phabricator language, and the report should request something like "Custom fields not changing their 'none' value shouldn't report 'changed to none'", or something along these lines.

I guess this can also include other fields such as the "Is Sprint" field (added by the sprint extension) which also gets added to project logs the same way.

I don't think it is a regression. When T479 was fixed we got this new type of message.
If rOPUP26192d415b59 was about setting default: default instead of default: none when creating a task, can it be that editing a task still sets default: none? Bare with me, I'm speaking blindly without looking at the code.

I can't reproduce on phab-01 (https://phab-01.wmflabs.org/T721). Are we using a different version of Phabricator over there?

Qgil added a comment.Mar 11 2015, 11:59 AM

"Is Sprint" is another custom field, so again not a good case for a report upstream.

As far as I'm aware, our Security extension is not enabled in phab-01, only here in production.

Qgil added a comment.Mar 18 2015, 2:55 PM

I wonder whether rOPUP269b542f567d changed anything here. I have noted that now the message is

... set Security to None.

Now "None" is uppercase. Was it before?

In T479 the message was

... changed Security from none to None.

See the lowercase first, and the uppercase next.

Also, before the message said "changed" while now is "set". Does it mean that tasks in fact don't have a security variable "set" until the description is edited?

I wonder whether rOPUP269b542f567d changed anything here. I have noted that now the message is

... set Security to None.

Now "None" is uppercase. Was it before?
In T479 the message was

... changed Security from none to None.

See the lowercase first, and the uppercase next.
Also, before the message said "changed" while now is "set". Does it mean that tasks in fact don't have a security variable "set" until the description is edited?

'None' wasn't uppercase before and that fixed things for me. I don't see this problem anymore for edited tasks. The set message still shows up for new tasks but I believe that is the way things are supposed to behave.

"Set" is only used when the task is first created (when the field is changed from null to 'None') and "Changed" is used when any other changes are made to the field (including changes from 'none' to 'None').

Oh I see what you mean. That is interesting. It seems the field isn't initialized if the description isn't set. Phabricator must order the fields so that if a field is edited farther down it changes the fields differently.

Qgil updated the task description. (Show Details)Mar 27 2015, 12:21 PM
Qgil moved this task from Need Discussion to Upstreamed on the Phabricator (Upstream) board.
Qgil added a comment.May 7 2015, 1:42 PM

Is it just me and my subjective (lack of) memory, or we haven't seen this message for a while now?

Yesterday: T98425

It happens the first time a task is edited after its creation (using the full form, not the comment form), on every task I think.

I've tried everything I can think of to fix this, I really don't know of any solution.

This issue is still present. After creating a new issue, I edit the description but see "Niedzielski set Security to None." in my notification mail.

@Niedzielski: it may still be happening but it doesn't seem like there is much hope of fixing it.

Qgil added a comment.Jun 26 2015, 6:13 PM

Maybe if one day we move our process for private tasks to Spaces (T823).

@Qgil: there is actually a better solution than spaces - epriestley implemented a "subscribers" policy which does exactly what our existing "software security bug" does - and template tasks can be used to pre-select the appropriate policy and CC list for a task.

Quiddity removed a subscriber: Quiddity.Jun 28 2015, 6:38 PM

It happens even if there are no other changes being made to the task. e.g. @Liuxinyu970226 now on T114978#1806221 . Can that be stopped somehow. e.g. a mass database change to set the security field to None for tasks without any value in the security field.

It happens even if there are no other changes being made to the task. e.g. @Liuxinyu970226 now on T114978#1806221 . Can that be stopped somehow. e.g. a mass database change to set the security field to None for tasks without any value in the security field.

A similar idea was suggested in T479#840405 and mentioned again in T479#983947.

I'm not sure why this issue is so intractable.

Dvorapa removed a subscriber: Dvorapa.Nov 23 2015, 6:09 AM
Restricted Application added a subscriber: Luke081515. · View Herald TranscriptDec 24 2015, 7:25 PM
Luke081515 lowered the priority of this task from Low to Lowest.Jan 12 2016, 8:26 PM

We can close this, after we updated phab. The security extension is obsolete after that, so no need to fix this.

mmodell closed this task as Resolved.Feb 18 2016, 3:33 AM
mmodell claimed this task.
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMay 23 2016, 6:03 PM