Page MenuHomePhabricator
Create Task
Maniphest T4484

Show Herald effects on objects in object transaction logs
Closed, DuplicatePublic
Actions

Description

When Herald acts on an object (by, e.g., adding reviewers or changing the owner) the change should be reflected in the object's transaction log and attributed to the appropriate Herald rule.

Original Description

We have been running Phabricator for quite a while now.

Lately we noticed that we are unable to claim/re-assign tasks. While a notification is sent out (email) and a comment begins to show up in the bug's history, the task remains assigned to the previous person.

We tried upgrading to the latest version of Maniphest/Phab but to no avail. Any ideas about how to debug this further?

Revisions and Commits

rP Phabricator
D10725rP7a163ca654e5 Pholio - upgrade herald effects to use transactions
D8405rP2ceffadee78a Support Herald rules for new Differential edits
D8404rP84020a363fee Let Herald activation depend on which transactions are being applied, and…

Related Objects
Search...

Event Timeline

kumarmay created this task.Feb 28 2014, 2:43 PM
kumarmay raised the priority of this task from to Needs Triage.
kumarmay updated the task description. (Show Details)
kumarmay added a project: Maniphest.
kumarmay added a subscriber: kumarmay.
epriestley claimed this task.Feb 28 2014, 4:03 PM
epriestley reassigned this task from epriestley to kumarmay.
epriestley claimed this task.
epriestley added a subscriber: epriestley.

It seems to work properly on this install. One theory is that someone has written a Herald rule which always assigns the task to them. You can check for this by going to Herald -> Transcripts, finding the transcript for the task edit, and then clicking "View Transcript". The detail page should show you if Herald took any actions.

kumarmay added a comment.Feb 28 2014, 4:15 PM

OMG! I feel so stupid about this one!

I had created a Herald rule which assigned all bugs without a project to me so that I could fix the project in those bugs. Now I see the correlation that this was happening for bugs where project was missing.

From a usability perspective, would it be any useful if an action performed on a Maniphest task indicates that it was performed by Herald Rule x?

kumarmay closed this task as Invalid.Feb 28 2014, 4:15 PM
epriestley added a comment.Feb 28 2014, 4:17 PM

Yeah, Herald actions should definitely show up in the transaction log, we just haven't quite gotten around to making that work. Let me repurpose this task to fix that explicitly, since it has a good example. :)

epriestley renamed this task from Unable to re-assign Maniphest tasks to Show Herald effects on objects in object transaction logs.Feb 28 2014, 4:19 PM
epriestley reopened this task as Open.
epriestley triaged this task as Normal priority.
epriestley updated the task description. (Show Details)
epriestley edited projects, added Herald; removed Maniphest.
kumarmay added a comment.Feb 28 2014, 4:28 PM

Thank you :). I can't say this enough, but have to repeat this once again - thanks for building Phabricator and sharing it :).

epriestley edited this Maniphest Task.Mar 5 2014, 3:03 AM
epriestley edited this Maniphest Task.Mar 5 2014, 3:53 AM
epriestley lowered the priority of this task from Normal to Low.Mar 5 2014, 4:07 AM

I'm going to bump this down in priority since the rest of it is tricky to act on for a while and it will probably drag out for a bit. D8404 fixes the specific issue you ran into, and records owner changes in the transaction records (here's an example):

Screen_Shot_2014-03-04_at_6.57.43_PM.png (146×1 px, 19 KB)

D8405 adds similar records for all Differential rules, which will apply fully as T2222 resolves.

The remaining cases are:

  • Pholio: Easy to swap over, I just didn't want to mingle it into D8404 since it would have added a lot more surface area to test. This can be done at any time. This is only CCs.
  • Maniphest: Projects and CCs don't generate transactions. These currently use nonstandard transactions which are messy to build. It would be cleaner to wait until we standardize them to swap them over.
  • Diffusion: Audits need to transition to proper ApplicationTransactions before we can really attack this. They also only fire once, so there's much less ambiguity.

I think none of these are likely to cause much confusion, and we can clean them up once other infrastructure changes make the cleanup easier and/or as users run into issues.

epriestley edited this Maniphest Task.Mar 5 2014, 8:07 PM
epriestley edited this Maniphest Task.
frgtn added a subscriber: frgtn.Mar 27 2014, 3:56 PM
epriestley added a comment.Aug 16 2014, 11:26 AM
  • Maniphest projects should work now.
  • Maniphest CCs won't work until T5604.
  • Diffusion is mostly updated but the initial interaction still isn't (T4896).
  • Haven't touched Pholio, this is still an easy fix that we just haven't gotten around to.
epriestley added subtasks: T5604: Properly integrate Maniphest with Subscribers, T4896: Move Audit to ApplicationTransactions.Aug 16 2014, 11:27 AM
epriestley mentioned this in T4896: Move Audit to ApplicationTransactions.Sep 14 2014, 5:21 PM
epriestley added a project: Support Impact.Oct 3 2014, 4:19 AM

Support Impact Inconsistent and confusing, although mostly fixed now.

btrahan closed subtask T4896: Move Audit to ApplicationTransactions as Resolved.Oct 15 2014, 8:14 PM
btrahan closed this task as Resolved.Oct 17 2014, 5:11 PM
btrahan added a subscriber: btrahan.

Cool if I yoink this? Yoink back if you're working on it.

I think the direct work here is Pholio and then T5604.

btrahan reopened this task as Open.Oct 17 2014, 5:11 PM
btrahan claimed this task.

(whoops, I don't know why I selected "resolve" and not "reassign")

epriestley added a comment.Oct 17 2014, 5:18 PM

Yep, all yours.

btrahan added a revision: D10725: Pholio - upgrade herald effects to use transactions.Oct 17 2014, 8:44 PM
btrahan added a commit: rP7a163ca654e5: Pholio - upgrade herald effects to use transactions.Oct 17 2014, 8:49 PM
btrahan removed btrahan as the assignee of this task.Nov 11 2014, 10:20 PM

Putting this upforgrabs as its just blocked on T5604 now and otherwise works. (i.e. Maniphest doesn't work everything else does.)

epriestley added a comment.Nov 12 2014, 1:12 AM

I'm just going to merge this into T5604, I think it will be hard to do that without getting this at the same time.

epriestley closed this task as a duplicate of T5604: Properly integrate Maniphest with Subscribers.Nov 12 2014, 1:12 AM
btrahan closed subtask T5604: Properly integrate Maniphest with Subscribers as Resolved.Dec 11 2014, 12:27 AM
btrahan reopened subtask T5604: Properly integrate Maniphest with Subscribers as Open.Dec 11 2014, 6:32 PM
btrahan closed subtask T5604: Properly integrate Maniphest with Subscribers as Resolved.Dec 18 2014, 10:42 PM
Phabricator · Built by Phacility