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

[ALPHA PHASE] Prompt Frequency Capping Flow #3189

Merged

Conversation

justinchou-google
Copy link
Contributor

@justinchou-google justinchou-google commented Sep 19, 2023

b/299846493
https://docs.google.com/document/d/1H99mlR47aZZv_ZsCRkI6Iv_WUcpwaoeGgCExWSkdYCI/edit?resourcekey=0-9djhO0eRtzh5bMMrZ1dtBg#heading=h.634013igpqpv

Extends triggering flow to follow new prompt frequency capping. Expands AutoPromptConfig model to handle FrequencyCapConfig. Refactors some action eligibility checks. Also refactors computation of prompt delay. Updates openaccess subscription flow to follow audience action order. Display delay is extended to all dismissible prompts. All changes guarded by experiment flag.

TODOs:

  • (b/300963305): manually triggered prompts should also be excluded from the impression counts.
  • Error handling of an actionType that is not mapped to an ImpressionStorageKey.
  • Error logging when frequency cap config is not defined (for when experiment becomes the default path, is not currently reachable by code path).

@justinchou-google justinchou-google self-assigned this Sep 19, 2023
src/model/auto-prompt-config.ts Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Show resolved Hide resolved
src/runtime/auto-prompt-manager.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 Show resolved Hide resolved
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.

I have a few more comments for nit, but overall looks really great! I love to see how simple the new logic is, yet it is more capable of.

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 Show resolved Hide resolved
src/runtime/auto-prompt-manager.ts Show resolved Hide resolved
@oyj9109
Copy link
Contributor

oyj9109 commented Sep 26, 2023

LGTM. Thanks for great work!

@justinchou-google justinchou-google enabled auto-merge (squash) September 26, 2023 15:47
@justinchou-google justinchou-google merged commit 1817630 into subscriptions-project:main Sep 26, 2023
5 checks passed
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.

3 participants