Differential D14670

reconcile some miscellaneous edit behaviors in Maniphest
ClosedPublic
Actions

Authored by epriestley on Dec 4 2015, 6:59 PM.

Details

Reviewers

chad

Maniphest Tasks

T9132: Build an ApplicationEditor abstraction

Commits

Restricted Diffusion Commit
rPf1744ac6d9f8: Change/drop/reconcile some miscellaneous edit behaviors in Maniphest

Summary

Ref T9132. Open to discussion here since it's mostly product stuff, but here's my gut on this:

Change Maniphest behavior to stop assigning tasks if they're unassigned when closed. I think this behavior often doesn't make much sense. We'll probably separately track "who closed this" in T4434 eventually.
Only add the actor as a subscriber if they comment, like in other applications. Previously, we added them as a subscriber for other types of changes (like priority and status changes). This is more consistent, but open to retaining the old behavior or some compromise between the two.
Retain the "when changing owner, subscribe the old owner" behavior.

Test Plan

Added a comment, got CC'd.
Changed owners, saw old owner get CC'd.

Diff Detail

Repository

rP Phabricator

Branch

ee41

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 9292
Build 11030: Run Core Tests
Build 11029: arc lint + arc unit

Event Timeline

epriestley updated this revision to Diff 35476.Dec 4 2015, 6:59 PM

epriestley retitled this revision from to Change/drop/reconcile some miscellaneous edit behaviors in Maniphest.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: chad.

epriestley added a task: T9132: Build an ApplicationEditor abstraction.

eh, I think it's kind of accurate that I own a ticket that I resolve. It's a nice convenience and I think down the road with Facts, will make it less of a burden on a user to claim it as well when closing.

Currently, we claim it for all closed statuses (e.g., wontfix, invalid). Do you want to retain that behavior too?

Yeah, mostly from a Facts point of view. In the future if a manager is generating reports for tasks the team handled, it'd be nice that these were accurate reflections. I can't see basically someone not wanting to claim the task when they reply and WONTFIX it. Or do you think maybe Global Herald is the answer here?

I guess I feel like this creates inaccurate results by destroying data -- we lose the fact that the task was never claimed/assigned from a Facts point of view.

The most common case of close-without-claim I see is general triage (close invalid, close wontfix, merge) where I'm usually doing no actual work and resolving the task doesn't represent anything meaningful from a reporting point of view -- I'm basically just undoing the fact that the task was filed. T9901 is a recent example where I closed a task and implicitly claimed it, but my claim on it is basically meaningless.

We can store "Closed by" (separate from "Owner") after T4434, and surface that in Facts. That would allow generation of two different reports ("Tasks closed by X", "Owned tasks completed by X"). This distinction seems potentially useful to me?

More than that, I just think the behavior is confusing in cases like T9901. The task ending up "Assigned To: epriestley" doesn't make much sense to me.

Anyway, I don't really feel strongly one way or the other. We can go with the current behavior for now, I'll update the diff.

Yeah, I agree this is a bit grey. I like it personally, but I understand it may not be fully reflective of intent. In any regards, has anyone complained about the current behaviour? I have vague recollections of a discussion on this in the past.

I don't immediately recall any prior discussion about it, but could definitely be forgetting something.

Yeah, I think maybe it was with WM, they wanted it to show the assigned field when someone changed the status of an unassigned ticket for clarification.