-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
Enable/Disable analytics based on user consent #3467
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3467 +/- ##
==========================================
- Coverage 74.71% 74.61% -0.11%
==========================================
Files 285 286 +1
Lines 10996 11033 +37
Branches 1337 1344 +7
==========================================
+ Hits 8216 8232 +16
- Misses 2398 2416 +18
- Partials 382 385 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
878bac9
to
ed56311
Compare
ed56311
to
8307f70
Compare
8339cea
to
1217a27
Compare
1217a27
to
fffdc2e
Compare
0ad3b39
to
ce7b9dc
Compare
49efc4f
to
d7d1e61
Compare
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.
Reviewed 5 of 12 files at r2, all commit messages.
Reviewable status: 5 of 12 files reviewed, 7 unresolved discussions (waiting on @andracc)
Backend/Models/User.cs
line 69 at r2 (raw file):
public string Username { get; set; } [BsonElement("otelConsent")]
Let's add /// <summary>
comments for these two consent bools, since it might be confusing what the difference is.
Backend.Tests/Otel/OtelKernelTests.cs
line 40 at r2 (raw file):
[Test] public void BuildersSetConsentAndSessionBaggageFromHeader()
How about just BuildersSetBaggageFromHeader
(and OnEndSetsTagsFromBaggage
below)?
Backend/Otel/OtelKernel.cs
line 108 at r2 (raw file):
public override async void OnEnd(Activity data) { var consentString = data?.GetBaggageItem("otelConsentBaggage");
Why is the ?
necessary on data?
throughout this function?
src/components/UserSettings/UserSettings.tsx
line 82 at r2 (raw file):
const show = (): void => setDisplayConsent(true); const handleConsentChange = (consentVal: boolean | undefined): void => {
This can be shortened to consentVal?: boolean
src/components/UserSettings/UserSettings.tsx
line 303 at r2 (raw file):
data-testid={UserSettingsIds.ButtonChangeConsent} id={UserSettingsIds.ButtonChangeConsent} onClick={show}
Since show
isn't used elsewhere, we can just use a lambda function here:
onClick={() => setDisplayConsent(true)}
src/backend/index.ts
line 60 at r2 (raw file):
? ((config.headers.otelConsent = true), (config.headers.sessionId = getSessionId())) : (config.headers.otelConsent = false);
This <...> ? ((<do one thing>), (<do another thing>)) : (<...>)
notation is compact and logically correct, but the presence of an ordered pair ((...), (...))
could be confusing. How about a slightly longer, more common and explicit
if (LocalStorage.getCurrentUser()?.otelConsent) {
config.headers.otelConsent = true;
config.headers.sessionId = getSessionId();
} else {
config.headers.otelConsent = false;
}
src/components/AnalyticsConsent/AnalyticsConsent.tsx
line 0 at r2 (raw file):
If this file is changed to src/components/AnalyticsConsent/index.tsx
and export function
changed to export default function
, then that will match our style elsewhere, and it can be imported with
import AnalyticsConsent from "components/AnalyticsConsent";
rather than
import { AnalyticsConsent } from "components/AnalyticsConsent/AnalyticsConsent";
src/components/AnalyticsConsent/AnalyticsConsent.tsx
line 45 at r2 (raw file):
<Typography variant="h6" style={{ color: "#1976d2", fontWeight: 600 }}
Use color: themeColors.primary
rather than using a hex code here.
src/components/AnalyticsConsent/AnalyticsConsent.tsx
line 46 at r2 (raw file):
variant="h6" style={{ color: "#1976d2", fontWeight: 600 }} color="primary"
This color="primary"
is unnecessary, being overridden by the manual color definition in the style.
src/components/AnalyticsConsent/AnalyticsConsent.tsx
line 70 at r2 (raw file):
<Grid item> <Button color="primary"
The color="primary"
and this and the next button are unnecessary because "primary" is the default color.
src/components/AnalyticsConsent/AnalyticsConsent.tsx
line 72 at r2 (raw file):
color="primary" onClick={acceptAnalytics} style={{
These button dimensions may be too small for some ui languages, so let's bump up to
style={{ height: 60, width: 155 }}
in both these buttons.
src/components/App/AppLoggedIn.tsx
line 58 at r2 (raw file):
async function handleConsentChange( otelConsent: boolean | undefined
This can be shortened to otelConsent?: boolean
.
src/components/App/AppLoggedIn.tsx
line 107 at r2 (raw file):
onChangeConsent={handleConsentChange} required ></AnalyticsConsent>
This can be shortened to
<AnalyticsConsent onChangeConsent={handleConsentChange} required />
Resolves #3410
This change is