Conversation
xlisachan
changed the title
xlisachan
force-pushed
the
PAY-3472
branch
3 times, most recently
from
2f12f94 to
cc61879
Compare
xlisachan
force-pushed
the
PAY-3472
branch
16 times, most recently
from
9846065 to
b516927
Compare
xlisachan
force-pushed
the
PAY-3472
branch
4 times, most recently
from
069c3bb to
a676273
Compare
| version: config.gleanClientConfig.version, | ||
| channel: config.gleanClientConfig.channel, | ||
| } | ||
| : undefined, |
There was a problem hiding this comment.
suggestion: update the type for config.gleanClientConfig to required
If the type is updated, then it should be possible to update this to just one line.
glean: config.gleanClientConfig
| "targets": { | ||
| "build": { | ||
| "dependsOn": ["l10n-bundle", "glean-generate"] | ||
| "dependsOn": ["l10n-bundle", "glean-generate", "payments-metrics:glean-generate-frontend"] |
There was a problem hiding this comment.
praise: ah nice, I like this change. Keeps the logic with the library. Should probably consider, as a polish PR at another time, updating glean-generate to follow this pattern as well.
| @Type(() => PaymentsGleanClientConfig) | ||
| @ValidateNested() | ||
| @IsDefined() | ||
| public readonly gleanClientConfig!: Partial<PaymentsGleanClientConfig>; |
There was a problem hiding this comment.
issue: Do not add gleanClientConfig to nestapp/config as it is not used by the NestApp
Instead could this be moved to the payments/next/app/config instead?
|
|
||
| import { getApp } from '../nestapp/app'; | ||
|
|
||
| export async function getMetricsOptOutAction(uid: string) { |
There was a problem hiding this comment.
question: What's the purpose of this action?
If it is to retrieve a users metrics preferences, could I suggest an alternative approach?
suggestion: update auth session to include users metrics preferences
If I remember correctly, during the payments-next auth flow when retrieving the users info, the users metrics preferences are included. At this time that info could be added to the auth session, which can then easily be retrieved on the client.
| name: Wait for Customs | ||
| command: yarn workspace fxa-customs-server start | ||
|
|
||
| ensure-glean-venv: |
There was a problem hiding this comment.
question: is it necessary to include this? If it's already included in the docker image, isn't it guaranteed that it'll be available?
xlisachan
force-pushed
the
PAY-3472
branch
2 times, most recently
from
398f9c0 to
d794f8d
Compare
xlisachan
force-pushed
the
PAY-3472
branch
12 times, most recently
from
8635d52 to
8e45cd4
Compare
- [x] Creates yaml file for Glean frontend metric definitions (page_view, retention_flow, interstitial_offer) - [x] Updates pipeline/config - [x] Creates glean-client.manager, factories, providers, types, tests in payments-metrics - [x] Adds server actions and validators - [x] Creates hook - [x] Data review (https://bugzilla.mozilla.org/show_bug.cgi?id=2021134) Closes: PAY-3472
|
@xlisachan 20000!! |
3 participants
Because
This pull request
Issue that this pull request solves
Closes: PAY-3472
Checklist
Put an
xin the boxes that apply