feat: add device permission diagnostics metrics 2 - #5169
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0008b3694
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const enrichedPayload = this.privacyAndSecurityPermissionEnricher.enrich({ | ||
| name, | ||
| payload, | ||
| scope: this.getPermissionScope(options), | ||
| }); |
There was a problem hiding this comment.
Enrich the fetch-based client-event path
When permission state has been set and callers use the documented buildClientEventFetchRequestOptions flow for tab-close/beforeUnload client events, this new enrichment never runs because it is only applied in submitClientEvent; the helper at lines 468-503 still forwards the original payload directly. That path exists specifically for cases where submitClientEvent cannot be used, so terminal events such as client.call.leave sent during page close will omit privacyAndSecurityPermission while normal submissions include it. Apply the same enrichment before building the fetch request options.
Useful? React with 👍 / 👎.
|
original PR comments can be found here: #5155 creating link for historical purposes |
| }); | ||
|
|
||
| return this.callDiagnosticMetrics.submitClientEvent({ | ||
| const enrichedPayload = this.privacyAndSecurityPermissionEnricher.enrich({ |
| @@ -0,0 +1,235 @@ | |||
| import {isEqual} from 'lodash'; | |||
There was a problem hiding this comment.
nitpick: Feels like this file deserves it's own folder with
index
utils
constants
types
| previous?: PrivacyAndSecurityPermissionState | ||
| ): boolean => isEqual(current, previous); | ||
|
|
||
| const resolveMediaResources = ( |
| }, | ||
| { | ||
| events: MEDIA_TX_PERMISSION_EVENTS, | ||
| resolve: (payload) => ({resources: resolveMediaResources(payload), terminal: false}), |
| { | ||
| events: CONTENT_SHARE_PERMISSION_EVENTS, | ||
| resolve: (payload) => ({ | ||
| resources: payload?.mediaType === 'share' ? ['contentShare'] : [], |
There was a problem hiding this comment.
might be better as a func
|
|
||
| const projectPrivacyAndSecurityPermission = ( | ||
| permission: PrivacyAndSecurityPermission, | ||
| resources: readonly PrivacyAndSecurityPermissionResource[] |
| current: PrivacyAndSecurityPermission, | ||
| previous: PrivacyAndSecurityPermission | ||
| ): PrivacyAndSecurityPermission | undefined => { | ||
| const changedPermission: PrivacyAndSecurityPermission = { |
There was a problem hiding this comment.
nitpick: the layered ternaries are a bit tough with the object spread too
|
|
||
| import {PERMISSION_ENRICHMENT_RULES} from '../../../src/privacy-and-security-permission-enricher'; | ||
|
|
||
| describe('PrivacyAndSecurityPermissionEnricher', () => { |
There was a problem hiding this comment.
This test feels highly incomplete, there should be a full suite of tests
There was a problem hiding this comment.
modularization of tests should apply here
COMPLETES https://jira-eng-gpk2.cisco.com/jira/browse/SPARK-842561
This pull request addresses
the event definitions were added here: https://sqbu-github.cisco.com/WebExSquared/event-dictionary/pull/2520
by making the following changes
adding metrics to applicable events as defined in https://confluence-eng-gpk2.cisco.com/conf/spaces/CAL/pages/866309853/Daily+Meeting+Issue+Report+Initiative
Change Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
Make sure to have followed the contributing guidelines before submitting.