-
-
Notifications
You must be signed in to change notification settings - Fork 325
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
feat: added Matomo event for user clicking on account deletion (issue #6351) #6410
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #6410 +/- ##
==========================================
- Coverage 9.54% 5.84% -3.71%
==========================================
Files 325 490 +165
Lines 16411 29252 +12841
==========================================
+ Hits 1567 1710 +143
- Misses 14844 27542 +12698 ☔ View full report in Codecov by Sentry. |
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.
Left some comments !
// Track Matomo event for clicking on Delete Account | ||
MatomoTracker.instance.trackEvent( | ||
eventInfo: EventInfo( | ||
category: 'User Action', |
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.
I think you should go throught the
packages\smooth_app\lib\helpers\analytics_helper.dart
file first.
I recommend creating the event in the Enum first then using it here.
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.
Done. Before pushing the commits though, I want to know whether I should revert the changes in the GeneratedPluginRegistrant.swift file or not. Thanks.
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.
Why is this change required btw ?
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.
Flutter's plugin system automatically updated GeneratedPluginRegistrant.swift when the pubsec.yaml file was changed. That is, the 'app_settings' package was added to the pubspec.yaml but I don't remember adding it. Should I revert it or let it be?
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.
We're not a macOS app, so we will include that in a future upgrade.
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.
We're not a macOS app, so we will include that in a future upgrade.
Got it. I'll revert those changes and make the necessary commits.
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.
@g123k I just noticed that someone had edited smooth-app/packages/smooth_app/lib/pages/preferences/user_preferences_advanced_settings.dart
file where they are using the app_settings package inside the onTap(_)
method over here:
@override
Widget build(BuildContext context) {
final AppLocalizations appLocalizations = AppLocalizations.of(context);
return UserPreferenceListTile(
onTap: (_) async => AppSettings.openAppSettings(), // comes from the 'app_settings' package
title: appLocalizations.native_app_settings,
subTitle: appLocalizations.native_app_description,
leading: const Icon(CupertinoIcons.settings_solid),
showDivider: true,
);
}
I don't know why these changes were reflected as my commits but I didn't change them. Will need to re-revert them and include app_settings in the pubsec.yaml which will in turn change the GeneratedPluginRegistrant.swift file as well. Else this file will have missing packages.
@AshAman999 @g123k Please review the changes and let me know. Thanks! |
I just noticed an earlier PR and it's commits regarding tracking Matomo events and found out that using |
builder: (BuildContext context) => AccountDeletionWebview(), | ||
), | ||
), | ||
() async { |
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.
Check line no 254 from the same file, that how we have done that
AnalyticsHelper.trackEvent(AnalyticsEvent.logoutAction);
I believe we can do something similar.
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.
yep, did it the same way now. Thanks!
packages/smooth_app/pubspec.lock
Outdated
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.
if this change is not needed, please revert
packages/smooth_app/pubspec.yaml
Outdated
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.
same here:
if this change is not needed, please revert
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.
I'm waiting for Edouard's response whether to include the 'app_settings' package in the pubspec.yaml or not. Then I'll make the final commits all-together.
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.
Please address the comments left
Hey @RathoreAbhiii |
Have been reverted, 'app_settings" and it's related config. is no longer there in the pubspec.yaml, pubspec.lock and GeneratedPluginRegistrant.swift files. (As seen in the commit saving changes before rebase) @AshAman999 |
Hey @RathoreAbhiii My thinking was not to include the changes which are not part of this issue. |
What
Fixes bug(s)
Part of