Cleanup FCA experiment flag and legacy code paths #3603
Conversation
justinchou-google
changed the title
justinchou-google
requested review from
ChrisAntaki,
jpettitt,
mhkawano,
oyj9109 and
Wayne-X
as code owners
|
Clean up LGTM. Left a suggestion for fallback mechanism we can be followed up later. |
| const nextOrchestration = await this.getInterventionOrchestration_( | ||
| clientConfig, | ||
| article | ||
| ); | ||
|
|
||
| if (!!nextOrchestration) { |
There was a problem hiding this comment.
Will this result in nothing being shown? Should this fallback aswell?
There was a problem hiding this comment.
Good question, this is WAI because it is valid for getInterventionOrchestration_ to not return any orchestration after checking, for example, action state or frequency capping eligibility. The fallback implemented for actionOrchestration is a fallback for an invalid case where the actionOrchestration object is absent from the article response.
There was a problem hiding this comment.
Makes sense, I see line 345 now. Maybe splitting the block assigning potentialAction into a function could make it easier to read?
There was a problem hiding this comment.
I agree it is slightly confusing, the reason why getting potentialAction is not its own function is because the overall function relies on setting dismissibility from the orchestration. I added a todo to clean this up in a future PR.
| const nextOrchestration = await this.getInterventionOrchestration_( | ||
| clientConfig, | ||
| article | ||
| ); | ||
|
|
||
| if (!!nextOrchestration) { |
There was a problem hiding this comment.
Makes sense, I see line 345 now. Maybe splitting the block assigning potentialAction into a function could make it easier to read?
justinchou-google
merged commit 80bb432
into
subscriptions-project:main
Post launch, cleanup legacy code: b/385185319
Final step of post-launch cleanup: