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

fix: use deviceId instead of uid for uninstallation event mapping #1668

Merged
merged 4 commits into from May 14, 2024

Conversation

nsrCodes
Copy link
Contributor

@nsrCodes nsrCodes commented May 7, 2024

The locally stored user id has many problems:

  1. not being cleared when logged out
  2. Randomly getting generated when not found (which is just generating fake new user data)
  3. Mapping events from an anon user to a random uid

shifting to using the deviceId instead

@nsrCodes nsrCodes requested a review from wrongsahil May 7, 2024 04:00
Copy link

deepsource-io bot commented May 7, 2024

Here's the code health analysis summary for commits 027228f..9c083e2. View details on DeepSource ↗.

Analysis Summary

AnalyzerStatusSummaryLink
DeepSource Shell LogoShell✅ SuccessView Check ↗
DeepSource JavaScript LogoJavaScript❌ Failure
❗ 1 occurence introduced
🎯 1 occurence resolved
View Check ↗

💡 If you’re a repository administrator, you can configure the quality gates from the settings.

@@ -90,7 +90,6 @@ const AuthHandler: React.FC<{}> = () => {
Logger.timeLog("AuthHandler-blockingOperations", "START");
const authData = getAuthData(user);
window.uid = user.uid;
localStorage.setItem("__rq_uid", user.uid);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still need this

@@ -11,7 +11,8 @@
window.location.href = 'https://app.requestly.io';
}

if ("https://app.requestly.io/goodbye/" === window.location.href) { const a = { api_key: "62ff1b46909e50358cfca0668d41f011", events: [{ user_id: localStorage.getItem("__rq_uid") || Math.random().toString(36).slice(2, 10), event_type: "extension_uninstalled" }] }, b = new Blob([JSON.stringify(a)]); navigator.sendBeacon("https://api2.amplitude.com/2/httpapi", b) }
const deviceId = localStorage.getItem("__rq_device_id")
if ("https://app.requestly.io/goodbye/" === window.location.href && deviceId) { const a = { api_key: "62ff1b46909e50358cfca0668d41f011", events: [{ device_id: deviceId, event_type: "extension_uninstalled" }] }, b = new Blob([JSON.stringify(a)]); navigator.sendBeacon("https://api2.amplitude.com/2/httpapi", b) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use uid first and if it is not found, then we should use the deviceId.

@@ -11,7 +11,8 @@
window.location.href = 'https://app.requestly.io';
}

if ("https://app.requestly.io/goodbye/" === window.location.href) { const a = { api_key: "62ff1b46909e50358cfca0668d41f011", events: [{ user_id: localStorage.getItem("__rq_uid") || Math.random().toString(36).slice(2, 10), event_type: "extension_uninstalled" }] }, b = new Blob([JSON.stringify(a)]); navigator.sendBeacon("https://api2.amplitude.com/2/httpapi", b) }
const deviceId = localStorage.getItem("__rq_device_id")
if ("https://app.requestly.io/goodbye/" === window.location.href && deviceId) { const a = { api_key: "62ff1b46909e50358cfca0668d41f011", events: [{ device_id: deviceId, event_type: "extension_uninstalled" }] }, b = new Blob([JSON.stringify(a)]); navigator.sendBeacon("https://api2.amplitude.com/2/httpapi", b) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to set extension_uninstall_date attribute too on uninstall

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setting attributes is not available in the http api

@@ -11,7 +11,8 @@
window.location.href = 'https://app.requestly.io';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is deprecated when we shifter to vite. Please delete this file to avoid confusion in future

@wrongsahil wrongsahil self-requested a review May 9, 2024 04:46
@nsrCodes nsrCodes merged commit f9916bb into master May 14, 2024
2 of 3 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.

None yet

2 participants