Page MenuHomePhabricator

Make "ADD_CC" and "REMOVE_CC" available as "standard" Herald effects
ClosedPublic

Authored by epriestley on Jun 6 2015, 12:33 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 2:51 PM
Unknown Object (File)
Mon, Dec 9, 4:44 PM
Unknown Object (File)
Sun, Dec 8, 5:32 PM
Unknown Object (File)
Sun, Dec 8, 1:46 AM
Unknown Object (File)
Sat, Dec 7, 7:11 AM
Unknown Object (File)
Fri, Nov 29, 10:34 AM
Unknown Object (File)
Thu, Nov 28, 4:50 AM
Unknown Object (File)
Wed, Nov 27, 10:53 AM
Subscribers

Details

Summary

Ref T8455. Begins consolidating the code for applying these effects:

  • Makes Add/Remove subscribers a standard effect, and uses it in Pholio.
  • This includes the "don't re-subscribe users who have explicitly unsubscribed" logic from Differential in the standard effect. I think this rule is always desirable.
  • This adds new filtering of invalid PHID types to resolve the arc diff issue in T8455 once Differential uses this standard effect.
  • Added "Remove Subscribers" to MockAdapter in order to test that it works.
  • Relabeled "CC" in Pholio to "Subscribers" for consistency.
Test Plan
  • Created several rules which add subscribers to (and remove subscribers from) mocks.
  • Updated mocks, changing properties and adding and removing subscribers.
  • Observed transactions applying and aggregating properly.
  • Observed add/remove rules each working correctly.
  • Observed the "don't re-add unsubscribed users" condition acting on subscribers who had previously been added but explicitly removed/unsubscribed.

Diff Detail

Repository
rP Phabricator
Branch
hcc3
Lint
Lint Passed
SeverityLocationCodeMessage
Advicesrc/applications/herald/adapter/HeraldAdapter.php:1701XHP16TODO Comment
Unit
No Test Coverage
Build Status
Buildable 6588
Build 6610: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Make "ADD_CC" and "REMOVE_CC" available as "standard" Herald effects.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan edited edge metadata.
btrahan added inline comments.
src/applications/herald/adapter/HeraldAdapter.php
1701–1703

I'm curious what else are you thinking should / could be here? (I figure we have the subscriber xaction itself so not immediately clear to me what else to surface here in time.)

This revision is now accepted and ready to land.Jun 8 2015, 4:46 PM
src/applications/herald/adapter/HeraldAdapter.php
1701–1703

We "shouldn't" be storing translated text in the database: we currently generate a bunch of error messages in some arbitrary user's translation, then store the raw text. We should really store some kind of machine-readable representation of the error so we can render and display it to users in their own language.

We're generally pretty good about this, but do it here, and also do it in Audit (storing audit reasons as text). Those are the only two I'm aware of.

In this particular case, it would be helpful to show the fate of each individual target: X, Y were subscribed, Z and A had already been unsubscribed, B was invalid. But doing this in a translatable way was a crazy amount of work, and that work to get the translations right was "wrong", and we can't really show handles properly, so it would be an unintelligible mess of PHIDs anyway, so I just skipped it for now.

I think this will get easier once Herald eventually modularizes and these actions can be split out per class and have translations in them, similar to how Transactions work. This sequence of diffs moves Herald toward that, although we still have a way to go.

Eventually, I'd imagine the output possibly looking something like this:

RULE                TARGET         ACTION

H23 Example Rule    epriestley     Added subscriber.
                    btrahan        This user previously unsubscribed.
                    MLST           Invalid subscriber.
                    
                                        ^
                                        |
  These would be translated properly ---+
This revision was automatically updated to reflect the committed changes.