Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FPA M0.5 #3543

Merged
merged 30 commits into from
Aug 27, 2024
Merged

FPA M0.5 #3543

merged 30 commits into from
Aug 27, 2024

Conversation

justinchou-google
Copy link
Contributor

@justinchou-google justinchou-google commented Aug 19, 2024

Adds a sequence flow to AutoPromptManager to process actionOrchestration from the article endpoint.

  • Defines many more interfaces from article response.
  • Continues to store and fetch frequency capping events by ACTION TYPE.
  • Maintains current fallback logic with default frequency cap configs.
  • Computes contentType (to determine which funnel to fetch) equivalent to what is written to the pageConfig. NOTE: this differs slightly from the current prod version's computation of isClosable, which also falls back to revenue model.
    • Question: how do we want to update this definition for OnsitePreview? It currently uses this definition of isClosable.
    • Follow up item to update Onsite Preview (b/362347951).
  • CLOSED contentType explicitly skips over FrequencyCapping logic, even if FrequencyCapping values are provided. I believe this is the only contentType specific logic in the flow.
  • Defines new secondsDuration proto on the frequencyCapDuration: this will be validated and converted as part of getTargetedInterventionFunnel Action (TODO b/362344744).
  • Fixes nano implementation from Duration.

@justinchou-google justinchou-google self-assigned this Aug 19, 2024
src/runtime/basic-runtime.ts Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
@@ -243,6 +244,7 @@ export class BasicRuntime implements BasicSubscriptions {
autoPromptType,
alwaysShow,
isClosable,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kristenwang Are we ok updating this value for OnsitePreview to also use contentType? I believe in today's flow we use isClosable, but this will be deprecated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: we can no longer assume the contribution prompt will be present in the audience actions for all contribution revenue models.

@oyj9109
Copy link
Contributor

oyj9109 commented Aug 22, 2024

Thanks for thoroughly documented description. The overall logic looks good and I left some minor comments.

Regarding some questions in the description

Question: how do we want to update this definition for OnsitePreview? It currently uses this definition of isClosable.

I feel like having a fallback value based on content type would generally make sense in case prompt level dimissibility is absent (e.g., no action orchestration available for onsite preview).

src/api/basic-subscriptions.ts Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Outdated Show resolved Hide resolved
(action) => action.configurationId === nextIntervention.configId
);
}
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, are there cases where flag is enabled but article.actionOrchestration is absent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When all experiment flags are on, we always expect actionOrchestration to be present. Even in the case when interventionFunnel is absent (where no funnel is matched).

This is to put prevent errors when the server side experiment is not on.

Copy link
Contributor

@oyj9109 oyj9109 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

src/api/basic-subscriptions.ts Show resolved Hide resolved
src/runtime/basic-runtime-test.js Show resolved Hide resolved
@justinchou-google justinchou-google marked this pull request as ready for review August 27, 2024 15:12
@justinchou-google justinchou-google enabled auto-merge (squash) August 27, 2024 15:36
@justinchou-google justinchou-google marked this pull request as draft August 27, 2024 15:37
auto-merge was automatically disabled August 27, 2024 15:37

Pull request was converted to draft

@justinchou-google justinchou-google marked this pull request as ready for review August 27, 2024 16:01
@justinchou-google justinchou-google merged commit f0deefb into subscriptions-project:main Aug 27, 2024
7 checks passed
justinchou-google added a commit that referenced this pull request Aug 29, 2024
This reverts commit f0deefb.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants