-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add enterprise newsletter API #3500
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Thanks for adding the demo site! I left a few questions.
return await onResult(request); | ||
return await onResult({ | ||
configurationId: this.params_.configurationId, | ||
data: request, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean to return the request?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result is used in handleSurveyDataTransferRequest_ as:
const dataTransferSuccess = await this.attemptSurveyDataTransfer(request);
The result of the callback is used to determine if the data has been stored correctly.
this.dialogManager_.completeView(this.activityIframeView_); | ||
this.entitlementsManager_.clear(); | ||
const userToken = response.getSwgUserToken(); | ||
if (userToken) { | ||
this.deps_.storage().set(Constants.USER_TOKEN, userToken, true); | ||
} | ||
if (this.isOptIn(this.params_.action) && onResult) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what's the plan to handle non-opt-in cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For surveys it is handled in the data transfer request. For rewarded ads it will not be called.
src/api/interventions.ts
Outdated
readonly configurationId?: string; | ||
// Indicates if the intervention should be Google provided, or publisher | ||
// provided. | ||
readonly preference?: string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to expose this now? Not sure if we would want to support BYOP for RRM:E
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a big deal if it's exposed? I can def try to rework it to hide Intervention
and keep the property out of AvailableInterventions
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chris brought it up before too, so I moved Intervention
back into the entitlements manager.
Expose funnel prompts to enterprise publishers through the API.
This change is only meant to support adding newsletters, but includes small changes to eventually expand it to support all prompt types.
Screencast in the first bug.
b/270686569
b/336516781
b/270686569