Page MenuHomePhabricator

Maniphest - load subscribers in getApplicationTransactionsObject
ClosedPublic

Authored by btrahan on Dec 11 2014, 7:03 PM.
Tags
None
Referenced Files
F13994119: D10976.id26355.diff
Wed, Oct 23, 3:59 AM
F13981388: D10976.id.diff
Sat, Oct 19, 4:13 PM
Unknown Object (File)
Oct 6 2024, 4:38 PM
Unknown Object (File)
Sep 29 2024, 1:06 PM
Unknown Object (File)
Sep 27 2024, 10:38 AM
Unknown Object (File)
Sep 27 2024, 10:31 AM
Unknown Object (File)
Sep 27 2024, 10:30 AM
Unknown Object (File)
Sep 27 2024, 10:28 AM
Subscribers

Details

Summary

Fixes T6734. This is a very generic fix, which basically attaches the subscribers if necessary. This seems like a good idea given there's some crazy generic code doing this sort of thing? This would end up being a new pattern for these types of objects that may be loaded by a general object query but then get some editor action against them.

Test Plan

I can't actually reproduce this in my sandbox so I'll verify live again.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

btrahan retitled this revision from to Maniphest - load subscribers in getApplicationTransactionsObject.
btrahan updated this object.
btrahan edited the test plan for this revision. (Show Details)
btrahan added a reviewer: epriestley.

How about doing this in Herald instead? That would be more consistent with how Herald loads other types of data.

I feel like that's a bit cleaner than loading in getApplicationTransactionObject().

This error in T6734 is from PhabricatorTokenGivenEditor, in the publishTransaction method.

(AFAIK Herald is okay because it gets invoked via the ManiphestEditor now, and the ManiphestTask is properly loaded.)

Oh, hrrm -- this is the trace I'm seeing (definitely looks like a Herald issue?) -- do you have a different one?

Screen_Shot_2014-12-11_at_11.17.38_AM.png (1×1 px, 278 KB)

I think to get this to repro, add a Herald rule which acts on subscribers, like this one:

Screen_Shot_2014-12-11_at_11.19.08_AM.png (1×1 px, 148 KB)

I don't get this to reproduce with a herald rule:

Screen_Shot_2014-12-11_at_11.24.13_AM.png (1×1 px, 377 KB)

I can give tokens to tasks with no errors regardless of whether xerxes is subscribed, not subscribed, unsubscribed, or subscribed with others.

oh my bad - that's with this patch applied. remove the patch i get the error.

btrahan edited edge metadata.

fix it by loading the subscribers like how we load projects in the herald adapter.

epriestley edited edge metadata.

Haha, I thought I was going crazy for a second.

This revision is now accepted and ready to land.Dec 11 2014, 7:29 PM

Haha, I thought I was going crazy for a second.

me too! :P

This revision was automatically updated to reflect the committed changes.

(also I guess all this is sort of a hint at how this can go awry with projects conditionally loading eventually...)