Follow-up from #2648284: Service container rebuild is required when events are configured:

oh, I just figured that we are doing this at the wrong place. The form is not the right place as this will miss any rules that get synced via CMI and/or are created via API. But moving this to a config entity crud event should be fine - once done so we should be able to remove the manual kernel rebuilds from EventIntegrationTest also.

Comments

fago created an issue. See original summary.

fago’s picture

fago
Primary language German
Location Vienna
commented
Issue tags: +Contributor
vasi1186’s picture

vasi1186 commented
Assigned: Unassigned » vasi1186
vasi1186’s picture

vasi1186 commented
Assigned: vasi1186 » Unassigned
Status: Active » Needs review

Here is the pull request: https://github.com/fago/rules/pull/381

One thing maybe: should we create more tests that check more extensive the rules update/delete and the container rebuild, or do you think the current ones are enough?

vasi1186’s picture

vasi1186 commented
Status: Needs review » Needs work

travis reported some issues.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review

The usual random test fail we see sometimes.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Fixed

Thanks, changes look really good. I think test coverage is ok as we've the functional API test coverage that does not need the manual rebuilds any more now. IT would be nice to cover the deletion case as well, but I don't think that would be easy to test?
Thus, I think that's good enough for now. Thanks, merged!

  • fago committed dbac19d on authored by vasi1186 
    Issue #2659990 by vasi1186: Creating reaction rules via API does not...
fago’s picture

fago
Primary language German
Location Vienna
commented

FYI: The tests issue: #2659954: TempStorageTest::testLocking has random fails

vasi1186’s picture

vasi1186 commented

Yes, testing the deletion is a bit harder to automate. Basically, we would have to check the container and see that the generic event is not registered. I guess the code in the test would be something similar with what we had before int he isRuleEventRegistered() method here https://github.com/fago/rules/pull/364/files

I don't know how crucial is to have this tests automated, I personally see this as a minor issue, because it does not affect the behavior of the site, and the container will be anyway rebuilt during the next cache clear or the next time when the user creates a rule.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.