THREESCALE-11500: Fix confusing design flaw in "promote" UI (Part 1) #3953
Conversation
| when ProxyConfigs::AffectingObjectChangedEvent then ProxyConfigAffectingChangeWorker.perform_later(event.event_id) | ||
| when ProxyConfigs::AffectingObjectChangedEvent then | ||
| proxy = Proxy.find(event.proxy_id) | ||
| proxy.affecting_change_history.touch |
There was a problem hiding this comment.
Should we maybe keep the exception catching and some of the error handling from the worker?
I think the chance that proxy would not be found here, or any other issue occur is minimized here - unlike in the case when it's done via a background job (where things can happen until the job gets executed, like the proxy can be deleted etc.). But not sure, maybe we're missing some scenario where an exception can indeed be thrown?
There was a problem hiding this comment.
I thought the same thing but, if an error happens here, won't it end up in logs and bugsnag anyway? I assumed there were no need to manually catch and report errors from here. In fact, I'm not sure it was needed in the previous code.
lvillen
force-pushed
the
THREESCALE-11500_fix-confusing-design-flaw-in-promote-ui
branch
from
e77d810 to
f2c1939
Compare
lvillen
force-pushed
the
THREESCALE-11500_fix-confusing-design-flaw-in-promote-ui
branch
from
f2c1939 to
19dc2f4
Compare
| @@ -8,7 +8,9 @@ class ProxyConfigEventSubscriberTest < ActiveSupport::TestCase | |||
| proxy_rule = FactoryBot.build_stubbed(:proxy_rule, proxy: proxy) | |||
| event = ProxyConfigs::AffectingObjectChangedEvent.create(proxy, proxy_rule) | |||
|
|
|||
| ProxyConfigAffectingChangeWorker.expects(:perform_later).with(event.event_id) | |||
| Proxy.stubs(:find).with(event.proxy_id).returns(proxy) | |||
There was a problem hiding this comment.
Do you need the stub? shouldn't the proxy be in the DB?
THREESCALE-11500: Fix confusing design flaw in "promote" UI
This issue exposes two problems, this PR deals with the first part:
"The Promote button is not enabled immediately when changes are done. Instead a job is enqueued and the button is enabled when this job is executed."
With the help of @jlledom, we've reached to the conclusion that not knowing when a job is going to be executed is a problem here. In this PR, we're removing the job who deals with updating
proxy_configs_affecting_changestable and, instead, we propose to update the table directly at theProxyConfigEventSubscriber. We don't think this will imply any substantial downgrade in the performance but we're open to discussion.Verification steps