Page MenuHomePhabricator

Editing Herald rules broken on IE8
Closed, ResolvedPublic

Assigned To
Authored By
test1212
Dec 19 2013, 2:51 PM
Referenced Files
F90858: Screen_Shot_2013-12-19_at_8.11.21_AM.png
Dec 19 2013, 4:13 PM
F90850: Screen_Shot_2013-12-19_at_8.09.02_AM.png
Dec 19 2013, 4:13 PM
F90860: Screen_Shot_2013-12-19_at_8.11.27_AM.png
Dec 19 2013, 4:13 PM
F90854: Screen_Shot_2013-12-19_at_8.11.02_AM.png
Dec 19 2013, 4:13 PM
F90856: Screen_Shot_2013-12-19_at_8.11.16_AM.png
Dec 19 2013, 4:13 PM
F90852: Screen_Shot_2013-12-19_at_8.09.06_AM.png
Dec 19 2013, 4:13 PM
Tokens
"Like" token, awarded by test1212.

Description

When creating a new herald rule for "Commits -> Body contains Diffusion -> Any Action", the error "Failed to decode rule data." is fired and the rule is not created. Is this a bug, or is the Herald documentation lacking in how to avoid the exception being thrown?

Event Timeline

test1212 assigned this task to epriestley.
test1212 raised the priority of this task from to High.
test1212 updated the task description. (Show Details)
test1212 added a project: Herald.
test1212 added a subscriber: test1212.

I think this should be fixed once D7796 lands. The "Body" field isn't really supposed to be selectable yet, I just overlooked it. D7796 actually implements it.

(And I just landed it -- try again?)

(Assuming this is fixed in HEAD since it works locally, but let me know if not.)

The issue still appears when selecting New rule for "Commits" rather than "Commit Hook: Commit Content". Looking to open an audit on commits which have not passed through Phabricator, which the commit hooks do not have as an option.

Ah! Sorry, I misunderstood. Let me see if I can reproduce this.

I can't immediately reproduce this. Here's what I did:

I created a new rule like this:

Screen_Shot_2013-12-19_at_8.09.02_AM.png (1×1 px, 138 KB)

Here's the detail view:

Screen_Shot_2013-12-19_at_8.09.06_AM.png (1×1 px, 134 KB)

I pushed a new commit to trigger the rule:

>>> orbital ~/repos/POEMS $ git commit -m 'test test test diffusion'
[blarp 99b742c] test test test diffusion
 1 file changed, 1 insertion(+)
>>> orbital ~/repos/POEMS $ git push
Counting objects: 5, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 278 bytes, done.
Total 3 (delta 2), reused 0 (delta 0)
To ssh://dweller@localhost/diffusion/POEMS/
   8f3e93b..99b742c  blarp -> blarp

After it parsed, it produced a valid transcript:

Screen_Shot_2013-12-19_at_8.11.02_AM.png (163×908 px, 32 KB)

This transcript showed the rule fired:

Screen_Shot_2013-12-19_at_8.11.16_AM.png (121×879 px, 20 KB)

The transcript body shows the field populated correctly ("Field: Body"):

Screen_Shot_2013-12-19_at_8.11.21_AM.png (409×870 px, 46 KB)

And the rule detail looks correct too:

Screen_Shot_2013-12-19_at_8.11.27_AM.png (122×879 px, 13 KB)

Any ideas about what I'm doing differently?

(One thought is that I'm using git for this test -- are you on a different VCS?)

Oh, sorry, it looks like I'm really bad at reading. If you aren't even getting as far as rule creation, maybe try checking your browser console for Javascript errors?

I had thought "Failed to decode rule data." was firing elsewhere, but it's only if your browser does not POST a valid JSON representation of the rule when you hit "Save". So this is probably something in the JS.

Meaning a better question than which VCS you're using is probably "Which browser are you using?"

Can confirm rule creation works fine using Firefox. Error I'm seeing appears in IE 8.0 but doesnt seem to throw a JS error, though this may just be my unfamiliarity with the IE tools. Happy for bug to be closed as IE's fault.

epriestley renamed this task from Herald Broken? to Editing Herald rules broken on IE8.Dec 19 2013, 4:35 PM
epriestley lowered the priority of this task from High to Low.

Ah, cool. We'll take a look at this -- we ostensibly support IE8, it just doesn't get tested much since we have fewer users on it than FF/Chrome/Safari.

Having said that, there is another bug: Though I can create an email rule just fine, selecting "Trigger an audit by" does not allow me to select a user: Typing a name does not bring down the typical list of matched names and leaving the field blank is equivalent to the "Do Nothing" choice.

Ideally, I'd like to leave the name field blank and have any willing developer pick up the audit.

The blankness is probably another JS browser issue, possibly caused by the first issue. Is that also IE-only, or IE+FF?

For "any developer", you should be able to put all your developers in a project called "Developers" and trigger an audit by the project.

IE+FF both fail to produce the drop down, though both do produce it on the other actions, e.g. send an email. Thanks for the project tip!

Another twist, the list IS produced for projects, just not users. Perhaps this is intentional?

However, selecting a project as the audit target seems to cause the rule to never fire. Exactly the same set up with an email target to a user instead does fire using the Herald Test Console.

Ah, yeah, the underlying source only included projects. I think it's rare to want to write a global rule which triggers for users, but there's no reason to disallow it and it's confusing that it's not available as an option. D7803 should fix this, and allow you to choose either users or projects.

Great! Have you been able to reproduce the Audit action causing the trigger to fail?

Apologies, the rule does seem to trigger. I hadn't noticed the "Rules that Affected Me" vs "Rules I Own" in the Herals rule test. It doesn't seem to be aware I'm a user of the selected project, but this isn't technically wrong.

So all seems to be working, thanks!

We're probably going to get rid of that sidebar or make it default to "All" (see T3506) -- it was appropriate at Facebook when everything triggered like 200 rules to default it to "Me", but is confusing in other cases.

chad added a subscriber: chad.

We don't support IE8 anymore and there would be lots to fix if so.